feat(canvas): HTML+CSS embed renderer with Chromium-aligned architecture#622
feat(canvas): HTML+CSS embed renderer with Chromium-aligned architecture#622softmarshmallow merged 3 commits intomainfrom
Conversation
…tecture Add a self-contained `htmlcss` rendering engine that converts HTML+CSS to Skia Pictures for opaque embedding on the canvas (HTMLEmbedNode). ## Architecture Three-phase pipeline inspired by Chromium's Blink renderer: 1. **Collect** (Stylo DOM → StyledElement tree) — no Skia objects 2. **Layout** (StyledElement → Taffy → LayoutBox tree) — block/flex/grid 3. **Paint** (LayoutBox → Skia Picture) — backgrounds, borders, text ## HTMLEmbedNode New node type for rendering HTML as an opaque picture on the canvas. Registered across all cg match arms (schema, painter, layout, geometry, resources, scene_graph, cost_prediction, compositor). ## CSS Support - **Layout**: block (margin collapsing), flex, grid via Taffy; subpixel precision (disable_rounding) - **Box model**: width/height/min/max, padding, margin, border (all sides, solid/dashed/dotted), per-corner elliptical border-radius - **Background**: solid color, linear/radial/conic gradients, multi-layer - **Text**: font properties, text-decoration (bitfield — simultaneous underline + line-through), text-align, text-transform, white-space, letter/word-spacing - **Inline elements**: merged into single Paragraph with per-run styles; OpenBox/CloseBox placeholders for padding/border spacing (Chromium kOpenTag/kCloseTag model); InlineBoxDecoration with background, border-radius, border via get_rects_for_range() - **Lists**: ul/ol with disc/circle/square/decimal/lower-alpha/upper-alpha markers, nested list counters - **Effects**: opacity, visibility, overflow clipping, box-shadow (outer), mix-blend-mode ## grida-dev - Embed/Convert dialog (rfd) for HTML file drops - Resize support for HTMLEmbedNode - Content-height measurement via measure_content_height() ## Documentation - `docs/wg/feat-2d/htmlcss.md` — feature doc with full CSS property table - `docs/wg/research/chromium/blink-rendering-pipeline.md` — Chromium research: ComputedStyle groups, LayoutNG, inline layout, paint phases, list markers Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
6 Skipped Deployments
|
WalkthroughAdds a full HTML+CSS embed renderer and a new Changes
Sequence Diagram(s)sequenceDiagram
participant HTML as HTML Source
participant Collect as Phase 1\ncollect_styled_tree()
participant Stylo as Stylo Cascade
participant Styled as StyledElement\nTree
participant Layout as Phase 2\ncompute_layout()
participant Taffy as Taffy Layout
participant LayoutTree as LayoutBox\nTree
participant Paint as Phase 3\npaint_to_picture()
participant Skia as Skia Picture
HTML->>Collect: provide HTML string
Collect->>Stylo: parse & resolve CSS
Stylo->>Styled: produce ComputedValues -> StyledElement
Styled->>Layout: styled root
Layout->>Taffy: build nodes + text measure callbacks
Taffy->>LayoutTree: computed positions/sizes
LayoutTree->>Paint: positioned LayoutBox tree
Paint->>Skia: draw backgrounds, borders, text, inline decorations
Skia-->>HTML: return Picture (with cull rect)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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: bbc301fe98
ℹ️ 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".
| let layout_root = layout::compute_layout(&root, width, fonts); | ||
| let content_height = layout_root.height; | ||
|
|
||
| Ok(paint::paint_to_picture( | ||
| &layout_root, |
There was a problem hiding this comment.
This renderer ignores the caller-provided height and always records the picture at content_height, so an HTMLEmbed node cannot enforce its own viewport height after resize. In the current paint path, HTML layers are clipped to the picture cull rect, which means content can extend past the node's intended height instead of being clipped to the layer bounds. This causes incorrect rendering whenever the node height differs from the document's intrinsic content height.
Useful? React with 👍 / 👎.
| if let Some(len) = lp.0.to_length() { | ||
| CssLength::Px(len.px()) | ||
| } else { | ||
| CssLength::Auto |
There was a problem hiding this comment.
Percentage width/height values are dropped here because LengthPercentage is converted only through to_length(), and non-length values fall back to CssLength::Auto. This makes common CSS like width: 50% or height: 100% lay out incorrectly as auto-sized boxes in Taffy. The conversion should emit CssLength::Percent when a percentage is present.
Useful? React with 👍 / 👎.
| items.push(InlineRunItem::Text(TextRun { | ||
| text: run.text.clone(), | ||
| font: el.font.clone(), | ||
| color: el.color, | ||
| decoration: None, |
There was a problem hiding this comment.
Flattening a child InlineGroup rewrites each text run with the parent element's font/color, which erases nested inline styling. For example, nested tags like <strong>...<em>...</em>...</strong> will lose the inner run's specific style during collection. Reusing the original run style is necessary to keep mixed inline formatting accurate.
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/grida-canvas/src/resources/mod.rs (1)
147-153:⚠️ Potential issue | 🟠 MajorImage URL extraction skips most HTMLEmbed nodes due link-iteration scope.
At Line [150],
scene.graph.iter()walks parent→children link entries, not all graph nodes. LeafHTMLEmbednodes without children are never visited, so their fill image URLs are missed.🐛 Proposed fix
fn extract_image_urls(scene: &Scene) -> Vec<String> { use crate::node::schema::Node; let mut urls = Vec::new(); - for (id, _) in scene.graph.iter() { - let Ok(node) = scene.graph.get_node(&id) else { - continue; - }; + for (_id, node) in scene.graph.nodes_iter() { match node { Node::Image(n) => {Also applies to: 225-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/resources/mod.rs` around lines 147 - 153, The extract_image_urls function currently iterates scene.graph.iter() which traverses parent→children link entries and therefore skips leaf nodes (like HTMLEmbed) that have no children; update extract_image_urls to iterate all graph nodes instead (e.g., use the graph's node collection/keys or nodes() iterator) so every Node is visited, then for each Node (match on Node::HTMLEmbed and other variants) extract fill image URLs via the same logic; apply the same change for the other occurrence noted around lines 225-227 so HTMLEmbed leaf nodes are included.
🧹 Nitpick comments (11)
crates/grida-canvas/src/painter/painter_debug_node.rs (1)
696-698: Acceptable placeholder consistent with sibling node types.The empty match arm with TODO mirrors the pattern used for
AttributedTextandMarkdown(lines 690-694). The production painter inpainter.rshas full HTMLEmbed rendering support per the context snippets, so this is appropriate for initial implementation.Would you like me to draft the debug rendering implementation for HTMLEmbed, drawing the shape bounds with a distinctive style (similar to how
draw_error_nodeworks)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/painter/painter_debug_node.rs` around lines 696 - 698, Implement the debug rendering for Node::HTMLEmbed by following the same placeholder pattern used for AttributedText and Markdown: draw the node bounds and a distinctive debug style (like the existing draw_error_node behavior) so HTMLEmbed is visible in debug mode; locate the match arm for Node::HTMLEmbed in painter_debug_node.rs and call the same helper(s) used for error/debug visualization (e.g., draw_error_node or the bounding-shape drawing routine) with the HTMLEmbed node's layout/rect and a clear debug color/style.docs/wg/research/chromium/blink-rendering-pipeline.md (2)
1-4: Addformat: mdfrontmatter for MDX compatibility.The frontmatter is missing the
format: mdfield. Since this file doesn't use JSX/MDX features and contains angle brackets in code blocks, addingformat: mdwill prevent potential MDX parsing issues.📝 Proposed fix
--- title: "Blink Rendering Pipeline — Style, Layout, Paint" +format: md tags: [research, chromium, css, layout, paint] ---🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/wg/research/chromium/blink-rendering-pipeline.md` around lines 1 - 4, The frontmatter for the Markdown document titled "Blink Rendering Pipeline — Style, Layout, Paint" is missing the format specifier; update the YAML frontmatter at the top of the file to include format: md so the block contains title, tags and format: md to ensure MDX parsers treat this file as plain Markdown (no JSX) and avoid parsing errors from angle-bracket code samples.
19-29: Add language specifier to fenced code block.The diagram code block could use
textorplaintextas a language specifier to satisfy the linter, though this is purely cosmetic for ASCII art diagrams.📝 Proposed fix
-``` +```text DOM + CSSOM ↓ [1. Style] ComputedStyle per element🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/wg/research/chromium/blink-rendering-pipeline.md` around lines 19 - 29, The fenced ASCII diagram block lacks a language specifier; update the triple-backtick fence surrounding the diagram (the block that begins with "DOM + CSSOM" and the arrows/steps [1. Style]...[4. Composite]) to include a language token such as text or plaintext (e.g., change ``` to ```text) so the linter is satisfied.crates/grida-canvas/examples/golden_htmlcss.rs (1)
25-27: Consider reusingFontRepositoryfor efficiency.
fonts()is called for each file, creating a newFontRepositoryeach time. For directories with many HTML files, this could add overhead. Consider passing a sharedFontRepositorytorender_to_png:♻️ Proposed refactor for performance
-fn render_to_png(html: &str, width: f32, name: &str, out_dir: &Path) { - let fonts = fonts(); +fn render_to_png(html: &str, width: f32, name: &str, out_dir: &Path, fonts: &FontRepository) { let picture = htmlcss::render(html, width, 600.0, &fonts).expect("render failed");Then update callers to pass a shared
fontsinstance created once inmain().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/examples/golden_htmlcss.rs` around lines 25 - 27, The render_to_png function currently calls fonts() each time which constructs a new FontRepository per call; refactor render_to_png to accept a shared FontRepository reference (e.g., &FontRepository) instead of calling fonts() internally, update its signature and body to use that passed repository, and then modify main to create a single FontRepository once and pass it into render_to_png for each file processed to avoid repeatedly rebuilding the font repository.crates/grida-canvas/src/htmlcss/collect.rs (1)
55-111: Debug string matching for list style types is fragile but acceptable.The use of
format!("{:?}", lst)and string matching is a workaround for Stylo's generated enum types. The comment at lines 56-66 explains the rationale well.Consider adding a comment noting that this approach may break if Stylo changes its Debug representation:
📝 Optional: Add maintenance note
/// Generate marker text for a list item. /// /// Mirrors Chromium's `ListMarker::MarkerText()` which uses `CounterStyle` /// to produce the prefix (bullet character or formatted number). +/// +/// NOTE: Uses Debug format string matching because Stylo's ListStyleType +/// is a generated enum without stable pattern matching. This may need +/// updates if Stylo changes its Debug representation. fn generate_marker_text<T: std::fmt::Debug>(lst: &T, ordinal: i32) -> Option<String> {🤖 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 55 - 111, Add a short maintenance note above the generate_marker_text function explaining that the current approach relies on format!("{:?}", lst) and string matching of Stylo's Debug output (matching variants like "Disc", "Circle", "Decimal", etc.) and may break if Stylo changes its Debug representation or enum variant names; mention that callers use to_roman and ordinal-based logic and that a future improvement would be to switch to a typed API or pattern-match the generated enum when available.crates/grida-canvas/src/htmlcss/mod.rs (1)
21-47: Clarify the unused_heightparameter.The
_heightparameter is intentionally unused since the renderer computes content height via Taffy'sMaxContentlayout. Consider either:
- Documenting this behavior in the doc comment, or
- Removing the parameter if height constraints aren't planned
The current implementation correctly uses
layout_root.height(computed content height) for the picture bounds.📝 Option 1: Document the parameter
/// Render HTML+CSS to a Skia Picture. +/// +/// # Parameters +/// - `html`: HTML+CSS source string +/// - `width`: Available width for text wrapping +/// - `_height`: Reserved for future use (content height is computed automatically) +/// - `fonts`: Font repository for text rendering pub fn render( html: &str, width: f32, _height: f32, fonts: &FontRepository, ) -> Result<skia_safe::Picture, String> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/mod.rs` around lines 21 - 47, The render function's _height parameter is intentionally unused because content height is computed from layout::compute_layout and used via layout_root.height; update the render signature or docs to reflect this: either remove the unused _height parameter from pub fn render(...) if no external height constraint will be supported, or add a short doc comment above pub fn render explaining that height is ignored and layout uses Taffy's MaxContent to determine content height (see layout::compute_layout and layout_root.height) so future readers know why _height is unused.crates/grida-canvas/src/htmlcss/paint.rs (1)
248-253: Minor: Unnecessaryas scalarcast.The
scalartype is alreadyf32in skia-safe, so0.5 as scalaris redundant. Same applies to lines 265-266.📝 Cleaner syntax
skia_safe::shaders::radial_gradient( - (skia_safe::Point::new(0.5, 0.5), 0.5 as scalar), + (skia_safe::Point::new(0.5, 0.5), 0.5), &gradient, Some(&matrix), )skia_safe::shaders::sweep_gradient( skia_safe::Point::new(0.5, 0.5), - (0.0 as scalar, 360.0 as scalar), + (0.0, 360.0), &gradient, Some(&matrix), )🤖 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 248 - 253, The radial_gradient shader calls use an unnecessary cast "0.5 as scalar" (and similar occurrences around the same function), so remove the redundant "as scalar" casts in the skia_safe::shaders::radial_gradient invocations (and any other places in this file where 0.5 is cast to scalar) and just pass 0.5 (or 0.5f32) directly to match the scalar type expected by skia_safe::Point and the radial_gradient function.crates/grida-canvas/src/htmlcss/layout.rs (4)
309-315: Consider reusingmap_align_itemsforalign_selfmapping.The
align_selfmapping duplicates the logic inmap_align_items. While minor, consolidating would reduce code duplication.♻️ Reuse map_align_items
- align_self: el.align_self.map(|a| match a { - types::AlignItems::Start => taffy::AlignSelf::FlexStart, - types::AlignItems::End => taffy::AlignSelf::FlexEnd, - types::AlignItems::Center => taffy::AlignSelf::Center, - types::AlignItems::Stretch => taffy::AlignSelf::Stretch, - types::AlignItems::Baseline => taffy::AlignSelf::Baseline, - }), + align_self: el.align_self.map(|a| { + // AlignSelf variants match AlignItems + match map_align_items(a) { + taffy::AlignItems::FlexStart => taffy::AlignSelf::FlexStart, + taffy::AlignItems::FlexEnd => taffy::AlignSelf::FlexEnd, + taffy::AlignItems::Center => taffy::AlignSelf::Center, + taffy::AlignItems::Stretch => taffy::AlignSelf::Stretch, + taffy::AlignItems::Baseline => taffy::AlignSelf::Baseline, + _ => taffy::AlignSelf::Auto, + } + }),Or create a dedicated
map_align_selfhelper that reuses the core logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/layout.rs` around lines 309 - 315, The align_self mapping duplicates map_align_items logic; modify the code that sets align_self to call the existing map_align_items helper (or create a small map_align_self that delegates to map_align_items) instead of matching again. Update the block that currently maps el.align_self to use map_align_items(el.align_self) (or call map_align_self(el.align_self)), ensuring the helper accepts the same types::AlignItems input and returns taffy::AlignSelf so the conversion is centralized and duplication removed.
69-78: Consider propagating or logging Taffy layout errors.The
Resultfromcompute_layout_with_measureis discarded withlet _ = .... While layout errors are unlikely given valid tree construction, silently ignoring failures could mask issues during development.♻️ Optional: Add error handling
- let _ = taffy.compute_layout_with_measure( + taffy.compute_layout_with_measure( taffy_root, taffy::Size { width: AvailableSpace::Definite(available_width), height: AvailableSpace::MaxContent, }, |known_dimensions, available_space, _node_id, context, _style| { text_measure_func(known_dimensions, available_space, context, &fc) }, - ); + ).expect("Taffy layout computation failed");Or return a
Resultfromcompute_layoutif graceful handling is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/layout.rs` around lines 69 - 78, The call to taffy.compute_layout_with_measure currently discards its Result (let _ = ...), so update compute_layout to handle failures: capture the Result from compute_layout_with_measure (for the taffy_root call that passes text_measure_func and &fc), and either propagate the error by returning a Result from the surrounding compute_layout function or log the error (using the crate's logger) with context including taffy_root and the error details; ensure you reference the same symbols (taffy.compute_layout_with_measure, taffy_root, text_measure_func, fc) so the error path is handled instead of ignored.
480-493:LineHeight::Normaluses 1.5, which differs from typical browser default (~1.2).CSS
line-height: normaltypically resolves to approximately 1.1-1.2 depending on the font. Using 1.5 may produce looser line spacing than expected for HTML content.♻️ Consider using a browser-typical value
types::LineHeight::Normal => { ts.set_height_override(true); - ts.set_height(1.5); + ts.set_height(1.2); // Closer to browser default for most fonts }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/layout.rs` around lines 480 - 493, The code treats types::LineHeight::Normal as 1.5 which is larger than typical browser defaults; update the handling in the match arm for types::LineHeight::Normal so ts.set_height_override(true) is followed by setting ts.set_height to a browser-typical value (e.g. ~1.15–1.2) or compute a normal line-height from font metrics if available instead of hardcoding 1.5; adjust the logic around font.line_height, ts.set_height_override, ts.set_height and consider using font.size if you need to convert px->unitless.
459-467: Platform-specific font family mapping may cause rendering issues on non-macOS.The generic font families are mapped to macOS-specific fonts (
.AppleSystemUIFont,Menlo). On Windows/Linux, these fonts may not exist, potentially causing fallback to unexpected fonts.♻️ Consider cross-platform font mapping
let families: Vec<String> = font .families .iter() .map(|s| match s.as_str() { - "system-ui" | "-apple-system" | "BlinkMacSystemFont" => { - ".AppleSystemUIFont".to_string() - } - "sans-serif" => "Helvetica".to_string(), - "serif" => "Times".to_string(), - "monospace" => "Menlo".to_string(), + "system-ui" | "-apple-system" | "BlinkMacSystemFont" => { + // Skia typically handles system-ui; keep as-is or use platform detection + "system-ui".to_string() + } + "sans-serif" => "sans-serif".to_string(), + "serif" => "serif".to_string(), + "monospace" => "monospace".to_string(), other => other.to_string(), }) .collect();Or implement platform detection to select appropriate concrete fonts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/layout.rs` around lines 459 - 467, The current font-family mapping closure (the map matching s.as_str() that returns ".AppleSystemUIFont", "Helvetica", "Times", "Menlo", or other.to_string()) uses macOS-specific concrete fonts which can break rendering on Windows/Linux; update the mapping in that closure to return cross-platform safe values instead (e.g., keep "system-ui" or map to generic families like "sans-serif", "serif", "monospace") or add platform detection (using cfg!(target_os = "windows") / cfg!(target_os = "linux") branches) to choose appropriate concrete fonts per OS; locate the closure that performs map(|s| match s.as_str() { ... }) in layout.rs and replace macOS-only entries (".AppleSystemUIFont", "Menlo") with either generic family names or OS-specific alternatives via cfg checks.
🤖 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-canvas/src/htmlcss/layout.rs`:
- Around line 514-516: The code hardcodes TextDecorationStyle::Solid when
applying decorations in layout.rs (ts.set_decoration_style) which ignores the
font.decoration_style; replace the hardcoded value by converting
font.decoration_style through the existing mapping in sk/mappings.rs (use the
mapping function that returns a textlayout::TextDecorationStyle) and pass that
result to ts.set_decoration_style; make the identical change in the other
occurrence in painter/markdown.rs so both places use the mapped
font.decoration_style instead of Solid.
- Around line 195-199: The computation of max_width inside the text measuring
code incorrectly sets AvailableSpace::MinContent to 0.0 causing premature
wrapping; update the match arm for AvailableSpace::MinContent (the same match
that assigns to max_width using known_dimensions.width.unwrap_or(w)) to use
100_000.0 (same as AvailableSpace::MaxContent) so min-content measures the
unwrapped intrinsic width instead of forcing wraps.
In `@crates/grida-canvas/src/htmlcss/paint.rs`:
- Around line 273-299: paint_borders currently draws straight lines with
canvas.draw_line and ignores StyledElement border-radius; update paint_borders
to detect when any corner radius > 0 (from the element's border radius data on
StyledElement) and switch to rounded-rect drawing: compute inner/outer rects
adjusted by each side's width and radii, build a rounded rectangle stroke (use
canvas.draw_round_rect or construct a path with rounded corners) and paint it
with border_paint for each side or stroke the rounded rect so borders follow the
radii; keep the existing straight-line path as the fallback when all corner
radii are zero. Ensure you reference paint_borders, border_paint,
StyledElement.border and the Canvas draw_round_rect/path APIs when making the
change.
In `@crates/grida-canvas/src/htmlcss/types.rs`:
- Around line 67-73: The BoxSizing enum currently marks BorderBox as the default
but the CSS spec initial value is content-box; update the default so ContentBox
is the Default. Locate the BoxSizing enum (symbols: BoxSizing, ContentBox,
BorderBox, #[derive(Default)]/#[default]) and move the #[default] attribute to
the ContentBox variant (or implement Default for BoxSizing to return
BoxSizing::ContentBox) so the Rust default matches the CSS initial value.
In `@crates/grida-canvas/src/node/schema.rs`:
- Line 1171: Add the missing FlatBuffers table and union entry for the new Rust
variant: define a HTMLEmbed table in format/grida.fbs matching the Rust
HTMLEmbedNodeRec/HTMLEmbedNode fields, then append the HTMLEmbed member to the
union Node so the HTMLEmbed variant can be serialized/deserialized; after
updating grida.fbs run flatc --schema --binary -o /tmp/grida-fbs-check
format/grida.fbs to validate compilation and then add round-trip tests
exercising the HTMLEmbed case in crates/grida-canvas/tests/fbs_roundtrip.rs and
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts to ensure
encode/decode fidelity.
In `@crates/grida-canvas/src/painter/painter.rs`:
- Around line 2130-2136: The Markdown/HTMLEmbed branch drops base opacity when
effects is non-empty because draw_shape_with_effects is called directly; wrap
the draw_shape_with_effects call in the same opacity wrapper used elsewhere so
opacity is applied before effects. Concretely, replace the direct call to
self.draw_shape_with_effects(effects, shape, draw_content) with a call to
self.with_opacity(opacity, Some(&shape.rect), ||
self.draw_shape_with_effects(effects, shape, draw_content)) (or equivalent
closure/adapter) so that with_opacity is invoked for the branch where effects
are present (mirroring how with_opacity is used for shape/text/vector paths).
In `@crates/grida-dev/src/main.rs`:
- Around line 377-395: ask_html_import_mode() currently returns a bool so
callers (e.g., load_master_scenes_from_path) cannot tell a user dismissed the
dialog from an explicit "Convert" selection, causing a dismissed dialog to be
treated as Convert; change ask_html_import_mode() to return an
Option<HtmlImportMode> (or a small enum HtmlImportMode { Embed, Convert }) where
Some(Embed)/Some(Convert) correspond to explicit choices and None represents
dismissal, update the match on MessageDialogResult in ask_html_import_mode to
return the appropriate Some variant or None for cancel/close, and update
load_master_scenes_from_path (and the other occurrence around the second
reported block) to abort the load when None is returned instead of treating it
as Convert.
- Around line 347-357: The measurement uses a new FontRepository (temp_fonts)
with repo.enable_system_fallback(), causing mismatch with the renderer's font
setup; change measure_content_height(&html_source, width, &temp_fonts) to use
the runtime's actual font configuration instead of constructing
temp_fonts—either pass the existing runtime FontRepository/byte store into
cg::htmlcss::measure_content_height or build the temp repo from the runtime
flags (respecting use_system_fonts and embedded-font settings) so the
measurement and renderer share the same font fallback/embedding behavior (refer
to FontRepository, enable_system_fallback, use_system_fonts,
measure_content_height, and html_source to locate the code).
---
Outside diff comments:
In `@crates/grida-canvas/src/resources/mod.rs`:
- Around line 147-153: The extract_image_urls function currently iterates
scene.graph.iter() which traverses parent→children link entries and therefore
skips leaf nodes (like HTMLEmbed) that have no children; update
extract_image_urls to iterate all graph nodes instead (e.g., use the graph's
node collection/keys or nodes() iterator) so every Node is visited, then for
each Node (match on Node::HTMLEmbed and other variants) extract fill image URLs
via the same logic; apply the same change for the other occurrence noted around
lines 225-227 so HTMLEmbed leaf nodes are included.
---
Nitpick comments:
In `@crates/grida-canvas/examples/golden_htmlcss.rs`:
- Around line 25-27: The render_to_png function currently calls fonts() each
time which constructs a new FontRepository per call; refactor render_to_png to
accept a shared FontRepository reference (e.g., &FontRepository) instead of
calling fonts() internally, update its signature and body to use that passed
repository, and then modify main to create a single FontRepository once and pass
it into render_to_png for each file processed to avoid repeatedly rebuilding the
font repository.
In `@crates/grida-canvas/src/htmlcss/collect.rs`:
- Around line 55-111: Add a short maintenance note above the
generate_marker_text function explaining that the current approach relies on
format!("{:?}", lst) and string matching of Stylo's Debug output (matching
variants like "Disc", "Circle", "Decimal", etc.) and may break if Stylo changes
its Debug representation or enum variant names; mention that callers use
to_roman and ordinal-based logic and that a future improvement would be to
switch to a typed API or pattern-match the generated enum when available.
In `@crates/grida-canvas/src/htmlcss/layout.rs`:
- Around line 309-315: The align_self mapping duplicates map_align_items logic;
modify the code that sets align_self to call the existing map_align_items helper
(or create a small map_align_self that delegates to map_align_items) instead of
matching again. Update the block that currently maps el.align_self to use
map_align_items(el.align_self) (or call map_align_self(el.align_self)), ensuring
the helper accepts the same types::AlignItems input and returns taffy::AlignSelf
so the conversion is centralized and duplication removed.
- Around line 69-78: The call to taffy.compute_layout_with_measure currently
discards its Result (let _ = ...), so update compute_layout to handle failures:
capture the Result from compute_layout_with_measure (for the taffy_root call
that passes text_measure_func and &fc), and either propagate the error by
returning a Result from the surrounding compute_layout function or log the error
(using the crate's logger) with context including taffy_root and the error
details; ensure you reference the same symbols
(taffy.compute_layout_with_measure, taffy_root, text_measure_func, fc) so the
error path is handled instead of ignored.
- Around line 480-493: The code treats types::LineHeight::Normal as 1.5 which is
larger than typical browser defaults; update the handling in the match arm for
types::LineHeight::Normal so ts.set_height_override(true) is followed by setting
ts.set_height to a browser-typical value (e.g. ~1.15–1.2) or compute a normal
line-height from font metrics if available instead of hardcoding 1.5; adjust the
logic around font.line_height, ts.set_height_override, ts.set_height and
consider using font.size if you need to convert px->unitless.
- Around line 459-467: The current font-family mapping closure (the map matching
s.as_str() that returns ".AppleSystemUIFont", "Helvetica", "Times", "Menlo", or
other.to_string()) uses macOS-specific concrete fonts which can break rendering
on Windows/Linux; update the mapping in that closure to return cross-platform
safe values instead (e.g., keep "system-ui" or map to generic families like
"sans-serif", "serif", "monospace") or add platform detection (using
cfg!(target_os = "windows") / cfg!(target_os = "linux") branches) to choose
appropriate concrete fonts per OS; locate the closure that performs map(|s|
match s.as_str() { ... }) in layout.rs and replace macOS-only entries
(".AppleSystemUIFont", "Menlo") with either generic family names or OS-specific
alternatives via cfg checks.
In `@crates/grida-canvas/src/htmlcss/mod.rs`:
- Around line 21-47: The render function's _height parameter is intentionally
unused because content height is computed from layout::compute_layout and used
via layout_root.height; update the render signature or docs to reflect this:
either remove the unused _height parameter from pub fn render(...) if no
external height constraint will be supported, or add a short doc comment above
pub fn render explaining that height is ignored and layout uses Taffy's
MaxContent to determine content height (see layout::compute_layout and
layout_root.height) so future readers know why _height is unused.
In `@crates/grida-canvas/src/htmlcss/paint.rs`:
- Around line 248-253: The radial_gradient shader calls use an unnecessary cast
"0.5 as scalar" (and similar occurrences around the same function), so remove
the redundant "as scalar" casts in the skia_safe::shaders::radial_gradient
invocations (and any other places in this file where 0.5 is cast to scalar) and
just pass 0.5 (or 0.5f32) directly to match the scalar type expected by
skia_safe::Point and the radial_gradient function.
In `@crates/grida-canvas/src/painter/painter_debug_node.rs`:
- Around line 696-698: Implement the debug rendering for Node::HTMLEmbed by
following the same placeholder pattern used for AttributedText and Markdown:
draw the node bounds and a distinctive debug style (like the existing
draw_error_node behavior) so HTMLEmbed is visible in debug mode; locate the
match arm for Node::HTMLEmbed in painter_debug_node.rs and call the same
helper(s) used for error/debug visualization (e.g., draw_error_node or the
bounding-shape drawing routine) with the HTMLEmbed node's layout/rect and a
clear debug color/style.
In `@docs/wg/research/chromium/blink-rendering-pipeline.md`:
- Around line 1-4: The frontmatter for the Markdown document titled "Blink
Rendering Pipeline — Style, Layout, Paint" is missing the format specifier;
update the YAML frontmatter at the top of the file to include format: md so the
block contains title, tags and format: md to ensure MDX parsers treat this file
as plain Markdown (no JSX) and avoid parsing errors from angle-bracket code
samples.
- Around line 19-29: The fenced ASCII diagram block lacks a language specifier;
update the triple-backtick fence surrounding the diagram (the block that begins
with "DOM + CSSOM" and the arrows/steps [1. Style]...[4. Composite]) to include
a language token such as text or plaintext (e.g., change ``` to ```text) so the
linter is satisfied.
🪄 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: 05705b5e-2b92-4a03-9aaa-744e4db3affb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
crates/csscascade/src/cascade.rscrates/grida-canvas/examples/golden_htmlcss.rscrates/grida-canvas/examples/tool_io_grida.rscrates/grida-canvas/examples/tool_io_svg.rscrates/grida-canvas/src/cache/compositor/promotion.rscrates/grida-canvas/src/htmlcss/collect.rscrates/grida-canvas/src/htmlcss/layout.rscrates/grida-canvas/src/htmlcss/mod.rscrates/grida-canvas/src/htmlcss/paint.rscrates/grida-canvas/src/htmlcss/style.rscrates/grida-canvas/src/htmlcss/types.rscrates/grida-canvas/src/layout/engine.rscrates/grida-canvas/src/layout/into_taffy.rscrates/grida-canvas/src/lib.rscrates/grida-canvas/src/node/factory.rscrates/grida-canvas/src/node/scene_graph.rscrates/grida-canvas/src/node/schema.rscrates/grida-canvas/src/painter/geometry.rscrates/grida-canvas/src/painter/layer.rscrates/grida-canvas/src/painter/painter.rscrates/grida-canvas/src/painter/painter_debug_node.rscrates/grida-canvas/src/resources/mod.rscrates/grida-canvas/src/runtime/cost_prediction.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-dev/Cargo.tomlcrates/grida-dev/src/bench/load_bench.rscrates/grida-dev/src/editor/mutation.rscrates/grida-dev/src/main.rsdocs/wg/feat-2d/htmlcss.mddocs/wg/research/chromium/blink-rendering-pipeline.mddocs/wg/research/chromium/index.mdfixtures/test-html/L0/text-inline-elements.htmlfixtures/test-html/L0/text-lists.html
| if decoration != textlayout::TextDecoration::NO_DECORATION { | ||
| ts.set_decoration_type(decoration); | ||
| ts.set_decoration_style(textlayout::TextDecorationStyle::Solid); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify TextDecorationStyle variants in cg::prelude match Skia's variants
ast-grep --pattern 'pub enum TextDecorationStyle {
$$$
}'Repository: gridaco/grida
Length of output: 795
🏁 Script executed:
#!/bin/bash
# Check the actual code at lines 514-516
sed -n '510,520p' crates/grida-canvas/src/htmlcss/layout.rsRepository: gridaco/grida
Length of output: 518
🏁 Script executed:
#!/bin/bash
# Search for TextDecorationStyle in textlayout or related modules
rg 'enum TextDecorationStyle' --type rustRepository: gridaco/grida
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Check if font.decoration_style exists in FontProps
rg 'decoration_style' crates/grida-canvas/src/htmlcss/layout.rs -B 2 -A 2Repository: gridaco/grida
Length of output: 347
🏁 Script executed:
#!/bin/bash
# Find textlayout module to check TextDecorationStyle variants
rg 'mod textlayout|textlayout::TextDecorationStyle' crates/grida-canvas/src/ -B 2 -A 5 --type rust | head -50Repository: gridaco/grida
Length of output: 2602
🏁 Script executed:
#!/bin/bash
# Search for FontProps struct and decoration_style field
rg 'struct FontProps|pub decoration_style' crates/grida-canvas/src/ --type rust -B 2 -A 10Repository: gridaco/grida
Length of output: 1987
🏁 Script executed:
#!/bin/bash
# Check what textlayout module is being used (could be external crate)
rg 'use.*textlayout' crates/grida-canvas/src/htmlcss/layout.rsRepository: gridaco/grida
Length of output: 177
Apply font decoration style mapping for CSS text-decoration-style support.
The font.decoration_style field is ignored; TextDecorationStyle::Solid is hardcoded at lines 515-516 in layout.rs (and identically at painter/markdown.rs). This prevents CSS text-decoration-style variations (dashed, dotted, wavy, double) from rendering. A conversion mapping already exists in sk/mappings.rs for all variants.
Update both occurrences to use the font's decoration style:
Fix for layout.rs
if decoration != textlayout::TextDecoration::NO_DECORATION {
ts.set_decoration_type(decoration);
- ts.set_decoration_style(textlayout::TextDecorationStyle::Solid);
+ ts.set_decoration_style(font.decoration_style.into());📝 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.
| if decoration != textlayout::TextDecoration::NO_DECORATION { | |
| ts.set_decoration_type(decoration); | |
| ts.set_decoration_style(textlayout::TextDecorationStyle::Solid); | |
| if decoration != textlayout::TextDecoration::NO_DECORATION { | |
| ts.set_decoration_type(decoration); | |
| ts.set_decoration_style(font.decoration_style.into()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/htmlcss/layout.rs` around lines 514 - 516, The code
hardcodes TextDecorationStyle::Solid when applying decorations in layout.rs
(ts.set_decoration_style) which ignores the font.decoration_style; replace the
hardcoded value by converting font.decoration_style through the existing mapping
in sk/mappings.rs (use the mapping function that returns a
textlayout::TextDecorationStyle) and pass that result to
ts.set_decoration_style; make the identical change in the other occurrence in
painter/markdown.rs so both places use the mapped font.decoration_style instead
of Solid.
| fn paint_borders(canvas: &Canvas, style: &StyledElement, w: f32, h: f32) { | ||
| let b = &style.border; | ||
|
|
||
| if b.top.width > 0.0 && b.top.style != types::BorderStyle::None { | ||
| let paint = border_paint(&b.top); | ||
| let by = b.top.width / 2.0; | ||
| canvas.draw_line((0.0, by), (w, by), &paint); | ||
| } | ||
|
|
||
| if b.bottom.width > 0.0 && b.bottom.style != types::BorderStyle::None { | ||
| let paint = border_paint(&b.bottom); | ||
| let by = h - b.bottom.width / 2.0; | ||
| canvas.draw_line((0.0, by), (w, by), &paint); | ||
| } | ||
|
|
||
| if b.left.width > 0.0 && b.left.style != types::BorderStyle::None { | ||
| let paint = border_paint(&b.left); | ||
| let bx = b.left.width / 2.0; | ||
| canvas.draw_line((bx, 0.0), (bx, h), &paint); | ||
| } | ||
|
|
||
| if b.right.width > 0.0 && b.right.style != types::BorderStyle::None { | ||
| let paint = border_paint(&b.right); | ||
| let bx = w - b.right.width / 2.0; | ||
| canvas.draw_line((bx, 0.0), (bx, h), &paint); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are test fixtures or docs that mention border + radius behavior
rg -n "border.*radius" fixtures/test-html/Repository: gridaco/grida
Length of output: 3764
🏁 Script executed:
#!/bin/bash
# Check paint-border-radius.html test to see if it includes borders
cat fixtures/test-html/L0/paint-border-radius.htmlRepository: gridaco/grida
Length of output: 1992
🏁 Script executed:
#!/bin/bash
# Search for border-radius handling in paint.rs
rg -n "border.radius|border_radius" crates/grida-canvas/src/htmlcss/paint.rs -A 2 -B 2Repository: gridaco/grida
Length of output: 1188
🏁 Script executed:
#!/bin/bash
# Check how borders are drawn in paint.rs and if border-radius is considered
rg -n "paint_borders|draw_line" crates/grida-canvas/src/htmlcss/paint.rs -A 5Repository: gridaco/grida
Length of output: 1489
🏁 Script executed:
#!/bin/bash
# Look for border-radius handling in the entire htmlcss module
rg -n "border.radius|border_radius" crates/grida-canvas/src/htmlcss/ --type rsRepository: gridaco/grida
Length of output: 83
Border painting doesn't account for border-radius.
The paint_borders function draws borders using simple straight lines via canvas.draw_line() without considering the element's border-radius property. When both are present, this creates visual artifacts where borders don't follow the rounded corners. The dedicated test fixture paint-border-radius.html only tests background rendering with border-radius; it doesn't test borders combined with border-radius, leaving this case untested.
To fix this, paint_borders should use canvas.draw_round_rect() or path-based drawing to render borders that respect the border-radius values when present.
🤖 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 273 - 299,
paint_borders currently draws straight lines with canvas.draw_line and ignores
StyledElement border-radius; update paint_borders to detect when any corner
radius > 0 (from the element's border radius data on StyledElement) and switch
to rounded-rect drawing: compute inner/outer rects adjusted by each side's width
and radii, build a rounded rectangle stroke (use canvas.draw_round_rect or
construct a path with rounded corners) and paint it with border_paint for each
side or stroke the rounded rect so borders follow the radii; keep the existing
straight-line path as the fallback when all corner radii are zero. Ensure you
reference paint_borders, border_paint, StyledElement.border and the Canvas
draw_round_rect/path APIs when making the change.
| BooleanOperation(BooleanPathOperationNodeRec), | ||
| Image(ImageNodeRec), | ||
| Markdown(MarkdownNodeRec), | ||
| HTMLEmbed(HTMLEmbedNodeRec), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the expected round-trip test files exist
fd -a 'fbs_roundtrip.rs'
fd -a 'format-roundtrip.test.ts'
# Check whether HTMLEmbed was appended/covered in round-trip tests
rg -n -C2 'HTMLEmbed|HTML.?Embed|Node::HTMLEmbed' \
crates/grida-canvas/tests/fbs_roundtrip.rs \
packages/grida-canvas-io/__tests__/format-roundtrip.test.tsRepository: gridaco/grida
Length of output: 197
🏁 Script executed:
#!/bin/bash
# Check if HTMLEmbed exists at line 1171 in schema.rs
echo "=== Checking schema.rs for HTMLEmbed around line 1171 ==="
sed -n '1165,1180p' crates/grida-canvas/src/node/schema.rs
echo ""
echo "=== Checking fbs_roundtrip.rs test structure ==="
head -100 crates/grida-canvas/tests/fbs_roundtrip.rs
echo ""
echo "=== Checking what other union members are in tests ==="
rg -n 'Node::' crates/grida-canvas/tests/fbs_roundtrip.rs | head -20Repository: gridaco/grida
Length of output: 5419
🏁 Script executed:
#!/bin/bash
# Check if HTMLEmbed is in the FlatBuffers schema
echo "=== Checking grida.fbs for HTMLEmbed ==="
rg -n 'HTMLEmbed' format/grida.fbs
# Check the union definition in grida.fbs
echo ""
echo "=== Looking for Node union definition ==="
rg -B5 -A20 'union Node' format/grida.fbs | head -60Repository: gridaco/grida
Length of output: 3094
🏁 Script executed:
#!/bin/bash
# Search entire repo for HTMLEmbed
echo "=== Searching entire repo for HTMLEmbed ==="
rg -n 'HTMLEmbed' --type rust --type typescript
echo ""
echo "=== Check if HTMLEmbedNodeRec is defined in schema.rs ==="
rg -n 'HTMLEmbedNodeRec' crates/grida-canvas/src/node/schema.rs
echo ""
echo "=== Check the Node enum in schema.rs to see full context ==="
rg -B2 -A2 'enum Node' crates/grida-canvas/src/node/schema.rs | head -80Repository: gridaco/grida
Length of output: 743
🏁 Script executed:
#!/bin/bash
# Check the end of the Node union in grida.fbs to see what was last added
echo "=== Last entries in Node union in grida.fbs ==="
rg -B3 'union Node' format/grida.fbs | tail -40
echo ""
echo "=== Check if grida.fbs is still valid (search for Markdown which should be there) ==="
rg -n 'MarkdownNode' format/grida.fbsRepository: gridaco/grida
Length of output: 718
🏁 Script executed:
#!/bin/bash
# Search for Markdown in grida.fbs (case-sensitive)
echo "=== Search for Markdown in grida.fbs ==="
rg -i 'markdown' format/grida.fbs
echo ""
echo "=== Show the full Node union definition in grida.fbs ==="
rg -A100 '^union Node {' format/grida.fbs | head -120Repository: gridaco/grida
Length of output: 262
🏁 Script executed:
#!/bin/bash
# Check if HTMLEmbed or related mentions exist anywhere in format directory
echo "=== Search entire format directory for HTMLEmbed ==="
rg 'HTMLEmbed' format/
echo ""
echo "=== Count of Node variants in schema.rs ==="
rg '^\s+[A-Z]\w+\(' crates/grida-canvas/src/node/schema.rs | grep -c '^\s'
echo ""
echo "=== List all Node variants in schema.rs ==="
rg '^\s+[A-Z]\w+\(' crates/grida-canvas/src/node/schema.rs | head -30Repository: gridaco/grida
Length of output: 1164
🏁 Script executed:
#!/bin/bash
# Check if grida.fbs exists and its size
ls -lah format/grida.fbs
echo ""
echo "=== First 50 lines of grida.fbs ==="
head -50 format/grida.fbs
echo ""
echo "=== Search for 'union Node' in grida.fbs ==="
grep -n 'union Node' format/grida.fbsRepository: gridaco/grida
Length of output: 3200
🏁 Script executed:
#!/bin/bash
# Check what's after the union Node line
echo "=== Content around union Node definition ==="
sed -n '/^union Node/,/^}/p' format/grida.fbsRepository: gridaco/grida
Length of output: 378
🏁 Script executed:
#!/bin/bash
# Check when HTMLEmbed was added to schema.rs
echo "=== Check git history for HTMLEmbed in schema.rs ==="
git log --oneline -p crates/grida-canvas/src/node/schema.rs | grep -B5 -A5 'HTMLEmbed' | head -50
echo ""
echo "=== Check if there are any pending changes or TODOs about HTMLEmbed ==="
rg -i 'todo|fixme|xxx' crates/grida-canvas/src/node/schema.rs | grep -i 'html'
echo ""
echo "=== Check if grida.fbs has any references to HTML ==="
rg -i 'html' format/grida.fbsRepository: gridaco/grida
Length of output: 1585
🏁 Script executed:
#!/bin/bash
# Check the README for how to compile/validate grida.fbs
echo "=== Check format/README.md for compilation instructions ==="
cat format/README.md 2>/dev/null || echo "No README.md found"
echo ""
echo "=== Look for flatc validation/compilation in CI or build scripts ==="
fd -a '*.yml' -o '*.yaml' -o 'Makefile' -o '*.sh' | xargs rg -l 'flatc' 2>/dev/null || echo "No flatc references found"Repository: gridaco/grida
Length of output: 3811
Add HTMLEmbedNode table to grida.fbs and append to union Node to complete the schema change.
HTMLEmbed was added to the Rust runtime model in schema.rs but is missing from the FlatBuffers schema in format/grida.fbs. This breaks serialization; the variant cannot be encoded or decoded without a corresponding FlatBuffers table definition. After adding the table and union member, validate compilation with flatc --schema --binary -o /tmp/grida-fbs-check format/grida.fbs (see format/README.md), then add round-trip test coverage in both crates/grida-canvas/tests/fbs_roundtrip.rs and packages/grida-canvas-io/__tests__/format-roundtrip.test.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/node/schema.rs` at line 1171, Add the missing
FlatBuffers table and union entry for the new Rust variant: define a HTMLEmbed
table in format/grida.fbs matching the Rust HTMLEmbedNodeRec/HTMLEmbedNode
fields, then append the HTMLEmbed member to the union Node so the HTMLEmbed
variant can be serialized/deserialized; after updating grida.fbs run flatc
--schema --binary -o /tmp/grida-fbs-check format/grida.fbs to validate
compilation and then add round-trip tests exercising the HTMLEmbed case in
crates/grida-canvas/tests/fbs_roundtrip.rs and
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts to ensure
encode/decode fidelity.
| if opacity >= 1.0 && effects.is_empty() { | ||
| draw_content(); | ||
| } else if effects.is_empty() { | ||
| self.with_opacity(opacity, Some(&shape.rect), draw_content); | ||
| } else { | ||
| self.draw_shape_with_effects(effects, shape, draw_content); | ||
| } |
There was a problem hiding this comment.
Opacity is dropped for Markdown/HTMLEmbed layers whenever effects is non-empty.
In these branches the else arm jumps straight to draw_shape_with_effects(), but unlike the shape/text/vector paths there is no with_opacity() / combined-opacity wrapper around that call. Any shadow/blur/glass therefore makes base.opacity a no-op for these layer types.
Also applies to: 2208-2214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/painter/painter.rs` around lines 2130 - 2136, The
Markdown/HTMLEmbed branch drops base opacity when effects is non-empty because
draw_shape_with_effects is called directly; wrap the draw_shape_with_effects
call in the same opacity wrapper used elsewhere so opacity is applied before
effects. Concretely, replace the direct call to
self.draw_shape_with_effects(effects, shape, draw_content) with a call to
self.with_opacity(opacity, Some(&shape.rect), ||
self.draw_shape_with_effects(effects, shape, draw_content)) (or equivalent
closure/adapter) so that with_opacity is invoked for the branch where effects
are present (mirroring how with_opacity is used for shape/text/vector paths).
| let width = 800.0f32; | ||
| let temp_fonts = { | ||
| use cg::resources::ByteStore; | ||
| use cg::runtime::font_repository::FontRepository; | ||
| let mut repo = | ||
| FontRepository::new(std::sync::Arc::new(std::sync::Mutex::new(ByteStore::new()))); | ||
| repo.enable_system_fallback(); | ||
| repo | ||
| }; | ||
| let height = | ||
| cg::htmlcss::measure_content_height(&html_source, width, &temp_fonts).unwrap_or(600.0); |
There was a problem hiding this comment.
measure_content_height() is using a different font configuration than the renderer.
This helper builds a fresh FontRepository, unconditionally enables system fallback, and never inherits the runtime's use_system_fonts / embedded-font setup. On the default --system-fonts=false path the measured height can diverge from the actual render enough to clip or over-size the embed.
🤖 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 - 357, The measurement uses a
new FontRepository (temp_fonts) with repo.enable_system_fallback(), causing
mismatch with the renderer's font setup; change
measure_content_height(&html_source, width, &temp_fonts) to use the runtime's
actual font configuration instead of constructing temp_fonts—either pass the
existing runtime FontRepository/byte store into
cg::htmlcss::measure_content_height or build the temp repo from the runtime
flags (respecting use_system_fonts and embedded-font settings) so the
measurement and renderer share the same font fallback/embedding behavior (refer
to FontRepository, enable_system_fallback, use_system_fonts,
measure_content_height, and html_source to locate the code).
| /// Show a dialog asking the user to choose between Embed and Convert for HTML files. | ||
| /// Returns true for Embed, false for Convert. | ||
| fn ask_html_import_mode() -> bool { | ||
| use rfd::MessageButtons; | ||
| use rfd::MessageDialog; | ||
| use rfd::MessageDialogResult; | ||
|
|
||
| let result = MessageDialog::new() | ||
| .set_title("HTML Import Mode") | ||
| .set_description("How would you like to import this HTML file?\n\n\u{2022} Embed \u{2014} Render as opaque picture (CSS-accurate, non-editable)\n\u{2022} Convert \u{2014} Convert to editable Grida IR nodes (lossy CSS)") | ||
| .set_buttons(MessageButtons::OkCancelCustom("Embed".to_string(), "Convert".to_string())) | ||
| .show(); | ||
|
|
||
| match result { | ||
| MessageDialogResult::Ok => true, | ||
| MessageDialogResult::Custom(s) if s == "Embed" => true, | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
Dialog dismissal currently falls through to Convert.
ask_html_import_mode() only returns a bool, so the caller can't distinguish “user chose Convert” from “user dismissed the dialog”. load_master_scenes_from_path() treats every false as Convert, so closing/escaping the chooser still replaces the current scene with an imported document. Return an enum/Option here and abort the load when the dialog is dismissed.
Also applies to: 496-502
🤖 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 377 - 395, ask_html_import_mode()
currently returns a bool so callers (e.g., load_master_scenes_from_path) cannot
tell a user dismissed the dialog from an explicit "Convert" selection, causing a
dismissed dialog to be treated as Convert; change ask_html_import_mode() to
return an Option<HtmlImportMode> (or a small enum HtmlImportMode { Embed,
Convert }) where Some(Embed)/Some(Convert) correspond to explicit choices and
None represents dismissal, update the match on MessageDialogResult in
ask_html_import_mode to return the appropriate Some variant or None for
cancel/close, and update load_master_scenes_from_path (and the other occurrence
around the second reported block) to abort the load when None is returned
instead of treating it as Convert.
…content - Fix percentage width/height values being dropped to auto in extract_size and extract_max_size (use to_percentage() fallback) - Fix inline group flattening overwriting nested child run styling — preserve original font/color for nested inline elements like <strong><em>text</em></strong> - Fix box-sizing default to content-box per CSS spec (was border-box) - Fix MinContent measure returning 0 — use Skia's min_intrinsic_width() for correct word-break minimum width; MaxContent uses max_intrinsic_width() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/grida-canvas/src/htmlcss/layout.rs (2)
128-157: Consider using reference-counted types to avoid cloning text content.Lines 137 and 152 clone
TextRunandInlineRunItemvectors into theTextMeasurecontext. For documents with large text content, this could impact memory and performance. UsingRc<TextRun>andRc<Vec<InlineRunItem>>would allow sharing without ownership transfer.This is acceptable for an initial implementation but worth revisiting if performance profiling reveals allocation pressure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/layout.rs` around lines 128 - 157, The code currently clones large text data when creating TextMeasure in the StyledNode::Text and StyledNode::InlineGroup branches; modify TextMeasure to hold reference-counted data (e.g., Rc<TextRun> and Rc<Vec<InlineRunItem>> or Rc<[InlineRunItem]>) and then pass Rc::clone(...) into new_leaf_with_context instead of cloning the run/items so StyledNode::Text uses Rc::new(run) / Rc::clone(&run_rc) and StyledNode::InlineGroup uses Rc::new(group.items) / Rc::clone(&items_rc); update the TextMeasure type and any callers to use Rc-wrapped types to enable shared ownership without expensive cloning.
486-499:LineHeight::Normaluses 1.5 instead of CSS default ~1.2.Line 489 sets height to 1.5 for
LineHeight::Normal. The CSS specification'snormalline-height is typically around 1.0–1.2 depending on the font. Using 1.5 provides more generous spacing but may cause layout discrepancies when rendering HTML that expects CSS-compliant behavior.If this is intentional for improved readability, consider documenting the deviation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/layout.rs` around lines 486 - 499, The code handling types::LineHeight::Normal currently forces ts.set_height(1.5) which deviates from the CSS "normal" (~1.0–1.2); update the Branch handling LineHeight::Normal in the match (the block that calls ts.set_height_override(true) and ts.set_height(...)) to use a CSS-consistent default (e.g., 1.2 or derive from font metrics) instead of 1.5, and if 1.5 was intentional keep the value but add a comment/docstring noting the deliberate deviation; reference the match on font.line_height and the ts.set_height / ts.set_height_override calls when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grida-canvas/src/htmlcss/layout.rs`:
- Around line 461-480: The current mapping of CSS generic families in the
closure that builds `families` (used to create `family_refs` and passed to
`ts.set_font_families`) uses single platform-specific names; replace each
mapping with a prioritized, cross-platform list of fallback family names (e.g.,
for system-ui use Apple, Windows and generic fallbacks; for
sans-serif/serif/monospace include common Windows/Linux/Mac fonts and generic
names) so that `families` contains multiple alternatives per input family, then
pass the flattened ordered list to `ts.set_font_families(&family_refs)` (mirror
the fallback pattern used in `markdown.rs` and `text_edit/skia_layout.rs` when
constructing the arrays before calling `set_font_families`).
---
Nitpick comments:
In `@crates/grida-canvas/src/htmlcss/layout.rs`:
- Around line 128-157: The code currently clones large text data when creating
TextMeasure in the StyledNode::Text and StyledNode::InlineGroup branches; modify
TextMeasure to hold reference-counted data (e.g., Rc<TextRun> and
Rc<Vec<InlineRunItem>> or Rc<[InlineRunItem]>) and then pass Rc::clone(...) into
new_leaf_with_context instead of cloning the run/items so StyledNode::Text uses
Rc::new(run) / Rc::clone(&run_rc) and StyledNode::InlineGroup uses
Rc::new(group.items) / Rc::clone(&items_rc); update the TextMeasure type and any
callers to use Rc-wrapped types to enable shared ownership without expensive
cloning.
- Around line 486-499: The code handling types::LineHeight::Normal currently
forces ts.set_height(1.5) which deviates from the CSS "normal" (~1.0–1.2);
update the Branch handling LineHeight::Normal in the match (the block that calls
ts.set_height_override(true) and ts.set_height(...)) to use a CSS-consistent
default (e.g., 1.2 or derive from font metrics) instead of 1.5, and if 1.5 was
intentional keep the value but add a comment/docstring noting the deliberate
deviation; reference the match on font.line_height and the ts.set_height /
ts.set_height_override calls when making 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: 2f0ea457-0ffa-4e20-a263-0c3f15be0ca8
📒 Files selected for processing (3)
crates/grida-canvas/src/htmlcss/collect.rscrates/grida-canvas/src/htmlcss/layout.rscrates/grida-canvas/src/htmlcss/types.rs
✅ Files skipped from review due to trivial changes (1)
- crates/grida-canvas/src/htmlcss/types.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/grida-canvas/src/htmlcss/collect.rs
| // Map CSS generic families to platform-concrete names | ||
| let families: Vec<String> = font | ||
| .families | ||
| .iter() | ||
| .map(|s| match s.as_str() { | ||
| "system-ui" | "-apple-system" | "BlinkMacSystemFont" => { | ||
| ".AppleSystemUIFont".to_string() | ||
| } | ||
| "sans-serif" => "Helvetica".to_string(), | ||
| "serif" => "Times".to_string(), | ||
| "monospace" => "Menlo".to_string(), | ||
| other => other.to_string(), | ||
| }) | ||
| .collect(); | ||
| let family_refs: Vec<&str> = families.iter().map(|s| s.as_str()).collect(); | ||
| if family_refs.is_empty() { | ||
| ts.set_font_families(&["Helvetica", "Arial"]); | ||
| } else { | ||
| ts.set_font_families(&family_refs); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Skia's FontCollection handles generic family names directly
rg -n "FontCollection|set_font_families|font.*families" crates/grida-canvas/src/ --type=rust -B2 -A2 | head -40Repository: gridaco/grida
Length of output: 3412
🏁 Script executed:
# Search for how set_font_families is used throughout the codebase
rg -n "set_font_families" crates/ --type=rust -B5 -A5Repository: gridaco/grida
Length of output: 34063
🏁 Script executed:
# Look for any platform-specific code around font handling
rg -n "cfg.*target|target_os|windows|macos|linux" crates/grida-canvas/src/htmlcss/ --type=rust -B2 -A2Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Check if there's any documentation or examples of Skia's FontCollection behavior
rg -n "FontCollection|skia_safe::textlayout" crates/ --type=rust -B2 -A2 | head -60Repository: gridaco/grida
Length of output: 5317
🏁 Script executed:
# Get more context around the problematic code in layout.rs
sed -n '450,490p' crates/grida-canvas/src/htmlcss/layout.rsRepository: gridaco/grida
Length of output: 1299
🏁 Script executed:
# Check what platform fonts are actually available in Skia's defaults
rg -n "Helvetica|Arial|Menlo|Consolas|DejaVu" crates/grida-canvas/ --type=rust | head -20Repository: gridaco/grida
Length of output: 2521
Font family fallbacks need cross-platform alternatives.
The generic font family mappings (lines 466-471) currently use platform-specific fonts that don't exist on non-Apple platforms:
.AppleSystemUIFontis macOS-onlyHelveticaandTimesare unreliable on Windows/LinuxMenlois macOS-only
Provide multiple platform-specific alternatives in priority order, as shown in markdown.rs (line 132) and text_edit/skia_layout.rs (line 232):
Corrected cross-platform fallback pattern
let families: Vec<String> = font
.families
.iter()
.map(|s| match s.as_str() {
- "system-ui" | "-apple-system" | "BlinkMacSystemFont" => {
- ".AppleSystemUIFont".to_string()
- }
- "sans-serif" => "Helvetica".to_string(),
- "serif" => "Times".to_string(),
- "monospace" => "Menlo".to_string(),
+ "system-ui" | "-apple-system" | "BlinkMacSystemFont" => "system-ui".to_string(),
+ "sans-serif" => "sans-serif".to_string(),
+ "serif" => "serif".to_string(),
+ "monospace" => "monospace".to_string(),
other => other.to_string(),
})
.collect();Then set fallback families in set_font_families() with platform alternatives:
let family_refs: Vec<&str> = families.iter().map(|s| s.as_str()).collect();
if family_refs.is_empty() {
- ts.set_font_families(&["Helvetica", "Arial"]);
+ ts.set_font_families(&["Helvetica", "Arial", "Segoe UI", "sans-serif"]);
} else {
- ts.set_font_families(&family_refs);
+ // For monospace, add Windows/Linux alternatives
+ let mut all_families = family_refs.clone();
+ if family_refs.contains(&"monospace") {
+ all_families = vec!["Menlo", "Courier New", "Consolas", "monospace"];
+ }
+ ts.set_font_families(&all_families);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/htmlcss/layout.rs` around lines 461 - 480, The
current mapping of CSS generic families in the closure that builds `families`
(used to create `family_refs` and passed to `ts.set_font_families`) uses single
platform-specific names; replace each mapping with a prioritized, cross-platform
list of fallback family names (e.g., for system-ui use Apple, Windows and
generic fallbacks; for sans-serif/serif/monospace include common
Windows/Linux/Mac fonts and generic names) so that `families` contains multiple
alternatives per input family, then pass the flattened ordered list to
`ts.set_font_families(&family_refs)` (mirror the fallback pattern used in
`markdown.rs` and `text_edit/skia_layout.rs` when constructing the arrays before
calling `set_font_families`).
Stylo uses a process-global DOM slot that is not thread-safe. When tests run in parallel (CI), concurrent access to bootstrap_dom() causes "Why are we here?" panics in Stylo's traversal.rs. Add a static Mutex guard to all htmlcss tests that use Stylo to serialize their execution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/grida-canvas/src/htmlcss/mod.rs (1)
70-73: This mutex is local to this file, not crate-shared.If the goal is to serialize Stylo across
htmlcssandhtmltests, Line 73 does not achieve that: a privatehtmlcss::tests::TEST_LOCKis not importable by sibling test modules. Move the lock into a shared#[cfg(test)] pub(crate)helper/module if other Stylo-using tests need the same guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/mod.rs` around lines 70 - 73, The TEST_LOCK Mutex defined in this file is private to htmlcss and won't serialize tests across sibling modules; create a shared test-only guard by moving the mutex into a common #[cfg(test)] pub(crate) helper (e.g. a tests::sync or test_utils module) and expose it as pub(crate) static TEST_LOCK: Mutex<()> so both the htmlcss tests and the html module's tests can import and lock the same Mutex before running Stylo-dependent code; update references in htmlcss tests (and html tests) to use the new pub(crate) TEST_LOCK.
🤖 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-canvas/src/htmlcss/mod.rs`:
- Around line 21-24: The public height parameter on render is currently unused;
either make height drive the viewport or remove it. Fix by plumbing a single
viewport-height decision through the rendering pipeline: accept the height in
render(html, width, height), pass that height into the layout stage (so
layout_root uses it rather than computing from width only), ensure the paint
stage receives the same layout_root.height, and make the empty-document path and
measure_content_height() return consistent bounds when height is specified so
fixed-height embeds get a real fixed viewport. Update any helper functions that
compute or read layout_root.height (e.g., layout, paint, measure_content_height)
to accept or consult the passed-in height so all entrypoints agree.
- Line 27: The module must enforce serialization when calling Stylo: wrap all
calls to collect::collect_styled_tree() inside an internal static mutex so
callers don't need to manage TEST_LOCK. Add a private static (e.g., lazy_static
or once_cell) Mutex/MutexGuard in this module and acquire the lock at the start
of render() and measure_content_height() before calling
collect::collect_styled_tree(), then release after the call; keep the lock scope
minimal and mirror the synchronization pattern used by tests but internal to
this module so production callers (painter.rs, grida-dev) are safe.
---
Nitpick comments:
In `@crates/grida-canvas/src/htmlcss/mod.rs`:
- Around line 70-73: The TEST_LOCK Mutex defined in this file is private to
htmlcss and won't serialize tests across sibling modules; create a shared
test-only guard by moving the mutex into a common #[cfg(test)] pub(crate) helper
(e.g. a tests::sync or test_utils module) and expose it as pub(crate) static
TEST_LOCK: Mutex<()> so both the htmlcss tests and the html module's tests can
import and lock the same Mutex before running Stylo-dependent code; update
references in htmlcss tests (and html tests) to use the new pub(crate)
TEST_LOCK.
🪄 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: 7ceb8bd5-2b4a-4dbc-9668-b5d0dda467cc
📒 Files selected for processing (1)
crates/grida-canvas/src/htmlcss/mod.rs
| pub fn render( | ||
| html: &str, | ||
| width: f32, | ||
| _height: f32, |
There was a problem hiding this comment.
The public height parameter is currently a no-op.
Line 24 is ignored end-to-end: layout runs from width alone, painting uses layout_root.height, and the empty-document path records a 1.0-high picture while measure_content_height() returns 0.0. Fixed-height embeds therefore never get a fixed-height viewport, and the two entrypoints do not even agree on empty-content bounds.
Either plumb a single viewport-height decision through layout/paint, or remove height until that behavior exists.
Also applies to: 29-45, 52-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/htmlcss/mod.rs` around lines 21 - 24, The public
height parameter on render is currently unused; either make height drive the
viewport or remove it. Fix by plumbing a single viewport-height decision through
the rendering pipeline: accept the height in render(html, width, height), pass
that height into the layout stage (so layout_root uses it rather than computing
from width only), ensure the paint stage receives the same layout_root.height,
and make the empty-document path and measure_content_height() return consistent
bounds when height is specified so fixed-height embeds get a real fixed
viewport. Update any helper functions that compute or read layout_root.height
(e.g., layout, paint, measure_content_height) to accept or consult the passed-in
height so all entrypoints agree.
| _height: f32, | ||
| fonts: &FontRepository, | ||
| ) -> Result<skia_safe::Picture, String> { | ||
| let root = collect::collect_styled_tree(html)?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find crates/grida-canvas -name "mod.rs" | grep htmlcssRepository: gridaco/grida
Length of output: 95
🏁 Script executed:
cat -n crates/grida-canvas/src/htmlcss/mod.rs | head -70Repository: gridaco/grida
Length of output: 2557
🏁 Script executed:
cat -n crates/grida-canvas/src/htmlcss/mod.rs | sed -n '1,100p'Repository: gridaco/grida
Length of output: 3781
🏁 Script executed:
rg "collect_styled_tree" --type rs -B 3 -A 3 | head -50Repository: gridaco/grida
Length of output: 83
🏁 Script executed:
rg "render\(|measure_content_height\(" --type rs -B 2 -A 2 | grep -A 5 -B 5 "htmlcss"Repository: gridaco/grida
Length of output: 83
🏁 Script executed:
cat -n crates/grida-canvas/src/htmlcss/collect.rs | head -40Repository: gridaco/grida
Length of output: 1916
🏁 Script executed:
rg "htmlcss::render|htmlcss::measure_content_height" | head -30Repository: gridaco/grida
Length of output: 524
🏁 Script executed:
rg "use.*htmlcss" -A 5 -B 2 | head -60Repository: gridaco/grida
Length of output: 748
🏁 Script executed:
find crates/grida-canvas/src -name "*.rs" -exec grep -l "render\|measure_content_height" {} \;Repository: gridaco/grida
Length of output: 3457
🏁 Script executed:
cat -n crates/grida-canvas/src/painter/painter.rs | grep -A 10 -B 10 "htmlcss::render"Repository: gridaco/grida
Length of output: 1471
🏁 Script executed:
cat -n crates/grida-canvas/src/painter/layer.rs | grep -A 10 -B 10 "htmlcss"Repository: gridaco/grida
Length of output: 889
🏁 Script executed:
cat -n crates/grida-dev/src/main.rs | grep -A 10 -B 10 "measure_content_height"Repository: gridaco/grida
Length of output: 924
🏁 Script executed:
cat -n crates/grida-canvas/src/painter/painter.rs | sed -n '2150,2180p'Repository: gridaco/grida
Length of output: 2022
🏁 Script executed:
rg "STYLO_LOCK|Mutex.*htmlcss" -B 3 -A 3Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
cat -n crates/grida-canvas/src/runtime/frame_loop.rs | head -50Repository: gridaco/grida
Length of output: 2500
🏁 Script executed:
rg "thread_state::initialize" -B 3 -A 3Repository: gridaco/grida
Length of output: 2854
🏁 Script executed:
cat -n crates/grida-canvas/src/htmlcss/collect.rs | sed -n '25,40p'Repository: gridaco/grida
Length of output: 855
🏁 Script executed:
rg "pub fn render|pub fn measure_content_height" -A 1 crates/grida-canvas/src/htmlcss/mod.rsRepository: gridaco/grida
Length of output: 137
Enforce Stylo serialization at the API boundary, not the caller's responsibility.
The public functions render() (line 27) and measure_content_height() (line 57) call collect::collect_styled_tree(), which accesses Stylo's process-global DOM slot. The module documentation states "Calls must be serialized externally," but this contract is not enforced. Production calls in painter.rs and grida-dev invoke these functions without synchronization, risking race conditions or panics on concurrent access. Tests protect themselves with TEST_LOCK, but production code does not.
Move the serialization responsibility into the module by wrapping Stylo access with an internal static mutex, similar to the approach suggested in the diff.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/htmlcss/mod.rs` at line 27, The module must enforce
serialization when calling Stylo: wrap all calls to
collect::collect_styled_tree() inside an internal static mutex so callers don't
need to manage TEST_LOCK. Add a private static (e.g., lazy_static or once_cell)
Mutex/MutexGuard in this module and acquire the lock at the start of render()
and measure_content_height() before calling collect::collect_styled_tree(), then
release after the call; keep the lock scope minimal and mirror the
synchronization pattern used by tests but internal to this module so production
callers (painter.rs, grida-dev) are safe.
1 participant
Summary
htmlcssrendering engine (src/htmlcss/) that converts HTML+CSS to Skia Pictures for opaque canvas embeddingHTMLEmbedNode— new node type with Embed/Convert dialog in grida-devWhat's supported
Layout: block (margin collapsing via Taffy), flex, grid, subpixel precision
Box model: width/height/min/max, padding, margin, border (solid/dashed/dotted), per-corner elliptical border-radius
Background: solid color, linear/radial/conic gradients, multi-layer
Text: font properties, text-decoration (simultaneous underline + line-through), text-align, text-transform, white-space, letter/word-spacing
Inline elements:
<strong>,<em>,<code>,<kbd>,<mark>,<s>,<del>,<u>,<ins>,<small>— merged into single Paragraph with per-run styles and InlineBoxDecoration (background, border, border-radius, padding via Skia placeholders)Lists:
<ul>/<ol>with disc/circle/square/decimal/lower-alpha/upper-alpha markersEffects: opacity, visibility, overflow clipping, box-shadow (outer), mix-blend-mode
Test plan
cargo check -p cg -p grida-canvas-wasm -p grida-dev— 0 errors, 0 warningscargo test -p cg --lib -- htmlcss— 9 tests passcargo build -p cg --examples— all examples buildcargo run -p cg --example golden_htmlcssrenders fixture HTML files to tmp directorycargo run -p grida-dev --release -- --system-fonts→ drop HTML file → Embed dialog → verify rendering🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests