feat(htmlcss): inline <svg> + standalone .svg via Skia svg::Dom (stop-gap)#695
Merged
softmarshmallow merged 4 commits intomainfrom Apr 26, 2026
Merged
feat(htmlcss): inline <svg> + standalone .svg via Skia svg::Dom (stop-gap)#695softmarshmallow merged 4 commits intomainfrom
softmarshmallow merged 4 commits intomainfrom
Conversation
Nine-document research subdirectory under docs/wg/research/chromium/svg/ covering Blink's SVG pipeline from DOM to composite, plus a factual cross-engine comparison with Servo and resvg. Scope: pipeline overview, coordinate systems (viewBox/CTM/non-scaling-stroke), paint servers, resources and effects (clip/mask/filter/marker), path geometry and stroking, SVG text layout algorithm, <use>/<foreignObject> bridging, and SVG-as-image embedding modes.
Adds two public entry points on the htmlcss module — render_svg for
standalone SVG documents and render_any for sniffed HTML-or-SVG input —
plus inline <svg> support inside HTML via the same Skia svg::Dom path.
Mirrors Servo's replaced-element + subtree-serialization pattern while
swapping resvg+tiny-skia for Skia's built-in SVG DOM (GPU-capable, no
pixmap detour).
Wires a --renderer {iosvg|htmlcss} flag into `grida-dev reftest` so the
new backend can be graded against the same SVG suites as the Grida
scene-graph path.
Baseline on fixtures/local/resvg-test-suite (1,679 tests):
backend | avg | S99 | S95 | S90 | S75 | <S75 | 0% | err
----------|--------|-------|-----|-----|-----|------|----|----
iosvg | 64.12% | 529 | 49 | 85 | 178 | 835 | 3 | 3
htmlcss | 66.24% | 482 | 59 | 73 | 225 | 833 | 7 | 6
Limits (accepted for MVP, match Servo): no outer CSS cascade into SVG;
no <foreignObject> HTML recursion; no scripting / SMIL.
Tests: 3 new cg lib tests; 777 total cg tests green.
…rapping Introduces `--renderer sksvg` in `grida-dev reftest` as a thinnest-possible bridge from SVG bytes to Skia's native svg::Dom and a raster surface. No htmlcss module, no PictureRecorder round-trip, no Grida tree surgery. Purpose: attribute reftest failures correctly. If a test fails identically under `--renderer htmlcss` and `--renderer sksvg`, the cause is Skia's own svg::Dom module — not our plumbing. Any delta attributes to the PictureRecorder round-trip that htmlcss::render_svg adds. Baseline on fixtures/local/resvg-test-suite (1,679 tests): backend | avg | parse errors | notes ----------|--------|--------------|------ iosvg | 64.12% | 3 | usvg parse + cg::svg::pack scene graph htmlcss | 66.24% | 6 | cg::htmlcss::render_svg → PictureRecorder sksvg | 66.25% | 6 | direct skia_safe::svg::Dom htmlcss vs sksvg per-test comparison: 1672/1673 tests bit-identical, same 6 parse-error set. The lone exception (painting_mix-blend-mode_opacity- on-group: htmlcss 62.5%, sksvg 78.8%, +16pp) is htmlcss *regressing* from sksvg via PictureRecorder fidelity loss on blend-mode-over-group — worth noting but does not affect aggregate. Conclusion: htmlcss wrapping adds zero divergence in 1672/1673 cases; CSS selector / XML ENTITY / explicit-namespace failures are genuinely Skia's svg::Dom limitations, not Grida's wiring. Also adds: - fixtures/test-html/L0/svg-inline-basic.html — L0 fixture exercising inline <svg> shapes (rect/circle/path/g transform/linearGradient/ polyline) through the htmlcss pipeline. - .claude/launch.json entry `svg-reftest-viewer` — python http.server over target/reftests/viewer/ for inspecting reftest triads.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
WalkthroughThis pull request adds comprehensive inline SVG rendering support to the grida-canvas htmlcss pipeline. Changes include SVG detection and DOM serialization during style collection, new standalone SVG rendering functions, inline SVG painting via Skia's native SVG DOM, reftest infrastructure for multiple rendering backends, and extensive SVG architecture documentation for Chromium, Servo, and resvg. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- htmlcss.md: backtick `<S75` so MDX stops parsing it as a JSX tag (column-7 "Unexpected end of file in name" error) - tags.yml: add servo and resvg tag definitions referenced by wg/research/chromium/svg/comparison.md
![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: 74f80b399e
ℹ️ 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
+550
to
+551
| let name: &str = &attr.name.local; | ||
| if name == "xmlns" { |
There was a problem hiding this comment.
This serializer writes attributes using only attr.name.local, which drops namespace prefixes carried by the parsed DOM (xml:space, xlink:href, xmlns:xlink, etc.). As a result, inline SVGs that rely on foreign attributes are rewritten to different names before svg::Dom::from_bytes sees them, so features like <use> references or whitespace-preserving text can render incorrectly even when the source SVG is valid.
Useful? React with 👍 / 👎.
Comment on lines
+633
to
+635
| let svg_view_box = get_element_attr(node, "viewBox") | ||
| .as_deref() | ||
| .and_then(parse_view_box); |
There was a problem hiding this comment.
detect_svg_element parses and stores viewBox, but the replaced-element sizing path (layout::apply_replaced_intrinsic_size) still derives aspect ratio only from image data or width/height attributes. That means inline SVGs with only viewBox (a common pattern) fall back to the default 300×150 ratio, causing stretched layout/painting instead of honoring the SVG's intrinsic aspect ratio.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (15)
docs/wg/feat-2d/htmlcss.md-20-26 (1)
20-26:⚠️ Potential issue | 🟡 MinorWording mismatch: "Two" vs three entry points.
The intro says "Two public entry points" but the table immediately lists three (
render,render_svg,render_any).📝 Proposed fix
-Two public entry points, both returning a `skia_safe::Picture`: +Three public entry points, all returning a `skia_safe::Picture`:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/wg/feat-2d/htmlcss.md` around lines 20 - 26, The intro line incorrectly says "Two public entry points" while the table lists three functions; update the wording to match the table (e.g., "Three public entry points" or "Public entry points") so it's consistent with the listed symbols `render`, `render_svg`, and `render_any`; ensure the sentence near the table references those exact function names and their purposes.crates/grida-dev/src/reftest/runner.rs-215-228 (1)
215-228:⚠️ Potential issue | 🟡 MinorMinor: comment omits
sksvg.The comment lists "usvg for
iosvg, Skia forhtmlcss" butsksvgalso dispatches into Skia'ssvg::Domhere and is similarly panic-prone.📝 Proposed fix
- // Render SVG to PNG, scaling to match reference size. - // Wrap in catch_unwind to handle panics from underlying - // rendering code (usvg for `iosvg`, Skia for `htmlcss`). + // Render SVG to PNG, scaling to match reference size. + // Wrap in catch_unwind to handle panics from underlying + // rendering code (usvg for `iosvg`; Skia svg::Dom for + // `htmlcss` and `sksvg`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/reftest/runner.rs` around lines 215 - 228, Update the top comment above the panic::catch_unwind block to mention that sksvg can also panic; specifically reference the renderer match arms SvgRenderer::Sksvg and the function render_svg_to_png_via_sksvg (alongside SvgRenderer::Iosvg/usvg and SvgRenderer::Htmlcss/Skia) so the comment accurately states that both htmlcss and sksvg dispatch into Skia's svg::Dom and are panic-prone.docs/wg/feat-2d/htmlcss.md-28-29 (1)
28-29:⚠️ Potential issue | 🟡 MinorBroken in-page anchor
#inline-svg.There is no
## Inline SVGheading later in the file, so this link does not resolve (also flagged by markdownlint MD051). Either add the section it points to or drop the link.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/wg/feat-2d/htmlcss.md` around lines 28 - 29, The in-page anchor "#inline-svg" in the sentence referencing "Inline SVG" is broken because there is no corresponding "## Inline SVG" heading; either add a matching heading titled "Inline SVG" (exact text) later in this document so the anchor resolves, or remove/replace the link with plain text or a correct existing section anchor; update the sentence that contains `<svg>` and "Inline SVG" to point to the new or correct heading name so the markdownlint MD051 error is resolved.docs/wg/research/chromium/svg/pipeline.md-1-9 (1)
1-9:⚠️ Potential issue | 🟡 MinorAdd Markdown frontmatter for Docusaurus here.
This page is plain Markdown, so it should opt out of MDX with
format: md. While touching the frontmatter, adddescriptionandkeywordstoo.As per coding guidelines
docs/{wg,reference}/**/*.md: includeformat: mdin frontmatter, anddocs/{wg,reference,editor,forms,platform,with-figma,design,math}/**/*.md: prefer frontmatter withtitle,description, andkeywords.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/wg/research/chromium/svg/pipeline.md` around lines 1 - 9, The frontmatter is missing required metadata and should opt out of MDX; update the YAML at the top of the file (the existing frontmatter with title "Chromium SVG Pipeline Overview") to include format: md, a short description string under description, and a keywords array (e.g., ["chromium","svg","pipeline","rendering","research"]) so it matches the docs frontmatter guidelines; keep title unchanged and ensure the frontmatter remains valid YAML.docs/wg/research/chromium/svg/index.md-1-9 (1)
1-9:⚠️ Potential issue | 🟡 MinorPlease add
format: mdto this page's frontmatter.This is a plain Markdown index page, so leaving it on the MDX parser is unnecessary risk. Adding
descriptionandkeywordshere would also align it with the docs frontmatter conventions.As per coding guidelines
docs/{wg,reference}/**/*.md: includeformat: mdin frontmatter, anddocs/{wg,reference,editor,forms,platform,with-figma,design,math}/**/*.md: prefer frontmatter withtitle,description, andkeywords.🤖 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 1 - 9, The frontmatter for this Markdown index lacks the required format and metadata: add format: md to the YAML frontmatter and include description and keywords fields alongside the existing title so the page is parsed as plain Markdown and conforms to docs frontmatter conventions (ensure the new keys are placed with title in the top-level YAML frontmatter).docs/wg/research/chromium/svg/path-geometry.md-1-9 (1)
1-9:⚠️ Potential issue | 🟡 MinorAdd the Markdown frontmatter fields for this research page.
format: mdshould be present here, and this page would benefit from the standarddescription/keywordsmetadata while you're already editing the header.As per coding guidelines
docs/{wg,reference}/**/*.md: includeformat: mdin frontmatter, anddocs/{wg,reference,editor,forms,platform,with-figma,design,math}/**/*.md: prefer frontmatter withtitle,description, andkeywords.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/wg/research/chromium/svg/path-geometry.md` around lines 1 - 9, Add missing frontmatter fields: insert format: md and add a descriptive description and a keywords list into the existing YAML header while preserving the existing title and tags; ensure the frontmatter remains valid YAML with the current title ("Chromium SVG Path Geometry and Stroking"), the new format: md entry, a one-sentence description summarizing the research, and a small keywords array (e.g., "chromium", "svg", "path", "stroking", "rendering") to comply with the docs frontmatter conventions.docs/wg/research/chromium/svg/pipeline.md-16-31 (1)
16-31:⚠️ Potential issue | 🟡 MinorLabel these diagram fences as
text.
markdownlintis already flagging both unlabeled fenced blocks, so the docs lint will stay noisy until these fences declare a language.Also applies to: 93-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/wg/research/chromium/svg/pipeline.md` around lines 16 - 31, The fenced diagram blocks use unlabeled triple-backtick fences (```), which triggers markdownlint; label them as text by changing the opening fence to ```text for both occurrences (the diagram starting with "Parsing Style Layout..." and the second instance around lines 93-118) so the markdown parser treats them as plain text and the linter stops flagging them.docs/wg/research/chromium/svg/text.md-31-41 (1)
31-41:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced code block.
Static analysis (markdownlint MD040) flags the class-hierarchy block at Line 31 as missing a language. Use
text(ornone) so the block has a stable identifier and CI lint stays clean.-``` +```text LayoutSVGBlock └── LayoutSVGText <text> — the block container for SVG text … LayoutText └── LayoutSVGInlineText text runs inside the above<details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/wg/research/chromium/svg/text.mdaround lines 31 - 41, The fenced code
block that shows the class hierarchy (the block containing "LayoutSVGBlock └──
LayoutSVGText … LayoutText └── LayoutSVGInlineText") is missing a language
identifier; update the opening fence fromtotext (or ```none) so
markdownlint MD040 is satisfied and CI lint passes.</details> </blockquote></details> <details> <summary>docs/wg/research/chromium/svg/coordinate-systems.md-22-42 (1)</summary><blockquote> `22-42`: _⚠️ Potential issue_ | _🟡 Minor_ **Add a language identifier to the transform-chain code block.** markdownlint MD040 flags the block at Line 22. Tag it as `text` so CI lint stays clean. ```diff -``` +```text Shape local (user units) │ LocalSVGTransform() (the shape's own `transform` attribute) … ``` ``` <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/wg/research/chromium/svg/coordinate-systems.mdaround lines 22 - 42,
The code block showing the SVG transform chain (starting with "Shape local (user
units)" and containing entries like LocalSVGTransform(),
LocalToSVGParentTransform(), viewBoxToViewTransform, and
LocalToBorderBoxTransform()) needs a language identifier to satisfy markdownlint
MD040; update that fenced code block to begin with ```text (i.e., add the text
language tag) so the block is treated as plain text by the linter.</details> </blockquote></details> <details> <summary>docs/wg/research/chromium/svg/resources-and-effects.md-129-137 (1)</summary><blockquote> `129-137`: _⚠️ Potential issue_ | _🟡 Minor_ **Add a language identifier to the primitive-list code block.** markdownlint MD040 flags the block at Line 129 as missing a language. Tag it as `text` so CI lint stays clean. ```diff -``` +```text feBlend feColorMatrix feComponentTransfer … feTile feTurbulence ``` ``` <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/wg/research/chromium/svg/resources-and-effects.mdaround lines 129 -
137, The markdown code block containing the SVG filter primitives (e.g.,
feBlend, feColorMatrix, feComponentTransfer, feTile, feTurbulence) is missing a
language identifier; update that fenced code block to include the language tag
text(i.e., change the opening triple backticks to ```text) so markdownlint
MD040 is satisfied and CI lint stays clean.</details> </blockquote></details> <details> <summary>docs/wg/research/chromium/svg/use-and-foreign-object.md-85-95 (1)</summary><blockquote> `85-95`: _⚠️ Potential issue_ | _🟡 Minor_ **Add a language identifier to the unlabeled fenced code blocks.** markdownlint MD040 flags the blocks starting at Lines 85 and 91 as missing a language. Tag them as `xml` (or `text`) so CI lint stays clean. ```diff -``` +```xml <use href="#sym" x="10" y="10" width="50" height="50"/> ``` … -``` +```xml <svg x="10" y="10" width="50" height="50" viewBox="..."> (children of `#sym`) </svg> ``` ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/wg/research/chromium/svg/use-and-foreign-object.md` around lines 85 - 95, In the docs example with the two unlabeled fenced code blocks showing the <use href="#sym".../> snippet and the resulting <svg...> snippet, add a language identifier (e.g., xml) to both fenced code blocks so markdownlint MD040 is satisfied; find the blocks containing the <use href="#sym" x="10" y="10" width="50" height="50"/> snippet and the block containing the <svg x="10" y="10" width="50" height="50" viewBox="..."> (children of `#sym`) snippet and prefix their opening triple backticks with xml (or text). ``` </details> </blockquote></details> <details> <summary>crates/grida-canvas/src/htmlcss/mod.rs-260-279 (1)</summary><blockquote> `260-279`: _⚠️ Potential issue_ | _🟡 Minor_ **`looks_like_svg` misses UTF-8 BOM and unusual `<svg>` casings.** `trim_start()` does not strip `\u{FEFF}` (UTF-8 BOM), so `.svg` files saved with a BOM (common from many editors / exporters on Windows) will sniff as HTML and get routed through the HtmlCss pipeline, producing nonsense output. Similarly the case match only covers three literal prefixes (`<svg`, `<SVG`, `<Svg`); ASCII-case-insensitive matching would be more robust and a one-liner. <details> <summary>🛡️ Proposed fix</summary> ```diff fn looks_like_svg(input: &str) -> bool { - let trimmed = input.trim_start(); + // Strip BOM first, then whitespace. + let trimmed = input + .strip_prefix('\u{FEFF}') + .unwrap_or(input) + .trim_start(); // Skip leading XML declaration + doctype if present. let rest = trimmed .strip_prefix("<?xml") .map(|r| { // Skip past '?>' and any subsequent whitespace / doctype. r.find("?>") .map(|i| r[i + 2..].trim_start()) .unwrap_or(r) .trim_start() }) .unwrap_or(trimmed); let rest = rest .strip_prefix("<!DOCTYPE") .or_else(|| rest.strip_prefix("<!doctype")) .and_then(|r| r.find('>').map(|i| r[i + 1..].trim_start())) .unwrap_or(rest); - rest.starts_with("<svg") || rest.starts_with("<SVG") || rest.starts_with("<Svg") + let head = rest.as_bytes(); + head.len() >= 4 + && head[0] == b'<' + && head[1].eq_ignore_ascii_case(&b's') + && head[2].eq_ignore_ascii_case(&b'v') + && head[3].eq_ignore_ascii_case(&b'g') } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/mod.rs` around lines 260 - 279, The function looks_like_svg fails to handle a UTF-8 BOM and only checks three literal casings of "<svg"; update looks_like_svg to first strip a leading BOM (U+FEFF) from the trimmed input before further processing, then perform an ASCII-case-insensitive prefix check for "<svg" (instead of enumerating "<svg", "<SVG", "<Svg") when deciding whether the content is SVG; keep the existing XML/DOCTYPE skipping logic but apply the BOM-stripped string to those checks and use an eq_ignore_ascii_case / lowercased-prefix-style check for the "<svg" test. ``` </details> </blockquote></details> <details> <summary>crates/grida-canvas/src/htmlcss/mod.rs-219-234 (1)</summary><blockquote> `219-234`: _⚠️ Potential issue_ | _🟡 Minor_ **Container size can disagree with recording bounds for non-positive inputs.** `set_container_size(Size::new(width, height))` passes the raw `(width, height)` while the picture bounds use `width.max(1.0)` and `height.max(1.0)`. If a caller passes `0.0` (or a negative) for either dimension the container becomes degenerate while the recording surface clamps to 1px, which can mask reasoning bugs in callers and produce a "valid" picture for nonsensical inputs. Consider clamping (or returning `Err`) up front so the container and bounds agree. <details> <summary>🛡️ Suggested clamp</summary> ```diff - let data = skia_safe::Data::new_copy(svg.as_bytes()); - let mut dom = svg::Dom::from_bytes(&data, FontMgr::default()) - .map_err(|e| format!("SVG parse error: {e}"))?; - dom.set_container_size(Size::new(width, height)); - - let mut recorder = PictureRecorder::new(); - let bounds = Rect::from_xywh(0.0, 0.0, width.max(1.0), height.max(1.0)); + let w = width.max(1.0); + let h = height.max(1.0); + let data = skia_safe::Data::new_copy(svg.as_bytes()); + let mut dom = svg::Dom::from_bytes(&data, FontMgr::default()) + .map_err(|e| format!("SVG parse error: {e}"))?; + dom.set_container_size(Size::new(w, h)); + + let mut recorder = PictureRecorder::new(); + let bounds = Rect::from_xywh(0.0, 0.0, w, h); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/mod.rs` around lines 219 - 234, In render_svg, ensure the SVG container size and the recording bounds agree by handling non-positive width/height up front: validate or clamp inputs (e.g., let w = width.max(1.0); let h = height.max(1.0)) and use those same w,h for both dom.set_container_size(Size::new(w,h)) and Rect::from_xywh(...) (or alternatively return Err if width<=0.0 || height<=0.0); update references to set_container_size, Rect::from_xywh, and the bounds calculation so container size and recorded bounds are consistent. ``` </details> </blockquote></details> <details> <summary>crates/grida-dev/src/reftest/render.rs-219-242 (1)</summary><blockquote> `219-242`: _⚠️ Potential issue_ | _🟡 Minor_ **`sniff_svg_dimensions` parses unit-bearing values instead of falling back, contradicting its own doc comment.** The doc comment says "Any non-integer or unit-bearing length falls back to the caller's default", but the implementation cheerfully accepts `width="50%"`, `width="100mm"`, `width="2em"`, etc., extracting just the numeric prefix and returning that as a pixel count. For SVGs that use `width="100%"` (very common), the helper will return `(100, 100)` and reftest output will be a tiny 100×100 PNG instead of the documented 512×512 fallback. `viewBox` is also documented in the surrounding comment ("…sniff the SVG root for a `width`/`height` or `viewBox`…") but never actually consulted — viewBox-only SVGs always fall through to 512×512 too. <details> <summary>🛡️ Proposed fix — reject unit suffixes</summary> ```diff fn attr(tag: &str, name: &str) -> Option<u32> { let needle = format!("{}=", name); 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 end = rest.find(quote)?; let raw = &rest[..end]; - let numeric_end = raw - .find(|c: char| !(c.is_ascii_digit() || c == '.')) - .unwrap_or(raw.len()); - raw[..numeric_end].parse::<f32>().ok().map(|v| v as u32) + let trimmed = raw.trim(); + // Reject unit-bearing or percentage values; only accept bare numbers. + if !trimmed + .chars() + .all(|c| c.is_ascii_digit() || c == '.') + { + return None; + } + trimmed.parse::<f32>().ok().map(|v| v.max(1.0) as u32) } ``` </details> Optionally, also parse `viewBox="x y w h"` as a secondary fallback so the doc claim about viewBox holds. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/reftest/render.rs` around lines 219 - 242, sniff_svg_dimensions currently accepts numeric prefixes of unit-bearing width/height (via the inner attr function) which contradicts the doc: only plain integer lengths should be accepted; update attr in sniff_svg_dimensions to only accept a value that is a bare integer (no trailing units, no decimal point) and return None for anything else, and then implement a secondary parse path in sniff_svg_dimensions that, if width/height are not present or rejected, looks for a viewBox attribute and parses "minX minY width height" (use the third and fourth fields as u32 after validating they are integer/non-negative) to return dimensions; reference the sniff_svg_dimensions function and its inner attr helper when making changes. ``` </details> </blockquote></details> <details> <summary>docs/wg/research/chromium/svg/paint-servers.md-24-36 (1)</summary><blockquote> `24-36`: _⚠️ Potential issue_ | _🟡 Minor_ **Specify languages for fenced code blocks to satisfy markdownlint.** Line 24 and Line 172 open fenced blocks without language identifiers, triggering MD040. Please tag them (for example `text`) to keep lint clean. <details> <summary>Proposed fence-language patch</summary> ```diff -``` +```text LayoutSVGHiddenContainer never visual; extends LayoutSVGContainer └── LayoutSVGResourceContainer base for all <defs>-type resources ├── LayoutSVGResourcePaintServer ApplyShader() → cc::PaintFlags │ ├── LayoutSVGResourcePattern │ └── LayoutSVGResourceGradient │ ├── LayoutSVGResourceLinearGradient │ └── LayoutSVGResourceRadialGradient ├── LayoutSVGResourceClipper (see resources-and-effects.md) ├── LayoutSVGResourceMasker ├── LayoutSVGResourceFilter └── LayoutSVGResourceMarker ``` -``` +```text platform/graphics/ ├── Pattern abstract; holds cached PaintShader │ ├── ImagePattern raster-image patterns (CSS background-image) │ └── PaintRecordPattern SVG <pattern> — record-based tiling └── Gradient abstract; holds stops + spread + local matrix ├── LinearGradient ├── RadialGradient └── ConicGradient CSS conic-gradient() only (no SVG equivalent) ``` ``` </details> Also applies to: 172-181 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/wg/research/chromium/svg/paint-servers.mdaround lines 24 - 36, The
markdown has two fenced code blocks (the ASCII diagrams showing
LayoutSVGHiddenContainer/LayoutSVGResourcePaintServer and the platform/graphics
tree) opened without a language tag, triggering MD040; update those opening
fences to include a language identifier (e.g., changetotext) for both
blocks so markdownlint stops flagging them — look for the fenced blocks
containing "LayoutSVGHiddenContainer" and the "platform/graphics/" tree and add
the same language tag to their opening backticks.</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (3)</summary><blockquote> <details> <summary>crates/grida-dev/src/reftest/runner.rs (1)</summary><blockquote> `58-76`: **Note: backend suffix only applied when `--output-dir` is auto-derived.** When the user passes `--output-dir` explicitly, the `.htmlcss` / `.sksvg` suffix is not appended, so running two backends with the same explicit output-dir will trample each other (especially with `--overwrite` defaulting to true). Worth a one-line mention in the `--output-dir` arg help, or consider always qualifying non-default backends. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/reftest/runner.rs` around lines 58 - 76, The current logic only appends backend suffixes when output_dir is auto-derived (args.output_dir.is_none()), causing explicit --output-dir to be shared across backends; change the code so the backend suffix (computed from args.renderer using SvgRenderer::Iosvg/Htmlcss/Sksvg) is applied whenever a suite-name directory is added to output_dir, regardless of args.output_dir being present: compute suffix with the existing match on args.renderer, sanitize the suite name with sanitize_dir_name(&name) and join output_dir with format!("{}{}", sanitized_name, suffix) (same behavior currently inside the is_none branch) so explicit output dirs are qualified for non-default renderers and won't clash. ``` </details> </blockquote></details> <details> <summary>crates/grida-canvas/src/htmlcss/mod.rs (1)</summary><blockquote> `3304-3313`: **Test name implies an error assertion that isn't made.** The test is named `test_render_svg_malformed_errors` but the body just discards the result and asserts nothing — the only thing it actually exercises is "no panic". Consider renaming to `test_render_svg_malformed_does_not_panic` (or similar) so the intent matches the body, otherwise this name will mislead future readers if Skia ever flips its behavior. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/mod.rs` around lines 3304 - 3313, The test named test_render_svg_malformed_errors currently only calls render_svg("<svg>unclosed", 100.0, 100.0) and discards the result, so rename the test to something like test_render_svg_malformed_does_not_panic to match its behavior OR change the body to actually assert an Err from render_svg; update the function name or add an assertion on the Result returned by render_svg to make the test intent consistent with the name (refer to the test function test_render_svg_malformed_errors and the render_svg call). ``` </details> </blockquote></details> <details> <summary>crates/grida-dev/src/reftest/render.rs (1)</summary><blockquote> `162-303`: **Consider extracting the shared tail of the two SVG→PNG paths.** `render_svg_to_png_via_htmlcss` and `render_svg_to_png_via_sksvg` only differ in how they produce the canvas commands; the rest (surface creation, transparent clear, snapshot, PNG encode, mkdir-and-write) is duplicated verbatim. A small helper that takes a `FnOnce(&mut Canvas, i32, i32)` (or just the size + a `Picture`/`Dom`) would let both backends collapse to a few lines and keep error/output behavior in one place. This is a maintainability nit, not a bug — fine to defer if you expect the two paths to diverge further. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/reftest/render.rs` around lines 162 - 303, Both render_svg_to_png_via_htmlcss and render_svg_to_png_via_sksvg duplicate the same tail: surface creation, transparent clear, drawing, snapshot, PNG encoding and writing; extract that into a single helper (e.g. fn write_svg_png<F>(output_path: &Path, width: i32, height: i32, draw: F) -> Result<()> where F: FnOnce(&mut skia_safe::Canvas) ) that creates the raster surface, clears with SkColor::TRANSPARENT, calls the provided closure to perform drawing (for htmlcss call canvas.draw_picture(&picture, ..) and for sksvg call dom.render(canvas)), encodes the image to PNG and writes out the file (including mkdir logic and error context). Replace the duplicated blocks in render_svg_to_png_via_htmlcss and render_svg_to_png_via_sksvg to compute svg bytes/width/height and then call this new helper with the appropriate closure; keep sniff_svg_dimensions as-is for size extraction. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@crates/grida-canvas/src/htmlcss/collect.rs:
- Around line 538-558: The serialization is losing prefixes because it uses
data.name.local and attr.name.local; update write_svg_element so element and
attribute names are emitted with their qualified form (prefix:local when
attr.name.prefix/is_some() or data.name.prefix/is_some()), preserving "xmlns"
and "xmlns:prefix" attributes and any "xml:"/foreign" prefixes; i.e., construct
tag_name = if let Some(p) = &data.name.prefix { format!("{}:{}", p,
data.name.local) } else { data.name.local.to_string() } (and similarly for
attribute names using attr.name.prefix and attr.name.local), then write tag_name
instead of data.name.local and use that serialized name when checking for xmlns
and emitting attributes (keep escape_xml_attr for values and preserve
has_xmlns/inject_xmlns logic).In
@crates/grida-canvas/src/htmlcss/paint.rs:
- Around line 883-890: The SVG rendering currently creates a new FontMgr via
FontMgr::default() in paint_inline_svg (and likely in render_svg), which
bypasses the shared FontRepository and causes SVG to ignore
repo-registered faces; update the API so paint_inline_svg and render_svg accept
the shared font source (either the FontMgr accessor or the FontCollection) from
your FontRepository (e.g., pass fonts.font_collection() or a new public
get_font_mgr() from FontRepository) and use that when constructing or parsing
the svg::Dom instead of FontMgr::default(); if FontRepository cannot expose
FontMgr directly, change the SVG path to resolve fonts via the FontCollection
API and thread that FontCollection into paint_inline_svg/render_svg so SVG text
resolution matches the rest of htmlcss.In
@docs/wg/research/chromium/svg/comparison.md:
- Around line 1-10: Add missing frontmatter keys to the top of the document:
include format: md, a short description string in description, and a keywords
array under keywords; this ensures the page is treated as plain Markdown
(avoiding MDX parsing of bare tags like , , , ,
, ) and provides metadata for the wg/ page. Place these keys
alongside the existing title and tags in the YAML frontmatter; keep description
concise and populate keywords with relevant terms such as "svg", "chromium",
"servo", and "resvg".In
@docs/wg/research/chromium/svg/coordinate-systems.md:
- Around line 1-9: Update the YAML frontmatter to include format: md plus
description and keywords entries so the MDX parser won't treat inline tags like
, , , , , and as components;
specifically add format: md at the top-level, a short description string (e.g.,
describing "Chromium SVG Coordinate Systems"), and a keywords array to match
existing wg/ pages, leaving the existing title and tags intact.In
@docs/wg/research/chromium/svg/paint-servers.md:
- Around line 1-9: Add a top-level frontmatter field "format: md" and complete
metadata by adding "description" and "keywords" entries to the existing YAML
block (which currently contains "title" and "tags") so the doc avoids MDX
parsing issues and is discoverable; update the frontmatter in the Chromium SVG
Paint Servers doc by inserting format: md beneath the title line and adding a
short "description:" string plus a "keywords:" array (or comma-separated string)
alongside the existing "tags:" field in the same YAML block.In
@docs/wg/research/chromium/svg/resources-and-effects.md:
- Around line 1-9: Update the frontmatter for the "Chromium SVG Resources and
Effects" document to include format: md plus a short description and a keywords
list; specifically modify the existing frontmatter block containing the title
"Chromium SVG Resources and Effects" to add format: md at the top and add a
descriptive string under description and an array of relevant terms under
keywords (e.g., svg, chromium, rendering, resources) so the page is treated as
plain Markdown and MDX won't attempt to parse raw SVG elements like /
/ used in the body.In
@docs/wg/research/chromium/svg/svg-as-image.md:
- Around line 1-9: The frontmatter lacks the required keys causing Docusaurus to
parse this page as MDX and break on plain-text angle-bracket tokens like ,
In
@docs/wg/research/chromium/svg/text.md:
- Around line 1-9: The frontmatter only contains title and tags which can cause
the MDX parser to misinterpret inline angle-bracketed SVG identifiers; update
the YAML frontmatter by adding format: md and include description and keywords
fields alongside the existing title and tags (e.g., add format: md, a concise
description string, and a keywords array) so the page is treated as plain
Markdown and follows wg/ frontmatter conventions; ensure you keep the existing
title and tags keys intact.In
@docs/wg/research/chromium/svg/use-and-foreign-object.md:
- Around line 1-9: Add explicit frontmatter keys to opt out of MDX parsing and
improve metadata: add format: md to the existing frontmatter and include a short
description and keywords array so the title containing angle brackets ("Chromium
SVG and ") and inline identifiers like , ,
, won’t be MDX-parsed; update the top YAML block (the same block
that currently contains title and tags) to include format: md, description:
"", and keywords: [comma-separated, relevant
terms] so the page renders correctly.
Minor comments:
In@crates/grida-canvas/src/htmlcss/mod.rs:
- Around line 260-279: The function looks_like_svg fails to handle a UTF-8 BOM
and only checks three literal casings of "<svg"; update looks_like_svg to first
strip a leading BOM (U+FEFF) from the trimmed input before further processing,
then perform an ASCII-case-insensitive prefix check for "<svg" (instead of
enumerating "<svg", "<SVG", "<Svg") when deciding whether the content is SVG;
keep the existing XML/DOCTYPE skipping logic but apply the BOM-stripped string
to those checks and use an eq_ignore_ascii_case / lowercased-prefix-style check
for the "<svg" test.- Around line 219-234: In render_svg, ensure the SVG container size and the
recording bounds agree by handling non-positive width/height up front: validate
or clamp inputs (e.g., let w = width.max(1.0); let h = height.max(1.0)) and use
those same w,h for both dom.set_container_size(Size::new(w,h)) and
Rect::from_xywh(...) (or alternatively return Err if width<=0.0 || height<=0.0);
update references to set_container_size, Rect::from_xywh, and the bounds
calculation so container size and recorded bounds are consistent.In
@crates/grida-dev/src/reftest/render.rs:
- Around line 219-242: sniff_svg_dimensions currently accepts numeric prefixes
of unit-bearing width/height (via the inner attr function) which contradicts the
doc: only plain integer lengths should be accepted; update attr in
sniff_svg_dimensions to only accept a value that is a bare integer (no trailing
units, no decimal point) and return None for anything else, and then implement a
secondary parse path in sniff_svg_dimensions that, if width/height are not
present or rejected, looks for a viewBox attribute and parses "minX minY width
height" (use the third and fourth fields as u32 after validating they are
integer/non-negative) to return dimensions; reference the sniff_svg_dimensions
function and its inner attr helper when making changes.In
@crates/grida-dev/src/reftest/runner.rs:
- Around line 215-228: Update the top comment above the panic::catch_unwind
block to mention that sksvg can also panic; specifically reference the renderer
match arms SvgRenderer::Sksvg and the function render_svg_to_png_via_sksvg
(alongside SvgRenderer::Iosvg/usvg and SvgRenderer::Htmlcss/Skia) so the comment
accurately states that both htmlcss and sksvg dispatch into Skia's svg::Dom and
are panic-prone.In
@docs/wg/feat-2d/htmlcss.md:
- Around line 20-26: The intro line incorrectly says "Two public entry points"
while the table lists three functions; update the wording to match the table
(e.g., "Three public entry points" or "Public entry points") so it's consistent
with the listed symbolsrender,render_svg, andrender_any; ensure the
sentence near the table references those exact function names and their
purposes.- Around line 28-29: The in-page anchor "#inline-svg" in the sentence
referencing "Inline SVG" is broken because there is no corresponding "## Inline
SVG" heading; either add a matching heading titled "Inline SVG" (exact text)
later in this document so the anchor resolves, or remove/replace the link with
plain text or a correct existing section anchor; update the sentence that
contains<svg>and "Inline SVG" to point to the new or correct heading name so
the markdownlint MD051 error is resolved.In
@docs/wg/research/chromium/svg/coordinate-systems.md:
- Around line 22-42: The code block showing the SVG transform chain (starting
with "Shape local (user units)" and containing entries like LocalSVGTransform(),
LocalToSVGParentTransform(), viewBoxToViewTransform, and
LocalToBorderBoxTransform()) needs a language identifier to satisfy markdownlint
MD040; update that fenced code block to begin with ```text (i.e., add the text
language tag) so the block is treated as plain text by the linter.In
@docs/wg/research/chromium/svg/index.md:
- Around line 1-9: The frontmatter for this Markdown index lacks the required
format and metadata: add format: md to the YAML frontmatter and include
description and keywords fields alongside the existing title so the page is
parsed as plain Markdown and conforms to docs frontmatter conventions (ensure
the new keys are placed with title in the top-level YAML frontmatter).In
@docs/wg/research/chromium/svg/paint-servers.md:
- Around line 24-36: The markdown has two fenced code blocks (the ASCII diagrams
showing LayoutSVGHiddenContainer/LayoutSVGResourcePaintServer and the
platform/graphics tree) opened without a language tag, triggering MD040; update
those opening fences to include a language identifier (e.g., change ``` tofenced blocks containing "LayoutSVGHiddenContainer" and the "platform/graphics/" tree and add the same language tag to their opening backticks. In `@docs/wg/research/chromium/svg/path-geometry.md`: - Around line 1-9: Add missing frontmatter fields: insert format: md and add a descriptive description and a keywords list into the existing YAML header while preserving the existing title and tags; ensure the frontmatter remains valid YAML with the current title ("Chromium SVG Path Geometry and Stroking"), the new format: md entry, a one-sentence description summarizing the research, and a small keywords array (e.g., "chromium", "svg", "path", "stroking", "rendering") to comply with the docs frontmatter conventions. In `@docs/wg/research/chromium/svg/pipeline.md`: - Around line 1-9: The frontmatter is missing required metadata and should opt out of MDX; update the YAML at the top of the file (the existing frontmatter with title "Chromium SVG Pipeline Overview") to include format: md, a short description string under description, and a keywords array (e.g., ["chromium","svg","pipeline","rendering","research"]) so it matches the docs frontmatter guidelines; keep title unchanged and ensure the frontmatter remains valid YAML. - Around line 16-31: The fenced diagram blocks use unlabeled triple-backtick fences (```), which triggers markdownlint; label them as text by changing the opening fence to ```text for both occurrences (the diagram starting with "Parsing Style Layout..." and the second instance around lines 93-118) so the markdown parser treats them as plain text and the linter stops flagging them. In `@docs/wg/research/chromium/svg/resources-and-effects.md`: - Around line 129-137: The markdown code block containing the SVG filter primitives (e.g., feBlend, feColorMatrix, feComponentTransfer, feTile, feTurbulence) is missing a language identifier; update that fenced code block to include the language tag `text` (i.e., change the opening triple backticks to ```text) so markdownlint MD040 is satisfied and CI lint stays clean. In `@docs/wg/research/chromium/svg/text.md`: - Around line 31-41: The fenced code block that shows the class hierarchy (the block containing "LayoutSVGBlock └── LayoutSVGText … LayoutText └── LayoutSVGInlineText") is missing a language identifier; update the opening fence from ``` to ```text (or ```none) so markdownlint MD040 is satisfied and CI lint passes. In `@docs/wg/research/chromium/svg/use-and-foreign-object.md`: - Around line 85-95: In the docs example with the two unlabeled fenced code blocks showing the <use href="#sym".../> snippet and the resulting <svg...> snippet, add a language identifier (e.g., xml) to both fenced code blocks so markdownlint MD040 is satisfied; find the blocks containing the <use href="#sym" x="10" y="10" width="50" height="50"/> snippet and the block containing the <svg x="10" y="10" width="50" height="50" viewBox="..."> (children of `#sym`) snippet and prefix their opening triple backticks with xml (or text). --- Nitpick comments: In `@crates/grida-canvas/src/htmlcss/mod.rs`: - Around line 3304-3313: The test named test_render_svg_malformed_errors currently only calls render_svg("<svg>unclosed", 100.0, 100.0) and discards the result, so rename the test to something like test_render_svg_malformed_does_not_panic to match its behavior OR change the body to actually assert an Err from render_svg; update the function name or add an assertion on the Result returned by render_svg to make the test intent consistent with the name (refer to the test function test_render_svg_malformed_errors and the render_svg call). In `@crates/grida-dev/src/reftest/render.rs`: - Around line 162-303: Both render_svg_to_png_via_htmlcss and render_svg_to_png_via_sksvg duplicate the same tail: surface creation, transparent clear, drawing, snapshot, PNG encoding and writing; extract that into a single helper (e.g. fn write_svg_png<F>(output_path: &Path, width: i32, height: i32, draw: F) -> Result<()> where F: FnOnce(&mut skia_safe::Canvas) ) that creates the raster surface, clears with SkColor::TRANSPARENT, calls the provided closure to perform drawing (for htmlcss call canvas.draw_picture(&picture, ..) and for sksvg call dom.render(canvas)), encodes the image to PNG and writes out the file (including mkdir logic and error context). Replace the duplicated blocks in render_svg_to_png_via_htmlcss and render_svg_to_png_via_sksvg to compute svg bytes/width/height and then call this new helper with the appropriate closure; keep sniff_svg_dimensions as-is for size extraction. In `@crates/grida-dev/src/reftest/runner.rs`: - Around line 58-76: The current logic only appends backend suffixes when output_dir is auto-derived (args.output_dir.is_none()), causing explicit --output-dir to be shared across backends; change the code so the backend suffix (computed from args.renderer using SvgRenderer::Iosvg/Htmlcss/Sksvg) is applied whenever a suite-name directory is added to output_dir, regardless of args.output_dir being present: compute suffix with the existing match on args.renderer, sanitize the suite name with sanitize_dir_name(&name) and join output_dir with format!("{}{}", sanitized_name, suffix) (same behavior currently inside the is_none branch) so explicit output dirs are qualified for non-default renderers and won't clash.🪄 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:
7d54cc91-30d2-4b39-940a-b928d64fdd0b📒 Files selected for processing (22)
.claude/launch.jsoncrates/grida-canvas/src/htmlcss/collect.rscrates/grida-canvas/src/htmlcss/mod.rscrates/grida-canvas/src/htmlcss/paint.rscrates/grida-canvas/src/htmlcss/style.rscrates/grida-dev/src/reftest/args.rscrates/grida-dev/src/reftest/render.rscrates/grida-dev/src/reftest/runner.rsdocs/tags.ymldocs/wg/feat-2d/htmlcss.mddocs/wg/research/chromium/index.mddocs/wg/research/chromium/svg/comparison.mddocs/wg/research/chromium/svg/coordinate-systems.mddocs/wg/research/chromium/svg/index.mddocs/wg/research/chromium/svg/paint-servers.mddocs/wg/research/chromium/svg/path-geometry.mddocs/wg/research/chromium/svg/pipeline.mddocs/wg/research/chromium/svg/resources-and-effects.mddocs/wg/research/chromium/svg/svg-as-image.mddocs/wg/research/chromium/svg/text.mddocs/wg/research/chromium/svg/use-and-foreign-object.mdfixtures/test-html/L0/svg-inline-basic.html
Comment on lines
+538
to
+558
| fn write_svg_element(dom: &DemoDom, node: &DemoNode, out: &mut String, inject_xmlns: bool) { | ||
| let DemoNodeData::Element(ref data) = node.data else { | ||
| return; | ||
| }; | ||
| let tag: &str = &data.name.local; | ||
|
|
||
| out.push('<'); | ||
| out.push_str(tag); | ||
|
|
||
| // Emit attributes. | ||
| let mut has_xmlns = false; | ||
| for attr in &data.attrs { | ||
| let name: &str = &attr.name.local; | ||
| if name == "xmlns" { | ||
| has_xmlns = true; | ||
| } | ||
| out.push(' '); | ||
| out.push_str(name); | ||
| out.push_str("=\""); | ||
| escape_xml_attr(&attr.value, out); | ||
| out.push('"'); |
There was a problem hiding this comment.
Preserve qualified SVG/XML names during subtree serialization.
data.name.local / attr.name.local strips prefixes and namespace declarations, so valid inline SVG like xmlns:xlink + xlink:href, xml:space, or foreign-namespace descendants gets rewritten into a different document before svg::Dom sees it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/htmlcss/collect.rs` around lines 538 - 558, The
serialization is losing prefixes because it uses data.name.local and
attr.name.local; update write_svg_element so element and attribute names are
emitted with their qualified form (prefix:local when attr.name.prefix/is_some()
or data.name.prefix/is_some()), preserving "xmlns" and "xmlns:prefix" attributes
and any "xml:"/foreign" prefixes; i.e., construct tag_name = if let Some(p) =
&data.name.prefix { format!("{}:{}", p, data.name.local) } else {
data.name.local.to_string() } (and similarly for attribute names using
attr.name.prefix and attr.name.local), then write tag_name instead of
data.name.local and use that serialized name when checking for xmlns and
emitting attributes (keep escape_xml_attr for values and preserve
has_xmlns/inject_xmlns logic).
Comment on lines
+883
to
+890
| fn paint_inline_svg(canvas: &Canvas, xml: &[u8], w: f32, h: f32) -> bool { | ||
| use skia_safe::{svg, FontMgr, Size}; | ||
| let data = skia_safe::Data::new_copy(xml); | ||
| let Ok(mut dom) = svg::Dom::from_bytes(&data, FontMgr::default()) else { | ||
| return false; | ||
| }; | ||
| dom.set_container_size(Size::new(w, h)); | ||
| dom.render(canvas); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd '^font_repository\.rs$' crates -a | while read -r file; do
echo "=== ${file} ==="
sed -n '1,220p' "$file"
done
echo
echo "=== Font-related symbols ==="
rg -n -C3 'font_collection\(|FontMgr|typeface|register' crates/grida-canvas crates/grida-canvas-fonts
echo
echo "=== SVG DOM call sites ==="
rg -n -C3 'svg::Dom::from_bytes|paint_inline_svg' crates/grida-canvasRepository: gridaco/grida
Length of output: 50370
Use the shared font manager for SVG DOM parsing.
FontMgr::default() bypasses FontRepository, so SVG <text> will resolve fonts differently from the rest of htmlcss and ignore repo-registered faces. FontRepository does not expose its internal FontMgr publicly; you'll need to either:
- Add a public accessor on
FontRepositoryto return the font manager, or - Design an alternative SVG font resolution path that uses the
FontCollectionAPI instead of creating a newFontMgr.
Thread the shared font source that backs fonts.font_collection() (or its underlying FontMgr) into paint_inline_svg and the standalone render_svg() function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/htmlcss/paint.rs` around lines 883 - 890, The SVG
rendering currently creates a new FontMgr via FontMgr::default() in
paint_inline_svg (and likely in render_svg), which bypasses the shared
FontRepository and causes SVG <text> to ignore repo-registered faces; update the
API so paint_inline_svg and render_svg accept the shared font source (either the
FontMgr accessor or the FontCollection) from your FontRepository (e.g., pass
fonts.font_collection() or a new public get_font_mgr() from FontRepository) and
use that when constructing or parsing the svg::Dom instead of
FontMgr::default(); if FontRepository cannot expose FontMgr directly, change the
SVG path to resolve fonts via the FontCollection API and thread that
FontCollection into paint_inline_svg/render_svg so SVG text resolution matches
the rest of htmlcss.
Comment on lines
+1
to
+10
| --- | ||
| title: "SVG Rendering: Chromium vs Servo vs resvg" | ||
| tags: | ||
| - internal | ||
| - research | ||
| - chromium | ||
| - svg | ||
| - servo | ||
| - resvg | ||
| --- |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add format: md, description, and keywords to the frontmatter.
Same issue as the other new pages in this directory — body uses bare <svg>, <use>, <foreignObject>, <text>, <defs>, <animate> in prose, and there's no format: md, so the MDX parser will likely choke. description/keywords are also recommended for wg/ pages.
📝 Suggested frontmatter
---
title: "SVG Rendering: Chromium vs Servo vs resvg"
+description: "Cross-engine comparison of SVG rendering pipelines: Chromium (Blink), Servo, and resvg/usvg."
+keywords:
+ - svg
+ - chromium
+ - servo
+ - resvg
+ - rendering
+format: md
tags:
- internal
- research
- chromium
- svg
- servo
- resvg
---As per coding guidelines: "For most wg/ and reference/ Markdown pages that do not use MDX/JSX features, include format: md in frontmatter".
📝 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
| --- | |
| title: "SVG Rendering: Chromium vs Servo vs resvg" | |
| tags: | |
| - internal | |
| - research | |
| - chromium | |
| - svg | |
| - servo | |
| - resvg | |
| --- | |
| --- | |
| title: "SVG Rendering: Chromium vs Servo vs resvg" | |
| description: "Cross-engine comparison of SVG rendering pipelines: Chromium (Blink), Servo, and resvg/usvg." | |
| keywords: | |
| - svg | |
| - chromium | |
| - servo | |
| - resvg | |
| - rendering | |
| format: md | |
| tags: | |
| - internal | |
| - research | |
| - chromium | |
| - svg | |
| - servo | |
| - resvg | |
| --- |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/research/chromium/svg/comparison.md` around lines 1 - 10, Add missing
frontmatter keys to the top of the document: include format: md, a short
description string in description, and a keywords array under keywords; this
ensures the page is treated as plain Markdown (avoiding MDX parsing of bare tags
like <svg>, <use>, <foreignObject>, <text>, <defs>, <animate>) and provides
metadata for the wg/ page. Place these keys alongside the existing title and
tags in the YAML frontmatter; keep description concise and populate keywords
with relevant terms such as "svg", "chromium", "servo", and "resvg".
Comment on lines
+1
to
+9
| --- | ||
| title: "Chromium SVG Coordinate Systems" | ||
| tags: | ||
| - internal | ||
| - research | ||
| - chromium | ||
| - rendering | ||
| - svg | ||
| --- |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add format: md, description, and keywords to the frontmatter.
This page uses bare angle-bracketed tags (<svg>, <rect>, <symbol>, <pattern>, <marker>, <foreignObject>) throughout the prose; without format: md, the MDX parser will treat them as components and the build will likely fail. Add description/keywords to match the convention for wg/ pages.
📝 Suggested frontmatter
---
title: "Chromium SVG Coordinate Systems"
+description: "Blink's SVG coordinate hierarchy: LayoutSVGRoot, viewBox/preserveAspectRatio, transforms, percentage resolution, and pattern/gradient units."
+keywords:
+ - chromium
+ - blink
+ - svg
+ - coordinate-systems
+format: md
tags:
- internal
- research
- chromium
- rendering
- svg
---As per coding guidelines: "For most wg/ and reference/ Markdown pages that do not use MDX/JSX features, include format: md in frontmatter".
📝 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
| --- | |
| title: "Chromium SVG Coordinate Systems" | |
| tags: | |
| - internal | |
| - research | |
| - chromium | |
| - rendering | |
| - svg | |
| --- | |
| --- | |
| title: "Chromium SVG Coordinate Systems" | |
| description: "Blink's SVG coordinate hierarchy: LayoutSVGRoot, viewBox/preserveAspectRatio, transforms, percentage resolution, and pattern/gradient units." | |
| keywords: | |
| - chromium | |
| - blink | |
| - svg | |
| - coordinate-systems | |
| format: md | |
| tags: | |
| - internal | |
| - research | |
| - chromium | |
| - rendering | |
| - svg | |
| --- |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/research/chromium/svg/coordinate-systems.md` around lines 1 - 9,
Update the YAML frontmatter to include format: md plus description and keywords
entries so the MDX parser won't treat inline tags like <svg>, <rect>, <symbol>,
<pattern>, <marker>, and <foreignObject> as components; specifically add format:
md at the top-level, a short description string (e.g., describing "Chromium SVG
Coordinate Systems"), and a keywords array to match existing wg/ pages, leaving
the existing title and tags intact.
Comment on lines
+1
to
+9
| --- | ||
| title: "Chromium SVG Paint Servers" | ||
| tags: | ||
| - internal | ||
| - research | ||
| - chromium | ||
| - rendering | ||
| - svg | ||
| --- |
There was a problem hiding this comment.
Add format: md and complete frontmatter metadata.
Line 1–9 should include format: md (to avoid MDX parsing edge cases with inline <...> syntax in this doc) and add description + keywords for doc discoverability/consistency.
Proposed frontmatter patch
---
title: "Chromium SVG Paint Servers"
+description: "How Chromium/Blink resolves SVG paint servers (gradients/patterns) into Skia shaders during paint."
+keywords:
+ - chromium
+ - blink
+ - svg
+ - paint server
+ - gradient
+ - pattern
+format: md
tags:
- internal
- research
- chromium
- rendering
- svg
---As per coding guidelines, "docs/{wg,reference}/**/*.md: include format: md for non-MDX pages" and "docs/{wg,...}/**/*.md: prefer frontmatter with title, description, and keywords."
📝 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
| --- | |
| title: "Chromium SVG Paint Servers" | |
| tags: | |
| - internal | |
| - research | |
| - chromium | |
| - rendering | |
| - svg | |
| --- | |
| --- | |
| title: "Chromium SVG Paint Servers" | |
| description: "How Chromium/Blink resolves SVG paint servers (gradients/patterns) into Skia shaders during paint." | |
| keywords: | |
| - chromium | |
| - blink | |
| - svg | |
| - paint server | |
| - gradient | |
| - pattern | |
| format: md | |
| tags: | |
| - internal | |
| - research | |
| - chromium | |
| - rendering | |
| - svg | |
| --- |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/research/chromium/svg/paint-servers.md` around lines 1 - 9, Add a
top-level frontmatter field "format: md" and complete metadata by adding
"description" and "keywords" entries to the existing YAML block (which currently
contains "title" and "tags") so the doc avoids MDX parsing issues and is
discoverable; update the frontmatter in the Chromium SVG Paint Servers doc by
inserting format: md beneath the title line and adding a short "description:"
string plus a "keywords:" array (or comma-separated string) alongside the
existing "tags:" field in the same YAML block.
Comment on lines
+1
to
+9
| --- | ||
| title: "Chromium SVG Resources and Effects" | ||
| tags: | ||
| - internal | ||
| - research | ||
| - chromium | ||
| - rendering | ||
| - svg | ||
| --- |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add format: md, description, and keywords to the frontmatter.
Same pattern as the other new SVG research pages — body uses bare <clipPath>, <mask>, <filter>, <marker>, <feXxx>, <defs> in prose. Without format: md, MDX will try to parse these.
📝 Suggested frontmatter
---
title: "Chromium SVG Resources and Effects"
+description: "Blink's SVG resource pipeline: <clipPath>, <mask>, <filter>, <marker>, and how they integrate with paint order, render surfaces, and the compositor."
+keywords:
+ - chromium
+ - blink
+ - svg
+ - filter
+ - mask
+ - clip-path
+format: md
tags:
- internal
- research
- chromium
- rendering
- svg
---As per coding guidelines: "For most wg/ and reference/ Markdown pages that do not use MDX/JSX features, include format: md in frontmatter".
📝 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
| --- | |
| title: "Chromium SVG Resources and Effects" | |
| tags: | |
| - internal | |
| - research | |
| - chromium | |
| - rendering | |
| - svg | |
| --- | |
| --- | |
| title: "Chromium SVG Resources and Effects" | |
| description: "Blink's SVG resource pipeline: <clipPath>, <mask>, <filter>, <marker>, and how they integrate with paint order, render surfaces, and the compositor." | |
| keywords: | |
| - chromium | |
| - blink | |
| - svg | |
| - filter | |
| - mask | |
| - clip-path | |
| format: md | |
| tags: | |
| - internal | |
| - research | |
| - chromium | |
| - rendering | |
| - svg | |
| --- |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/research/chromium/svg/resources-and-effects.md` around lines 1 - 9,
Update the frontmatter for the "Chromium SVG Resources and Effects" document to
include format: md plus a short description and a keywords list; specifically
modify the existing frontmatter block containing the title "Chromium SVG
Resources and Effects" to add format: md at the top and add a descriptive string
under description and an array of relevant terms under keywords (e.g., svg,
chromium, rendering, resources) so the page is treated as plain Markdown and MDX
won't attempt to parse raw SVG elements like <clipPath> / <mask> / <filter> used
in the body.
Comment on lines
+1
to
+9
| --- | ||
| title: "Chromium SVG as Image" | ||
| tags: | ||
| - internal | ||
| - research | ||
| - chromium | ||
| - rendering | ||
| - svg | ||
| --- |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add format: md, description, and keywords to the frontmatter.
This page lives under docs/wg/research/..., doesn't use any MDX/JSX, and the prose freely contains angle-bracketed identifiers like <svg>, <img>, <iframe>, <object> as plain text (e.g. Lines 13–19, 87, 113, 116). Without format: md, Docusaurus parses the file as MDX and these will break the build (or render unexpectedly). Adding description + keywords also matches the documented frontmatter convention for wg/ pages.
📝 Suggested frontmatter
---
title: "Chromium SVG as Image"
+description: "How Blink renders SVG in image, document, and inline modes — SVGImage, IsolatedSVGDocumentHost, and SVGImageForContainer."
+keywords:
+ - chromium
+ - blink
+ - svg
+ - rendering
+format: md
tags:
- internal
- research
- chromium
- rendering
- svg
---As per coding guidelines: "For most wg/ and reference/ Markdown pages that do not use MDX/JSX features, include format: md in frontmatter" and "When using angle brackets in Docusaurus markdown (e.g., comparisons, URLs, generics), prefer format: md frontmatter to opt out of MDX parsing".
📝 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
| --- | |
| title: "Chromium SVG as Image" | |
| tags: | |
| - internal | |
| - research | |
| - chromium | |
| - rendering | |
| - svg | |
| --- | |
| --- | |
| title: "Chromium SVG as Image" | |
| description: "How Blink renders SVG in image, document, and inline modes — SVGImage, IsolatedSVGDocumentHost, and SVGImageForContainer." | |
| keywords: | |
| - chromium | |
| - blink | |
| - svg | |
| - rendering | |
| format: md | |
| tags: | |
| - internal | |
| - research | |
| - chromium | |
| - rendering | |
| - svg | |
| --- |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/research/chromium/svg/svg-as-image.md` around lines 1 - 9, The
frontmatter lacks the required keys causing Docusaurus to parse this page as MDX
and break on plain-text angle-bracket tokens like <svg>, <img>, <iframe>, and
<object>; update the YAML frontmatter at the top of the file (the block with
title/tags) to include format: md plus a brief description: string and keywords:
[ ... ] array so the page is treated as plain Markdown and follows wg/
frontmatter conventions. Ensure the new keys are added alongside the existing
title and tags entries in the frontmatter block.
Comment on lines
+1
to
+9
| --- | ||
| title: "Chromium SVG Text" | ||
| tags: | ||
| - internal | ||
| - research | ||
| - chromium | ||
| - rendering | ||
| - svg | ||
| --- |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add format: md, description, and keywords to the frontmatter.
The body uses <text>, <tspan>, <textPath>, <defs>, etc. as inline angle-bracketed identifiers in prose. Without format: md, the MDX parser is likely to choke on these. Also add description/keywords per the documented frontmatter convention for wg/ pages.
📝 Suggested frontmatter
---
title: "Chromium SVG Text"
+description: "Two-phase SVG text layout in Blink: LayoutNG inline + SvgTextLayoutAlgorithm, with text-anchor, textLength, and <textPath>."
+keywords:
+ - chromium
+ - blink
+ - svg
+ - text
+ - rendering
+format: md
tags:
- internal
- research
- chromium
- rendering
- svg
---As per coding guidelines: "For most wg/ and reference/ Markdown pages that do not use MDX/JSX features, include format: md in frontmatter" and "prefer frontmatter with title, description, and keywords".
📝 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
| --- | |
| title: "Chromium SVG Text" | |
| tags: | |
| - internal | |
| - research | |
| - chromium | |
| - rendering | |
| - svg | |
| --- | |
| --- | |
| title: "Chromium SVG Text" | |
| description: "Two-phase SVG text layout in Blink: LayoutNG inline + SvgTextLayoutAlgorithm, with text-anchor, textLength, and <textPath>." | |
| keywords: | |
| - chromium | |
| - blink | |
| - svg | |
| - text | |
| - rendering | |
| format: md | |
| tags: | |
| - internal | |
| - research | |
| - chromium | |
| - rendering | |
| - svg | |
| --- |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/research/chromium/svg/text.md` around lines 1 - 9, The frontmatter
only contains title and tags which can cause the MDX parser to misinterpret
inline angle-bracketed SVG identifiers; update the YAML frontmatter by adding
format: md and include description and keywords fields alongside the existing
title and tags (e.g., add format: md, a concise description string, and a
keywords array) so the page is treated as plain Markdown and follows wg/
frontmatter conventions; ensure you keep the existing title and tags keys
intact.
Comment on lines
+1
to
+9
| --- | ||
| title: "Chromium SVG <use> and <foreignObject>" | ||
| tags: | ||
| - internal | ||
| - research | ||
| - chromium | ||
| - rendering | ||
| - svg | ||
| --- |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add format: md, description, and keywords; the title's angle brackets make this especially important.
The title literally contains <use> and <foreignObject>, and the body is full of bracketed identifiers in prose (<symbol>, <g>, <defs>, <svg>, etc.). Without format: md, Docusaurus will MDX-parse the page and these are very likely to break rendering or the build.
📝 Suggested frontmatter
---
title: "Chromium SVG <use> and <foreignObject>"
+description: "How Blink instantiates <use> as a user-agent shadow tree and bridges <foreignObject> from SVG layout into HTML paint and hit testing."
+keywords:
+ - chromium
+ - blink
+ - svg
+ - use
+ - foreignObject
+format: md
tags:
- internal
- research
- chromium
- rendering
- svg
---As per coding guidelines: "When using angle brackets in Docusaurus markdown (e.g., comparisons, URLs, generics), prefer format: md frontmatter to opt out of MDX parsing".
📝 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
| --- | |
| title: "Chromium SVG <use> and <foreignObject>" | |
| tags: | |
| - internal | |
| - research | |
| - chromium | |
| - rendering | |
| - svg | |
| --- | |
| --- | |
| title: "Chromium SVG <use> and <foreignObject>" | |
| description: "How Blink instantiates <use> as a user-agent shadow tree and bridges <foreignObject> from SVG layout into HTML paint and hit testing." | |
| keywords: | |
| - chromium | |
| - blink | |
| - svg | |
| - use | |
| - foreignObject | |
| format: md | |
| tags: | |
| - internal | |
| - research | |
| - chromium | |
| - rendering | |
| - svg | |
| --- |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/research/chromium/svg/use-and-foreign-object.md` around lines 1 - 9,
Add explicit frontmatter keys to opt out of MDX parsing and improve metadata:
add format: md to the existing frontmatter and include a short description and
keywords array so the title containing angle brackets ("Chromium SVG <use> and
<foreignObject>") and inline identifiers like <symbol>, <g>, <defs>, <svg> won’t
be MDX-parsed; update the top YAML block (the same block that currently contains
title and tags) to include format: md, description: "<brief description of the
doc>", and keywords: [comma-separated, relevant terms] so the page renders
correctly.
4 tasks
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
Wires SVG into the htmlcss renderer via Skia's built-in
svg::Dom— the Servo-style replaced-element + subtree-serialization pattern, with Skia's GPU-capable SVG DOM in place of resvg+tiny-skia. Three commits:docs(research): chromium SVG pipeline— 9 research docs underdocs/wg/research/chromium/svg/covering Blink's SVG rendering end-to-end (DOM, coords, paint servers, resources/effects, path geometry, text,<use>/<foreignObject>, SVG-as-image, cross-engine comparison).feat(htmlcss): inline <svg> + standalone .svg via Skia svg::Dom— addshtmlcss::render_svgandhtmlcss::render_anyentry points; inline<svg>inside HTML works through the same pipeline. Wires--renderer {iosvg|htmlcss}intogrida-dev reftest.feat(reftest): sksvg backend— adds--renderer sksvg, the thinnest possible bridge from SVG bytes toskia_safe::svg::Domand a raster surface (no htmlcss, no PictureRecorder). Diagnostic split that proves htmlcss wrapping adds zero divergence vs. raw Skia (1672/1673 bit-identical), isolating the 6 parse failures as Skiasvg::Domlimitations rather than our wiring.Baseline (resvg-test-suite, 1,679 tests)
htmlcss vs sksvg: 1672/1673 tests bit-identical. The lone exception (
painting_mix-blend-mode_opacity-on-group: htmlcss 62.5%, sksvg 78.8%) is htmlcss regressing 16pp via PictureRecorder fidelity loss on blend-mode-over-group.Scope
render_svg/render_any/ inline-<svg>entry points, no caller churn.<foreignObject>HTML recursion; no scripting / SMIL.Test plan
cargo test -p cg— 777 cg tests greencargo test -p grida-dev— reftest harness greenhtmlcss::render_svgand confirm the API surface matches what a future native SVG renderer would want to exposeSummary by CodeRabbit
New Features
Documentation
Tests