feat: HTML/CSS → Grida IR import pipeline#608
Merged
softmarshmallow merged 11 commits intomainfrom Mar 30, 2026
Merged
Conversation
…y fixtures Removed all 12 original fixtures that relied on unsupported CSS features (grid, margin, tables, viewport units). Added 21 new fixtures organized by category (layout, box, paint, text, mixed), each testing a specific CSS property group using only features the HTML→IR pipeline supports. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
WalkthroughAdds a Stylo-based CSS cascade crate ( Changes
Sequence DiagramsequenceDiagram
participant Parser as HTML Parser
participant DemoDom as DemoDom (arena)
participant Cascade as CascadeDriver
participant Stylo as Stylo/Stylist
participant Scene as SceneBuilder
Parser->>DemoDom: parse HTML -> build arena DOM
DemoDom->>Cascade: provide document handle
Cascade->>DemoDom: scan <style> elements, collect author CSS
Cascade->>Stylo: build Stylist (UA + author CSS), flush
Stylo->>DemoDom: resolve computed styles per element (write ElementData)
Scene->>DemoDom: traverse styled DOM
Scene->>Scene: map computed CSS -> IR nodes (Container/TextSpan/Rectangle)
Scene-->>Caller: return SceneGraph
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
softmarshmallow
changed the title
csscascade support for rendering html/css contents
- Add csscascade adapter, cascade driver, and arena DOM modules - Add HTML import module (html/mod.rs) with Stylo-based CSS resolution - Wire grida-dev to load .html files via from_html_str - Support ~35 CSS properties: layout (flex, block, display:none), box model (width/height/min/max/padding/overflow), paint (backgrounds, gradients, borders, shadows, opacity), text (font, color, align, line-height, letter-spacing, word-spacing, text-transform, decoration), and inline style attributes - Map block containers to flex-column for correct flex-child sizing - Emit text-only elements as Container+TextSpan to preserve box model Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Keep HTML import pipeline additions alongside main's raster image refactoring (load_raster/RasterScene). Remove duplicate pub mod query. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The CSS cascade adapter uses a process-global DEMO_DOM static, so concurrent from_html_str calls race on that shared slot causing Stylo debug_assert panics. Added a mutex to serialize HTML tests. Also ran cargo fmt to fix formatting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace css_box_shadow_to_cg with comprehensive css_effects_to_cg handling box-shadow, filter: blur(), filter: drop-shadow(), and backdrop-filter: blur() in a single pass - Add css_text_shadow_to_effects for text-shadow on TextSpan nodes - Add css_blend_mode_to_cg mapping all 16 CSS mix-blend-mode values - Wire effects + blend_mode into all three emitters (container, rect, text) - Fix test mutex poisoning cascade with lock_html() helper - Add 8 deterministic tests for effects properties Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wire text-decoration-color and text-decoration-style from Stylo into TextDecorationRec. Mark thickness and skip-ink as TODO (gecko-only in Stylo servo mode). Add L0 fixtures for text-decoration variants and 2D CSS transforms (implementation pending). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add docs/wg/format/ with canonical tracking docs for the Grida IR schema and its mapping from CSS, HTML, and SVG. Tables are oxfmt- formatted to minimize diffs on future edits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add docs/wg/format/index.md as docusaurus entry point. Add format:md frontmatter to grida.md, css.md, svg.md for MDX compatibility. Link format mapping docs from format/README.md, format/AGENTS.md, and docs/AGENTS.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove CSS_PROPERTY_MAPPING.md (aspirational, marked unimplemented features as done) and html/TODO.md (superseded by docs/wg/format/). Update csscascade README roadmap to reflect current state. Add doc pointer in html/mod.rs to the canonical tracking docs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
softmarshmallow
changed the title
csscascade support for rendering html/css contents
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (9)
fixtures/test-html/L0/text-decoration.html (1)
22-33: LGTM! Coverage aligns with parser capabilities.The four text-decoration values tested match the supported
TextDecorationLinevariants in the parser. The shorthand syntax is appropriate for L0 baseline fixtures.Optional enhancement: Consider testing sub-properties.
The parser also supports
text-decoration-colorandtext-decoration-style(seehtml/mod.rs:366-396). If comprehensive property coverage is desired, consider adding test cases like:.underline-colored { text-decoration-line: underline; text-decoration-color: `#60a5fa`; text-decoration-style: wavy; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fixtures/test-html/L0/text-decoration.html` around lines 22 - 33, Add an optional test case exercising sub-properties: add a CSS rule such as `.underline-colored { text-decoration-line: underline; text-decoration-color: `#60a5fa`; text-decoration-style: wavy; }` to the same fixture so the parser paths that handle text-decoration-color and text-decoration-style (see html/mod.rs handling of TextDecorationLine and related properties) are covered; ensure you use the longhand properties (`text-decoration-line`, `text-decoration-color`, `text-decoration-style`) and include the class in the HTML so the test runner picks it up.docs/wg/format/index.md (1)
1-7: Addformat: mdto frontmatter.This file uses no JSX/MDX features, so it should opt out of MDX parsing to avoid potential issues with special characters.
📝 Suggested frontmatter update
--- title: Format & Import Mapping +format: md tags: - internal - wg - format ---As per coding guidelines: "For documentation files that don't use JSX/MDX features, opt out of MDX parsing by adding
format: mdto the frontmatter".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/wg/format/index.md` around lines 1 - 7, The frontmatter YAML block at the top of the document is missing the MDX opt-out; update the frontmatter (the existing YAML block containing title and tags) to include format: md (e.g., add a new line "format: md" under the title or alongside tags) so the file is treated as plain Markdown and MDX parsing is disabled.fixtures/test-html/L0/layout-block.html (1)
21-23: Consider removing the empty CSS rule.The
.block + .blockrule contains only a comment with no actual declarations. This is a vestige from the refactor and can be safely removed.🧹 Suggested cleanup
- .block + .block { - /* spacing via parent padding, not margin */ - } - .spacer {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fixtures/test-html/L0/layout-block.html` around lines 21 - 23, Remove the empty CSS rule ".block + .block" which contains only a comment; locate the rule in the stylesheet (the selector .block + .block) and delete the entire rule block so no empty selector remains, keeping spacing handled by parent padding as intended.crates/csscascade/README.md (1)
77-77: Minor grammar nit.Static analysis flagged "needs dedicated" as a double modal. Consider rephrasing.
📝 Suggested fix
-- [ ] `visibility: hidden` — needs dedicated IR field (not opacity:0) +- [ ] `visibility: hidden` — requires a dedicated IR field (not opacity:0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/csscascade/README.md` at line 77, Rephrase the fragment "visibility: hidden — needs dedicated IR field (not opacity:0)" to avoid the double-modal; e.g. change it to "visibility: hidden — requires a dedicated IR field (not opacity: 0)" or "visibility: hidden — should have its own IR field (not opacity: 0)" to make the intent clearer; update the README line that currently reads "visibility: hidden — needs dedicated IR field (not opacity:0)" accordingly.crates/grida-canvas/src/html/mod.rs (2)
52-52: Remove or gate debug output behind a feature flag.The
eprintln!on line 52 will print to stderr for every HTML import, which may be undesirable in production or library use. Consider removing it or gating it behind adebugfeature or log level.♻️ Suggested fix
- eprintln!("[html] resolved {} elements", styled_count); + #[cfg(feature = "html-debug")] + eprintln!("[html] resolved {} elements", styled_count);Or use the
logcrate if available:- eprintln!("[html] resolved {} elements", styled_count); + log::debug!("[html] resolved {} elements", styled_count);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/html/mod.rs` at line 52, The eprintln!("[html] resolved {} elements", styled_count) in the html module prints to stderr unconditionally; change this by either removing the call, gating it behind a Cargo feature (e.g., a "debug" feature) so it only compiles when enabled, or replace it with a logger call (using the log crate: e.g., trace!/debug!) to respect runtime log levels; locate the eprintln! in mod.rs (reference: styled_count and the html module) and implement one of those options so production builds won't always write to stderr.
1133-1148: Document thread-safety limitation in public API.The test mutex and comment clearly document the thread-safety issue, but the public
from_html_strfunction has no such documentation. Users calling this from multiple threads will encounter panics.Add documentation to the public API:
/// Parse an HTML string and convert it into a Grida [`SceneGraph`]. /// /// This is the main entry point, analogous to [`crate::svg::pack::from_svg_str`]. +/// +/// # Thread Safety +/// +/// This function uses a process-global DOM slot and is **not thread-safe**. +/// Concurrent calls from multiple threads will cause undefined behavior or panics. +/// Serialize calls externally if needed. pub fn from_html_str(html: &str) -> Result<SceneGraph, String> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/html/mod.rs` around lines 1133 - 1148, Add a public doc comment to from_html_str explaining it is not safe to call concurrently because the module uses a process-global DEMO_DOM and a global mutex (HTML_TEST_LOCK) to serialize access; mention that from_html_str internally acquires the lock via lock_html() which recovers from poison (unwrap_or_else(|e| e.into_inner())) so a panicked run won’t permanently poison the mutex, and recommend callers serialize calls or run them on one thread/worker to avoid panics.crates/csscascade/src/dom.rs (1)
103-105: Clarify the safety justification for Send + Sync.The comment says "The DOM is frozen after parsing; no mutable aliasing across threads" but
element_datausesAtomicRefCellwhich allows interior mutability. The actual safety comes fromAtomicRefCellproviding runtime borrow checking withSend + Syncguarantees, not from the DOM being "frozen."Update the safety comment to accurately describe the synchronization:
-// SAFETY: The DOM is frozen after parsing; no mutable aliasing across threads. +// SAFETY: The DOM structure (nodes, children, parent links) is immutable after parsing. +// The `element_data` slots use `AtomicRefCell` which provides thread-safe interior +// mutability with runtime borrow checking, making concurrent style resolution safe. unsafe impl Sync for DemoDom {} unsafe impl Send for DemoDom {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/csscascade/src/dom.rs` around lines 103 - 105, Update the unsafe impl comment for DemoDom to accurately reflect why Send and Sync are safe: mention that element_data uses AtomicRefCell which provides runtime borrow checking and the necessary Send+Sync synchronization guarantees (not that the DOM is simply "frozen"), and state that other interior mutability is protected by AtomicRefCell and any other concurrency-safe primitives used in DemoDom; reference DemoDom and element_data/AtomicRefCell in the comment so readers can find the rationale.crates/csscascade/src/adapter.rs (2)
59-66: Document the memory leak trade-off in the module doc.The intentional leak is documented in the function, but callers of
from_html_strmay not realize that each call leaks memory. For a dev/import tool processing many files, this could accumulate.The documentation is good. Consider adding a note about mitigation in the module-level doc:
//! # Limitations (PoC) //! //! The DOM is stored in a process-global slot via [`bootstrap_dom`]. Each call //! replaces the previous document (the old one is leaked). This means only one //! document is live at a time, but multiple documents can be processed //! sequentially within the same process. +//! +//! For batch processing of many files, consider running in a subprocess or +//! tracking memory growth. A future iteration will use `Arc`-based context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/csscascade/src/adapter.rs` around lines 59 - 66, Add module-level documentation noting that bootstrap_dom deliberately leaks the boxed DemoDom by storing a raw pointer in DEMO_DOM to keep &'static DemoDom references valid, and that callers of from_html_str (and other APIs that call bootstrap_dom) will therefore leak one DemoDom per call; mention mitigation strategies such as using a single long-lived DOM for batch processing, providing an explicit teardown API to replace and free the previous DOM, or refactoring to avoid &'static references. Reference the functions/types bootstrap_dom, DEMO_DOM, DemoDom, HtmlDocument, and from_html_str in the doc so readers can locate the leak and mitigation options.
416-425:traversal_childrenallocates a Vec on every call.This method is called frequently during style resolution. The allocation could be avoided by implementing a custom iterator that yields children lazily.
For a PoC this is acceptable, but for performance optimization:
// Consider implementing a custom iterator: pub struct ChildIter { children: &'static [NodeId], index: usize, }This is optional since profiling should guide optimization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/csscascade/src/adapter.rs` around lines 416 - 425, traversal_children currently builds a temporary Vec every call which allocates; instead implement a lazy iterator over the node().node().children slice to avoid allocation. Create a custom iterator type (e.g. ChildIter) that holds a reference to the children slice and an index, implement Iterator for it to yield HtmlNode items, and return LayoutIterator(ChildIter) from traversal_children; update the TraversalChildrenIterator type used by LayoutIterator accordingly so no intermediate Vec is created.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/csscascade/examples/resolve_and_print.rs`:
- Around line 101-104: The truncate function can panic when max splits a
multi-byte UTF‑8 character; update truncate (the fn truncate(s: &str, max:
usize) -> &str) to return a valid UTF‑8 slice by finding the previous character
boundary (e.g., using s.char_indices() or
s[..].char_indices().take_while(|(i,_)| *i < max).last() or similar) and slicing
at that boundary, returning s if len() <= max or the slice up to the last valid
char boundary before max.
- Around line 106-109: The unsafe transmute in all_longhand_ids() assumes
LonghandId is repr(u16) with contiguous 0..LONGHANDS discriminants; add a clear
SAFETY comment above all_longhand_ids() documenting that invariant and why it's
required, and replace or guard the transmute with a validation step before
yielding values (e.g., assert/checked check that mem::size_of::<LonghandId>() ==
2 and that the highest discriminant + 1 ==
properties::property_counts::LONGHANDS or convert via a safe from_u16/try_from
approach), then only use unsafe transmute after that validation; apply the same
SAFETY comment and validation to the identical pattern found in the LonghandId
usage in tree::mod.rs (the same transmute ranges).
In `@crates/csscascade/README.md`:
- Around line 52-59: README Phase 1 status is inconsistent with the code: while
TElement scaffolding exists in adapter.rs, the cascade resolution pipeline isn’t
integrated because StyleRuntime::compute_for() is still returning defaults (see
TODO in tree::mod.rs). Update the Phase 1 checklist to reflect that only the
TElement adapter has been promoted (leave items that require actual cascade
resolution unchecked or move them to Phase 2) and clarify in the
"Proof-of-concept" / "Stubbed / not yet functional" sections that full
per-element resolution is pending integration with StyleRuntime::compute_for()
and the TODO in tree::mod.rs.
In `@crates/csscascade/src/adapter.rs`:
- Around line 608-610: The current is_html_document_body_element method always
returns false; update it to detect HTML <body> elements by first verifying
self.is_html_element() and then checking the element's local name via
self.element_data().name.local (case-insensitively) equals "body" so it returns
true for a body element and false otherwise; ensure you use
ASCII-case-insensitive comparison (e.g., eq_ignore_ascii_case) on the local
name.
- Around line 774-776: The is_root() method incorrectly treats nodes with no
parent as :root; update the logic in is_root() (the method that calls
as_node().parent_node()) to return true when the node's parent exists and is the
document node (not when parent_node().is_none()). Concretely, get parent =
self.as_node().parent_node(), and return parent.map_or(false, |p| p.node_type()
== web_sys::Node::DOCUMENT_NODE) (or use the equivalent is_document check
available), so <html> with a document parent matches :root.
In `@crates/csscascade/src/dom.rs`:
- Around line 378-391: The inline style parsing currently hardcodes
StyleQuirksMode::NoQuirks inside the closure that maps style_value (see the
style_value.map block and the parse_style_attribute call); change this to query
and pass the document's actual quirks mode instead of NoQuirks — locate where
the DOM/document quirks mode is available (e.g. via the Document or
doc_shared_lock/adapter) and use that value when calling parse_style_attribute
so StyleQuirksMode passed reflects the document rather than being hardcoded.
In `@crates/grida-canvas/Cargo.toml`:
- Line 45: The git dependency declaration stylo = { git =
"https://github.com/gridaco/stylo" } must be pinned to an explicit revision to
ensure reproducible builds: edit each Cargo.toml that contains that exact
dependency entry and add rev = "<resolved_commit_sha>" (using the commit SHA
recorded in Cargo.lock or the desired commit), e.g. stylo = { git = "...", rev =
"abcdef123456..." }; after updating both Cargo.toml entries, regenerate or
update the lockfile (cargo update -p stylo or cargo generate-lockfile) so the
lockfile matches the pinned revision.
In `@crates/grida-canvas/src/html/mod.rs`:
- Around line 366-399: The current logic in the text-decoration block
(style.clone_text_decoration_line(), StyloTextDecorationLine,
TextDecorationLine, TextDecorationRec, node.text_style.text_decoration) only
preserves a single decoration because it uses an if/else-if chain; change it to
detect and combine all matching decoration flags (line-through, underline,
overline) instead of picking the first match and set a combined
TextDecorationLine (or equivalent multi-value/bitflag/Vec) so multiple
decorations like "underline line-through" are preserved; if the IR/type
TextDecorationLine cannot represent multiple values, update the mapping to a
container (or adjust TextDecorationRec) that can hold multiple lines and
populate text_decoration accordingly, or add a clear TODO comment noting the
limitation if changing types is not feasible.
- Around line 271-278: The two if-blocks in mod.rs that check
node.layout_dimensions.layout_target_width/height for is_none() and then assign
None are dead code; remove these redundant checks/assignments (the blocks
referencing node.layout_dimensions.layout_target_width and
node.layout_dimensions.layout_target_height) so the factory-default clearing
logic is not a no-op—if you intended to clear a 100x100 factory default instead,
change the condition to check for Some(_) before setting to None, otherwise
simply delete the entire no-op if statements.
In `@docs/wg/format/css.md`:
- Around line 198-200: The note under "**Unblocks:** `brightness()`,
`contrast()`, `grayscale()`, `sepia()`, `hue-rotate()`, `invert()`,
`saturate()`" contains a sentence fragment ("Could be modeled as a color matrix
or individual filter effect variants."); replace that fragment with a full
sentence such as "These non-blur filters could be modeled as a color matrix or
as individual filter-effect variants." Update the line containing "Could be
modeled as a color matrix or individual filter effect variants." so the wording
is explicitly scoped and grammatically complete.
In `@docs/wg/format/svg.md`:
- Around line 55-66: The Markdown table under the "Stroke" section has an extra
trailing column separator causing a fourth empty column; locate the table block
containing the header "| SVG Attribute | Grida IR Field | Status |" and fix the
row entries for `stroke-dashoffset` and `stroke-opacity` by either removing the
trailing "|" from those rows or adding a fourth header (e.g., "Notes") and
populating the cells appropriately so all rows match the header column count;
ensure `Stroke` table rows (`stroke-dashoffset`, `stroke-opacity`) now have the
same number of pipe-separated columns as the header.
In `@fixtures/test-html/L0/paint-backdrop-blur.html`:
- Around line 53-60: The fixture contains CSS using backdrop-filter (.blur-4 and
.blur-12) which is a known blocked feature but currently sits in the L0
supported bucket; move this test out of the primary L0 set (or mark it
explicitly as non-assertive/known-gap) so it does not imply support—either
relocate the file to an unsupported/known-gaps fixtures folder or add an
annotation/metadata flag in the test manifest or fixture header that marks
backdrop-filter as a known unsupported gap for this runner (reference the
.blur-4 and .blur-12 selectors when updating the manifest or header).
In `@fixtures/test-html/L0/paint-border-style.html`:
- Around line 52-57: The .per-side selector mixes different border-style values
per edge which our IR only preserves from the top edge; update the fixture so
.per-side only varies border-width per side while keeping a uniform border-style
(e.g., all solid) to avoid misleading the L0 baseline, or alternatively add an
explicit comment/annotation in the fixture noting the current IR limitation
(style-loss) so the test expectation reflects that limitation.
In `@fixtures/test-html/L0/paint-gradient-linear.html`:
- Around line 1-93: The project includes manual HTML fixtures that aren’t
exercised by CI; add a new test module (e.g., html_fixtures.rs) in the
grida-canvas tests that programmatically loads those fixture HTML files, passes
each file’s contents into the existing html_layout and html_graph helpers, and
asserts they run without error (or validate expected layout/graph outputs); use
the same parsing helpers used by current inline tests so these fixtures are
executed during cargo test and thus protect against regressions referenced by
the grida-dev reftest CLI.
In `@fixtures/test-html/L0/README.md`:
- Around line 73-74: Update the excluded-features list to remove
`text-transform` because it is implemented and mapped in the HTML→IR conversion;
specifically delete `text-transform` from the list in the README so it no longer
appears as excluded, and ensure the documentation aligns with the implementation
in crates/grida-canvas/src/html/mod.rs where `text-transform` variants
(uppercase/lowercase/capitalize) are handled.
In `@fixtures/test-html/L0/text-decoration-full.html`:
- Line 5: The fixture title claims it covers "Thickness" but there are no
text-decoration-thickness declarations; either add explicit thickness variants
(e.g., add CSS rules using text-decoration-thickness: <length>|<percentage>|auto
and with selectors used in this file such as the classes/IDs already present) so
the stylesheet includes examples, or update the <title> element to remove
"Thickness" so the title matches actual coverage; reference the <title> element
and the CSS property text-decoration-thickness when making the change.
In `@fixtures/test-html/L0/transform.html`:
- Around line 48-59: The L0 HTML fixture contains CSS using transform rules
(.translate, .rotate, .scale, .scale-xy, .skew-x, .skew-y, .matrix, .combined)
but transforms are not wired in this PR, so mark this fixture as a
known-gap/unsupported case: update the fixture metadata or test manifest to flag
this file as unsupported for transform feature (or move it into the
known-gaps/unsupported fixtures set) so it is excluded from support-claims;
ensure the change references this fixture (transform.html) and the transform
class selectors (.translate, .rotate, .combined, etc.) so tests and reports will
not treat it as implemented.
---
Nitpick comments:
In `@crates/csscascade/README.md`:
- Line 77: Rephrase the fragment "visibility: hidden — needs dedicated IR field
(not opacity:0)" to avoid the double-modal; e.g. change it to "visibility:
hidden — requires a dedicated IR field (not opacity: 0)" or "visibility: hidden
— should have its own IR field (not opacity: 0)" to make the intent clearer;
update the README line that currently reads "visibility: hidden — needs
dedicated IR field (not opacity:0)" accordingly.
In `@crates/csscascade/src/adapter.rs`:
- Around line 59-66: Add module-level documentation noting that bootstrap_dom
deliberately leaks the boxed DemoDom by storing a raw pointer in DEMO_DOM to
keep &'static DemoDom references valid, and that callers of from_html_str (and
other APIs that call bootstrap_dom) will therefore leak one DemoDom per call;
mention mitigation strategies such as using a single long-lived DOM for batch
processing, providing an explicit teardown API to replace and free the previous
DOM, or refactoring to avoid &'static references. Reference the functions/types
bootstrap_dom, DEMO_DOM, DemoDom, HtmlDocument, and from_html_str in the doc so
readers can locate the leak and mitigation options.
- Around line 416-425: traversal_children currently builds a temporary Vec every
call which allocates; instead implement a lazy iterator over the
node().node().children slice to avoid allocation. Create a custom iterator type
(e.g. ChildIter) that holds a reference to the children slice and an index,
implement Iterator for it to yield HtmlNode items, and return
LayoutIterator(ChildIter) from traversal_children; update the
TraversalChildrenIterator type used by LayoutIterator accordingly so no
intermediate Vec is created.
In `@crates/csscascade/src/dom.rs`:
- Around line 103-105: Update the unsafe impl comment for DemoDom to accurately
reflect why Send and Sync are safe: mention that element_data uses AtomicRefCell
which provides runtime borrow checking and the necessary Send+Sync
synchronization guarantees (not that the DOM is simply "frozen"), and state that
other interior mutability is protected by AtomicRefCell and any other
concurrency-safe primitives used in DemoDom; reference DemoDom and
element_data/AtomicRefCell in the comment so readers can find the rationale.
In `@crates/grida-canvas/src/html/mod.rs`:
- Line 52: The eprintln!("[html] resolved {} elements", styled_count) in the
html module prints to stderr unconditionally; change this by either removing the
call, gating it behind a Cargo feature (e.g., a "debug" feature) so it only
compiles when enabled, or replace it with a logger call (using the log crate:
e.g., trace!/debug!) to respect runtime log levels; locate the eprintln! in
mod.rs (reference: styled_count and the html module) and implement one of those
options so production builds won't always write to stderr.
- Around line 1133-1148: Add a public doc comment to from_html_str explaining it
is not safe to call concurrently because the module uses a process-global
DEMO_DOM and a global mutex (HTML_TEST_LOCK) to serialize access; mention that
from_html_str internally acquires the lock via lock_html() which recovers from
poison (unwrap_or_else(|e| e.into_inner())) so a panicked run won’t permanently
poison the mutex, and recommend callers serialize calls or run them on one
thread/worker to avoid panics.
In `@docs/wg/format/index.md`:
- Around line 1-7: The frontmatter YAML block at the top of the document is
missing the MDX opt-out; update the frontmatter (the existing YAML block
containing title and tags) to include format: md (e.g., add a new line "format:
md" under the title or alongside tags) so the file is treated as plain Markdown
and MDX parsing is disabled.
In `@fixtures/test-html/L0/layout-block.html`:
- Around line 21-23: Remove the empty CSS rule ".block + .block" which contains
only a comment; locate the rule in the stylesheet (the selector .block + .block)
and delete the entire rule block so no empty selector remains, keeping spacing
handled by parent padding as intended.
In `@fixtures/test-html/L0/text-decoration.html`:
- Around line 22-33: Add an optional test case exercising sub-properties: add a
CSS rule such as `.underline-colored { text-decoration-line: underline;
text-decoration-color: `#60a5fa`; text-decoration-style: wavy; }` to the same
fixture so the parser paths that handle text-decoration-color and
text-decoration-style (see html/mod.rs handling of TextDecorationLine and
related properties) are covered; ensure you use the longhand properties
(`text-decoration-line`, `text-decoration-color`, `text-decoration-style`) and
include the class in the HTML so the test runner picks it up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 530c3101-b2a7-4e79-8422-d593d88a7d13
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (57)
crates/csscascade/README.mdcrates/csscascade/examples/resolve_and_print.rscrates/csscascade/src/adapter.rscrates/csscascade/src/cascade.rscrates/csscascade/src/dom.rscrates/csscascade/src/lib.rscrates/grida-canvas/Cargo.tomlcrates/grida-canvas/src/html/mod.rscrates/grida-canvas/src/lib.rscrates/grida-dev/src/main.rsdocs/AGENTS.mddocs/wg/format/css.mddocs/wg/format/grida.mddocs/wg/format/html.mddocs/wg/format/index.mddocs/wg/format/svg.mdfixtures/test-html/L0/README.mdfixtures/test-html/L0/box-dimensions.htmlfixtures/test-html/L0/box-padding.htmlfixtures/test-html/L0/hello-div.htmlfixtures/test-html/L0/hello.htmlfixtures/test-html/L0/layout-block.htmlfixtures/test-html/L0/layout-display-none.htmlfixtures/test-html/L0/layout-flex-column.htmlfixtures/test-html/L0/layout-flex-row.htmlfixtures/test-html/L0/layout-flex.htmlfixtures/test-html/L0/mixed-card.htmlfixtures/test-html/L0/mixed-inline-style.htmlfixtures/test-html/L0/paint-backdrop-blur.htmlfixtures/test-html/L0/paint-background-solid.htmlfixtures/test-html/L0/paint-blend-mode.htmlfixtures/test-html/L0/paint-border-radius.htmlfixtures/test-html/L0/paint-border-style.htmlfixtures/test-html/L0/paint-filter-blur.htmlfixtures/test-html/L0/paint-gradient-linear.htmlfixtures/test-html/L0/paint-gradient-radial.htmlfixtures/test-html/L0/paint-opacity.htmlfixtures/test-html/L0/paint-shadow.htmlfixtures/test-html/L0/painting-bg-gradient.htmlfixtures/test-html/L0/painting-borders.htmlfixtures/test-html/L0/painting-fg-gradient.htmlfixtures/test-html/L0/structure-div-nested.htmlfixtures/test-html/L0/structure-table.htmlfixtures/test-html/L0/text-align.htmlfixtures/test-html/L0/text-article.htmlfixtures/test-html/L0/text-color.htmlfixtures/test-html/L0/text-decoration-full.htmlfixtures/test-html/L0/text-decoration.htmlfixtures/test-html/L0/text-font-properties.htmlfixtures/test-html/L0/text-inline-formatting.htmlfixtures/test-html/L0/text-letter-spacing.htmlfixtures/test-html/L0/text-line-height.htmlfixtures/test-html/L0/text-shadow.htmlfixtures/test-html/L0/text-typography.htmlfixtures/test-html/L0/transform.htmlformat/AGENTS.mdformat/README.md
💤 Files with no reviewable changes (11)
- fixtures/test-html/L0/hello-div.html
- fixtures/test-html/L0/text-inline-formatting.html
- fixtures/test-html/L0/painting-bg-gradient.html
- fixtures/test-html/L0/structure-table.html
- fixtures/test-html/L0/hello.html
- fixtures/test-html/L0/layout-flex.html
- fixtures/test-html/L0/painting-borders.html
- fixtures/test-html/L0/text-typography.html
- fixtures/test-html/L0/text-article.html
- fixtures/test-html/L0/painting-fg-gradient.html
- fixtures/test-html/L0/structure-div-nested.html
Comment on lines
+101
to
+104
| /// Truncate a string for display. | ||
| fn truncate(s: &str, max: usize) -> &str { | ||
| if s.len() <= max { s } else { &s[..max] } | ||
| } |
There was a problem hiding this comment.
truncate can panic on multi-byte UTF-8 characters.
Slicing &s[..max] will panic if max lands in the middle of a multi-byte UTF-8 character. For a display-only example this is low-risk, but consider using char_indices() for safe truncation.
🛡️ Safe truncation alternative
/// Truncate a string for display.
fn truncate(s: &str, max: usize) -> &str {
- if s.len() <= max { s } else { &s[..max] }
+ if s.len() <= max {
+ s
+ } else {
+ match s.char_indices().nth(max) {
+ Some((idx, _)) => &s[..idx],
+ None => s,
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/csscascade/examples/resolve_and_print.rs` around lines 101 - 104, The
truncate function can panic when max splits a multi-byte UTF‑8 character; update
truncate (the fn truncate(s: &str, max: usize) -> &str) to return a valid UTF‑8
slice by finding the previous character boundary (e.g., using s.char_indices()
or s[..].char_indices().take_while(|(i,_)| *i < max).last() or similar) and
slicing at that boundary, returning s if len() <= max or the slice up to the
last valid char boundary before max.
Comment on lines
+106
to
+109
| /// Iterate over every CSS longhand property ID. | ||
| fn all_longhand_ids() -> impl Iterator<Item = LonghandId> { | ||
| (0..properties::property_counts::LONGHANDS as u16).map(|idx| unsafe { mem::transmute(idx) }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Stylo provides a safe LonghandId iterator
echo "=== Looking for LonghandId iteration patterns in Stylo ==="
rg -n "LonghandId" crates/csscascade/ --type rust -A 2 -B 2 | head -50
echo ""
echo "=== Check LonghandId definition/repr ==="
ast-grep --pattern 'enum LonghandId { $$$ }'Repository: gridaco/grida
Length of output: 1780
🏁 Script executed:
# Search for documentation, comments, or usage patterns of LonghandId
echo "=== Searching for documentation or context around unsafe transmute ==="
rg -n "unsafe.*transmute" crates/csscascade/ --type rust -B 3 -A 3
echo ""
echo "=== Looking for any safe LonghandId iteration methods ==="
rg -n "LonghandId" crates/csscascade/ --type rust | grep -i "iter\|all\|each\|for_each"
echo ""
echo "=== Check for Cargo.toml to understand Stylo dependency ==="
fd "Cargo.toml" | xargs grep -l "style" | head -5
echo ""
echo "=== Examining Cargo.lock or Cargo.toml for style crate info ==="
fd "Cargo.toml" crates/csscascade/ -exec cat {} \;Repository: gridaco/grida
Length of output: 1850
🏁 Script executed:
# Read Cargo.toml for csscascade
echo "=== crates/csscascade/Cargo.toml ==="
cat crates/csscascade/Cargo.toml
echo ""
echo "=== Check for SAFETY comments around the unsafe block ==="
rg -n "SAFETY|unsafe" crates/csscascade/examples/resolve_and_print.rs --type rust -B 2 -A 2
echo ""
echo "=== Check if mem::transmute is used elsewhere and how ==="
rg -n "transmute\|mem::" crates/csscascade/ --type rust | head -20Repository: gridaco/grida
Length of output: 1896
🏁 Script executed:
# Search for any imports or uses of style properties module
echo "=== Check imports and usage context ==="
rg -n "use style::properties" crates/csscascade/ --type rust -A 5 | head -30
echo ""
echo "=== Search for any comments mentioning LonghandId or transmute patterns ==="
rg -n "LonghandId|transmute" crates/csscascade/ --type rust -B 5 | grep -E "//|/\*" | head -20
echo ""
echo "=== Check if properties module exposes any iteration helpers ==="
rg -n "properties::" crates/csscascade/ --type rust | grep -E "iter|all|each" | head -10Repository: gridaco/grida
Length of output: 2637
Add SAFETY comment and validation for unsafe transmute of LonghandId.
The unsafe transmute assumes LonghandId is #[repr(u16)] with contiguous discriminants from 0 to LONGHANDS. This invariant is undocumented and unvalidated, creating maintenance risk if the Stylo fork changes the enum representation.
Suggested fix
/// Iterate over every CSS longhand property ID.
fn all_longhand_ids() -> impl Iterator<Item = LonghandId> {
+ // SAFETY: LonghandId is #[repr(u16)] with contiguous discriminants 0..LONGHANDS.
+ // This assumption must be validated if Stylo is upgraded.
+ debug_assert!(
+ properties::property_counts::LONGHANDS <= u16::MAX as usize,
+ "LonghandId count exceeds u16 range"
+ );
(0..properties::property_counts::LONGHANDS as u16).map(|idx| unsafe { mem::transmute(idx) })
}Note: The same pattern appears in crates/csscascade/src/tree/mod.rs lines 483-485 and should receive the same treatment.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Iterate over every CSS longhand property ID. | |
| fn all_longhand_ids() -> impl Iterator<Item = LonghandId> { | |
| (0..properties::property_counts::LONGHANDS as u16).map(|idx| unsafe { mem::transmute(idx) }) | |
| } | |
| /// Iterate over every CSS longhand property ID. | |
| fn all_longhand_ids() -> impl Iterator<Item = LonghandId> { | |
| // SAFETY: LonghandId is #[repr(u16)] with contiguous discriminants 0..LONGHANDS. | |
| // This assumption must be validated if Stylo is upgraded. | |
| debug_assert!( | |
| properties::property_counts::LONGHANDS <= u16::MAX as usize, | |
| "LonghandId count exceeds u16 range" | |
| ); | |
| (0..properties::property_counts::LONGHANDS as u16).map(|idx| unsafe { mem::transmute(idx) }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/csscascade/examples/resolve_and_print.rs` around lines 106 - 109, The
unsafe transmute in all_longhand_ids() assumes LonghandId is repr(u16) with
contiguous 0..LONGHANDS discriminants; add a clear SAFETY comment above
all_longhand_ids() documenting that invariant and why it's required, and replace
or guard the transmute with a validation step before yielding values (e.g.,
assert/checked check that mem::size_of::<LonghandId>() == 2 and that the highest
discriminant + 1 == properties::property_counts::LONGHANDS or convert via a safe
from_u16/try_from approach), then only use unsafe transmute after that
validation; apply the same SAFETY comment and validation to the identical
pattern found in the LonghandId usage in tree::mod.rs (the same transmute
ranges).
Comment on lines
+52
to
+59
| ### Phase 1 — Cascade Resolution ✅ | ||
|
|
||
| ### (planned) Optional layout integration | ||
| Per-element style resolution via Stylo's `TElement` trait. | ||
|
|
||
| A future feature flag will allow the crate to output a **layout‑computed, render‑ready tree**, so you can plug it directly into your painter. | ||
| - [x] Promote `TElement` implementation into the crate (`adapter.rs`) | ||
| - [x] Collect CSS from `<style>` elements and inline `style` attributes | ||
| - [x] Inject a user-agent stylesheet (default HTML element styles) | ||
| - [x] Validate against HTML fixture files in `/fixtures/test-html/L0/` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify cascade resolution implementation status
echo "=== Checking StyleRuntime::compute_for implementation ==="
rg -n "fn compute_for" crates/csscascade/src/ -A 5
echo ""
echo "=== Checking if TElement is actually used for cascade ==="
rg -n "TElement" crates/csscascade/src/ --type rust
echo ""
echo "=== Check for stub/TODO comments in cascade path ==="
rg -n -i "stub|todo|not.*implement" crates/csscascade/src/ --type rustRepository: gridaco/grida
Length of output: 1372
🏁 Script executed:
sed -n '40,70p' crates/csscascade/README.mdRepository: gridaco/grida
Length of output: 1264
Reconcile Phase 1 completion status with actual implementation.
Phase 1 is marked complete (✅), but the README itself contradicts this in two places: (1) the "Proof-of-concept" section states TElement "has not been promoted to the main crate API," and (2) the "Stubbed / not yet functional" section confirms StyleRuntime::compute_for() returns defaults instead of resolved styles. The code at tree/mod.rs:527–530 contains a TODO explicitly stating "integrate with actual Stylist lookups per element."
Update Phase 1 sub-items to accurately reflect completion: The adapter.rs TElement implementation exists, but the cascade resolution pipeline doesn't use it yet. Either uncheck the items that depend on compute_for() integration, or move them to Phase 2 and clarify that Phase 1 only covers adapter scaffolding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/csscascade/README.md` around lines 52 - 59, README Phase 1 status is
inconsistent with the code: while TElement scaffolding exists in adapter.rs, the
cascade resolution pipeline isn’t integrated because StyleRuntime::compute_for()
is still returning defaults (see TODO in tree::mod.rs). Update the Phase 1
checklist to reflect that only the TElement adapter has been promoted (leave
items that require actual cascade resolution unchecked or move them to Phase 2)
and clarify in the "Proof-of-concept" / "Stubbed / not yet functional" sections
that full per-element resolution is pending integration with
StyleRuntime::compute_for() and the TODO in tree::mod.rs.
Comment on lines
+608
to
+610
| fn is_html_document_body_element(&self) -> bool { | ||
| false | ||
| } |
There was a problem hiding this comment.
is_html_document_body_element always returns false.
This method should return true for the <body> element when it's a child of an HTML document's <html> element. Returning false may affect certain CSS cascade behaviors (e.g., background propagation from body to viewport).
For completeness, consider implementing this check:
fn is_html_document_body_element(&self) -> bool {
if !self.is_html_element() {
return false;
}
let local = &self.element_data().name.local;
local.as_ref().eq_ignore_ascii_case("body")
}This is minor since the current HTML import likely doesn't rely on viewport background propagation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/csscascade/src/adapter.rs` around lines 608 - 610, The current
is_html_document_body_element method always returns false; update it to detect
HTML <body> elements by first verifying self.is_html_element() and then checking
the element's local name via self.element_data().name.local (case-insensitively)
equals "body" so it returns true for a body element and false otherwise; ensure
you use ASCII-case-insensitive comparison (e.g., eq_ignore_ascii_case) on the
local name.
Comment on lines
+774
to
+776
| fn is_root(&self) -> bool { | ||
| self.as_node().parent_node().is_none() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for :root usage in test fixtures to verify this affects real CSS
rg -l ':root' --type=html || echo "No :root selectors in fixtures"
# Check if there are any tests that verify :root matching
rg -n 'is_root\|:root' crates/csscascade/Repository: gridaco/grida
Length of output: 87
🏁 Script executed:
#!/bin/bash
# First, let's examine the adapter.rs file around the mentioned lines
echo "=== Examining adapter.rs at lines 774-776 ==="
sed -n '770,785p' crates/csscascade/src/adapter.rs
echo ""
echo "=== Checking context around is_root() implementation ==="
rg -B 5 -A 10 'fn is_root' crates/csscascade/src/adapter.rsRepository: gridaco/grida
Length of output: 1031
🏁 Script executed:
#!/bin/bash
# Search for parent_node() definition and usage
echo "=== Searching for parent_node() definition ==="
rg -n 'parent_node\(' crates/csscascade/src/adapter.rs | head -20
echo ""
echo "=== Searching for as_document() method ==="
rg -n 'as_document' crates/csscascade/src/adapter.rsRepository: gridaco/grida
Length of output: 367
🏁 Script executed:
#!/bin/bash
# Look for imports to understand the DOM model being used
echo "=== Checking imports in adapter.rs ==="
head -50 crates/csscascade/src/adapter.rs
echo ""
echo "=== Looking for Node/Document type definitions ==="
rg -n 'struct Node|impl.*Node|trait.*Node' crates/csscascade/src/adapter.rs | head -20Repository: gridaco/grida
Length of output: 2397
🏁 Script executed:
#!/bin/bash
# Examine the parent_node() implementation
echo "=== parent_node() implementation ==="
sed -n '304,320p' crates/csscascade/src/adapter.rs
echo ""
echo "=== as_document() implementation ==="
sed -n '348,360p' crates/csscascade/src/adapter.rs
echo ""
echo "=== Look at the full NodeInfo/TNode trait implementations ==="
sed -n '285,350p' crates/csscascade/src/adapter.rsRepository: gridaco/grida
Length of output: 2765
🏁 Script executed:
#!/bin/bash
# Look at DemoNode and NodeId structure to understand the DOM model
echo "=== Checking crates/dom/src/lib.rs for node structure ==="
fd -t f 'lib.rs|mod.rs' crates/dom/src/
echo ""
echo "=== Examining the DemoNode structure ==="
rg -B 2 -A 15 'pub struct DemoNode' crates/dom/src/Repository: gridaco/grida
Length of output: 332
🏁 Script executed:
#!/bin/bash
# Search for how nodes are created and what represents document vs element
echo "=== Looking for document node creation ==="
rg -n 'Document\|Root' crates/dom/src/ | head -30
echo ""
echo "=== Check if there are any tests using is_root ==="
rg -rn 'is_root\|:root' crates/ --type rust | head -20Repository: gridaco/grida
Length of output: 224
🏁 Script executed:
#!/bin/bash
# Find where DemoNode and DemoNodeData are defined
echo "=== Finding DemoNode definition ==="
fd -t f '*.rs' crates/ -x rg -l 'struct DemoNode|enum DemoNodeData' {}
echo ""
echo "=== Looking at the import statement in adapter.rs ==="
rg 'use crate::dom' crates/csscascade/src/adapter.rs
echo ""
echo "=== Find the dom module ==="
fd -t f 'mod.rs' crates/ | head -10Repository: gridaco/grida
Length of output: 958
🏁 Script executed:
#!/bin/bash
# Find and examine the dom module
echo "=== Listing crates directory structure ==="
ls -la crates/ | head -30
echo ""
echo "=== Finding all files in csscascade crate ==="
find crates/csscascade -name '*.rs' | head -20Repository: gridaco/grida
Length of output: 1275
🏁 Script executed:
#!/bin/bash
# Examine the dom.rs file
echo "=== dom.rs file size and structure ==="
wc -l crates/csscascade/src/dom.rs
echo ""
echo "=== Looking for DemoNode and DemoNodeData ==="
sed -n '1,100p' crates/csscascade/src/dom.rs
echo ""
echo "=== Searching for DemoNodeData enum definition ==="
rg -B 2 -A 20 'enum DemoNodeData' crates/csscascade/src/dom.rsRepository: gridaco/grida
Length of output: 3883
🏁 Script executed:
#!/bin/bash
# Look for how the document node is created and used
echo "=== Searching for Document node creation/usage ==="
rg -n 'DemoNodeData::Document' crates/csscascade/src/
echo ""
echo "=== Check what the root element is in the DOM tree ==="
rg -B 3 -A 3 'root\|Root' crates/csscascade/src/dom.rs | head -40Repository: gridaco/grida
Length of output: 394
🏁 Script executed:
#!/bin/bash
# Look at the HtmlNode/HtmlElement structure and where is_root is used
echo "=== Finding where is_root() is called/used ==="
rg -B 2 -A 2 'is_root\(\)' crates/
echo ""
echo "=== Check what trait is_root belongs to ==="
rg -B 20 'fn is_root' crates/csscascade/src/adapter.rs | head -40Repository: gridaco/grida
Length of output: 1122
🏁 Script executed:
#!/bin/bash
# Look for the selectors Element trait to understand is_root contract
echo "=== Checking what method is being implemented ==="
rg -B 5 'fn is_root' crates/csscascade/src/adapter.rs
echo ""
echo "=== Look for how parent() works on HtmlNode ==="
rg -B 3 -A 8 'fn parent\(' crates/csscascade/src/adapter.rsRepository: gridaco/grida
Length of output: 605
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or test about root element matching
echo "=== Looking for test files in csscascade ==="
find crates/csscascade -name '*.rs' -exec grep -l 'test\|#\[' {} \;
echo ""
echo "=== Check main.rs or examples for usage ==="
grep -A 20 'is_root' crates/csscascade/src/main.rs crates/csscascade/examples/*.rs 2>/dev/null || echo "No is_root usage in examples"Repository: gridaco/grida
Length of output: 2271
🏁 Script executed:
#!/bin/bash
# Verify what trait is_root() belongs to
echo "=== Find trait implementation for is_root ==="
rg -B 30 'fn is_root' crates/csscascade/src/adapter.rs | grep -E 'impl.*HtmlElement|trait' | head -10
echo ""
echo "=== Look at the full impl block ==="
sed -n '700,780p' crates/csscascade/src/adapter.rs | head -50Repository: gridaco/grida
Length of output: 1461
🏁 Script executed:
#!/bin/bash
# Check if is_root can be called on elements only or also on documents
echo "=== Verify HtmlElement vs HtmlNode ==="
rg -B 2 'struct HtmlElement|struct HtmlNode' crates/csscascade/src/adapter.rs
echo ""
echo "=== See if is_root is for Element trait ==="
rg -B 50 'fn is_root' crates/csscascade/src/adapter.rs | grep 'impl.*for HtmlElement' -A 50 | head -60Repository: gridaco/grida
Length of output: 305
🏁 Script executed:
#!/bin/bash
# Get more context around is_root implementation
echo "=== Get the impl block containing is_root ==="
sed -n '500,780p' crates/csscascade/src/adapter.rs | grep -B 50 'fn is_root' | head -80Repository: gridaco/grida
Length of output: 1235
🏁 Script executed:
#!/bin/bash
# Look at exactly which struct this is implemented for
echo "=== Find the exact impl line ==="
rg -n '^impl.*HtmlElement' crates/csscascade/src/adapter.rs
echo ""
echo "=== Show lines around is_root more carefully ==="
sed -n '750,810p' crates/csscascade/src/adapter.rsRepository: gridaco/grida
Length of output: 1825
🏁 Script executed:
#!/bin/bash
# Check what selectors::Element trait defines
echo "=== Look for the trait definition being implemented ==="
rg 'impl.*Element.*for HtmlElement' crates/csscascade/src/adapter.rs -A 2
echo ""
echo "=== Show the context of the impl block ==="
sed -n '550,775p' crates/csscascade/src/adapter.rs | tail -50Repository: gridaco/grida
Length of output: 1533
🏁 Script executed:
#!/bin/bash
# Verify the DOM structure and how parent works
echo "=== Check parent field in DemoNode ==="
sed -n '40,55p' crates/csscascade/src/dom.rs
echo ""
echo "=== Verify as_document implementation again ==="
sed -n '348,352p' crates/csscascade/src/adapter.rs
echo ""
echo "=== Test if there are any tests that parse HTML with :root ==="
rg ':root' crates/csscascade/ --type=css --type=htmlRepository: gridaco/grida
Length of output: 708
🏁 Script executed:
#!/bin/bash
# Create a simple test to understand the DOM structure
echo "=== Check examples to understand DOM structure ==="
head -100 crates/csscascade/examples/print_tree.rsRepository: gridaco/grida
Length of output: 693
🏁 Script executed:
#!/bin/bash
# Let's verify the DOM structure by checking how nodes are parsed
echo "=== Check TreeSink implementation to see parent assignment ==="
rg -B 3 -A 10 'fn append_parent' crates/csscascade/src/dom.rs
echo ""
echo "=== Look at how nodes are created during parsing ==="
rg -B 2 -A 5 'DemoNode {' crates/csscascade/src/dom.rs | head -30Repository: gridaco/grida
Length of output: 274
🏁 Script executed:
#!/bin/bash
# Check the HTML parsing flow to understand document structure
echo "=== Look for where document_id is set ==="
rg -n 'document_id\|self.nodes\[0\]' crates/csscascade/src/dom.rs | head -20
echo ""
echo "=== Check the TreeSink implementation ==="
sed -n '200,300p' crates/csscascade/src/dom.rsRepository: gridaco/grida
Length of output: 3281
🏁 Script executed:
#!/bin/bash
# Verify as_document method and complete DOM understanding
echo "=== Full context of how document and elements work ==="
sed -n '285,360p' crates/csscascade/src/adapter.rs
echo ""
echo "=== Verify the semantics by looking at parent_node on HtmlNode ==="
sed -n '299,330p' crates/csscascade/src/adapter.rsRepository: gridaco/grida
Length of output: 2957
🏁 Script executed:
#!/bin/bash
# Trace through what happens with a real element
echo "=== Check if there's any documentation or comment about root ==="
rg -B 3 -A 3 'root\|Root' crates/csscascade/src/adapter.rs | head -40
echo ""
echo "=== Verify that Document nodes are distinct from Element nodes ==="
rg 'DemoNodeData::Document\|DemoNodeData::Element' crates/csscascade/src/adapter.rsRepository: gridaco/grida
Length of output: 189
🏁 Script executed:
#!/bin/bash
# Understand the full picture - let's trace through an actual element
echo "=== Get all parent() related code ==="
rg -B 2 -A 5 'fn parent\(' crates/csscascade/src/adapter.rs
echo ""
echo "=== Verify parent field in DemoNode is Option<NodeId> ==="
sed -n '40,65p' crates/csscascade/src/dom.rs
echo ""
echo "=== Understand how HTML is structured: document > html > body ==="
rg -B 3 -A 3 'append_to_root\|root\(' crates/csscascade/src/dom.rs | head -30Repository: gridaco/grida
Length of output: 942
🏁 Script executed:
#!/bin/bash
# Final verification: check the selectors crate documentation or trait definition
echo "=== Look for selectors Element trait definition or comments ==="
rg 'is_root' crates/csscascade/src/adapter.rs -B 50 | grep -E '//|Element|trait' | tail -20
echo ""
echo "=== Verify if any other implementation exists ==="
find crates -name '*.rs' -exec rg -l 'impl.*Element.*for' {} \; | head -10Repository: gridaco/grida
Length of output: 252
is_root() implementation is incorrect for CSS :root selector.
The HTML root element (typically <html>) has a parent—the document node—so the current implementation that checks parent_node().is_none() will return false for the root element, breaking CSS :root selector matching. The root element should be identified as the element whose parent is the document node, not an element with no parent.
🐛 Proposed fix
fn is_root(&self) -> bool {
- self.as_node().parent_node().is_none()
+ // The root element's parent is the document node.
+ self.as_node()
+ .parent_node()
+ .and_then(|p| p.as_document())
+ .is_some()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/csscascade/src/adapter.rs` around lines 774 - 776, The is_root()
method incorrectly treats nodes with no parent as :root; update the logic in
is_root() (the method that calls as_node().parent_node()) to return true when
the node's parent exists and is the document node (not when
parent_node().is_none()). Concretely, get parent = self.as_node().parent_node(),
and return parent.map_or(false, |p| p.node_type() ==
web_sys::Node::DOCUMENT_NODE) (or use the equivalent is_document check
available), so <html> with a document parent matches :root.
Comment on lines
+52
to
+57
| .per-side { | ||
| border-top: 2px solid #6366f1; | ||
| border-right: 4px dashed #22c55e; | ||
| border-bottom: 6px dotted #f59e0b; | ||
| border-left: 8px solid #ec4899; | ||
| } |
There was a problem hiding this comment.
Per-side border-style case exceeds current IR fidelity.
per-side mixes style variants per edge, but current mapping uses only top border style for dash behavior. This case is likely misleading as an L0 baseline. Consider changing this fixture to per-side width-only differences (with uniform style), or annotate the style-loss limitation explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fixtures/test-html/L0/paint-border-style.html` around lines 52 - 57, The
.per-side selector mixes different border-style values per edge which our IR
only preserves from the top edge; update the fixture so .per-side only varies
border-width per side while keeping a uniform border-style (e.g., all solid) to
avoid misleading the L0 baseline, or alternatively add an explicit
comment/annotation in the fixture noting the current IR limitation (style-loss)
so the test expectation reflects that limitation.
Comment on lines
+1
to
+93
| <!doctype html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="utf-8" /> | ||
| <title>Paint: Linear Gradient</title> | ||
| <style> | ||
| body { | ||
| background: #030712; | ||
| color: #e2e8f0; | ||
| font-family: system-ui, sans-serif; | ||
| font-size: 14px; | ||
| padding: 24px; | ||
| } | ||
|
|
||
| .label { | ||
| font-size: 11px; | ||
| color: #94a3b8; | ||
| padding-bottom: 6px; | ||
| } | ||
|
|
||
| .grid { | ||
| display: flex; | ||
| flex-direction: row; | ||
| flex-wrap: wrap; | ||
| gap: 16px; | ||
| } | ||
|
|
||
| .rect { | ||
| width: 180px; | ||
| height: 120px; | ||
| border-radius: 8px; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| font-size: 12px; | ||
| color: #fff; | ||
| } | ||
|
|
||
| .to-right { | ||
| background-image: linear-gradient(to right, #6366f1, #ec4899); | ||
| } | ||
| .to-bottom { | ||
| background-image: linear-gradient(to bottom, #22c55e, #0ea5e9); | ||
| } | ||
| .angle135 { | ||
| background-image: linear-gradient(135deg, #f59e0b, #ef4444); | ||
| } | ||
| .to-bl { | ||
| background-image: linear-gradient(to bottom left, #8b5cf6, #06b6d4); | ||
| } | ||
| .multi-stop { | ||
| background-image: linear-gradient( | ||
| to right, | ||
| #ef4444, | ||
| #f59e0b 30%, | ||
| #22c55e 60%, | ||
| #3b82f6 | ||
| ); | ||
| } | ||
| .to-top { | ||
| background-image: linear-gradient(to top, #1e293b, #6366f1); | ||
| } | ||
| </style> | ||
| </head> | ||
| <body> | ||
| <div class="grid"> | ||
| <div> | ||
| <div class="label">to right</div> | ||
| <div class="rect to-right">to right</div> | ||
| </div> | ||
| <div> | ||
| <div class="label">to bottom</div> | ||
| <div class="rect to-bottom">to bottom</div> | ||
| </div> | ||
| <div> | ||
| <div class="label">135deg</div> | ||
| <div class="rect angle135">135deg</div> | ||
| </div> | ||
| <div> | ||
| <div class="label">to bottom left</div> | ||
| <div class="rect to-bl">to bottom left</div> | ||
| </div> | ||
| <div> | ||
| <div class="label">multi-stop (4 colors)</div> | ||
| <div class="rect multi-stop">multi-stop</div> | ||
| </div> | ||
| <div> | ||
| <div class="label">to top</div> | ||
| <div class="rect to-top">to top</div> | ||
| </div> | ||
| </div> | ||
| </body> | ||
| </html> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find references to L0 HTML fixture paths =="
rg -n --hidden -S 'fixtures/test-html/L0|test-html/L0|include_str!\(|read_to_string\('
echo
echo "== Find HTML importer tests =="
rg -n --hidden -S 'mod tests|html_layout\(|html_graph\(|from_html_str\(' crates
echo
echo "== List added L0 fixtures to cross-check coverage =="
fd -t f . fixtures/test-html/L0Repository: gridaco/grida
Length of output: 10369
🏁 Script executed:
# Check if grida-dev has automated test integration
fd -t f Cargo.toml | xargs grep -l "grida-dev" | head -5Repository: gridaco/grida
Length of output: 95
🏁 Script executed:
# Inspect the reftest framework structure
head -100 crates/grida-dev/src/reftest/render.rsRepository: gridaco/grida
Length of output: 3268
🏁 Script executed:
# Check for CI configuration that might run fixtures
git ls-files | grep -E '\.github/workflows|\.gitlab-ci|ci\.yml|Makefile|\.travis'Repository: gridaco/grida
Length of output: 383
🏁 Script executed:
# Check if grida-dev is a test/dev dependency
cat Cargo.toml | grep -A 20 "\[dev-dependencies\]"Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Check what grida-dev is (binary, lib, example)
cat crates/grida-dev/Cargo.toml | head -30Repository: gridaco/grida
Length of output: 785
🏁 Script executed:
# Check the main test workflow to see what it runs
cat .github/workflows/test-crates.yml | head -80Repository: gridaco/grida
Length of output: 1952
🏁 Script executed:
# Look for any HTML fixture references in test configuration
rg -n "fixtures/test-html|reftest|paint-gradient" .github/workflows/Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Check if reftest is invoked anywhere in the codebase
rg -n "reftest" --type rust crates/grida-dev/src/main.rsRepository: gridaco/grida
Length of output: 240
🏁 Script executed:
# Check if any test modules load fixtures
rg -n "fixtures/test-html" crates/grida-canvas/src/html/mod.rsRepository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Look at grida-dev main.rs to see if reftest is a CLI command
sed -n '1,50p' crates/grida-dev/src/main.rsRepository: gridaco/grida
Length of output: 1885
🏁 Script executed:
# Check csscascade README for fixture validation claim
cat crates/csscascade/README.md | grep -A 5 -B 5 "Validate against"Repository: gridaco/grida
Length of output: 534
🏁 Script executed:
# Verify that test workflow only runs cargo test, not fixture-based tests
tail -30 .github/workflows/test-crates.ymlRepository: gridaco/grida
Length of output: 910
🏁 Script executed:
# Double-check if any test module actually loads the paint-gradient-linear.html fixture
rg -n "paint-gradient-linear" --type rust --type tomlRepository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Check csscascade examples to see if they're part of CI
ls -la crates/csscascade/examples/Repository: gridaco/grida
Length of output: 504
🏁 Script executed:
# Final check: are there any test directives or #[test] macros that reference fixtures?
rg -n "#\[test\]" crates/grida-canvas/src/html/mod.rs | head -5Repository: gridaco/grida
Length of output: 141
🏁 Script executed:
# Confirm no fixture loading in the actual test functions
sed -n '1126,1160p' crates/grida-canvas/src/html/mod.rsRepository: gridaco/grida
Length of output: 1528
These L0 fixtures are not consumed by automated tests and won't protect against regressions.
The CI workflow runs cargo test --workspace --exclude grida-canvas-wasm—which excludes the grida-dev binary crate entirely. HTML parser tests in crates/grida-canvas/src/html/mod.rs use inline HTML strings passed to the html_layout and html_graph helper functions, not fixture files. While examples and the grida-dev reftest CLI tool reference fixtures, these are manual/development artifacts, not part of the automated test pipeline.
To protect against regressions, add fixture-loading tests to a test module (e.g., crates/grida-canvas/tests/html_fixtures.rs) that runs in CI, or clarify these fixtures' purpose if they are documentation-only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fixtures/test-html/L0/paint-gradient-linear.html` around lines 1 - 93, The
project includes manual HTML fixtures that aren’t exercised by CI; add a new
test module (e.g., html_fixtures.rs) in the grida-canvas tests that
programmatically loads those fixture HTML files, passes each file’s contents
into the existing html_layout and html_graph helpers, and asserts they run
without error (or validate expected layout/graph outputs); use the same parsing
helpers used by current inline tests so these fixtures are executed during cargo
test and thus protect against regressions referenced by the grida-dev reftest
CLI.
Comment on lines
+48
to
+59
| /* Individual transforms */ | ||
| .translate { transform: translate(20px, 10px); } | ||
| .rotate { transform: rotate(45deg); } | ||
| .scale { transform: scale(1.5); } | ||
| .scale-xy { transform: scale(0.5, 1.5); } | ||
| .skew-x { transform: skewX(15deg); } | ||
| .skew-y { transform: skewY(15deg); } | ||
| .matrix { transform: matrix(0.866, 0.5, -0.5, 0.866, 10, 20); } | ||
|
|
||
| /* Combined transforms */ | ||
| .combined { transform: translate(10px, 10px) rotate(30deg) scale(0.8); } | ||
| </style> |
There was a problem hiding this comment.
Transform fixture currently targets an unmapped feature path.
Since transforms are listed as not wired in this PR, this L0 fixture should be marked as a known-gap/unsupported fixture (or excluded from support claims) to avoid signaling completed support.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fixtures/test-html/L0/transform.html` around lines 48 - 59, The L0 HTML
fixture contains CSS using transform rules (.translate, .rotate, .scale,
.scale-xy, .skew-x, .skew-y, .matrix, .combined) but transforms are not wired in
this PR, so mark this fixture as a known-gap/unsupported case: update the
fixture metadata or test manifest to flag this file as unsupported for transform
feature (or move it into the known-gaps/unsupported fixtures set) so it is
excluded from support-claims; ensure the change references this fixture
(transform.html) and the transform class selectors (.translate, .rotate,
.combined, etc.) so tests and reports will not treat it as implemented.
- Remove dead no-op code (is_none → assign None) in emit_container - Remove unconditional eprintln in from_html_str (library code) - Add thread safety warning to from_html_str doc comment - Document text-decoration single-line limitation with TODO - Fix index.md missing format:md frontmatter - Fix svg.md stroke table missing Notes header column - Fix fixture title claiming "Thickness" coverage - Remove text-shadow/text-transform from README excluded list Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/grida-canvas/src/html/mod.rs (1)
41-65: Unused variablestyled_count— consider removing or using it.The return value from
driver.style_document(document)on line 57 is assigned tostyled_countbut never used. If this count isn't needed for diagnostics or logging, remove the binding to silence warnings and clarify intent.// 4. Flush stylist + resolve all styles driver.flush(document); - let styled_count = driver.style_document(document); + let _ = driver.style_document(document); // 5. Build scene graph from styled DOM🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/html/mod.rs` around lines 41 - 65, In from_html_str, the result of driver.style_document(document) is assigned to styled_count but never used; remove the unused binding by discarding the value (e.g., call driver.style_document(document) with let _ = driver.style_document(document) or simply driver.style_document(document);) or, if you intend to keep the count, use styled_count for logging/diagnostics—refer to the functions driver.style_document and the from_html_str function when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/grida-canvas/src/html/mod.rs`:
- Around line 41-65: In from_html_str, the result of
driver.style_document(document) is assigned to styled_count but never used;
remove the unused binding by discarding the value (e.g., call
driver.style_document(document) with let _ = driver.style_document(document) or
simply driver.style_document(document);) or, if you intend to keep the count,
use styled_count for logging/diagnostics—refer to the functions
driver.style_document and the from_html_str function when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c726b635-0dd4-4dab-931f-72327f140ba8
📒 Files selected for processing (5)
crates/grida-canvas/src/html/mod.rsdocs/wg/format/index.mddocs/wg/format/svg.mdfixtures/test-html/L0/README.mdfixtures/test-html/L0/text-decoration-full.html
✅ Files skipped from review due to trivial changes (2)
- fixtures/test-html/L0/text-decoration-full.html
- docs/wg/format/index.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/wg/format/svg.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First integration of HTML/CSS import into the Grida canvas engine. Parses HTML via Stylo CSS cascade, converts the styled DOM tree into Grida IR nodes (containers, rectangles, text spans), and runs the Taffy-based layout engine for computed positions.
csscascade— embeds Stylo for full CSS cascade resolution (selectors, specificity, inheritance, shorthand expansion,!important,var())crates/grida-canvas/src/html/— tree walker + CSS→IR conversion, analogous to the existing SVG import pipelinefixtures/test-html/L0/— each testing a specific CSS property groupdocs/wg/format/— canonical property mapping tables for CSS, HTML, SVG→Grida IR with status and IR gap analysisCSS properties mapped
display(flex/block/none),flex-direction,flex-wrap,align-items,justify-content,flex-grow,gap,position(absolute/relative)width,height,min-*,max-*,padding,overflow→ clipbackground-color,linear-gradient,radial-gradient,conic-gradient,opacity,mix-blend-modeborder-width(per-side),border-color,border-style(solid/dashed/dotted),border-radius(per-corner, elliptical)box-shadow(drop + inset),text-shadow,filter: blur(),filter: drop-shadow(),backdrop-filter: blur()(code ready, blocked by Stylo servo mode)font-size,font-weight,font-family,font-style,color,text-align,line-height,letter-spacing,word-spacing,text-transform,text-decoration(line + color + style)Known limitations
display: blockis approximated as flex column (no true block flow model)visibility: hiddenneeds a dedicated IR field (notopacity: 0)transformnot yet wired (IR field exists, fixture drafted)backdrop-filterparsed but inactive in Stylo servo modetext-decoration-thickness/skip-inkare gecko-only in Stylodocs/wg/format/css.md§ IR Gaps for the full listTest plan
cargo test -p cg --lib -- html::tests— 42 tests passcargo check -p cg -p grida-canvas-wasm -p grida-dev— all crates compile🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Examples
Documentation
Tests / Fixtures