feat(htmlcss): in-tree Skia-backed SVG renderer (htmlcss::svg)#698
Open
softmarshmallow wants to merge 59 commits intomainfrom
Open
feat(htmlcss): in-tree Skia-backed SVG renderer (htmlcss::svg)#698softmarshmallow wants to merge 59 commits intomainfrom
htmlcss::svg)#698softmarshmallow wants to merge 59 commits intomainfrom
Conversation
Adds upstream surveys covering Blink's SVG rendering (clip-path, feImage, feTile, textPath, module structure) plus external-CSS lifecycle, and a companion feat-2d doc for htmlcss-svg. Rewrites the research skill to enforce pure-survey docs: forbid Grida content in research docs, add a pre-save review checklist, drop plan-doc coupling.
softmarshmallow
added
documentation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (42)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds an in-tree SVG renderer under Changes
Sequence Diagram(s)sequenceDiagram
participant Host as Host API
participant Entry as htmlcss::svg Entry
participant Parser as DemoDom Parser
participant Style as Stylesheet
participant Resources as Resources Index
participant Root as Svg Root Painter
participant Container as Container Painter
participant Element as Element Painter
participant Canvas as Skia Canvas
Host->>Entry: render_into(canvas, svg_xml, viewport, images)
Entry->>Parser: parse_dom(svg_xml)
Parser->>Style: Stylesheet::collect(dom, css_loader)
Entry->>Resources: Resources::build(dom, css_loader)
Entry->>Root: paint_root(canvas, dom, viewport, RenderContext)
Root->>Root: compute viewBox→viewport CTM, apply root clip/filter/opacity
Root->>Container: paint_children(canvas, ctx, root_id)
Container->>Container: iterate children, cascade properties
Container->>Element: dispatch to element-specific painter
Element->>Resources: resolve paint-server/filter/mask/clip
Element->>Canvas: draw (fill/stroke/text/image/marker)
Container-->>Root: children painted
Root-->>Entry: Ok / SvgError
Entry-->>Host: return
sequenceDiagram
participant Paint as Container Painter
participant Clip as Clipper
participant Filter as Filter Resolver
participant Mask as Mask Resolver
participant Recorder as PictureRecorder
participant Canvas as Skia Canvas
Paint->>Paint: read transform, opacity, clip-path, filter, mask
Paint->>Clip: apply_clip_path(raw)
Clip->>Canvas: canvas.clip_path(path, Intersect)
Paint->>Filter: resolve_filter_chain(value)
Filter->>Recorder: may record subtree → ImageFilter
Paint->>Mask: resolve mask → MaskInvocation
Mask->>Recorder: paint mask content into layer (DstIn)
Paint->>Canvas: save_layer(alpha) if opacity<1
Paint->>Canvas: concat_matrix(transform)
Paint->>Paint: dispatch element painter
Paint->>Canvas: restore() layers in reverse order
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
softmarshmallow
changed the title
htmlcss::svg)
Strict module structure aligned with Chromium/Blink's `core/svg/`,
`core/layout/svg/`, and `core/paint/` split. Reftest score
unchanged at 89.50% (1679 fixtures, baseline match — zero regression).
Moves:
- paint/pattern_painter.rs -> resources/pattern.rs (pattern is a
resource container; produces Shader output, not a paint operation)
- layout/use_expand.rs -> paint/svg_use_painter.rs (paint logic
belongs in paint/, not layout/)
- paint/basic_shape_clip.rs -> geometry/basic_shape.rs (new
geometry/ module owns shape -> SkPath data)
Renames (Blink-aligned filenames):
- paint/{root,container,shape,image,text,marker,object}_painter.rs
-> paint/svg_*_painter.rs
- paint/paint_state.rs -> paint/scoped_svg_paint_state.rs
- paint/clip.rs -> paint/clip_path_clipper.rs
- resources/container.rs -> resources/svg_resources.rs
- resources/filter_effect.rs -> resources/svg_filter_builder.rs
New files:
- error.rs (extracted SvgError from mod.rs)
- geometry/mod.rs + basic_shape.rs (phase-neutral geometry module)
- layout/layout_svg_element.rs (bridge type for future migration)
- paint/svg_painter.rs (uniform SvgPainter trait contract)
- resources/svg_resource_container.rs (uniform resource trait)
- resources/cache.rs (per-client cache scaffolding)
- style/stylo_bridge.rs (placeholder for Stylo migration)
- tests/htmlcss_svg_architecture.rs (CI rule enforcement: no Skia
operation types in dom/style/geometry/layout, no Canvas argument
in resources/, phase dependency direction)
Also cleans up 15 pre-existing clippy warnings (doc_lazy_continuation,
single_match, manual_strip, etc.) so the commit hook passes.
Snapshot of pre-refactor state: backup/svg-pre-refactor
See docs/wg/research/chromium/svg/module-structure.md for the
Blink layout this mirrors.
…rop dead filter::apply Continues the Blink-aligned reorganization with the items previously flagged as regression risk. Reftest score unchanged at 89.50% (1679 fixtures, exact baseline match). dom/element.rs split into three: - dom/element.rs: keeps ElementKind, get_attr, element_kind (pure DOM) - paint/visibility.rs: is_painted, has_display_none, is_visible_self, is_visible_inherited (paint dispatch concerns) - style/cascade.rs: cascade_property, get_attr_or_style (single funnel for property resolution; future Stylo migration target) Updates the three callers (svg_root_painter, svg_container_painter, svg_shape_painter) to import from the new locations. paint/svg_text_painter.rs (2,468 LOC) split into a directory: - svg_text_painter/mod.rs: orchestrator, attribute flattening, layout - svg_text_painter/shaping.rs (~190 LOC): SkShaper integration, ShapedGlyph, compute_kerned_advances, shape_text Further text submodules (textPath, layout algorithm) deferred — their cross-dependencies risk regression for marginal gain. Removes resources/filter.rs::apply (dead code; the painter applies filters inline at svg_container_painter.rs). Eliminates the only resources/ Canvas violation, removing two allowlist entries from tests/htmlcss_svg_architecture.rs. README module map updated.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de1265b890
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+348
to
+350
| let needle = format!("{}=", name); | ||
| let start = tag.find(&needle)? + needle.len(); | ||
| let rest = &tag[start..]; |
There was a problem hiding this comment.
The new SVG embed sizing helper looks up width/height with a raw substring search (find("width=")), which can match unrelated attributes like stroke-width on the root <svg> and return the wrong intrinsic size. In that case imported SVGs get an incorrect node size (often tiny), so the embed path renders at the wrong scale. Please parse attributes with boundary-aware matching (or reuse an XML parser) so only exact width/height attributes are considered.
Useful? React with 👍 / 👎.
Comment on lines
+36
to
+40
| let mut anc = node.parent; | ||
| while let Some(id) = anc { | ||
| if id == target_id { | ||
| return; | ||
| } |
There was a problem hiding this comment.
<use> cycles before recursing
The cycle guard only checks target_id against the current node and its ancestors, so mutual references between siblings (e.g. #a -> #b -> #a) still recurse until MAX_USE_DEPTH. That paints intermediate content multiple times and can significantly skew output/perf on cyclic fixtures instead of stopping at the first detected loop. Track visited <use>/target ids in the recursion chain so any repeated target aborts immediately.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/grida_dev/src/reftest/args.rs (1)
21-31:⚠️ Potential issue | 🟡 MinorHelp text still names the removed
grida::htmlcss::render_svgentrypoint.This backend now routes through the new
htmlcss::svgrenderer, so these comments are stale and point readers at an API this PR is explicitly retiring. Please update the wording to the current entrypoint (render_to_picture*or the reftest helper) so future debugging starts in the right place.Also applies to: 84-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida_dev/src/reftest/args.rs` around lines 21 - 31, Update the stale help text that references the removed entrypoint grida::htmlcss::render_svg to instead mention the current htmlcss::svg renderer entrypoint(s) such as render_to_picture (or render_to_picture_sync/async if applicable) or the reftest helper used in this PR; locate the text blocks describing the Htmlcss backend (references to "render_svg" in the help/comments around the Htmlcss choice) and replace the old API name and description with a brief note pointing users to htmlcss::svg's render_to_picture* entrypoint or the reftest helper so readers are directed to the correct code path.docs/wg/research/chromium/svg/index.md (1)
25-45:⚠️ Potential issue | 🟡 MinorAdd the frontmatter fields required for maintained Markdown docs.
This is a meaningful edit to an actively maintained research index, but the page still has no
format: md,description, orkeywordsin frontmatter. Please add them as part of this update. As per coding guidelines,docs/{wg,reference,editor,forms,platform,with-figma,design,math}/**/*.{md,mdx}should add SEO frontmatter withtitle,description, andkeywordswhen meaningfully editing actively maintained doc pages, anddocs/**/*.mdshould addformat: mdfor plain Markdown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/wg/research/chromium/svg/index.md` around lines 25 - 45, Add YAML frontmatter at the top of index.md including: format: "md", title: a concise page title (e.g., "SVG Research Index"), description: one-sentence summary of the page, and keywords: a short array of relevant terms (e.g., ["SVG","Chromium","rendering","layout"]); ensure the frontmatter is valid YAML (--- delimited) and placed before the existing table so the page conforms to the docs frontmatter policy.crates/grida/src/htmlcss/paint.rs (1)
800-816:⚠️ Potential issue | 🟠 MajorInline
<svg>failure still falls back to placeholder renderingWhen
content.svg_xmlexists andpaint_inline_svgfails, control falls through and draws the generic placeholder. That contradicts the “no fallback” intent and makes inline SVG failures look like<img>missing-resource failures.Suggested control-flow fix
- let svg_handled = if let Some(ref xml) = content.svg_xml { - paint_inline_svg(canvas, xml.as_bytes(), w, h, images) - } else { - false - }; - - if svg_handled { + if let Some(ref xml) = content.svg_xml { + let _ = paint_inline_svg(canvas, xml.as_bytes(), w, h, images); canvas.restore(); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/paint.rs` around lines 800 - 816, The current flow lets inline SVG failures fall back to the generic image placeholder: when content.svg_xml is Some and paint_inline_svg(...) returns false we must still treat that as terminal. Update the control flow around svg_handled/paint_inline_svg so that if content.svg_xml.is_some() and paint_inline_svg(...) returns false you restore the canvas and return early (no fallback to images.get or placeholder). Locate the logic using svg_handled, content.svg_xml, paint_inline_svg, canvas.restore, and images.get (or image_opt) and change it so any inline-SVG path that fails exits immediately instead of letting execution fall through.
🟠 Major comments (28)
crates/grida/src/htmlcss/svg/paint/svg_text_painter/shaping.rs-157-168 (1)
157-168:⚠️ Potential issue | 🟠 MajorDo not borrow the next cluster's advance for zero-glyph characters.
position(|g| g.cluster >= start_byte)makes a combining mark or other char with no glyph atstart_byteinherit the next character's advance. That breaks the zero-advance behavior described on Lines 121-123 and shifts per-character positioning forward.🩹 Proposed fix
- let start_glyph = shaped.iter().position(|g| g.cluster >= start_byte); + let start_glyph = shaped.iter().position(|g| g.cluster == start_byte); let end_glyph = shaped.iter().position(|g| g.cluster >= end_byte);crates/grida/src/htmlcss/svg/paint/svg_text_painter/shaping.rs-194-199 (1)
194-199:⚠️ Potential issue | 🟠 MajorStop forcing every shaped run to LTR.
This always passes
trueforleft_to_right, so RTL text will shape and cluster in the wrong direction. Arabic/Hebrew SVG text needs direction to come from the caller or a bidi pass.🩹 Proposed fix
-pub(super) fn shape_text(text: &str, font: &Font) -> (Vec<ShapedGlyph>, Point) { +pub(super) fn shape_text( + text: &str, + font: &Font, + left_to_right: bool, +) -> (Vec<ShapedGlyph>, Point) { if text.is_empty() { return (Vec::new(), Point::new(0.0, 0.0)); } @@ shaper.shape( text, font, - /* left_to_right */ true, + left_to_right, f32::INFINITY, &mut collector, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/paint/svg_text_painter/shaping.rs` around lines 194 - 199, The code currently forces left-to-right shaping by calling shaper.shape(..., /* left_to_right */ true), which breaks RTL scripts; change the call site in shaping.rs to pass the actual direction instead of a hardcoded true—either accept a boolean or an enum direction parameter from the caller (e.g., a new left_to_right: bool or text_direction argument on the surrounding function) or run a bidi pass and derive the direction there, then forward that value into shaper.shape; update the function signature that calls shaper.shape (and its callers) to propagate the real direction so Arabic/Hebrew text clusters correctly.crates/grida/src/htmlcss/svg/dom/attrs.rs-239-283 (1)
239-283:⚠️ Potential issue | 🟠 MajorReject malformed transform argument lists instead of defaulting them.
filter_mapdrops bad tokens, and thetranslate/scale/rotatebranches then fill missing args with defaults. That turns invalid input liketranslate(foo)orrotate(45, 10)into a different valid matrix instead of returningNone, which contradicts the contract on Lines 4-6.🩹 Proposed fix
- let nums: Vec<f32> = s[args_start..i] - .split(|c: char| c.is_ascii_whitespace() || c == ',') - .filter(|p| !p.is_empty()) - .filter_map(|p| p.parse::<f32>().ok()) - .collect(); + let nums: Vec<f32> = s[args_start..i] + .split(|c: char| c.is_ascii_whitespace() || c == ',') + .filter(|p| !p.is_empty()) + .map(|p| p.parse::<f32>()) + .collect::<Result<_, _>>() + .ok()?; @@ - "translate" => { + "translate" if nums.len() == 1 || nums.len() == 2 => { let tx = *nums.first().unwrap_or(&0.0); let ty = *nums.get(1).unwrap_or(&0.0); Matrix::translate((tx, ty)) } - "scale" => { + "scale" if nums.len() == 1 || nums.len() == 2 => { let sx = *nums.first().unwrap_or(&1.0); let sy = *nums.get(1).unwrap_or(&sx); Matrix::scale((sx, sy)) } - "rotate" => { + "rotate" if nums.len() == 1 || nums.len() == 3 => { let a = *nums.first().unwrap_or(&0.0); if nums.len() == 3 { Matrix::rotate_deg_pivot(a, (nums[1], nums[2])) } else { Matrix::rotate_deg(a) } } - "skewX" => { + "skewX" if nums.len() == 1 => { let a = nums.first().copied().unwrap_or(0.0).to_radians(); Matrix::skew((a.tan(), 0.0)) } - "skewY" => { + "skewY" if nums.len() == 1 => { let a = nums.first().copied().unwrap_or(0.0).to_radians(); Matrix::skew((0.0, a.tan())) } _ => return None,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/dom/attrs.rs` around lines 239 - 283, The parser currently drops invalid tokens via filter_map into nums and then silently supplies defaults, so inputs like translate(foo) or rotate(45,10) get coerced into a different valid matrix; instead, change the token collection to fail on any parse error (e.g. collect tokens as Result<Vec<f32>, _> and return None on Err) and validate the argument count per transform exactly: "matrix" must have 6, "translate" must have 1 or 2, "scale" 1 or 2, "rotate" 1 or 3, "skewX"/"skewY" exactly 1; if counts don’t match return None; keep the existing Matrix::... constructors (Matrix::new_identity, Matrix::translate, Matrix::scale, Matrix::rotate_deg/_pivot, Matrix::skew) but only call them after successful parse and count validation.crates/grida_dev/src/main.rs-389-448 (1)
389-448:⚠️ Potential issue | 🟠 MajorEmbed mode drops the source base path for external SVG resources.
Unlike
scene_from_html_embed_path, this path neither preloads referenced assets nor preserves a base directory. Once the SVG is collapsed intonode.html, relative<image href>,<feImage href>, or linked stylesheet URLs no longer have enough context to resolve against the dropped file, so local SVGs with external resources will render with missing content in embed mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida_dev/src/main.rs` around lines 389 - 448, scene_from_svg_embed_path drops the SVG file's base path so relative resources (image href, feImage, linked stylesheets) no longer resolve; fix it by preserving the SVG's base directory and either preloading referenced assets into HtmlEmbedScene.images (same mechanism used by scene_from_html_embed_path) or by injecting a proper HTML base element into node.html pointing to the file:// base (derived from path.parent()), and ensure the logic that discovers and loads resources mirrors the resource-preload code in scene_from_html_embed_path (update scene_from_svg_embed_path, HtmlEmbedScene construction, and NodeFactory/HTMLEmbed creation accordingly).crates/grida_dev/src/main.rs-347-363 (1)
347-363:⚠️ Potential issue | 🟠 MajorDon't coerce arbitrary SVG length units into px.
attr()stops at the first non-numeric character, so values like100%,1em, or10cmbecome concrete pixel sizes here. That bypasses theviewBox/measurement fallback and produces wrong embed dimensions for common responsive SVGs. Only accept unitless or explicitpxlengths in this helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida_dev/src/main.rs` around lines 347 - 363, The helper attr() currently strips after the first non-numeric char which converts values like "100%", "1em", or "10cm" into pixels; change it to only accept unitless numbers or explicit "px" units: after computing numeric_end from raw, extract unit = raw[numeric_end..].trim(); if unit is empty or exactly "px" continue to parse and return the f32 (keeping the existing v.max(1.0) clamp), otherwise return None so non-px units (%, em, cm, etc.) are rejected and the viewBox/measurement fallback can run; keep this logic inside attr() and the existing call site (the if let (Some(w), Some(h)) = (attr(tag, "width"), attr(tag, "height")) { ... } should continue to use the updated attr().crates/grida/tests/htmlcss_svg_architecture.rs-90-120 (1)
90-120:⚠️ Potential issue | 🟠 MajorThis gate is too text-based to enforce the boundary reliably.
content.contains(...)misses common Rust forms likeuse skia_safe::{Canvas, Paint};or aliased imports, and it can also fail on comments/docstrings that only mention a forbidden path. That gives you both false negatives and false positives in a test that is supposed to be authoritative.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/tests/htmlcss_svg_architecture.rs` around lines 90 - 120, The test currently scans file text with content.contains(...) which yields false positives/negatives; replace the substring check in assert_no_imports by parsing each Rust file into a syn::File and visiting its AST to find actual import/use trees and path usages (handle use trees, nested paths, aliased imports, and explicit paths like skia_safe::Canvas) rather than raw text search; implement a small visitor (using syn::visit::Visit) that records any Path or UseTree that begins with a forbidden segment and then call is_allowlisted(&rel, f) for each recorded match (preserve the violations push/format and panic behavior), keeping rs_files_under, read, and is_allowlisted as helpers.crates/grida/src/htmlcss/mod.rs-219-223 (1)
219-223:⚠️ Potential issue | 🟠 MajorDon't promise malformed-input errors unless this path validates strictly.
The updated malformed-SVG test below now accepts
Okas well asErr, so callers can no longer treatrender_svg()as an XML validity check even though this doc comment says they can. Either add a strict parse beforerender_to_picture, or relax the public contract/docs to match the best-effort behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/mod.rs` around lines 219 - 223, The doc comment for render_svg promises Err "only when the input is malformed XML" but render_to_picture is best-effort and can return Ok for invalid XML; update the public contract to reflect that behavior: change the comment on fn render_svg to say it performs a best-effort rendering and returns Err only on rendering/parsing failures (not as a strict XML validator), and ensure callers rely on this contract (or, if you prefer strict validation instead, perform a strict parse step—e.g., using roxmltree::Document::parse or quick-xml—inside render_svg before calling crate::htmlcss::svg::render_to_picture and return an Err on parse failure). Include references to render_svg and crate::htmlcss::svg::render_to_picture when making the change.crates/grida/src/htmlcss/svg/layout/bbox.rs-8-93 (1)
8-93: 🛠️ Refactor suggestion | 🟠 MajorUse
math2for layout geometry and convert at paint boundaryThis layout helper is doing core geometry math with
skia_safe::Rectand manual min/max math. Please keep geometry inmath2types here and convert to Skia only at the paint/resource boundary.As per coding guidelines,
crates/grida/**/*.rsshould “Usemath2crate for geometry and common math operations.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/layout/bbox.rs` around lines 8 - 93, The function element_object_bbox (and any helpers like apply_node_transform) currently does layout math with skia_safe::Rect and manual min/max; change it to use math2 geometry types (e.g., math2::rect::Rect, math2::point::Point/vec2, math2::size::Size) for all computations and aggregations, replace uses of Rect::from_xywh, Rect::new, .join(), .is_empty(), and manual min/max with the corresponding math2 constructors and methods, and have this function return a math2 Rect (or internal geometry type); perform a single conversion from math2 Rect to skia_safe::Rect at the paint/resource boundary instead of inside layout code.crates/grida_dev/scripts/reftest-view.sh-34-40 (1)
34-40:⚠️ Potential issue | 🟠 MajorPrevent destructive cleanup of existing result-dir content
The script can overwrite/remove pre-existing
index.html/testspaths inRESULT_DIR, andln -sfwithout-nbehaves badly when the destination is an existing directory.Proposed hardening
LINK="$RESULT_DIR/index.html" TESTS_LINK="$RESULT_DIR/tests" -ln -sf "$DASHBOARD" "$LINK" + +for p in "$LINK" "$TESTS_LINK"; do + if [[ -e "$p" && ! -L "$p" ]]; then + echo "refusing to overwrite non-symlink path: $p" >&2 + exit 1 + fi +done + +ln -sfn "$DASHBOARD" "$LINK" if [[ -d "$FIXTURES_DIR" ]]; then - ln -sf "$FIXTURES_DIR" "$TESTS_LINK" + ln -sfn "$FIXTURES_DIR" "$TESTS_LINK" else echo "warning: fixtures not found at $FIXTURES_DIR — original SVG tile will be empty" >&2 fi -trap 'rm -f "$LINK" "$TESTS_LINK"' EXIT INT TERM +trap '[[ -L "$LINK" ]] && rm -f "$LINK"; [[ -L "$TESTS_LINK" ]] && rm -f "$TESTS_LINK"' EXIT INT TERM🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida_dev/scripts/reftest-view.sh` around lines 34 - 40, The cleanup/linking logic can destructively remove or replace real files/dirs in RESULT_DIR because ln -sf will overwrite directories and the trap blindly rm -f "$LINK" "$TESTS_LINK"; update the code to: create links with safe flags (use ln -sfn or ln -s -n where supported) and before removing in the trap check that each target is a symlink (e.g. test -L "$LINK" and test -L "$TESTS_LINK") and only unlink/remove when true; also when creating links, detect if the destination already exists and is not a symlink (a real file/dir) and skip linking with a warning instead of forcing overwrite; apply these changes around the variables LINK, TESTS_LINK, DASHBOARD and FIXTURES_DIR and the trap that currently calls rm -f.crates/grida/src/htmlcss/svg/style/cascade.rs-45-54 (1)
45-54:⚠️ Potential issue | 🟠 MajorInline
!importantis currently parsed as part of the value.
get_inline_style/get_attr_or_stylenever strip or track!important, sostyle="fill:red !important"becomes"red !important"and also loses to author!importantrules incascade_property. Parse the priority separately and hand downstream code the cleaned value.Also applies to: 57-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/style/cascade.rs` around lines 45 - 54, The issue is that inline styles with "!important" are not parsed separately, causing the priority flag to be embedded in the value string and breaking proper cascading. To fix this, modify the parsing logic in `get_inline_style` and `get_attr_or_style` so that they extract and remove the "!important" priority suffix from style values, returning the cleaned value and priority as separate outputs. Adjust `cascade_property` and other relevant functions to handle these separate components for proper style precedence. Apply the same fix for the code portions between lines 57-89.crates/grida_dev/scripts/reftest_dashboard.html-265-283 (1)
265-283:⚠️ Potential issue | 🟠 MajorDon't inject
report.jsonfields intoinnerHTML.
cat,t.name, and the derived image paths are copied straight into HTML and attribute contexts here. A crafted fixture/report name can execute script when someone opens the dashboard. Build these cards with DOM APIs (createElement/textContent) or escape every interpolated field before insertion.Also applies to: 360-391
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida_dev/scripts/reftest_dashboard.html` around lines 265 - 283, Do not inject untrusted values into innerHTML: replace the innerHTML assembly for the "categories" element with DOM construction using document.createElement and element.textContent/element.setAttribute so that variables like cat, c, pct (and any t.name or derived image paths referenced elsewhere) are never interpolated into HTML; create the outer card element, a span for the category name using textContent, a container div for the progress track and an inner bar div whose classList includes the computed barColor and whose style.width is set from pct.toFixed(1) + "%", and numeric spans for pct and c.n using textContent, then append these nodes to document.getElementById("categories"); apply the same DOM-API replacement to the similar block referenced at lines 360-391.crates/grida/src/htmlcss/svg/layout/viewport.rs-17-25 (1)
17-25:⚠️ Potential issue | 🟠 MajorHandle missing and non-positive nested SVG sizes separately.
This branch treats omitted
width/heightas0and still paints children without the nested viewport/translation. That makes<svg x=... viewBox=...>render in the wrong coordinate space, and non-positive sizes should suppress rendering instead of painting.Proposed fix
let w = get_attr(node, "width").and_then(parse_length_px); let h = get_attr(node, "height").and_then(parse_length_px); - let viewport_w = w.unwrap_or(0.0); - let viewport_h = h.unwrap_or(0.0); + let (parent_w, parent_h) = ancestor_svg_viewport(ctx, id); + let viewport_w = w.unwrap_or(parent_w); + let viewport_h = h.unwrap_or(parent_h); if viewport_w <= 0.0 || viewport_h <= 0.0 { - paint_children(canvas, ctx, id); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/layout/viewport.rs` around lines 17 - 25, The code treats missing width/height as 0 and returns early, which breaks nested <svg x=... viewBox=...> layouts; instead only suppress rendering when an explicit width or height is present and non-positive. Change the check to detect presence: keep w and h as Option<f32>, compute viewport_w = w.unwrap_or(0.0) and viewport_h = h.unwrap_or(0.0) as before, but replace the early-return condition with something like: if (w.is_some() && viewport_w <= 0.0) || (h.is_some() && viewport_h <= 0.0) { return; } so omitted attributes do not trigger the return and children are painted in the nested viewport/translation; reference get_attr, parse_length_px, w, h, viewport_w, viewport_h, and paint_children.crates/grida/src/htmlcss/svg/paint/clip_path_clipper.rs-37-49 (1)
37-49:⚠️ Potential issue | 🟠 MajorDon't turn an unresolved
<clipPath>into a paint bailout.If
lookup(id)finds a<clipPath>butresolve_to_pathreturnsNone, this returnsfalse, and the caller drops the whole element. That makes empty/unsupported/cyclic clip paths hide content instead of falling back to “no clip”.Proposed fix
let bbox = element_object_bbox(ctx.dom, node); let Some(path) = clipper::resolve_to_path(ctx.dom, ctx.resources, target, bbox) else { - return !target_is_clip_path; + return true; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/paint/clip_path_clipper.rs` around lines 37 - 49, When clipper::resolve_to_path(ctx.dom, ctx.resources, target, bbox) returns None we should not treat an unresolved <clipPath> as a paint bailout; update the fallback so that if target_is_clip_path is true we return true (fall back to "no clip") and only bail out for non-clipPath targets. Concretely, replace the current fallback logic that returns !target_is_clip_path with logic that returns true when target_is_clip_path (and preserves the existing bailout behavior for non-clipPath cases), leaving ctx.resources.lookup, target_is_clip_path, element_object_bbox, and the call to clipper::resolve_to_path intact.crates/grida/src/htmlcss/svg/paint/visibility.rs-40-67 (1)
40-67:⚠️ Potential issue | 🟠 MajorLet inline
stylewin overdisplay/visibilityattributes.These helpers short-circuit on the presentation attribute, so
style="visibility:visible"cannot overridevisibility="hidden"andstyle="display:inline"cannot overridedisplay="none". That flips SVG/CSS cascade order for the paint gates.Also applies to: 85-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/paint/visibility.rs` around lines 40 - 67, The code currently checks presentation attributes before inline style, letting attributes override style; change both helpers to check inline style first and return based on style_contains_pair(style, ...) if get_attr(node, "style") is Some, then fall back to matches_attr(node, ...); specifically update has_display_none and is_visible_self to test style (using get_attr and style_contains_pair) before calling matches_attr so style="display:inline"/"visibility:visible" can override display/visibility presentation attributes.crates/grida/src/htmlcss/svg/resources/pattern.rs-248-264 (1)
248-264:⚠️ Potential issue | 🟠 MajorDon't collapse
href-chain failure into “use the local pattern”.If recursive resolution hits the depth cap or a cycle, this branch falls back to
node_idand can still build a shader from the local node. That turns an invalidhrefchain into a paintable pattern instead of failing closed.crates/grida/src/htmlcss/svg/paint/svg_container_painter.rs-71-76 (1)
71-76:⚠️ Potential issue | 🟠 Major
<switch>should test effective visibility before picking a winner.
is_painted(n)only checksdisplay:noneand the child's ownvisibility. If the<switch>or an ancestor isvisibility:hidden, the first child without an override is still selected, paints nothing, and incorrectly blocks later visible siblings. Use the inherited visibility result here instead.Suggested fix
- if !is_painted(n) { + if has_display_none(n) || !is_visible_inherited(ctx.dom, id) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/paint/svg_container_painter.rs` around lines 71 - 76, The selection logic for <switch> wrongly uses is_painted(n) which only checks display and the node's own visibility; update the check to use the node's effective/inherited visibility (i.e., the computed visibility that considers ancestors and the <switch> itself) instead of is_painted(n) so that a child hidden only by an ancestor doesn't block later siblings—replace the is_painted(n) gate with a test against the effective visibility result before calling paint_node(canvas, ctx, id).crates/grida/src/htmlcss/svg/resources/pattern.rs-186-202 (1)
186-202:⚠️ Potential issue | 🟠 MajorDon't record the pattern picture to the tile cell.
picture_boundsis currentlytile.width()/tile.height(), so any valid pattern content that extends past one cell is clipped beforeto_shaderrepeats it. This breaks common cases likepatternContentUnits="objectBoundingBox"with content larger than the tile. Record to a conservative content bounds, and let the shader's tile rect define repetition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/resources/pattern.rs` around lines 186 - 202, The recording currently uses picture_bounds sized to the tile (Rect::from_xywh(0.0, 0.0, tile.width(), tile.height())), which clips content that extends beyond a single cell; change the recording bounds used by PictureRecorder::new()/recorder.begin_recording from the tile rect to a conservative content bounds computed from the pattern content (for example the content node's bounding box, the pattern's viewport or an expanded rect that covers potential overflow) so children painted by paint_children are captured fully; keep applying content_to_tile before painting and rely on the shader's tile rect (used later in to_shader) to enforce repetition, and use ctx.with_deeper_pattern and recorder.finish_recording_as_picture as before.crates/grida/src/htmlcss/svg/paint/svg_text_painter/mod.rs-1976-1987 (1)
1976-1987:⚠️ Potential issue | 🟠 MajorSplit the
font-sizekeyword cases.
parent.max(16.0)is wrong for all four keywords bundled here:medium/initialshould resolve to 16px, whileinheritandunsetshould preserve the parent size becausefont-sizeis inherited. The current branch changes small inherited sizes to 16px and lets large parents overridemedium.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/paint/svg_text_painter/mod.rs` around lines 1976 - 1987, Split the bundled font-size cases so inheritance and initial/default are handled correctly: for v == "" or "medium" or "initial" return 16.0; for v == "inherit" or "unset" return parent (i.e., preserve the parent's size) instead of parent.max(16.0); keep the other keyword branches (e.g., "xx-small", "smaller", "larger") unchanged — update the match on v (using the existing v and parent variables in the font-size parsing code in svg_text_painter/mod.rs) accordingly.crates/grida/src/htmlcss/svg/paint/svg_text_painter/mod.rs-2249-2257 (1)
2249-2257:⚠️ Potential issue | 🟠 MajorThese out-of-range weight assertions contradict the implementation.
resolve_font_weight_tokenreturns the inherited weight for invalid numeric values, but this test expects clamping to1/1000. As written it will fail as soon as the unit tests run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/paint/svg_text_painter/mod.rs` around lines 2249 - 2257, The test's expectations for out-of-range numeric font weights conflict with the implementation of resolve_font_weight_token (which returns the inherited weight for invalid numeric values); update the two assertions that currently expect clamping ("0" and "1500") to assert the inherited value (use 400 as the inherited weight used in these tests) so they match resolve_font_weight_token's behavior.crates/grida/src/htmlcss/svg/paint/svg_marker_painter.rs-71-90 (1)
71-90:⚠️ Potential issue | 🟠 MajorAssign start/end markers per subpath, not just once globally.
Every
Movestarts a new subpath, but onlyout.first_mut()andout.last_mut()are retyped toStart/End. Any earlier open subpath, plus the synthetic position emitted byClose, staysMid, somarker-start/marker-enddisappear on multi-subpath paths.Also applies to: 191-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/paint/svg_marker_painter.rs` around lines 71 - 90, When starting a new subpath in Verb::Move, mark the pushed MarkerPosition as Start instead of leaving it Mid and record its index (use subpath_start_idx and out.push). When you begin a new Move while a previous subpath is still open, update the previous subpath's last marker (using the stored subpath_start_idx and the last index of that subpath in out) to MarkerKind::End. Likewise, when handling Close (and when finishing the entire path), ensure the synthetic/closing MarkerPosition is set to End for that subpath (update its MarkerPosition.kind) rather than relying on global out.first_mut()/out.last_mut(); use the per-subpath indices (subpath_start_idx, subpath start/end indexes) to set Start/End for each subpath.crates/grida/src/htmlcss/svg/paint/svg_text_painter/mod.rs-1669-1700 (1)
1669-1700:⚠️ Potential issue | 🟠 MajorHandle the common
font-size/line-heightshorthand form.The tokenizer never breaks on
/, sofont: italic 12px/14px sans-serifleavessize_tokas12px/14pxand the whole shorthand is discarded. That attached form is the normal CSS syntax.Also applies to: 1807-1828
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/paint/svg_text_painter/mod.rs` around lines 1669 - 1700, The tokenizer in the font parsing loop (the variables tokens, buf, in_quote inside the for ch in value.chars() block) never treats '/' as a delimiter, so shorthand like "italic 12px/14px sans-serif" yields a single token "12px/14px" and breaks font-size/line-height parsing; update the None branch of the match to treat '/' like whitespace/delimiters (i.e., when ch == '/' and not in a quote, flush buf into tokens and skip adding the slash) so size and line-height become separate tokens; apply the same change to the duplicate logic later around the 1807-1828 region to keep behavior consistent.crates/grida/src/htmlcss/svg/style/stylesheet.rs-428-500 (1)
428-500:⚠️ Potential issue | 🟠 MajorMake
@importparsing case-insensitive end-to-end.CSS treats both
@importandurl(...)case-insensitively, butscan_importsonly searches for lowercase@importandparse_import_urlonly accepts lowercaseurl(. Valid sheets like@IMPORT URL(theme.css)won't preload or collect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/style/stylesheet.rs` around lines 428 - 500, scan_imports and parse_import_url currently only match lowercase forms so mixed-case like `@IMPORT URL(...)` is missed; update parse_import_url to accept "url(" case-insensitively (e.g., check rest[..4].eq_ignore_ascii_case("url(") or lowercase the prefix before comparing) and update scan_imports to locate `@import` in a case-insensitive way (don’t use rest.find("@import") literal; search for '@' then test the following 6 bytes with eq_ignore_ascii_case("import") and keep the same whitespace/boundary logic), and also ensure parse_at_import’s use of parse_import_url remains correct with trimmed body so both `parse_at_import`, `scan_imports`, and `parse_import_url` handle ASCII case-insensitive keywords uniformly.crates/grida/src/htmlcss/svg/paint/svg_text_painter/mod.rs-1125-1147 (1)
1125-1147:⚠️ Potential issue | 🟠 MajorSupport
currentColorconsistently for text strokes.The fill path handles
Paint::CurrentColor, but both stroke builders accept onlyPaint::Color, andresolve_current_coloronly looks at a literalcolorattribute.stroke="currentColor"orstyle="color: ..."on text/decoration anchors will therefore render as no stroke or black fallback.Also applies to: 1460-1486, 2174-2195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/paint/svg_text_painter/mod.rs` around lines 1125 - 1147, The stroke-handling code only accepts Paint::Color and ignores Paint::CurrentColor; update the stroke branches (the block using read_inherited(..., "stroke") and parse_paint(v) that currently matches Paint::Color(c)) to also handle Paint::CurrentColor by calling resolve_current_color(ctx, node) (or otherwise reading the computed text color) and using that resolved SkColor for painting, applying stroke-opacity the same way as for explicit colors; ensure you use the resolved color's alpha when computing a for paint.set_alpha_f(a) and call draw_glyphs(canvas, run, font, &paint) with the resulting paint. Apply the same change to the other analogous stroke blocks mentioned (around the ranges corresponding to 1460-1486 and 2174-2195) so strokes honoring "currentColor" render consistently.crates/grida/src/htmlcss/svg/paint/svg_root_painter.rs-95-100 (1)
95-100:⚠️ Potential issue | 🟠 MajorRoot-level effects bypass the CSS path.
Both branches only consult
get_attr, sostyle="clip-path:...",style="filter:...", or stylesheet matches on the outer<svg>are ignored here even though container painting supports CSS-authored effects. Root clips/filters will silently disappear on those documents.Also applies to: 145-167
crates/grida/src/htmlcss/svg/resources/gradient.rs-277-317 (1)
277-317:⚠️ Potential issue | 🟠 MajorNormalize stop offsets before handing them to Skia.
Each stop offset is only clamped into
[0, 1]here. SVG requires the resolved sequence to be nondecreasing, so a stop list like0.8, 0.2, 1should become0.8, 0.8, 1, not stay out of order. Passing the raw values through will render the wrong gradient for those fixtures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/resources/gradient.rs` around lines 277 - 317, When building the vector of stops (the stops.push(Stop { ... }) block), ensure the sequence of stop offsets is normalized to be nondecreasing per SVG rules: clamp each parsed offset to [0.0,1.0] then replace it with max(previous_normalized_offset, current_clamped_offset) so offsets like 0.8,0.2,1 become 0.8,0.8,1. Implement this by tracking a last_offset variable (initial 0.0 or first clamped value) as you append Stop entries (referencing the offset local, the stops Vec and the Stop struct) and use offset = last_offset.max(offset.clamp(0.0,1.0)) before creating/pushing each Stop.crates/grida/src/htmlcss/svg/paint/scoped_svg_paint_state.rs-82-89 (1)
82-89:⚠️ Potential issue | 🟠 MajorDon't collapse nested
<use>inheritance to a single node.
with_use_inheritoverwrites the previous host, so once a clone expands another<use>, properties from the outer<use>stop participating in inheritance. Nested use shadow trees need a stack/chain, not a single slot.Also applies to: 114-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/paint/scoped_svg_paint_state.rs` around lines 82 - 89, The current single-slot design for tracking `<use>` inheritance (pub use_inherit: Option<NodeId>) and the setter with_use_inherit overwrites the previous host, which loses outer `<use>` properties for nested `<use>` trees; change the representation to a stack/chain (e.g., Vec<NodeId> or a linked list) and update associated APIs so pushing a new host pushes onto the stack and popping restores the previous host; update references to use_inherit and the with_use_inherit method (and the other locations around the 114-119 region) to push/pop or peek instead of replace so nested use shadow trees correctly accumulate inherited properties.crates/grida/src/htmlcss/svg/paint/svg_text_painter/mod.rs-661-663 (1)
661-663:⚠️ Potential issue | 🟠 Major
<textPath>glyphs still consume the parent coordinate lists.You stop reading ancestor
x/y/dx/dyonce atextPathframe is hit, butconsumed += 1still advances every frame instack. Characters inside<textPath>will therefore eat the parent<text>list entries and shift later sibling text to the wrong positions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/paint/svg_text_painter/mod.rs` around lines 661 - 663, The bug is that you blindly increment elem.consumed for every frame in stack, which advances ancestor x/y/dx/dy entries even when the current glyph is inside a <textPath>; change the loop so you only increment consumed on frames that actually contributed coordinates for this glyph. Replace the unconditional for elem in stack.iter_mut() { elem.consumed += 1; } with logic that walks stack and increments elem.consumed only while the frame is a coordinate-owner (e.g., not a TextPath frame) — detect the TextPath frame via the same symbol used elsewhere (Frame::TextPath or elem.is_text_path / elem.reads_coordinates) and break/stop incrementing once you hit it so ancestors are not consumed by glyphs inside a textPath.crates/grida/src/htmlcss/svg/geometry/basic_shape.rs-384-388 (1)
384-388:⚠️ Potential issue | 🟠 MajorResolve ellipse radii per axis.
resolve_radius(..., false)still useswplus circle-style closest/farthest-side math for both calls, sory="50%"resolves against width andellipse(closest-side)produces symmetric radii instead of horizontal/vertical ones.rxandryneed separate axis-aware resolution rules.Also applies to: 465-478
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/geometry/basic_shape.rs` around lines 384 - 388, The ellipse branch (BasicShape::Ellipse) resolves both rx and ry with the same axis rules causing ry="50%" and closest/farthest-side math to use the width; update the rx/ry resolve_radius calls so rx uses the horizontal axis rules and distances and ry uses the vertical axis rules (pass the axis flag/params appropriately — e.g. keep resolve_radius(*rx, w, h, cx - box_rect.left, cy - box_rect.top, false) for the horizontal radius and call resolve_radius(*ry, w, h, cx - box_rect.left, cy - box_rect.top, true) or the equivalent vertical-axis variant), and apply the same fix to the other ellipse block around the later range mentioned (the similar ellipse code at the other location).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 409b0993-5e32-4533-b8d3-8068a9525bdd
📒 Files selected for processing (73)
.agents/skills/research/SKILL.mdcrates/grida/examples/tool_gen_bench_fixture.rscrates/grida/examples/tool_sk_svgdom.rscrates/grida/src/htmlcss/collect.rscrates/grida/src/htmlcss/mod.rscrates/grida/src/htmlcss/paint.rscrates/grida/src/htmlcss/style.rscrates/grida/src/htmlcss/svg/README.mdcrates/grida/src/htmlcss/svg/context.rscrates/grida/src/htmlcss/svg/dom/attrs.rscrates/grida/src/htmlcss/svg/dom/element.rscrates/grida/src/htmlcss/svg/dom/href.rscrates/grida/src/htmlcss/svg/dom/mod.rscrates/grida/src/htmlcss/svg/dom/parser.rscrates/grida/src/htmlcss/svg/dom/path_d.rscrates/grida/src/htmlcss/svg/error.rscrates/grida/src/htmlcss/svg/geometry/basic_shape.rscrates/grida/src/htmlcss/svg/geometry/mod.rscrates/grida/src/htmlcss/svg/layout/bbox.rscrates/grida/src/htmlcss/svg/layout/layout_svg_element.rscrates/grida/src/htmlcss/svg/layout/mod.rscrates/grida/src/htmlcss/svg/layout/transform.rscrates/grida/src/htmlcss/svg/layout/viewport.rscrates/grida/src/htmlcss/svg/mod.rscrates/grida/src/htmlcss/svg/paint/clip_path_clipper.rscrates/grida/src/htmlcss/svg/paint/effects.rscrates/grida/src/htmlcss/svg/paint/mod.rscrates/grida/src/htmlcss/svg/paint/scoped_svg_paint_state.rscrates/grida/src/htmlcss/svg/paint/svg_container_painter.rscrates/grida/src/htmlcss/svg/paint/svg_image_painter.rscrates/grida/src/htmlcss/svg/paint/svg_marker_painter.rscrates/grida/src/htmlcss/svg/paint/svg_object_painter.rscrates/grida/src/htmlcss/svg/paint/svg_painter.rscrates/grida/src/htmlcss/svg/paint/svg_root_painter.rscrates/grida/src/htmlcss/svg/paint/svg_shape_painter.rscrates/grida/src/htmlcss/svg/paint/svg_text_painter/mod.rscrates/grida/src/htmlcss/svg/paint/svg_text_painter/shaping.rscrates/grida/src/htmlcss/svg/paint/svg_use_painter.rscrates/grida/src/htmlcss/svg/paint/visibility.rscrates/grida/src/htmlcss/svg/resources/cache.rscrates/grida/src/htmlcss/svg/resources/clipper.rscrates/grida/src/htmlcss/svg/resources/filter.rscrates/grida/src/htmlcss/svg/resources/gradient.rscrates/grida/src/htmlcss/svg/resources/masker.rscrates/grida/src/htmlcss/svg/resources/mod.rscrates/grida/src/htmlcss/svg/resources/paint_server.rscrates/grida/src/htmlcss/svg/resources/pattern.rscrates/grida/src/htmlcss/svg/resources/svg_filter_builder.rscrates/grida/src/htmlcss/svg/resources/svg_resource_container.rscrates/grida/src/htmlcss/svg/resources/svg_resources.rscrates/grida/src/htmlcss/svg/style/cascade.rscrates/grida/src/htmlcss/svg/style/mod.rscrates/grida/src/htmlcss/svg/style/stylesheet.rscrates/grida/src/htmlcss/svg/style/stylo_bridge.rscrates/grida/tests/htmlcss_svg_architecture.rscrates/grida/tests/htmlcss_svg_checkpoint1.rscrates/grida_dev/examples/render_one_svg.rscrates/grida_dev/scripts/reftest-run.shcrates/grida_dev/scripts/reftest-view.shcrates/grida_dev/scripts/reftest_dashboard.htmlcrates/grida_dev/src/main.rscrates/grida_dev/src/reftest/args.rscrates/grida_dev/src/reftest/render.rscrates/grida_dev/src/reftest/runner.rsdocs/wg/feat-2d/htmlcss-svg.mddocs/wg/research/chromium/external-css.mddocs/wg/research/chromium/index.mddocs/wg/research/chromium/svg/clip-path.mddocs/wg/research/chromium/svg/fe-image.mddocs/wg/research/chromium/svg/fe-tile.mddocs/wg/research/chromium/svg/index.mddocs/wg/research/chromium/svg/module-structure.mddocs/wg/research/chromium/svg/text-on-path.md
💤 Files with no reviewable changes (2)
- crates/grida/examples/tool_gen_bench_fixture.rs
- crates/grida/examples/tool_sk_svgdom.rs
12 high-confidence correctness fixes from the CodeRabbit + Codex review on PR #698. Reftest: 89.50% -> 89.38% (-0.12pp; 2 masking fixtures degraded, all other categories identical). Unit tests: 200/202 -> 202/202 passing (two pre-existing failing tests corrected). Rendering correctness: - shaping.rs: combining marks / zero-glyph chars now get advance 0 (was inheriting the next cluster's advance via `cluster >= start`). - visibility.rs: inline `style="..."` now wins over the presentation attribute for `display` / `visibility`, matching CSS specificity. - clip_path_clipper.rs: unresolved `<clipPath>` renders unclipped rather than dropping the element (matches design study S-clip-path "Differ" formulation; this is the change that moved the masking score). - gradient.rs: stop offsets normalized to non-decreasing per SVG 1.1 §13.2.4 ("use the previous offset value" when out of order). - cascade.rs: strip `!important` from inline-style values so downstream parsers see `red`, not `red !important`. - stylesheet.rs: `@import` keyword and `url(` token now matched case-insensitively per CSS Syntax §4.3. - scoped_svg_paint_state.rs: `<use>` inheritance switched from a single-slot `Option<NodeId>` to a stack of frames (`UseFrame` linked list, keeps `PaintCtx: Copy`). Adds non-ancestor cycle detection (`#a -> #b -> #a`) and proper inheritance through multi-level `<use>` nesting. - svg_container_painter.rs: `<switch>` now skips candidates whose *inherited* visibility is hidden, not just their own — a hidden ancestor no longer pins the first child as winner. - paint.rs: inline-SVG render failures terminate at the `paint_inline_svg` call instead of falling through to the gray `<img>` placeholder; matches the documented "no fallback" intent. Tests: - font_weight_named_and_numeric: was asserting clamping for out-of-range numeric values; impl correctly returns the inherited value per CSS Fonts 4 §3.2.4 (Blink: `ConsumeFontWeightNumber` returns nullptr -> cascade fallback). Test now matches the spec-correct behavior. - length_px_handles_unit_suffix: was asserting `None` for `10em`; impl returns the documented default-context approximation (em/rem = 16px). Test updated to match the doc. Doc cleanups: - reftest/args.rs: `Htmlcss` backend description points at `htmlcss::svg::render_to_picture*` (the old `htmlcss::render_svg` is just a thin wrapper but the API surface in `htmlcss::svg` is the canonical entry). - htmlcss/mod.rs: `render_svg`'s doc no longer claims the function is an XML validator — the parser is permissive by design. Deferred from the review (not "deserved enough"): - RTL shaping (no fixtures, design decision needed) - Pattern href fallback / picture bounds (high regression risk on paint-servers fixtures) - Per-subpath markers, font shorthand, currentColor for stroke, textPath consumed counter (text/marker logic risk) - Root effects via CSS, nested-svg defaults, ellipse axes (moderate regression risk) - math2 migration (out of scope) - AST-based architecture test, doc frontmatter (style nits) See the aggregate in the PR conversation for full triage.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
crates/grida/src/htmlcss/mod.rs (1)
3303-3305: Test intent and test name are now out of sync.Lines 3303-3305 intentionally allow either
OkorErr, but the test nametest_render_svg_malformed_errorsstill impliesErris required. Renaming would make CI output less confusing.🧪 Suggested rename
- fn test_render_svg_malformed_errors() { + fn test_render_svg_malformed_no_panic() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/mod.rs` around lines 3303 - 3305, The test name test_render_svg_malformed_errors is misleading because the block purposely accepts either Ok or Err; rename the test function to reflect permissive behavior (e.g., test_render_svg_malformed_permissive or test_render_svg_malformed_accepts_ok_or_err) and update any references to that function so CI output matches intent; locate the test definition named test_render_svg_malformed_errors in the htmlcss::svg-related tests (mod.rs) and change only the identifier and any occurrences used in annotations or test harness.crates/grida/src/htmlcss/svg/paint/svg_use_painter.rs (1)
88-94: Opacity layer restore relies on encompassing sym_restore.The opacity save_layer opened at line 93 is implicitly restored by
canvas.restore_to_count(sym_restore)at line 130. This works correctly becausesym_restorewas captured before the opacity layer was opened (line 88). However, this coupling could be fragile if future refactoring moves the sym_restore capture.Consider adding a brief comment noting that
sym_restoreintentionally covers the opacity layer:let sym_restore = canvas.save(); +// This save covers any optional opacity layer opened below, plus the +// clip/viewBox transforms — a single restore_to_count unwinds all. let opacity = group_opacity(target);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/paint/svg_use_painter.rs` around lines 88 - 94, Add a brief inline comment near the sym_restore capture to document that sym_restore (from canvas.save()) intentionally encloses the subsequent opacity save_layer so that the later canvas.restore_to_count(sym_restore) will also restore that layer; reference the variables/symbols sym_restore, canvas.save(), opacity, canvas.save_layer and canvas.restore_to_count to make the coupling explicit and guard against fragility during refactors.crates/grida/src/htmlcss/svg/style/stylesheet.rs (1)
486-510: Keepscan_importson the same parser path as the renderer.This substring scan can preload URLs from places the renderer would ignore, like string literals containing
@importor late-invalid@importrules after a style rule. Reusingparse_stylesheetand collecting onlyParsedRule::Importkeeps preloading behavior aligned with actual stylesheet parsing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida/src/htmlcss/svg/style/stylesheet.rs` around lines 486 - 510, The current scan_imports substring scan can pick up `@import` occurrences that the real parser would ignore (e.g., inside strings or invalid positions); replace its implementation to call the same parser used by the renderer (parse_stylesheet), iterate the returned rules, and collect URLs from ParsedRule::Import only so preloading matches actual parsing behavior; use parse_stylesheet(text) and for each rule that matches ParsedRule::Import extract the import URL (replacing the existing parse_import_url-based substring logic) so imports are discovered exactly as the renderer would handle them.
🤖 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/grida/src/htmlcss/mod.rs`:
- Around line 219-223: The doc comment above the SVG render entry point in
crates::grida::htmlcss::mod.rs currently states "Returns `Err` only when SVG
structure is unrecoverable...", which is too narrow; update that docstring (the
comment for the SVG render entry-point function in this module) to say that Err
can also be returned for other failures from the SVG pipeline such as
non-structural parse errors or unsupported features (e.g., parser or pipeline
failures), so callers are not misled into assuming only
unrecoverable-structure/picture-recording cases produce Err.
In `@crates/grida/src/htmlcss/svg/paint/visibility.rs`:
- Around line 84-107: The visibility check in read_visibility should prioritize
inline style over the presentation attribute; update read_visibility so it first
reads get_attr(node, "style") and parses visibility declarations (the existing
style-splitting logic) returning Some(true/false) when found, and only if no
inline style visibility is present then fall back to matches_attr(node,
"visibility", ...) checks (the current attribute-based branches). Keep the same
comparison logic (eq_ignore_ascii_case against "visible", "hidden", "collapse")
and the same return semantics.
In `@crates/grida/src/htmlcss/svg/style/cascade.rs`:
- Around line 41-54: The inline-style readers (called by cascade_property via
get_inline_style) currently return the first matching declaration and drop the
!important flag; change the inline parser(s) to scan the entire inline
declaration block for the requested property and track the winning declaration
by CSS ordering (later declarations win) while preserving whether that winning
declaration had !important, then return the winning (value, important: bool)
(e.g., Option<(String, bool)>) instead of a single String; update
cascade_property to use that returned (value, important) when comparing against
the stylesheet candidate (matched which is Option<(value, spec, important: bool,
order)>) so inline !important and declaration order are considered correctly.
In `@crates/grida/src/htmlcss/svg/style/stylesheet.rs`:
- Around line 267-283: The comparison key ignores declaration position so
duplicate properties in the same rule never let later declarations win; modify
the key used when building candidate/prev_key (inside the loop over rule.decls)
to include the declaration's own order/position (e.g., use decl.order or the
decl index) instead of only rule.order, so cand_key and prev_key become
(decl.important, spec, rule.order, decl_order) and later declarations with equal
priority will correctly override earlier ones.
---
Nitpick comments:
In `@crates/grida/src/htmlcss/mod.rs`:
- Around line 3303-3305: The test name test_render_svg_malformed_errors is
misleading because the block purposely accepts either Ok or Err; rename the test
function to reflect permissive behavior (e.g.,
test_render_svg_malformed_permissive or
test_render_svg_malformed_accepts_ok_or_err) and update any references to that
function so CI output matches intent; locate the test definition named
test_render_svg_malformed_errors in the htmlcss::svg-related tests (mod.rs) and
change only the identifier and any occurrences used in annotations or test
harness.
In `@crates/grida/src/htmlcss/svg/paint/svg_use_painter.rs`:
- Around line 88-94: Add a brief inline comment near the sym_restore capture to
document that sym_restore (from canvas.save()) intentionally encloses the
subsequent opacity save_layer so that the later
canvas.restore_to_count(sym_restore) will also restore that layer; reference the
variables/symbols sym_restore, canvas.save(), opacity, canvas.save_layer and
canvas.restore_to_count to make the coupling explicit and guard against
fragility during refactors.
In `@crates/grida/src/htmlcss/svg/style/stylesheet.rs`:
- Around line 486-510: The current scan_imports substring scan can pick up
`@import` occurrences that the real parser would ignore (e.g., inside strings or
invalid positions); replace its implementation to call the same parser used by
the renderer (parse_stylesheet), iterate the returned rules, and collect URLs
from ParsedRule::Import only so preloading matches actual parsing behavior; use
parse_stylesheet(text) and for each rule that matches ParsedRule::Import extract
the import URL (replacing the existing parse_import_url-based substring logic)
so imports are discovered exactly as the renderer would handle them.
🪄 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: 17db9a62-44e1-46c6-94a2-4a237bec1f40
📒 Files selected for processing (15)
crates/grida/src/htmlcss/mod.rscrates/grida/src/htmlcss/paint.rscrates/grida/src/htmlcss/svg/dom/attrs.rscrates/grida/src/htmlcss/svg/paint/clip_path_clipper.rscrates/grida/src/htmlcss/svg/paint/scoped_svg_paint_state.rscrates/grida/src/htmlcss/svg/paint/svg_container_painter.rscrates/grida/src/htmlcss/svg/paint/svg_shape_painter.rscrates/grida/src/htmlcss/svg/paint/svg_text_painter/mod.rscrates/grida/src/htmlcss/svg/paint/svg_text_painter/shaping.rscrates/grida/src/htmlcss/svg/paint/svg_use_painter.rscrates/grida/src/htmlcss/svg/paint/visibility.rscrates/grida/src/htmlcss/svg/resources/gradient.rscrates/grida/src/htmlcss/svg/style/cascade.rscrates/grida/src/htmlcss/svg/style/stylesheet.rscrates/grida_dev/src/reftest/args.rs
✅ Files skipped from review due to trivial changes (1)
- crates/grida_dev/src/reftest/args.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/grida/src/htmlcss/svg/paint/clip_path_clipper.rs
- crates/grida/src/htmlcss/svg/resources/gradient.rs
- crates/grida/src/htmlcss/svg/paint/svg_text_painter/shaping.rs
- crates/grida/src/htmlcss/paint.rs
Comment on lines
+219
to
+223
| /// Returns `Err` only when SVG structure is unrecoverable (e.g. no | ||
| /// `<svg>` element, picture-recording failure). The XML parser is | ||
| /// permissive — best-effort recoverable input may render as `Ok` | ||
| /// even if it isn't strictly well-formed. Use this as a render | ||
| /// entry point, not as an XML validator. |
There was a problem hiding this comment.
Docstring narrows Err conditions more than the implementation.
Line 219 says Err is only for unrecoverable structure/picture-recording cases, but this path can also surface non-structural parse/unsupported failures from the SVG pipeline. Please broaden the wording to avoid misleading callers.
✏️ Suggested doc fix
-/// Returns `Err` only when SVG structure is unrecoverable (e.g. no
-/// `<svg>` element, picture-recording failure). The XML parser is
-/// permissive — best-effort recoverable input may render as `Ok`
-/// even if it isn't strictly well-formed. Use this as a render
-/// entry point, not as an XML validator.
+/// Returns `Err` for unrecoverable SVG parse/structure/recording failures.
+/// The parser is permissive for some malformed inputs, so best-effort
+/// recovery may still render as `Ok`. Use this as a render entry point,
+/// not as an XML validator.📝 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
| /// Returns `Err` only when SVG structure is unrecoverable (e.g. no | |
| /// `<svg>` element, picture-recording failure). The XML parser is | |
| /// permissive — best-effort recoverable input may render as `Ok` | |
| /// even if it isn't strictly well-formed. Use this as a render | |
| /// entry point, not as an XML validator. | |
| /// Returns `Err` for unrecoverable SVG parse/structure/recording failures. | |
| /// The parser is permissive for some malformed inputs, so best-effort | |
| /// recovery may still render as `Ok`. Use this as a render entry point, | |
| /// not as an XML validator. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida/src/htmlcss/mod.rs` around lines 219 - 223, The doc comment
above the SVG render entry point in crates::grida::htmlcss::mod.rs currently
states "Returns `Err` only when SVG structure is unrecoverable...", which is too
narrow; update that docstring (the comment for the SVG render entry-point
function in this module) to say that Err can also be returned for other failures
from the SVG pipeline such as non-structural parse errors or unsupported
features (e.g., parser or pipeline failures), so callers are not misled into
assuming only unrecoverable-structure/picture-recording cases produce Err.
Comment on lines
+84
to
+107
| fn read_visibility(node: &DemoNode) -> Option<bool> { | ||
| if matches_attr(node, "visibility", "visible") { | ||
| return Some(true); | ||
| } | ||
| if matches_attr(node, "visibility", "hidden") || matches_attr(node, "visibility", "collapse") { | ||
| return Some(false); | ||
| } | ||
| if let Some(style) = get_attr(node, "style") { | ||
| for decl in style.split(';') { | ||
| if let Some((k, v)) = decl.split_once(':') { | ||
| if k.trim().eq_ignore_ascii_case("visibility") { | ||
| let v = v.trim(); | ||
| if v.eq_ignore_ascii_case("visible") { | ||
| return Some(true); | ||
| } | ||
| if v.eq_ignore_ascii_case("hidden") || v.eq_ignore_ascii_case("collapse") { | ||
| return Some(false); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
Specificity bug: read_visibility checks attribute before inline style.
This function checks the visibility attribute (lines 85-90) before the inline style="visibility:..." declaration (lines 91-105). CSS specificity requires inline styles to override presentation attributes.
This is inconsistent with has_display_none (lines 45-50) and is_visible_self (lines 60-65), which both correctly check inline style first. The inconsistency means an element with visibility="visible" style="visibility:hidden" would incorrectly report as visible during the ancestor walk.
🔧 Proposed fix to prioritize inline style
fn read_visibility(node: &DemoNode) -> Option<bool> {
+ // Inline style overrides presentation attribute (CSS specificity).
+ if let Some(style) = get_attr(node, "style") {
+ for decl in style.split(';') {
+ if let Some((k, v)) = decl.split_once(':') {
+ if k.trim().eq_ignore_ascii_case("visibility") {
+ let v = v.trim();
+ if v.eq_ignore_ascii_case("visible") {
+ return Some(true);
+ }
+ if v.eq_ignore_ascii_case("hidden") || v.eq_ignore_ascii_case("collapse") {
+ return Some(false);
+ }
+ }
+ }
+ }
+ }
+ // Fall through to presentation attribute.
if matches_attr(node, "visibility", "visible") {
return Some(true);
}
if matches_attr(node, "visibility", "hidden") || matches_attr(node, "visibility", "collapse") {
return Some(false);
}
- if let Some(style) = get_attr(node, "style") {
- for decl in style.split(';') {
- if let Some((k, v)) = decl.split_once(':') {
- if k.trim().eq_ignore_ascii_case("visibility") {
- let v = v.trim();
- if v.eq_ignore_ascii_case("visible") {
- return Some(true);
- }
- if v.eq_ignore_ascii_case("hidden") || v.eq_ignore_ascii_case("collapse") {
- return Some(false);
- }
- }
- }
- }
- }
None
}📝 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
| fn read_visibility(node: &DemoNode) -> Option<bool> { | |
| if matches_attr(node, "visibility", "visible") { | |
| return Some(true); | |
| } | |
| if matches_attr(node, "visibility", "hidden") || matches_attr(node, "visibility", "collapse") { | |
| return Some(false); | |
| } | |
| if let Some(style) = get_attr(node, "style") { | |
| for decl in style.split(';') { | |
| if let Some((k, v)) = decl.split_once(':') { | |
| if k.trim().eq_ignore_ascii_case("visibility") { | |
| let v = v.trim(); | |
| if v.eq_ignore_ascii_case("visible") { | |
| return Some(true); | |
| } | |
| if v.eq_ignore_ascii_case("hidden") || v.eq_ignore_ascii_case("collapse") { | |
| return Some(false); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| None | |
| } | |
| fn read_visibility(node: &DemoNode) -> Option<bool> { | |
| // Inline style overrides presentation attribute (CSS specificity). | |
| if let Some(style) = get_attr(node, "style") { | |
| for decl in style.split(';') { | |
| if let Some((k, v)) = decl.split_once(':') { | |
| if k.trim().eq_ignore_ascii_case("visibility") { | |
| let v = v.trim(); | |
| if v.eq_ignore_ascii_case("visible") { | |
| return Some(true); | |
| } | |
| if v.eq_ignore_ascii_case("hidden") || v.eq_ignore_ascii_case("collapse") { | |
| return Some(false); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| // Fall through to presentation attribute. | |
| if matches_attr(node, "visibility", "visible") { | |
| return Some(true); | |
| } | |
| if matches_attr(node, "visibility", "hidden") || matches_attr(node, "visibility", "collapse") { | |
| return Some(false); | |
| } | |
| None | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida/src/htmlcss/svg/paint/visibility.rs` around lines 84 - 107, The
visibility check in read_visibility should prioritize inline style over the
presentation attribute; update read_visibility so it first reads get_attr(node,
"style") and parses visibility declarations (the existing style-splitting logic)
returning Some(true/false) when found, and only if no inline style visibility is
present then fall back to matches_attr(node, "visibility", ...) checks (the
current attribute-based branches). Keep the same comparison logic
(eq_ignore_ascii_case against "visible", "hidden", "collapse") and the same
return semantics.
Comment on lines
+41
to
+54
| let matched = match (dom, sheet) { | ||
| (Some(d), Some(s)) => s.match_property(d, node, name), | ||
| _ => None, | ||
| }; | ||
| if let Some((value, _spec, true, _order)) = &matched { | ||
| return Some(value.clone()); | ||
| } | ||
| if let Some(v) = get_inline_style(node, name) { | ||
| return Some(v); | ||
| } | ||
| if let Some((v, _, _, _)) = matched { | ||
| return Some(v); | ||
| } | ||
| get_attr(node, name).map(|s| s.to_string()) |
There was a problem hiding this comment.
Inline style parsing loses declaration order and !important.
Both inline-style readers return the first matching declaration and discard whether it was !important. That makes style="fill:red; fill:blue" resolve to red, and it also means inline !important can never outrank a stylesheet !important in cascade_property. The inline parser needs to evaluate the whole declaration block, keep the winning inline declaration (value, important), and let cascade_property compare that against the stylesheet candidate.
Also applies to: 58-69, 76-89, 95-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida/src/htmlcss/svg/style/cascade.rs` around lines 41 - 54, The
inline-style readers (called by cascade_property via get_inline_style) currently
return the first matching declaration and drop the !important flag; change the
inline parser(s) to scan the entire inline declaration block for the requested
property and track the winning declaration by CSS ordering (later declarations
win) while preserving whether that winning declaration had !important, then
return the winning (value, important: bool) (e.g., Option<(String, bool)>)
instead of a single String; update cascade_property to use that returned (value,
important) when comparing against the stylesheet candidate (matched which is
Option<(value, spec, important: bool, order)>) so inline !important and
declaration order are considered correctly.
Comment on lines
+267
to
+283
| for decl in &rule.decls { | ||
| if !decl.name.eq_ignore_ascii_case(name) { | ||
| continue; | ||
| } | ||
| let candidate = (decl.value.clone(), spec, decl.important, rule.order); | ||
| best = Some(match best { | ||
| None => candidate, | ||
| Some(prev) => { | ||
| let prev_key = (prev.2, prev.1, prev.3); | ||
| let cand_key = (candidate.2, candidate.1, candidate.3); | ||
| if cand_key > prev_key { | ||
| candidate | ||
| } else { | ||
| prev | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Later declarations in the same rule never win.
order is tracked per rule, so duplicate properties inside one declaration block produce identical comparison keys. Because Line 277 uses >, .x { fill:red; fill:blue } keeps red instead of the later blue. Either include declaration order in the key or treat equal keys as “later wins” while iterating in source order.
🛠️ Minimal fix
- if cand_key > prev_key {
+ if cand_key >= prev_key {
candidate
} else {
prev
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida/src/htmlcss/svg/style/stylesheet.rs` around lines 267 - 283, The
comparison key ignores declaration position so duplicate properties in the same
rule never let later declarations win; modify the key used when building
candidate/prev_key (inside the loop over rule.decls) to include the
declaration's own order/position (e.g., use decl.order or the decl index)
instead of only rule.order, so cand_key and prev_key become (decl.important,
spec, rule.order, decl_order) and later declarations with equal priority will
correctly override earlier ones.
The field is still written by the comparator, but consumers now route through a separately-computed `diff_pct` value (in reftest::runner). Allow the dead_code warning so `cargo clippy --no-deps -- -D warnings` stays clean while the reftest tooling overhaul is in flight.
CSS Values 4 §3.4 / CSS Paint Order Level 1: a property declaration
that contains any invalid token is invalid as a whole — the value
must be treated as if the property weren't set. Previously
`resolve_paint_order` silently skipped unknown keywords and used
the valid prefix ("stroke markers fill qwe" parsed as "stroke
markers fill"). Chrome/Firefox + the resvg-test-suite expecteds
fall back to default (`fill stroke markers`) when any token is
unknown.
Pass-rate: 1140 → 1141 (+1, 0.8000 → 0.8007).
- painting_paint-order_trailing-data: 0.940 → 1.000
…oke-width CSS Masking 1 §1.2 / SVG 2: the `stroke-box` reference box for a basic-shape `clip-path` value is the element's stroke bounding box — the fill bbox expanded by half the stroke-width on every side. Previously we collapsed `stroke-box` to `fill-box`, so `clip-path: circle() stroke-box` clipped at the fill bbox and chopped off half of the stroke ring. Implementation: when the parsed `ReferenceBox::StrokeBox` is in play, expand `element_object_bbox(node)` by `stroke_half_extent` on all four sides. Stroke-width reads `stroke-width` attribute (default 1px) and only applies when `stroke` is set to a non-`none` value. Pass-rate: 1141 → 1142 (+1, 0.8007 → 0.8014). - masking_clipPath_circle-shorthand-with-stroke-box: 0.902 → 1.000
SVG 2 §11.4: `visibility: hidden` on a `<tspan>` makes the glyphs invisible while preserving their advances in text layout. Previously the painter rendered every glyph regardless — `<tspan visibility="hidden">Text2</tspan>` rendered fully, instead of being invisible with the surrounding text laid out around the reserved space. Implementation: in `paint_glyph_groups`, before the per-source run draws, check `is_visible_inherited` for the source element. When hidden/collapse, skip the run's fill+stroke. The advances were already counted by `resolve_positions` upstream, so layout is preserved (matching the expected `Text1 [gap] Text3` output). `is_visible_inherited` walks ancestors so `<g visibility="hidden"><text>...</text></g>` propagates down through descendants, matching CSS inheritance. A descendant `<tspan visibility="visible">` correctly overrides. Pass-rate held at 1142 (0.8014). Both fixtures advanced toward floor: - painting_visibility_hidden-on-tspan: 0.834 → 0.908 - painting_visibility_collapse-on-tspan: 0.834 → 0.908 Residual is sub-pixel font-metric drift on the surrounding Text1 / Text3, not a renderer bug.
CSS Values 4 §6.1.1: `1ch` = the advance width of the `0` (ZERO)
glyph in the inherited font. Previously we approximated it as
`0.5em` which under-shoots most proportional fonts (Noto Sans `0`
is ~0.55em). The 10ch-wide rect in `shapes_rect_ch-values.svg`
rendered ~10% too narrow.
Implementation: when resolving a `ch` length on shape attributes
(rect x/y/width/height etc.), walk the inherited `font-family`
chain through the FontResolver, build a `Font` at the inherited
font-size, and call `measure_str("0")`. Falls back to the 0.5em
approximation if the font can't be resolved.
Pass-rate: 1142 → 1143 (+1, 0.8014 → 0.8021).
- shapes_rect_ch-values: 0.738 → 1.000
CSS Images 3 / SVG 2: `image-rendering: auto` (the default for `<feImage>`) uses smooth resampling. Skia's `SamplingOptions::default()` is nearest-neighbor — produced blocky upscaled images for fixtures whose `feImage href` points at a source that gets stretched into a larger primitive subregion. Switch to `FilterMode::Linear` for the bilinear resample. Matches Chrome / resvg / Blink default. Pass-rate held at 1160 (0.8140). All feImage fixtures advanced toward floor: - filters_feImage_simple-case: 0.734 → 0.765 - filters_feImage_embedded-png: 0.734 → 0.765 - filters_feImage_svg: 0.891 → 0.848 (regression on SVG-source — small, no crossing) - filters_feImage_preserveAspectRatio=none: 0.883 → 0.896 - filters_feImage_with-subregion-1..5: improved across the board Residual sub-pixel differences in remaining fixtures are font/ position drift, not the sampling mode. No fixture crossed 0.95.
`<marker overflow="inherit">` inside an `<svg overflow="visible">` should inherit visible and skip the marker viewport clip. Previously `inherit` fell to the default-hidden branch, so a marker child larger than markerWidth/markerHeight (e.g. r=12 in a 3×3 viewport) clipped to a tiny dot. `is_overflow_visible(ctx, node)` walks the ancestor chain on `inherit`, reading attribute or `style="…"`. visible/auto → true; hidden/scroll/clip → false; root with no value → false (UA default for marker). Pass-rate: 1160 → 1161 (+1, 0.8140 → 0.8147). - painting_overflow_inherit-on-marker: 0.822 → 1.000
SVG 2 §11.4: a `display:none` element does not contribute to its parent's bounding box. Previously `element_object_bbox` walked all element children of a `<g>` / `<svg>` / `<switch>` / `<a>` unconditionally; a display-none child enlarged the union and distorted any clip / mask / filter / pattern that referenced the parent's bbox via `objectBoundingBox` units. The `painting_display_bBox-impact.svg` fixture documents this: `<g clip-path="url(#circle-bbox)"><rect display="none" size=160/><rect size=120/></g>` — the clipPath is sized by g's bbox, expected = 120-bbox circle, current = 160-bbox circle. Skip display:none children in the bbox accumulator. Pass-rate: 1161 → 1162 (+1, 0.8147 → 0.8154). - painting_display_bBox-impact: 0.734 → 1.000
…bjects SVG 2 §6.13: a path's object bounding box is the tight bbox of the rendered geometry — Bézier/quadratic CONTROL points do not contribute. Previously `element_object_bbox` for a `<path>` used Skia's `path.bounds()` which returns the loose control-point bbox, overshooting any path with control points outside the rendered curve. The `filters_filter_path-bbox.svg` fixture documents this with a Q curve whose control point at (100, 45) sits well above the actual curve apex (~y=65); the filter region (objectBoundingBox %) was stretched up to y=45 instead of stopping at the apex. Use `path.compute_tight_bounds()` instead. Pass-rate: 1162 → 1163 (+1, 0.8154 → 0.8161). - filters_filter_path-bbox: 0.802 → 1.000
…ing) CSS Paint Order Level 1 / SVG 2 §11.3: `paint-order` reorders fill/stroke (and markers, but text has no markers). Previously the text painter hardcoded fill→stroke; a `<tspan paint-order="stroke fill">` rendered with the default order, leaving the green fill covered by the blue stroke instead of the spec-correct green fill visible above the stroke. Implementation: resolve fill/stroke into `SkPaint`s up-front, then apply them in the order returned by `resolve_text_paint_order`. Mirrors the iter 42 shape-painter fix (with the markers phase folded out since text doesn't have markers). Markers tokens in the `paint-order` value are silently consumed; invalid tokens fall back to default per CSS Values 4 §3.4. Pass-rate: 1163 → 1165 (+2, 0.8161 → 0.8175). - painting_paint-order_on-text: 0.675 → 0.989 - painting_paint-order_on-tspan: 0.834 → 0.966
…object-bbox) - resolve url() paint references for text fill - text stroke resolves url() to gradient/pattern - mask= on tspan/<a>; tighten text run bbox - filter= on tspan/<a> - coarse object-bbox for <text>
`read_inherited` and `resolve_font_size_at` walked the cloned target's source-DOM parents alongside the `<use>` chain — so a `<use font-size="48">` referencing `<text>` whose source DOM is inside `<g font-size="18">` resolved to 18, with the source-DOM sibling overriding the use's value. Mirror the iter 7 fix from `inherited_paint`: bound the source-DOM walk at the innermost `UseFrame::target_id`. When `node` descends from the target, walk up to and including the target then break; when `node` is the target itself (or unrelated), skip the source- DOM walk entirely. The use chain still cascades from the use element outward. resvg-test-suite (consensus, pass_floor=0.95): 1176 → 1177 of 1425 (0.8253 → 0.8260). Fixes structure_defs_style-inheritance-on-text (0.58 → 0.999).
…3.10
The `font:` shorthand resets every sub-property the grammar can
accept (font-style/weight/stretch/size/family) plus several it
can't (font-variant, font-kerning, font-size-adjust, line-height).
Parsing the shorthand and only returning the named sub-properties
let parent declarations of those longhands leak through — a
`<g style="font-kerning: none; font-weight: bold">` parenting a
`<text style="font: 50px sans">` resolved to bold and no-kerning,
when CSS says both should reset to their initial values.
`read_local_font` now returns the initial value ("normal") for
shorthand-resettable longhands when the shorthand is parsed but
didn't include them. `inherited_style_only` (used for the non-
presentation longhands like font-kerning) treats a `font:` on the
same or an ancestor element as a cascade barrier, returning
"normal" so cascades above the shorthand don't leak through.
resvg-test-suite (consensus, pass_floor=0.95): 1177 → 1178 of 1425
(0.8260 → 0.8267). Fixes text_font_font-shorthand (0.55 → 0.973).
`parse_dash_intervals_with_extent` resolved each token via `parse_length_px`, which treats `em`/`ex`/`rem`/`ch` against the constant-16 fallback rather than the cascaded `font-size`. A `stroke-dasharray="2em 1em"` on a `<g font-size="20">` rendered 32-px / 16-px dashes instead of the spec's 40-px / 20-px. Thread `ctx`/`node` through and resolve font-relative units the same way `len_attr` does — `inherited_font_size` for em/ex, `root_font_size` for rem, `ch_advance` for ch. % continues to resolve against the viewport diagonal. resvg-test-suite (consensus, pass_floor=0.95): 1178 → 1179 of 1425 (0.8267 → 0.8274). Fixes painting_stroke-dasharray_em-units (0.63 → 1.000).
…rSpaceOnUse `<mask maskUnits="userSpaceOnUse" x="30%" width="40%">` ignored the percent values — `masker::resolve` had no SVG viewport on hand and silently fell back to the masked element's bbox. Per SVG 2 §14.4 a `<length-percentage>` on these attributes resolves against the user coordinate system at the point of reference, which for `maskUnits=userSpaceOnUse` is the nearest SVG viewport. Thread `(vw, vh)` from `nearest_svg_viewport` through `resolve` and fold % into px against the appropriate axis (x/width against vw, y/height against vh). resvg-test-suite (consensus, pass_floor=0.95): 1179 → 1180 of 1425 (0.8274 → 0.8281). Fixes masking_mask_maskUnits=userSpaceOnUse- with-percent (0.59 → 0.995).
`preserveAspectRatio="… slice"` scales the image to fully cover its viewport box, with overflow clipped at the box edges (SVG 2 §8.13). `compute_image_dst_rect` returns the (over-sized) scaled rect, but the painter drew it without clipping — slice-mode `<image>` bled past the box and overlapped sibling content. Add a viewport clip when the computed dst rect extends past any edge. resvg-test-suite (consensus, pass_floor=0.95): 1180 → 1183 of 1425 (0.8281 → 0.8302). Fixes structure_image_preserveAspectRatio= xMinYMin-slice (0.69 → 0.971) plus two cross-cluster wins from the same overflow path.
`<textPath xlink:href="#p">` folded the referenced `<path>`'s own `transform` attribute into the path geometry but ignored `transform-origin`. A `<path transform="rotate(90)" transform- origin="center">` rotated around (0, 0) instead of the viewport center, sending the textPath's arc-length walk along an off-screen curve. Wrap the path's transform as `translate(o) * t * translate( -o)` per CSS Transforms 1 §3.5, using the existing `transform_origin_for` helper. resvg-test-suite (consensus, pass_floor=0.95): held at 1183 of 1425 (0.8302). structure_transform-origin_on-text-path 0.65 → 0.775 — text now follows the rotated path; residual is the same font-metric drift that bottoms out other text-on-path fixtures around 0.85.
…ith aspect preservation - rasterize inline SVG to host <image>'s pixel size - preserve aspect ratio when rasterizing inline SVG
A nested `<svg viewBox="0 0 200 100">` with no explicit `width`/ `height` had `viewport_w/h = 0`, fell through `paint_nested_svg` to `paint_children` raw, and the `viewBox` transform was silently dropped — children rendered in the parent's coordinate space at full scale. SVG 2 §5.1.2: when `viewBox` is present, missing `width`/`height` default to `100%` of the containing block. resvg-test-suite (consensus, pass_floor=0.95): 1185 → 1186 of 1425 (0.8316 → 0.8323). Fixes structure_svg_nested-svg-one-with-rect- and-one-with-viewBox (0.75 → 1.000).
…ndGlyphs - textLength on <text> (lengthAdjust=spacing) - textLength lengthAdjust=spacingAndGlyphs - per-tspan textLength
`<feImage>` upscales the source image into the filter primitive's
subregion via Skia's `image_filters::image`. We were passing plain
bilinear, which loses high-frequency edges on tile / logo sources —
fixtures showed a 1-2 pixel halo of differences along every edge of
the SVG-logo cluster. Switch to Mitchell bicubic (B=C=1/3), which
matches Chrome / Blink's default feImage upscaling much more
closely.
resvg-test-suite (consensus, pass_floor=0.95): 1187 → 1195 of 1425
(0.8330 → 0.8386). Fixes filters_feImage_simple-case (0.76 → 1.0),
embedded-png (0.76 → 1.0), with-subregion-{1..5} (0.92 → 1.0).
Per-character `draw_str` calls broke Arabic joining and Indic cluster shaping — Skia's HarfBuzz integration only sees one codepoint at a time, producing isolated forms. Detect runs that contain Hebrew (U+0590-05FF), Arabic (U+0600-06FF + supplements + presentation forms), Syriac, Thaana, NKo, Samaritan, Mandaic, Devanagari + other Indic blocks (U+0900-0DFF), and Tibetan, then draw the contiguous portion as a single string anchored at the first glyph's resolved position. Skia's shaper picks the right joining / cluster forms. resvg-test-suite (consensus, pass_floor=0.95): held at 1195 of 1425 (0.8386); average similarity 90.86% → 90.92%. Arabic / bidi cluster scores shift in both directions: text_unicode-bidi_ bidi-override 0.90 → 0.92, text_tspan_bidi-reordering 0.81 → 0.88; text_text_bidi-reordering 0.83 → 0.76, text_textLength_arabic 0.64 → 0.60. Net pass count unchanged — joining is now correct but bidi reordering (RTL flip) still missing; needs HarfBuzz direction flag + cluster-mapping reversal in `compute_kerned_advances` to land. This commit is the first half of the bidi work.
…IRI fallback, length-or-percent Pure dedup pass after the 50 surgical commits left repeated patterns: - Add `layout::transform::wrap_with_origin(m, origin)` and route the five `T(o) ∘ M ∘ T(-o)` callers (gradient, pattern, clipper, container, textPath) through it - Drop `transform.rs::read_attr_or_style` (duplicated `style/cascade.rs:: get_attr_or_style`) and use the shared helper - Reuse `paint_server::paint_fallback` for the two funcIRI trail-color branches in svg_text_painter (text fill + text stroke) - Promote `viewport.rs::length_or_percent` to `pub(crate)` and replace the marker painter's local `parse_len` closure with it No behavior change — sites collapse to the existing helpers' logic.
…mmary, view, multi-oracle scoring - Add `reftest bake` to drive Chrome/Puppeteer baking of resvg-test-suite fixtures (scripts/reftest_bake_chrome.mjs + reftest-bake-chrome.sh) - Add `reftest inspect` for per-fixture diagnostics (prints oracle status, vs_expected / vs_chrome scores, and points at diff PNG) - Add `reftest summary` (CLI + JSON) with worst-N consensus list - Add `reftest view` to serve an interactive dashboard against a result dir - Add multi-oracle scoring (consensus / disputed / UB) and surface per-oracle similarity in report.rs / dashboard.html - Update reftest-run.sh / reftest-view.sh and dashboard CSS/JS - Add `dev-render-htmlcss-svg-feature` skill (sibling to the HTML/CSS feature loop), update `render-reftest` skill notes, refresh htmlcss::svg README and grida_dev AGENTS.md, ignore the bake-cache dir
…one package.json) Rewrites the Chrome-bake script as TypeScript using `@playwright/test`'s `chromium` (already present in the workspace via @grida/reftest) and invokes it through `pnpm --filter @grida/reftest exec tsx`. This matches the pattern used by `.agents/skills/render-reftest/scripts/ refbrowser_render.ts` — the script resolves its dependency through the monorepo's existing `node_modules` instead of needing its own package.json/lockfile/node_modules tree alongside the script. - Replace `reftest_bake_chrome.mjs` (puppeteer + standalone package.json) with `reftest_bake_chrome.ts` (playwright via workspace resolution) - Update `bake.rs` to spawn `pnpm --filter @grida/reftest exec tsx` and drop the `ensure_puppeteer_installed` precheck - Smoke-tested: chromium launches, fixture-discovery + filter work, exits cleanly when no fixtures match
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
Introduces
htmlcss::svg, a new in-tree Skia-backed SVG renderer that replaces allskia_safe::svg::Domusage in the htmlcss module. Pipeline shape is Blink-derived (parse → style → layout → paint); resvg/usvg are secondary references. Companion design study atdocs/wg/feat-2d/htmlcss-svg.md.Why
htmlcssalready accepts SVG content in two places — standalone.svgbytes and inline<svg>— and both delegated to Skia'ssvg::Dom(SkSVGDOM). Per Chromium's own docs,SkSVGDOMis "for embedders that need standalone SVG rendering without DOM/CSS/JS integration" — Blink itself does not use it. We want the same thing Blink wants: a pipeline that integrates with the rest of htmlcss (Stylo cascade, font repository, image provider) and validates against the resvg-test-suite (1,679 fixtures already on disk underfixtures/local/resvg-test-suite/).What's in this PR
Design study —
docs/wg/feat-2d/htmlcss-svg.md. Captures the architecture decisions (Blink lineage, what we adopt vs. differ from Blink / usvg / resvg) before code lands. Each section ends with an explicitAdopt / Differline so deviations are auditable in one pass.New module —
crates/grida/src/htmlcss/svg/:dom/— XML → typedSvgDocumentviaroxmltree. Pathd=parser with arc-to-cubic decomposition.<switch>resolved at parse time.style/— temporary in-tree CSS subset; the Stylo bridge is future work.layout/— viewport +viewBox/preserveAspectRatiomatrix, transform composition, bbox propagation,<use>shadow-instance expansion.resources/—url(#id)resolution for paint servers (gradients, patterns), clippers, maskers, markers, and the filter-effect graph.paint/— Blink-shaped painter family:root_painter,container_painter,shape_painter,image_painter,text_painter,marker_painter,pattern_painter. One DFS pass; isolate-on-effect group rule (resvg formulation).context.rs—RenderContextbundlingImageProvider/FontResolver/CssLoader.Wiring changes in
htmlcss/{mod,paint,collect,style}.rs:htmlcss::render_svgnow routes throughhtmlcss::svg::render_to_pictureinstead ofskia_safe::svg::Dom.htmlcss::paint::paint_inline_svgnow routes throughhtmlcss::svg::render_into.ReplacedContent::svg_xml,collect::serialize_svg_subtree, andcollect::detect_svg_elementstay as-is — they already extract the<svg>subtree as XML, which is what the new entry points consume.Supporting research under
docs/wg/research/chromium/:svg/module-structure.md— directory organization inthird_party/blink/renderer/(the directory-level companion topipeline.md).svg/clip-path.md,svg/text-on-path.md,svg/fe-image.md,svg/fe-tile.md— feature-level Blink surveys cited from the design study.external-css.md—@import/<link rel=stylesheet>lifecycle, cycle detection, cascade ordering.What's deliberately not in this PR
Everything Blink does that doesn't apply to a static, single-shot, embedder-side renderer:
baseVal/animValtear-offs.Pictureand let Skia handle internal compositing.The Stylo bridge is also out of scope; the in-tree CSS subset under
style/is explicitly a placeholder for it.Status
Best-effort renderer: features still under construction (notably text, filters) render unsupported elements as gaps rather than falling back to
SkSVGDOM. The intent is to make missing pixels visible so the work surfaces. Per-feature implementation proceeds through.agents/skills/dev-render-htmlcss-feature/SKILL.md, validated against the resvg-test-suite reftest corpus.Test plan
cargo test -p grida --test htmlcss_svg_checkpoint1passes.render_svg/paint_inline_svgtests inhtmlcss/still pass with the new backend.grida_devreftest runner).cargo check -p grida -p grida-canvas-wasm -p grida_devclean.Summary by CodeRabbit
New Features
Tools
Tests
Chore
Documentation