feat(htmlcss): in-tree Skia-backed SVG renderer (htmlcss::svg)#698
Merged
softmarshmallow merged 78 commits intomainfrom May 3, 2026
Merged
feat(htmlcss): in-tree Skia-backed SVG renderer (htmlcss::svg)#698softmarshmallow merged 78 commits intomainfrom
htmlcss::svg)#698softmarshmallow merged 78 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.
6 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a full in-tree SVG renderer under Changeshtmlcss::svg renderer, core parsers, resources, and integration
Sequence Diagram(s)sequenceDiagram
participant Host as Host (caller)
participant Entry as htmlcss::svg Entry
participant Parser as DemoDom Parser
participant Style as Stylesheet
participant Resources as Resources Index
participant Painter as Svg Root/Container Painters
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->>Painter: paint_root(canvas, dom, viewport, RenderContext)
Painter->>Painter: compute CTM, apply root clip/filter/opacity
Painter->>Painter: traverse children, resolve cascade
Painter->>Resources: resolve paint-server/filter/mask/clip
Painter->>Canvas: draw fills/strokes/text/images/markers
Painter-->>Entry: Result / SvgError
Entry-->>Host: return
sequenceDiagram
participant Container as Container Painter
participant Clipper as Clipper Resolver
participant Filter as Filter Builder
participant Masker as Mask Resolver
participant Recorder as PictureRecorder
participant Canvas as Skia Canvas
Container->>Clipper: resolve clip-path (url or basic-shape)
Clipper->>Canvas: canvas.clip_path(path, Intersect)
Container->>Filter: resolve_filter_chain(node, bbox)
Filter->>Recorder: may record subtree → ImageFilter
Container->>Masker: resolve mask -> MaskInvocation
Masker->>Recorder: paint mask into layer (DstIn)
Container->>Canvas: save_layer/apply transforms
Container->>Canvas: draw element contents
Container->>Canvas: restore layers
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 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.
…ate per-glyph advance accumulation Skia's `Font` defaults to integer-pixel advance quantization. For SVG text, each glyph's draw position is computed by accumulating `measure_str` advances of preceding glyphs — without subpixel positioning, every advance rounds to an integer, and a 14-character text run accumulates a visible left/right drift versus a renderer that uses fractional advances (which the resvg-test-suite expecteds were rendered against). The drift is most visible under `text-anchor=middle` / `end`, where the integer-quantized total width offsets the entire chunk by half a pixel per glyph from the expected center / end. Calling `font.set_subpixel(true)` lets Skia's text drawing accumulate sub-pixel advances internally; the change is local to the painter's font instance and doesn't touch shaping or layout math elsewhere. Net: +27 consensus fixtures (1268 → 1295 of 1425; 0.8898 → 0.9088). Cluster wins span text-anchor / tspan / textPath / text-decoration / font-family / baseline-shift fixtures previously bound by integer advance drift.
…btree SVG 2 §11.10.2.6 / CSS Inline Layout 3 §3.3: a `<tspan dominant-baseline="…">` declares the dominant baseline for ITS subtree, not the whole `<text>`. Previously only the root's dominant-baseline was honored (via `baseline_y`); a tspan setting its own dominant-baseline was silently ignored, so a fixture like `<text dominant-baseline=auto>Some<tspan dominant-baseline=middle> long</tspan>text</text>` rendered all four runs on the alphabetic baseline instead of dropping the tspan onto the middle baseline. Symmetric to iter 35's per-tspan `alignment-baseline` mechanism: - Add `ElemAttrs.dominant_baseline: Option<BaselineKind>`, parsed from the local `dominant-baseline` attribute alongside the existing `alignment_baseline`. - In `emit_glyph`, walk frames innermost-first SKIPPING the root (its baseline is already in `baseline_y`) and pick the first alignment- or dominant-baseline override. alignment wins over dominant on the same frame per spec. - Treat the SVG-1.1-deprecated "no-change" / "use-script" / "reset-size" keywords as no-override (return None). Otherwise `resolve_baseline` would map them to Alphabetic and wipe an inherited middle/hanging parent baseline — that regressed `dominant-baseline_no-change` in development. Cluster effect (no fixtures crossed the 0.95 floor — residual is font-metric drift on top of the now-correct baseline): - text_dominant-baseline_sequential: 0.733 → 0.914 - text_dominant-baseline_complex: 0.794 → 0.896 - text_dominant-baseline_nested: 0.746 → 0.872 Pass count held at 1295/1425; avg_consensus 0.972 → 0.973.
…hs ranges covering all glyphs SVG 1.1 §10.5: `lengthAdjust=spacingAndGlyphs` scales BOTH the advance between glyphs AND the glyph width. Previously the Font's x-axis scale was only set when textLength was on the text root; a `<textPath textLength=… lengthAdjust=spacingAndGlyphs>` (or single `<tspan>` covering all glyphs) scaled advances correctly but kept glyphs at natural width — text-on-path squashed against the start of the curve instead of stretching to the targeted arc length. Extend the per-range loop to also scale the font's x-axis when: - the range uses `spacingAndGlyphs`, AND - the range covers all glyphs (`first == 0` and `last+1 == count`). The all-glyphs guard is intentional: `Font::set_scale_x` is global to the painter's single Font instance, so applying it for a partial range would scale glyphs outside that range too. Multi-range `spacingAndGlyphs` would need per-range Font instances; that's a larger refactor and the resvg-test-suite has no fixtures that exercise it. Fixes: - text_lengthAdjust_text-on-path: 0.710 → 1.000 Net: +1 consensus fixture (1295 → 1296 of 1425; 0.9088 → 0.9095).
…ywords + percentages `<rect font-size="xx-large" width="10em">` was rendering at the parent's 10em (= 10×12px = 120px) instead of the rect's own 10em (= 10×32px = 320px). The bug: `inherited_font_size` resolved the declared font-size value via `parse_length_px`, which returns None for CSS keywords (`xx-small`…`xx-large`), the relative `smaller`/`larger`, and percentages — so the function fell through to the parent's font-size. Restructure to mirror `svg_text_painter::resolve_font_size_at`: collect the chain of declared `font-size` values from the element up to the root, then resolve outermost-to-innermost via a `resolve_font_size_step` that handles keywords, percentages, and em / px units. Each step sees the parent's already-resolved size, so a `font-size="50%"` correctly halves the parent's resolved px. Lifts (no fixtures crossed the 0.95 floor — residual is sub-pixel glyph rendering on now-correctly-sized geometry): - text_font-size_named-value: 0.823 → 0.846 (12 rects with per-keyword widths now render at correct distinct sizes) Pass count held at 1296/1425; avg_consensus 0.973.
CSS Text Decoration 3 §2.2: `text-decoration-line`'s grammar is a **space-separated** list of `none | underline | overline | line-through | blink`. Commas are NOT a valid separator — `"underline,overline"` is a syntactically invalid value and per CSS Values 4 cascading rules falls back to `none` (the initial value). The resvg-test-suite spells this out in the fixture's name (`all-types-inline-comma-separated`): expected.png renders zero decoration lines. Previously the parser tokenised on whitespace OR commas, so an invalid comma-separated value was silently treated as space-separated and painted all three lines. Tighten the parser: - Bail to `None` if the value contains any comma. - Use `split_ascii_whitespace()` (instead of a custom predicate that also accepted commas). Fixes: - text_text-decoration_all-types-inline-comma-separated: 0.869 → 1.000 Net: +1 consensus fixture (1296 → 1297 of 1425; 0.9095 → 0.9102).
…/svg+xml CSS Images 4 §5: an SVG document loaded via `<image href="data:image/ svg+xml,…">` is a *resource document* — sandboxed, with no script execution and no font cascade. Chrome / Firefox / Safari render the shapes but skip text glyphs entirely (no fonts available in the sandboxed context). Previously our `rasterize_inline_svg` passed the host's full RenderContext (including SystemFonts), so embedded SVGs that contained `<text>` elements had their glyphs painted, producing visible "Text" inside the box where Chrome left it blank. Pass an empty `PreloadedFonts` to the inner `render_to_picture_with_ context` call. The embedded text painter's `pick_typeface` returns None and `paint()` short-circuits before draw. The host's image provider stays wired so nested `<image>` references resolve as before. Fixes: - structure_image_embedded-svg-with-text: 0.753 → 1.000 Net: +1 consensus fixture (1297 → 1298 of 1425; 0.9102 → 0.9109).
…baseline CSS Fonts 4 §3.3 / SVG 2 §11.4: `font-size` inherits, and a tspan can override the cascaded value for its own subtree. Previously the painter resolved font-size only at the text root and rendered every glyph at that size — `<text font-size=48><tspan font-size=80>ex </tspan></text>` rendered all glyphs at 48px instead of mixing the inner tspan's "ex" at 80px. Symmetric to iter 96's per-tspan dominant-baseline: - ElemAttrs gains `font_size: f32`, computed in `from_node` against the parent frame's resolved size via the existing `resolve_font_size_step` (handles em / % / `xx-small`…`xx-large` / `larger`/`smaller`). - GlyphAttr / ResolvedGlyph gain `font_size_scale: f32` = innermost-frame font-size ÷ root frame font-size. - `compute_kerned_advances` is followed by an in-place pass that scales each per-glyph advance by `font_size_scale`, applied BEFORE textLength so a downstream textLength still hits its target after the size cascade. - `draw_glyphs` swaps to a font sized at `root_font.size × scale` for any glyph whose scale ≠ 1.0, with a tiny memo to avoid rebuilding the same scaled font for adjacent same-scale runs. - The bidi/cursive run-merge path breaks on a font-size boundary so the scaled font is used per run. Fixes: - text_tspan_mixed-font-size: 0.581 → 1.000 Net: +1 consensus fixture (1298 → 1299 of 1425; 0.9109 → 0.9116). avg_disputed 0.815 → 0.817 (textLength_arabic etc. lifted slightly where the inner tspan was sized differently from the root).
…rl paint refs) CSS Text Decoration 3 §3.1 / SVG 1.1 §10.12.1: text decorations are filled with the same paint as the text itself. When a `<text>` has `fill="url(#patt1)"`, both the glyphs and the underline / overline / line-through should share that pattern (or gradient). Previously `decoration_paints` only resolved CSS color values — `parse_paint` returns None for `url(#…)`, falling through to the `_` arm that set the line color to solid black. Result: a `text fill="url(#patt)" text-decoration="underline"` rendered correct pattern glyphs but a solid-black underline beneath them. Restructure decoration_paints to mirror `build_text_fill_paint`'s funcIRI handling: - Detect `url(` and resolve via `paint_server::resolve` against the anchor's covered-glyph bbox (objectBoundingBox-units patterns / gradients use the bbox; userSpaceOnUse ignores it). - For pattern targets, build the shader via `pattern::build_shader`. - Honor the trailing `url(#g) red` fallback color per SVG 2 §11.3. - Keep the existing `currentColor` / solid-color path. `paint_anchor_decoration` now precomputes the union bbox of the anchor's glyphs (via the existing `run_bbox`) and passes it to `decoration_paints`. Strokes routed through the same builder so `stroke="url(#g)"` on a decoration anchor would also fill correctly. Lifts text_text_rotate-with-multiple-values-underline-and-pattern 0.672 → 0.717 (residual is per-glyph rotate/decoration positioning; the pattern-fill on decoration is now correct). Pass count held at 1299/1425; avg_consensus 0.973 unchanged.
…e-axis x/dx leak through `<textPath>` is not a valid host for x/y/dx/dy/rotate per SVG 2 §11.4 — clear those on the frame so a `<textPath x=… y=…>` doesn't shift its own glyphs. In return, allow an enclosing `<text>`'s `x`/`dx` (inline- axis) to flow through to textPath glyphs as along-path arc-length offsets; only `y`/`dy`/`rotate` (perpendicular / per-glyph rotation) remain blocked at the textPath cascade barrier since they would fight the path's per-tangent placement. Mirrors Blink's `SvgTextLayoutAttributesBuilder` — for textPath chars the algorithm collects `x`/`dx` from ancestor `<text>` arrays but drops `y`/`dy`/`rotate`. Fixes: text_textPath_with-coordinates-on-textPath 0.671 → 0.958 (passes) text_textPath_with-coordinates-on-text 0.677 → 0.945 (just below) Pass count 1234 → 1235; avg similarity 91.52% → 91.55%; no regressions.
A `<textPath>` element must not have descendant `<textPath>` elements; the spec is explicit and Blink/WebKit/Gecko all drop the nested element together with its text content. Our walker was recursing into the inner textPath and emitting a duplicate run on the second path. Skip the inner textPath at the walk entry when `path_stack` is non-empty (we're already inside a textPath). The outer textPath's own text content still renders. Fixes text_textPath_nested 0.774 → 0.964 (passes). 1235 → 1236; avg 91.55% → 91.57%; no regressions.
…alid (lacuna 0) SVG 2 §15.13.4: stdDeviation accepts one or two `<number>`s. Per CSS Values 4 §3.2 grammar matching, supplying more (e.g. `stdDeviation="5 10 15 20"`) fails the grammar and the attribute falls back to its lacuna value (0), making the filter a pass-through. We were silently taking the first two numbers and applying the blur. Fixes filters_feGaussianBlur_stdDeviation-with-multiple-values 0.859 → 1.000. 1236 → 1237; avg 91.57% (held at 4dp); no regressions.
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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/svg/paint/svg_image_painter.rs`:
- Around line 163-166: The current starts_with checks on trimmed are only
partially case-sensitive and can miss mixed-case data URIs; change the logic to
perform a case-insensitive match by lowercasing the input (e.g., compute a
lowercase version of trimmed via to_ascii_lowercase()) and then check
starts_with("data:image/svg+xml") against that lowercase string, and apply the
same change to the other occurrence around lines 296-297 (use the same
lowercased variable when testing the data:image/svg+xml prefix).
- Around line 294-310: The percent-decoding step in decode_data_uri currently
converts the payload to a String and back to UTF-8, corrupting non-ASCII/binary
data; change the plain-data path to percent-decode directly to raw bytes instead
of a String (use percent_decode(payload).collect::<Vec<u8>>() or the equivalent
API) so decode_data_uri returns the original bytes for non-base64 data, and
apply the same fix to the other percent-decode sites referenced in the diff
(replace uses of percent_decode(...).into_bytes() or String-based decoding with
direct byte collection).
In `@crates/grida/src/htmlcss/svg/paint/svg_shape_painter.rs`:
- Around line 918-1124: The helpers (inherited_paint, resolve_current_color,
opacity_property and their inner read functions) only inspect raw attributes and
inline style so they ignore stylesheet cascade; update each to first query the
cascade-aware computed value from the engine's style resolver (e.g. call the
existing context/style API such as a
computed-style/get_computed_property/get_cascade_value method on ctx or a
style_resolver) for the given node and property (or "color"/opacity) and use
that result when present, falling back to the current attribute/inline-style
parsing logic only if the computed-style lookup returns None/unspecified; apply
the same change to the for-use-chain ancestor walks so inherited values come
from computed style rather than just raw attrs.
- Around line 737-779: The branch handling unparseable paint values currently
falls back to default_solid when parse_paint(v) returns None; change this to
treat malformed tokens as "absent" so inheritance can apply. Specifically, in
the match on parse_paint(v) replace the final arm that returns
default_solid.map(PaintChoice::Solid) with a call to inherited_paint(ctx, node,
property, default_solid) (returning its result), leaving the existing arms for
Paint::None, Paint::Color and Paint::CurrentColor unchanged; this ensures
parse_paint, inherited_paint, resolve_current_color, apply_funciri_fallback and
default_solid are used appropriately.
- Around line 292-323: The resolve_font_size_step function incorrectly maps
""/"medium"/"initial"/"inherit"/"unset" to parent.max(16.0); change this so that
"inherit" and "unset" (and empty string if used as inherit in this codepath)
return parent, "initial" and "medium" return the absolute 16.0 value, and keep
other keyword handling (xx-small..xx-large, smaller/larger) as-is; update the
match arms in resolve_font_size_step to use these specific returns instead of
parent.max(16.0) so inherited sizes don't jump to 16px and medium/initial are
treated as 16px.
- Around line 854-905: The current parse_dash_intervals_with_extent uses
filter_map(resolve) which silently ignores invalid tokens (e.g. "5 foo 3");
change it to detect and reject any malformed token: when splitting s into tokens
(the same split on is_ascii_whitespace or ','), map each token through the
resolve closure and if any token returns None, immediately return None from
parse_dash_intervals_with_extent; otherwise collect all resolved f32s into nums
and keep the existing empty/all-zero check and odd-length duplication logic.
This change touches parse_dash_intervals_with_extent and the resolve closure
usage—replace the filter_map(resolve).collect() step with a loop or iterator
that returns None on the first unresolved token.
In `@crates/grida/src/htmlcss/svg/resources/clipper.rs`:
- Around line 195-197: The code currently drops supported children inside
clipPath by using "continue" in the loop where is_supported_clipper_child(kind)
is checked; change that logic so that if the child is a group element (check for
the group tag used in your parser, e.g., "g" or Tag::G) you either recurse into
that group's children to process them (call the same clip-parsing routine on the
group's child nodes) or, if groups are deliberately unsupported, return None
instead of continuing; update the branch around is_supported_clipper_child(kind)
to handle the group case explicitly and only skip/continue for truly unsupported
node kinds.
- Around line 239-245: The current code calls chained_clip_path(...) with
object_bbox and then intersects it into child_path even when the chained clip
uses objectBoundingBox units that should be resolved in the parent's
clipPath-mapped space; instead, detect when the referenced clipPath uses
clipPathUnits="objectBoundingBox" (via get_attr(child, "clip-path") lookup) and
we cannot thread the child's bbox through yet, and in that case bail out of this
branch by returning None rather than performing skia_safe::op intersection;
update the logic around chained_clip_path, clipper_inverse and child_path so
that you only compute and intersect a chained clip when the clipPathUnits and
transforms are already compatible (or when you can apply with_transform(&inv)
correctly), otherwise return None.
In `@crates/grida/src/htmlcss/svg/resources/svg_filter_builder.rs`:
- Line 573: parse_fe_image currently yields None for unusable sources which
causes parse_one to drop the primitive and alter filter-chain semantics; change
the behavior so a transparent-black primitive is returned instead. Either make
parse_fe_image(...) return Some(primitive_that_represents_transparent_black)
when no usable source is found, or update parse_one(...) to convert a None from
parse_fe_image into a transparent-black primitive before adding to the chain;
reference the parse_fe_image and parse_one symbols and apply the same fix to the
other occurrence around the 1350–1357 block so missing feImage sources always
produce a transparent-black primitive rather than being dropped.
- Around line 527-533: The code seeds color-interpolation-filters from the
immediate filter node object (parent) instead of using the DOM cascade, so
inherited values on ancestors are ignored; change calls to parse_color_interp_or
to pass the filter node id (filter_id) rather than the node object so the helper
can walk ancestors (e.g., replace parse_color_interp_or(dom, parent,
ColorInterp::LinearRgb) with parse_color_interp_or(dom, filter_id,
ColorInterp::LinearRgb)), and make the same substitution for the other
occurrence around lines 609-631 to ensure the cascade is respected.
- Around line 1159-1170: The code in svg_filter_builder.rs currently reads
flood-color with read_presentation and parses flood-opacity using
parse_length_px, which ignores 'inherit'/'currentColor' and mis-parses opacity;
replace the raw presentation reads with the existing color/alpha resolvers used
elsewhere (use the module's paint/color resolver instead of read_presentation +
parse_paint and use the alpha/opacity parser instead of parse_length_px) so that
feDropShadow honors inherit/currentColor and proper opacity semantics, then
compute alpha the same way and call Color::from_argb as before (refer to
read_presentation, parse_paint, parse_length_px, Color::from_argb and the local
node variable to locate the change).
🪄 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: 5fd91ce8-a602-4213-805e-915c36ce5731
📒 Files selected for processing (6)
crates/grida/src/htmlcss/svg/paint/svg_image_painter.rscrates/grida/src/htmlcss/svg/paint/svg_marker_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/resources/clipper.rscrates/grida/src/htmlcss/svg/resources/svg_filter_builder.rs
✅ Files skipped from review due to trivial changes (1)
- crates/grida/src/htmlcss/svg/paint/svg_marker_painter.rs
Comment on lines
+163
to
+166
| if trimmed.starts_with("data:image/svg+xml") | ||
| || trimmed.starts_with("DATA:image/svg+xml") | ||
| || trimmed.starts_with("data:IMAGE/SVG+XML") | ||
| { |
There was a problem hiding this comment.
data: and image/svg+xml matching is only partially case-insensitive
Mixed-case valid inputs (e.g. Data:image/SvG+XmL,...) can bypass the inline-SVG path and fail rendering.
Proposed fix
pub fn resolve_image_sized(
@@
) -> Option<Image> {
let trimmed = href.trim();
- if trimmed.starts_with("data:image/svg+xml")
- || trimmed.starts_with("DATA:image/svg+xml")
- || trimmed.starts_with("data:IMAGE/SVG+XML")
- {
+ let lower = trimmed.to_ascii_lowercase();
+ if lower.starts_with("data:image/svg+xml") {
if let Some(bytes) = decode_data_uri(trimmed) {
return rasterize_inline_svg(&bytes, ctx, target_px);
}
}
@@
pub fn decode_data_uri(uri: &str) -> Option<Vec<u8>> {
- let body = uri
- .strip_prefix("data:")
- .or_else(|| uri.strip_prefix("DATA:"))?;
+ let body = if uri.get(0..5).is_some_and(|p| p.eq_ignore_ascii_case("data:")) {
+ &uri[5..]
+ } else {
+ return None;
+ };Also applies to: 296-297
🤖 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_image_painter.rs` around lines 163 -
166, The current starts_with checks on trimmed are only partially case-sensitive
and can miss mixed-case data URIs; change the logic to perform a
case-insensitive match by lowercasing the input (e.g., compute a lowercase
version of trimmed via to_ascii_lowercase()) and then check
starts_with("data:image/svg+xml") against that lowercase string, and apply the
same change to the other occurrence around lines 296-297 (use the same
lowercased variable when testing the data:image/svg+xml prefix).
Comment on lines
+198
to
+206
| let (rw, rh) = match target_px { | ||
| Some((tw, th)) => { | ||
| let sx = tw as f32 / svg_w as f32; | ||
| let sy = th as f32 / svg_h as f32; | ||
| let s = sx.max(sy).max(1.0); | ||
| ( | ||
| ((svg_w as f32 * s).round() as u32).max(1), | ||
| ((svg_h as f32 * s).round() as u32).max(1), | ||
| ) |
There was a problem hiding this comment.
Clamp inline-SVG raster size before surface allocation
rw/rh are unbounded and flow directly into raster_n32_premul (Line 228). Large values can trigger huge allocations or invalid dimension casts.
Proposed fix
fn rasterize_inline_svg(
@@
) -> Option<Image> {
+ const MAX_INLINE_SVG_RASTER_DIM: u32 = 8192;
@@
- let (rw, rh) = match target_px {
+ let (rw, rh) = match target_px {
@@
- None => (svg_w, svg_h),
+ None => (svg_w, svg_h),
};
+ let rw = rw.min(MAX_INLINE_SVG_RASTER_DIM).min(i32::MAX as u32).max(1);
+ let rh = rh.min(MAX_INLINE_SVG_RASTER_DIM).min(i32::MAX as u32).max(1);
@@
let mut surface = skia_safe::surfaces::raster_n32_premul((rw as i32, rh as i32))?;Also applies to: 228-228
Comment on lines
+294
to
+310
| pub fn decode_data_uri(uri: &str) -> Option<Vec<u8>> { | ||
| let body = uri | ||
| .strip_prefix("data:") | ||
| .or_else(|| uri.strip_prefix("DATA:"))?; | ||
| let comma = body.find(',')?; | ||
| let (header, payload) = (&body[..comma], &body[comma + 1..]); | ||
| let is_base64 = header | ||
| .split(';') | ||
| .any(|p| p.trim().eq_ignore_ascii_case("base64")); | ||
| if is_base64 { | ||
| decode_base64(payload) | ||
| } else { | ||
| // Plain (URL-encoded) payload — quick percent-decode, suitable | ||
| // for `data:image/svg+xml,<svg ...>` cases. Reftest fixtures | ||
| // rarely use this form for raster images. | ||
| Some(percent_decode(payload).into_bytes()) | ||
| } |
There was a problem hiding this comment.
Percent-decoding currently corrupts non-ASCII/binary payloads
The plain data: path decodes %xx into a String and then re-encodes to UTF-8 (Line 309), which changes raw bytes for non-ASCII values.
Proposed fix
pub fn decode_data_uri(uri: &str) -> Option<Vec<u8>> {
@@
if is_base64 {
decode_base64(payload)
} else {
- Some(percent_decode(payload).into_bytes())
+ Some(percent_decode(payload))
}
}
-fn percent_decode(s: &str) -> String {
- let mut out = String::with_capacity(s.len());
+fn percent_decode(s: &str) -> Vec<u8> {
+ let mut out = Vec::with_capacity(s.len());
let bytes = s.as_bytes();
let mut i = 0;
while i < bytes.len() {
if bytes[i] == b'%' && i + 2 < bytes.len() {
if let (Some(h), Some(l)) = (hex_digit(bytes[i + 1]), hex_digit(bytes[i + 2])) {
- out.push((h * 16 + l) as char);
+ out.push(h * 16 + l);
i += 3;
continue;
}
}
- out.push(bytes[i] as char);
+ out.push(bytes[i]);
i += 1;
}
out
}Also applies to: 313-329
🤖 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_image_painter.rs` around lines 294 -
310, The percent-decoding step in decode_data_uri currently converts the payload
to a String and back to UTF-8, corrupting non-ASCII/binary data; change the
plain-data path to percent-decode directly to raw bytes instead of a String (use
percent_decode(payload).collect::<Vec<u8>>() or the equivalent API) so
decode_data_uri returns the original bytes for non-base64 data, and apply the
same fix to the other percent-decode sites referenced in the diff (replace uses
of percent_decode(...).into_bytes() or String-based decoding with direct byte
collection).
Comment on lines
+292
to
+323
| fn resolve_font_size_step(value: &str, parent: f32) -> f32 { | ||
| let v = value.trim(); | ||
| match v { | ||
| "" | "medium" | "initial" | "inherit" | "unset" => return parent.max(16.0), | ||
| "xx-small" => return 9.0, | ||
| "x-small" => return 10.0, | ||
| "small" => return 13.0, | ||
| "large" => return 18.0, | ||
| "x-large" => return 24.0, | ||
| "xx-large" => return 32.0, | ||
| "smaller" => return parent / 1.2, | ||
| "larger" => return parent * 1.2, | ||
| _ => {} | ||
| } | ||
| if let Some(pct) = v.strip_suffix('%') { | ||
| return pct | ||
| .trim() | ||
| .parse::<f32>() | ||
| .ok() | ||
| .map(|n| (n / 100.0) * parent) | ||
| .unwrap_or(parent); | ||
| } | ||
| if let Some(num) = v.strip_suffix("em").or_else(|| v.strip_suffix("EM")) { | ||
| return num | ||
| .trim() | ||
| .parse::<f32>() | ||
| .ok() | ||
| .map(|n| n * parent) | ||
| .unwrap_or(parent); | ||
| } | ||
| parse_length_px(v).unwrap_or(parent) | ||
| } |
There was a problem hiding this comment.
Fix font-size keyword resolution.
inherit/unset and medium are all folded into parent.max(16.0). That makes inherited sizes jump to 16px and makes medium depend on the parent, which is not correct CSS behavior.
♻️ Proposed fix
- match v {
- "" | "medium" | "initial" | "inherit" | "unset" => return parent.max(16.0),
+ match v {
+ "" => return parent,
+ "inherit" | "unset" => return parent,
+ "medium" | "initial" => return 16.0,
"xx-small" => return 9.0,📝 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 resolve_font_size_step(value: &str, parent: f32) -> f32 { | |
| let v = value.trim(); | |
| match v { | |
| "" | "medium" | "initial" | "inherit" | "unset" => return parent.max(16.0), | |
| "xx-small" => return 9.0, | |
| "x-small" => return 10.0, | |
| "small" => return 13.0, | |
| "large" => return 18.0, | |
| "x-large" => return 24.0, | |
| "xx-large" => return 32.0, | |
| "smaller" => return parent / 1.2, | |
| "larger" => return parent * 1.2, | |
| _ => {} | |
| } | |
| if let Some(pct) = v.strip_suffix('%') { | |
| return pct | |
| .trim() | |
| .parse::<f32>() | |
| .ok() | |
| .map(|n| (n / 100.0) * parent) | |
| .unwrap_or(parent); | |
| } | |
| if let Some(num) = v.strip_suffix("em").or_else(|| v.strip_suffix("EM")) { | |
| return num | |
| .trim() | |
| .parse::<f32>() | |
| .ok() | |
| .map(|n| n * parent) | |
| .unwrap_or(parent); | |
| } | |
| parse_length_px(v).unwrap_or(parent) | |
| } | |
| fn resolve_font_size_step(value: &str, parent: f32) -> f32 { | |
| let v = value.trim(); | |
| match v { | |
| "" => return parent, | |
| "inherit" | "unset" => return parent, | |
| "medium" | "initial" => return 16.0, | |
| "xx-small" => return 9.0, | |
| "x-small" => return 10.0, | |
| "small" => return 13.0, | |
| "large" => return 18.0, | |
| "x-large" => return 24.0, | |
| "xx-large" => return 32.0, | |
| "smaller" => return parent / 1.2, | |
| "larger" => return parent * 1.2, | |
| _ => {} | |
| } | |
| if let Some(pct) = v.strip_suffix('%') { | |
| return pct | |
| .trim() | |
| .parse::<f32>() | |
| .ok() | |
| .map(|n| (n / 100.0) * parent) | |
| .unwrap_or(parent); | |
| } | |
| if let Some(num) = v.strip_suffix("em").or_else(|| v.strip_suffix("EM")) { | |
| return num | |
| .trim() | |
| .parse::<f32>() | |
| .ok() | |
| .map(|n| n * parent) | |
| .unwrap_or(parent); | |
| } | |
| parse_length_px(v).unwrap_or(parent) | |
| } |
🤖 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_shape_painter.rs` around lines 292 -
323, The resolve_font_size_step function incorrectly maps
""/"medium"/"initial"/"inherit"/"unset" to parent.max(16.0); change this so that
"inherit" and "unset" (and empty string if used as inherit in this codepath)
return parent, "initial" and "medium" return the absolute 16.0 value, and keep
other keyword handling (xx-small..xx-large, smaller/larger) as-is; update the
match arms in resolve_font_size_step to use these specific returns instead of
parent.max(16.0) so inherited sizes don't jump to 16px and medium/initial are
treated as 16px.
Comment on lines
+737
to
+779
| Some(v) => { | ||
| let v = v.trim(); | ||
| if v.eq_ignore_ascii_case("none") { | ||
| return None; | ||
| } | ||
| // SVG 2 / CSS: explicit `inherit` walks the ancestor chain | ||
| // for the same property and applies the inherited value. | ||
| // Without a CSS cascade we approximate by re-running the | ||
| // resolver on the parent's value of the same attribute. | ||
| if v.eq_ignore_ascii_case("inherit") { | ||
| return inherited_paint(ctx, node, property, default_solid); | ||
| } | ||
| if v.starts_with("url(") { | ||
| let bbox = approx_bbox(node); | ||
| let (_, _, vw, vh) = viewport_box_for(ctx, node); | ||
| match paint_server::resolve(ctx.dom, ctx.resources, v, bbox, (vw, vh)) { | ||
| Some(Resolved::Shader(s)) => return Some(PaintChoice::Shader(s)), | ||
| Some(Resolved::Pattern { | ||
| node: pid, | ||
| bbox: pbbox, | ||
| }) => { | ||
| // Defer-resolved pattern: render its subtree | ||
| // into a Picture and wrap as a tiled shader. | ||
| if let Some(s) = | ||
| super::super::resources::pattern::build_shader(ctx, pid, pbbox) | ||
| { | ||
| return Some(PaintChoice::Shader(s)); | ||
| } | ||
| return apply_funciri_fallback(ctx, node, v, default_solid); | ||
| } | ||
| _ => return apply_funciri_fallback(ctx, node, v, default_solid), | ||
| } | ||
| } | ||
| // Fall through to the simple paint parser. | ||
| match parse_paint(v) { | ||
| Some(Paint::None) => None, | ||
| Some(Paint::Color(c)) => Some(PaintChoice::Solid(c)), | ||
| Some(Paint::CurrentColor) => { | ||
| Some(PaintChoice::Solid(resolve_current_color(ctx, node))) | ||
| } | ||
| None => default_solid.map(PaintChoice::Solid), | ||
| } | ||
| } |
There was a problem hiding this comment.
Treat unparseable paint values as absent, not as the initial color.
If fill/stroke contains a malformed token, this falls back to default_solid immediately. That turns a typo on a child into black/none instead of letting the inherited value win.
♻️ Proposed fix
- None => default_solid.map(PaintChoice::Solid),
+ None => inherited_paint(ctx, node, property, default_solid),📝 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
| Some(v) => { | |
| let v = v.trim(); | |
| if v.eq_ignore_ascii_case("none") { | |
| return None; | |
| } | |
| // SVG 2 / CSS: explicit `inherit` walks the ancestor chain | |
| // for the same property and applies the inherited value. | |
| // Without a CSS cascade we approximate by re-running the | |
| // resolver on the parent's value of the same attribute. | |
| if v.eq_ignore_ascii_case("inherit") { | |
| return inherited_paint(ctx, node, property, default_solid); | |
| } | |
| if v.starts_with("url(") { | |
| let bbox = approx_bbox(node); | |
| let (_, _, vw, vh) = viewport_box_for(ctx, node); | |
| match paint_server::resolve(ctx.dom, ctx.resources, v, bbox, (vw, vh)) { | |
| Some(Resolved::Shader(s)) => return Some(PaintChoice::Shader(s)), | |
| Some(Resolved::Pattern { | |
| node: pid, | |
| bbox: pbbox, | |
| }) => { | |
| // Defer-resolved pattern: render its subtree | |
| // into a Picture and wrap as a tiled shader. | |
| if let Some(s) = | |
| super::super::resources::pattern::build_shader(ctx, pid, pbbox) | |
| { | |
| return Some(PaintChoice::Shader(s)); | |
| } | |
| return apply_funciri_fallback(ctx, node, v, default_solid); | |
| } | |
| _ => return apply_funciri_fallback(ctx, node, v, default_solid), | |
| } | |
| } | |
| // Fall through to the simple paint parser. | |
| match parse_paint(v) { | |
| Some(Paint::None) => None, | |
| Some(Paint::Color(c)) => Some(PaintChoice::Solid(c)), | |
| Some(Paint::CurrentColor) => { | |
| Some(PaintChoice::Solid(resolve_current_color(ctx, node))) | |
| } | |
| None => default_solid.map(PaintChoice::Solid), | |
| } | |
| } | |
| Some(v) => { | |
| let v = v.trim(); | |
| if v.eq_ignore_ascii_case("none") { | |
| return None; | |
| } | |
| // SVG 2 / CSS: explicit `inherit` walks the ancestor chain | |
| // for the same property and applies the inherited value. | |
| // Without a CSS cascade we approximate by re-running the | |
| // resolver on the parent's value of the same attribute. | |
| if v.eq_ignore_ascii_case("inherit") { | |
| return inherited_paint(ctx, node, property, default_solid); | |
| } | |
| if v.starts_with("url(") { | |
| let bbox = approx_bbox(node); | |
| let (_, _, vw, vh) = viewport_box_for(ctx, node); | |
| match paint_server::resolve(ctx.dom, ctx.resources, v, bbox, (vw, vh)) { | |
| Some(Resolved::Shader(s)) => return Some(PaintChoice::Shader(s)), | |
| Some(Resolved::Pattern { | |
| node: pid, | |
| bbox: pbbox, | |
| }) => { | |
| // Defer-resolved pattern: render its subtree | |
| // into a Picture and wrap as a tiled shader. | |
| if let Some(s) = | |
| super::super::resources::pattern::build_shader(ctx, pid, pbbox) | |
| { | |
| return Some(PaintChoice::Shader(s)); | |
| } | |
| return apply_funciri_fallback(ctx, node, v, default_solid); | |
| } | |
| _ => return apply_funciri_fallback(ctx, node, v, default_solid), | |
| } | |
| } | |
| // Fall through to the simple paint parser. | |
| match parse_paint(v) { | |
| Some(Paint::None) => None, | |
| Some(Paint::Color(c)) => Some(PaintChoice::Solid(c)), | |
| Some(Paint::CurrentColor) => { | |
| Some(PaintChoice::Solid(resolve_current_color(ctx, node))) | |
| } | |
| None => inherited_paint(ctx, node, property, default_solid), | |
| } | |
| } |
🤖 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_shape_painter.rs` around lines 737 -
779, The branch handling unparseable paint values currently falls back to
default_solid when parse_paint(v) returns None; change this to treat malformed
tokens as "absent" so inheritance can apply. Specifically, in the match on
parse_paint(v) replace the final arm that returns
default_solid.map(PaintChoice::Solid) with a call to inherited_paint(ctx, node,
property, default_solid) (returning its result), leaving the existing arms for
Paint::None, Paint::Color and Paint::CurrentColor unchanged; this ensures
parse_paint, inherited_paint, resolve_current_color, apply_funciri_fallback and
default_solid are used appropriately.
Comment on lines
+195
to
+197
| if !is_supported_clipper_child(kind) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Don't silently drop <g> wrappers.
continue here turns a valid <clipPath> that groups shapes under <g> into an empty clip path, so the element clips everything instead of falling back unclipped. If groups are still unsupported, return None here; otherwise recurse into the group's children.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida/src/htmlcss/svg/resources/clipper.rs` around lines 195 - 197,
The code currently drops supported children inside clipPath by using "continue"
in the loop where is_supported_clipper_child(kind) is checked; change that logic
so that if the child is a group element (check for the group tag used in your
parser, e.g., "g" or Tag::G) you either recurse into that group's children to
process them (call the same clip-parsing routine on the group's child nodes) or,
if groups are deliberately unsupported, return None instead of continuing;
update the branch around is_supported_clipper_child(kind) to handle the group
case explicitly and only skip/continue for truly unsupported node kinds.
Comment on lines
+239
to
+245
| if let Some(mut chained) = chained_clip_path(ctx, get_attr(child, "clip-path"), object_bbox) | ||
| { | ||
| if let Some(inv) = clipper_inverse { | ||
| chained = chained.with_transform(&inv); | ||
| } | ||
| child_path = skia_safe::op(&child_path, &chained, PathOp::Intersect)?; | ||
| } |
There was a problem hiding this comment.
Resolve chained clip paths in the child's coordinate space.
chained_clip_path() is still resolved against the outer object_bbox, but this intersection happens before the parent clipPath's objectBoundingBox mapping is applied. That puts nested clip paths in the wrong space whenever the clipPath uses bbox units. If you can't thread the child's bbox through yet, fail this branch with None instead of intersecting mismatched coordinates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida/src/htmlcss/svg/resources/clipper.rs` around lines 239 - 245,
The current code calls chained_clip_path(...) with object_bbox and then
intersects it into child_path even when the chained clip uses objectBoundingBox
units that should be resolved in the parent's clipPath-mapped space; instead,
detect when the referenced clipPath uses clipPathUnits="objectBoundingBox" (via
get_attr(child, "clip-path") lookup) and we cannot thread the child's bbox
through yet, and in that case bail out of this branch by returning None rather
than performing skia_safe::op intersection; update the logic around
chained_clip_path, clipper_inverse and child_path so that you only compute and
intersect a chained clip when the clipPathUnits and transforms are already
compatible (or when you can apply with_transform(&inv) correctly), otherwise
return None.
Comment on lines
+527
to
+533
| pub fn parse_filter_children(dom: &DemoDom, filter_id: NodeId) -> Vec<Primitive> { | ||
| let mut out = Vec::new(); | ||
| let parent = dom.node(filter_id); | ||
| // `color-interpolation-filters` cascades through the DOM: usually | ||
| // declared on `<filter>` and inherited by every primitive child. | ||
| // Each primitive may still override locally. | ||
| let parent_color_interp = parse_color_interp_or(dom, parent, ColorInterp::LinearRgb); |
There was a problem hiding this comment.
Propagate color-interpolation-filters through the DOM cascade.
This only seeds from the immediate <filter> node, so inherited values on ancestors above the filter are ignored. That can flip filter output from linearRGB to sRGB in nested SVGs.
♻️ Suggested fix
- let parent_color_interp = parse_color_interp_or(dom, parent, ColorInterp::LinearRgb);
+ let parent_color_interp =
+ resolve_color_interp(dom, parent).unwrap_or(ColorInterp::LinearRgb);
-fn parse_color_interp_or(_dom: &DemoDom, node: &DemoNode, inherited: ColorInterp) -> ColorInterp {
+fn resolve_color_interp(dom: &DemoDom, node: &DemoNode) -> Option<ColorInterp> {
+ let raw = read_property_with_inheritance(dom, node, "color-interpolation-filters", true)?;
+ match raw.trim() {
+ s if s.eq_ignore_ascii_case("sRGB") => Some(ColorInterp::SRgb),
+ s if s.eq_ignore_ascii_case("linearRGB") => Some(ColorInterp::LinearRgb),
+ _ => None,
+ }
+}Also applies to: 609-631
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida/src/htmlcss/svg/resources/svg_filter_builder.rs` around lines
527 - 533, The code seeds color-interpolation-filters from the immediate filter
node object (parent) instead of using the DOM cascade, so inherited values on
ancestors are ignored; change calls to parse_color_interp_or to pass the filter
node id (filter_id) rather than the node object so the helper can walk ancestors
(e.g., replace parse_color_interp_or(dom, parent, ColorInterp::LinearRgb) with
parse_color_interp_or(dom, filter_id, ColorInterp::LinearRgb)), and make the
same substitution for the other occurrence around lines 609-631 to ensure the
cascade is respected.
| "feDropShadow" => parse_drop_shadow(node, is_first)?, | ||
| "feTurbulence" => parse_turbulence(node)?, | ||
| "feDisplacementMap" => parse_displacement_map(node, is_first)?, | ||
| "feImage" => parse_fe_image(node)?, |
There was a problem hiding this comment.
Return transparent black when <feImage> has no usable source.
Right now parse_fe_image() returns None, and parse_one() drops the primitive entirely. That changes the filter chain semantics: later primitives see the previous output instead of an unavailable-image transparent-black result.
♻️ Suggested fix
fn parse_fe_image(node: &DemoNode) -> Option<FilterEffect> {
use crate::htmlcss::svg::dom::attrs::parse_preserve_aspect_ratio;
- let href = href_attr(node)?.to_string();
+ let href = match href_attr(node) {
+ Some(href) => href.to_string(),
+ None => return Some(transparent_flood()),
+ };
let par = get_attr(node, "preserveAspectRatio")
.map(parse_preserve_aspect_ratio)
.unwrap_or_default();
Some(FilterEffect::Image { href, par })
}Also applies to: 1350-1357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida/src/htmlcss/svg/resources/svg_filter_builder.rs` at line 573,
parse_fe_image currently yields None for unusable sources which causes parse_one
to drop the primitive and alter filter-chain semantics; change the behavior so a
transparent-black primitive is returned instead. Either make parse_fe_image(...)
return Some(primitive_that_represents_transparent_black) when no usable source
is found, or update parse_one(...) to convert a None from parse_fe_image into a
transparent-black primitive before adding to the chain; reference the
parse_fe_image and parse_one symbols and apply the same fix to the other
occurrence around the 1350–1357 block so missing feImage sources always produce
a transparent-black primitive rather than being dropped.
Comment on lines
+1159
to
+1170
| let raw_color = read_presentation(node, "flood-color").unwrap_or_else(|| "black".to_string()); | ||
| let base = match crate::htmlcss::svg::dom::attrs::parse_paint(&raw_color) { | ||
| Some(crate::htmlcss::svg::dom::attrs::Paint::Color(c)) => c, | ||
| _ => Color::BLACK, | ||
| }; | ||
| let opacity = read_presentation(node, "flood-opacity") | ||
| .as_deref() | ||
| .and_then(parse_length_px) | ||
| .unwrap_or(1.0) | ||
| .clamp(0.0, 1.0); | ||
| let alpha = (base.a() as f32 * opacity).round().clamp(0.0, 255.0) as u8; | ||
| let color = Color::from_argb(alpha, base.r(), base.g(), base.b()); |
There was a problem hiding this comment.
Use the color/alpha resolvers here instead of raw presentation reads.
feDropShadow currently ignores inherit/currentColor for flood-color, and it parses flood-opacity with the length parser instead of the alpha parser. That makes common shadow styles render with the wrong tint or opacity.
♻️ Suggested fix
- let raw_color = read_presentation(node, "flood-color").unwrap_or_else(|| "black".to_string());
- let base = match crate::htmlcss::svg::dom::attrs::parse_paint(&raw_color) {
- Some(crate::htmlcss::svg::dom::attrs::Paint::Color(c)) => c,
- _ => Color::BLACK,
- };
- let opacity = read_presentation(node, "flood-opacity")
- .as_deref()
- .and_then(parse_length_px)
- .unwrap_or(1.0)
- .clamp(0.0, 1.0);
+ let base = resolve_color_property(dom, node, "flood-color", Color::BLACK, false);
+ let opacity = resolve_opacity_property(dom, node, "flood-opacity", false).unwrap_or(1.0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida/src/htmlcss/svg/resources/svg_filter_builder.rs` around lines
1159 - 1170, The code in svg_filter_builder.rs currently reads flood-color with
read_presentation and parses flood-opacity using parse_length_px, which ignores
'inherit'/'currentColor' and mis-parses opacity; replace the raw presentation
reads with the existing color/alpha resolvers used elsewhere (use the module's
paint/color resolver instead of read_presentation + parse_paint and use the
alpha/opacity parser instead of parse_length_px) so that feDropShadow honors
inherit/currentColor and proper opacity semantics, then compute alpha the same
way and call Color::from_argb as before (refer to read_presentation,
parse_paint, parse_length_px, Color::from_argb and the local node variable to
locate the change).
The test was reading from fixtures/local/, which is gitignored, so it passed locally but failed in CI with NotFound. Materialize the minimal SVG/CSS layout in a temp dir instead.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_dev/src/reftest/render.rs`:
- Around line 281-302: sniff_view_box currently uses rest.split_at(1) which will
panic if the viewBox= is followed by nothing or a non-quote; add a safe guard
before splitting: after computing rest = &tag[start..], check that rest is
non-empty and that rest.chars().next() is either '"' or '\'' (or bail None) and
then proceed to take the quote and the remaining slice safely (use slicing only
after confirming length >= 1); apply the same defensive check to the attr helper
at the same pattern so both functions return None for malformed/truncated
attributes instead of panicking.
🪄 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: 81dfcfa2-9106-42b2-9e21-33c9af16ecfe
📒 Files selected for processing (1)
crates/grida_dev/src/reftest/render.rs
Comment on lines
+281
to
302
| fn sniff_view_box(tag: &str) -> Option<(f32, f32)> { | ||
| let needle = "viewBox="; | ||
| let start = tag.find(needle)? + needle.len(); | ||
| let rest = &tag[start..]; | ||
| let (quote, rest) = rest.split_at(1); | ||
| let quote = quote.chars().next()?; | ||
| if quote != '"' && quote != '\'' { | ||
| return None; | ||
| } | ||
|
|
||
| let image = surface.image_snapshot(); | ||
| let encoded = image | ||
| .encode(None, EncodedImageFormat::PNG, None) | ||
| .ok_or_else(|| anyhow!("Failed to encode PNG"))?; | ||
|
|
||
| if let Some(parent) = output_path.parent() { | ||
| fs::create_dir_all(parent) | ||
| .with_context(|| format!("failed to create output directory {}", parent.display()))?; | ||
| let end = rest.find(quote)?; | ||
| let raw = &rest[..end]; | ||
| let parts: Vec<f32> = raw | ||
| .split(|c: char| c == ',' || c.is_ascii_whitespace()) | ||
| .filter(|s| !s.is_empty()) | ||
| .filter_map(|s| s.parse::<f32>().ok()) | ||
| .collect(); | ||
| if parts.len() == 4 { | ||
| Some((parts[2], parts[3])) | ||
| } else { | ||
| None | ||
| } | ||
| fs::write(output_path, encoded.as_bytes()) | ||
| .with_context(|| format!("failed to write PNG to {}", output_path.display()))?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Potential panic on malformed input in sniff_view_box.
If an SVG has a malformed viewBox= attribute with no value (e.g., <svg viewBox=> or truncated <svg viewBox=), rest will be empty or not start with a quote, and rest.split_at(1) will panic. The same issue exists in the attr helper at line 259.
While unlikely in well-formed test fixtures, a quick guard prevents crashes during bulk reftest runs:
🛡️ Proposed fix
fn sniff_view_box(tag: &str) -> Option<(f32, f32)> {
let needle = "viewBox=";
let start = tag.find(needle)? + needle.len();
let rest = &tag[start..];
+ if rest.is_empty() {
+ return None;
+ }
let (quote, rest) = rest.split_at(1);
let quote = quote.chars().next()?;Apply the same guard to attr (line 259).
📝 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 sniff_view_box(tag: &str) -> Option<(f32, f32)> { | |
| let needle = "viewBox="; | |
| let start = tag.find(needle)? + needle.len(); | |
| let rest = &tag[start..]; | |
| let (quote, rest) = rest.split_at(1); | |
| let quote = quote.chars().next()?; | |
| if quote != '"' && quote != '\'' { | |
| return None; | |
| } | |
| let image = surface.image_snapshot(); | |
| let encoded = image | |
| .encode(None, EncodedImageFormat::PNG, None) | |
| .ok_or_else(|| anyhow!("Failed to encode PNG"))?; | |
| if let Some(parent) = output_path.parent() { | |
| fs::create_dir_all(parent) | |
| .with_context(|| format!("failed to create output directory {}", parent.display()))?; | |
| let end = rest.find(quote)?; | |
| let raw = &rest[..end]; | |
| let parts: Vec<f32> = raw | |
| .split(|c: char| c == ',' || c.is_ascii_whitespace()) | |
| .filter(|s| !s.is_empty()) | |
| .filter_map(|s| s.parse::<f32>().ok()) | |
| .collect(); | |
| if parts.len() == 4 { | |
| Some((parts[2], parts[3])) | |
| } else { | |
| None | |
| } | |
| fs::write(output_path, encoded.as_bytes()) | |
| .with_context(|| format!("failed to write PNG to {}", output_path.display()))?; | |
| Ok(()) | |
| } | |
| fn sniff_view_box(tag: &str) -> Option<(f32, f32)> { | |
| let needle = "viewBox="; | |
| let start = tag.find(needle)? + needle.len(); | |
| let rest = &tag[start..]; | |
| if rest.is_empty() { | |
| return None; | |
| } | |
| let (quote, rest) = rest.split_at(1); | |
| let quote = quote.chars().next()?; | |
| if quote != '"' && quote != '\'' { | |
| return None; | |
| } | |
| let end = rest.find(quote)?; | |
| let raw = &rest[..end]; | |
| let parts: Vec<f32> = raw | |
| .split(|c: char| c == ',' || c.is_ascii_whitespace()) | |
| .filter(|s| !s.is_empty()) | |
| .filter_map(|s| s.parse::<f32>().ok()) | |
| .collect(); | |
| if parts.len() == 4 { | |
| Some((parts[2], parts[3])) | |
| } else { | |
| None | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida_dev/src/reftest/render.rs` around lines 281 - 302,
sniff_view_box currently uses rest.split_at(1) which will panic if the viewBox=
is followed by nothing or a non-quote; add a safe guard before splitting: after
computing rest = &tag[start..], check that rest is non-empty and that
rest.chars().next() is either '"' or '\'' (or bail None) and then proceed to
take the quote and the remaining slice safely (use slicing only after confirming
length >= 1); apply the same defensive check to the attr helper at the same
pattern so both functions return None for malformed/truncated attributes instead
of panicking.
The indented `drop-shadow( [<color>]? && <length>{2,3} )` was treated
as a Rust doctest and failed to compile. Use a `text` fenced block.
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