feat(canvas): add MarkdownNode with direct Skia rendering#620
Merged
softmarshmallow merged 6 commits intomainfrom Apr 2, 2026
Merged
feat(canvas): add MarkdownNode with direct Skia rendering#620softmarshmallow merged 6 commits intomainfrom
softmarshmallow merged 6 commits intomainfrom
Conversation
Convert fixed positive CSS margins to wrapper containers with padding during HTML→IR import. For containers without visual properties (no fills/strokes), margins are merged directly into the container's own padding to avoid unnecessary wrapper nodes. Key behaviors: - Fixed margins: wrapper+padding (or merge into padding for non-visual containers) - Auto margins: not supported yet (requires SpacerNode) - Negative margins: silently dropped - Margin collapse: inherently unsupported (flex never collapses) Includes test for h1 margin merging and documents all known limitations and tree surgery patterns in docs/wg/format/css.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Inline HTML elements (<strong>, <em>, <code>, <span>, etc.) within a paragraph are now merged into a single AttributedTextNodeRec with per-run styling instead of producing separate stacked TextSpan nodes. Detection uses the HTML spec guarantee: phrasing content containers can only hold text and inline elements. When all child elements have display: inline, the parent emits an AttributedText node. Changes: - Extract css_text_style_to_cg shared helper for text style extraction - Add emit_attributed_text + collect_inline_text for DOM-order walk - Add collapse_whitespace for CSS white-space normalization - Add HtmlElement::from_node_id for child element traversal - Add AttributedStringBuilder::is_empty - Add article-style fixture: text-inline-elements.html Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughAdds Markdown node support and a Markdown-to-Skia renderer across the canvas pipeline, plus HTML inline-text merging and margin tree-surgery for inline containers; integrates schema, layout, painter, resources, runtime cost, editor resize, CLI loading, tests, docs, and fixtures. Changes
Sequence DiagramsequenceDiagram
participant Factory as NodeFactory
participant Layout as LayoutEngine
participant LayerBuilder as LayerList/PainterGeometry
participant Painter as PainterLayer
participant Renderer as MarkdownRenderer
participant Skia as SkiaCanvas
Factory->>Factory: create_markdown_node()
Layout->>Layout: get_schema_size()/get_schema_position()
LayerBuilder->>LayerBuilder: build_shape() / flatten_node()
LayerBuilder->>Renderer: render_markdown_picture(markdown, width, fonts)
Renderer->>Renderer: parse GFM, build paragraphs, tables, decorations
Renderer->>Skia: record draw commands into Picture
Renderer-->>Painter: return Picture
Painter->>Skia: draw_layer_standard() (transform, clip, fills, effects)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Introduced a new markdown fixture that showcases a variety of markdown features including headings, text styles (bold, italic, strikethrough), lists (ordered and unordered), code blocks (plain and syntax-highlighted), blockquotes, tables, inline and block math, images, and HTML elements. This fixture serves as a reference for testing and demonstrating markdown rendering capabilities.
Introduce a new `Node::Markdown` variant that stores GFM source and renders it directly to a Skia Picture via pulldown-cmark + Skia Paragraph API — no HTML/CSS pipeline involved. Supported: headings (h1–h6 with borders), paragraphs, bold/italic/ strikethrough, inline code, fenced code blocks, blockquotes, ordered/ unordered/task lists, tables with column alignment, horizontal rules, inline/display math (raw LaTeX fallback), and image placeholders. grida-dev: drop .md files onto the window or pass via CLI; resize via surface handles works out of the box.
softmarshmallow
changed the title
![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: 5aa598f23c
ℹ️ 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
+132
to
+136
| if has_text_children && all_children_inline && !is_structural { | ||
| // All children are text or inline elements → emit as a single | ||
| // Container (for box model) with an AttributedText child (for text). | ||
| let container_id = self.emit_container(style, &display, parent); | ||
| self.emit_attributed_text(element, style, Parent::NodeId(container_id)); |
There was a problem hiding this comment.
When this branch is taken, child elements are no longer traversed with build_element, so elements like <img>, <br>, <input>, or inline SVG inside mixed text paragraphs are dropped from the imported scene. emit_attributed_text only collects text runs, so these inline non-text nodes disappear entirely in cases such as <p>Hello <img ...> world</p>, which is a regression from the previous recursive behavior.
Useful? React with 👍 / 👎.
softmarshmallow
moved this from Todo
to In Progress
in Grida Canvas Core Graphics Engine
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (7)
crates/grida-canvas/src/node/factory.rs (1)
391-394: Consider centralizing the markdown default size constant.At Line 391,
400x300is hardcoded while other defaults are centralized. ADEFAULT_MARKDOWN_SIZEconstant would make future tuning safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/node/factory.rs` around lines 391 - 394, Introduce a central constant (e.g. DEFAULT_MARKDOWN_SIZE) and replace the hardcoded Size { width: 400.0, height: 300.0 } in factory.rs with that constant; define DEFAULT_MARKDOWN_SIZE alongside the other default constants in the same module (or constants file) so functions/constructors that use Size can reference it, and update any references in the factory functions that construct markdown sizes to use DEFAULT_MARKDOWN_SIZE instead of literal values.crates/grida-canvas/src/resources/mod.rs (1)
222-224: Markdown inline images use placeholders; no preloading is performed.At Line 222,
extract_image_urlsonly collects from markdown background fills. The markdown renderer intentionally uses placeholder rendering (becomes a styled rect with alt text) rather than loading actual images. Inline markdown image URLs are parsed but never extracted or preloaded—this is a deliberate design constraint of the current markdown implementation.If inline image loading is desired, it would require changes to the markdown renderer to extract image URLs from parsed content and integrate with the resource pipeline.
🤖 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 222 - 224, The Node::Markdown branch inside extract_image_urls currently only calls collect_image_urls_from_paints(&n.fills, &mut urls) and therefore doesn't extract inline image URLs from markdown content; either update that branch to parse the markdown node's content (use the markdown renderer/parser to walk AST and collect inline image src values and append them to urls) or explicitly document/annotate the behavior (e.g., add a TODO/clarifying comment) so callers know inline markdown images are not preloaded; refer to extract_image_urls, the Node::Markdown arm, and collect_image_urls_from_paints when making the change.fixtures/test-markdown/L0/kitchen-sink.md (1)
94-96: Split the remote-image case out of the stable kitchen-sink fixture.Using
https://via.placeholder.com/300x200makes this asset network-dependent, so renders will vary with connectivity and the third-party service. I'd keep the kitchen-sink fixture repo-local and move the remote-image example into a separate manual sample.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fixtures/test-markdown/L0/kitchen-sink.md` around lines 94 - 96, The fixture includes a network-dependent remote image URL (https://via.placeholder.com/300x200) inside fixtures/test-markdown/L0/kitchen-sink.md which makes the kitchen-sink sample non-repo-local; remove or replace that remote-image line and move it into a new separate sample file (e.g., a new manual-sample markdown) so kitchen-sink.md only references local assets (like ./image.png) and the remote example lives in its own repo entry for manual testing.crates/grida-canvas/src/painter/markdown.rs (1)
367-387: List item prefix uses outer list's ordered_list_index for nested lists.When rendering a list item, the prefix is determined by
ordered_list_indexwhich is set atStart(Tag::List). For nested lists, the inner list'sStart(Tag::List)will overwriteordered_list_index, but when the inner list ends, the outer list's index is not restored. This could cause numbered items in the outer list to continue from where the inner list left off, or use bullets incorrectly.Consider using a stack to track list context for proper nested list rendering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/painter/markdown.rs` around lines 367 - 387, The list prefix logic uses a single ordered_list_index that gets clobbered by nested lists; change to a stack (e.g., list_stack: Vec<Option<usize>>) and push the incoming first_item on Event::Start(Tag::List(first_item)) and pop on Event::End(Tag::List) while still updating list_depth; then in Event::Start(Tag::Item) read and increment the top of list_stack (not a single ordered_list_index) to produce the prefix, and fall back to the bullet string when the top is None. Ensure all references to ordered_list_index are replaced to use the stack’s top and that End(Tag::List) decrements list_depth and pops the stack.crates/grida-canvas/src/html/mod.rs (3)
112-136: Consider unifying text-only elements to use AttributedText.When
has_element_childrenis false,all_children_inlineshort-circuits to false, causing text-only elements (e.g.,<p>Hello</p>) to useemit_text_spanwhile mixed inline content usesemit_attributed_text. This creates two different node types for semantically similar content.This is not a bug and works correctly, but consider unifying to always emit
AttributedTextfor consistency whenhas_text_childrenis true:- let all_children_inline = has_element_children && { + let all_children_inline = !has_element_children || {This would make
all_children_inline = truewhen there are no element children, routing text-only elements toAttributedTextas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/html/mod.rs` around lines 112 - 136, The current logic sets all_children_inline to false when has_element_children is false, which causes text-only nodes (has_text_children true but no element children) to be handled via emit_text_span instead of emit_attributed_text; change the computation of all_children_inline (used by the has_text_children && all_children_inline && !is_structural branch) so that when has_element_children is false it evaluates to true (i.e., treat “no element children” as “all children inline”), ensuring text-only elements are routed to emit_attributed_text rather than emit_text_span; update the boolean expression around has_element_children / all_children_inline accordingly and keep the emit_attributed_text, emit_text_span, has_text_children, and is_structural checks intact.
475-482: Thestd::mem::takepattern works but signals an API friction.Using
std::mem::take(builder).push(...)is a correct workaround for consuming-selfbuilder methods through a&mutreference. However, this pattern is slightly fragile—ifpush()panics, the builder is left in a default (empty) state.Consider whether
AttributedStringBuilder::pushcould accept&mut selfinstead if this pattern appears frequently elsewhere. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/html/mod.rs` around lines 475 - 482, The code currently uses std::mem::take(builder).push(...) to call the consuming AttributedStringBuilder::push via a &mut reference (in the loop handling DemoNodeData::Text and collapse_whitespace), which can leave builder in a default state if push panics; change AttributedStringBuilder::push to take &mut self (i.e., fn push(&mut self, text: &str, style: &RunStyle, color: Color) -> &mut Self) and update this call to simply builder.push(&collapsed, &run_style, run_color); also update all other call sites of AttributedStringBuilder::push to the new &mut self signature to remove the std::mem::take workaround and avoid leaving builder replaced on panic.
713-713: Oblique font style is not mapped.The check only handles
font-style: italic, but CSS also supportsfont-style: oblique. While many rendering systems treat oblique similarly to italic or synthesize it, explicit oblique styling will be lost.This is acceptable for most cases since oblique is less common than italic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/html/mod.rs` at line 713, The current assignment sets text_style.font_style_italic only when font.font_style == style::values::computed::FontStyle::ITALIC, which ignores the OBLIQUE value; update the condition in the code that assigns text_style.font_style_italic (the line using text_style, font.font_style and style::values::computed::FontStyle::ITALIC) to also treat FontStyle::OBLIQUE as italic (e.g., check for == ITALIC || == OBLIQUE or match both variants) so oblique styles are preserved/synthesized same as italic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/csscascade/src/adapter.rs`:
- Around line 139-146: The public constructor HtmlElement::from_node_id exposes
an unchecked creation path that can wrap non-element NodeId values (causing
later panics in element_data); change the API to either (a) make from_node_id
non-public (e.g., pub(crate) or remove pub) and provide a checked public
constructor that validates the node kind and returns
Option<HtmlElement>/Result<HtmlElement, Error>, or (b) keep from_node_id but
mark it unsafe and document the precondition, and implement a new safe method
(e.g., try_from_node_id or HtmlElement::from_node_id_checked) that calls the
node-kind check used by element_data and returns None/Err on failure; update
call sites to use the safe checked API or the unsafe constructor accordingly and
reference HtmlElement::from_node_id and element_data when making these changes.
In `@crates/grida-canvas/src/html/mod.rs`:
- Around line 679-694: The collapse_whitespace function currently uses
ch.is_ascii_whitespace(), which ignores Unicode whitespace; update
collapse_whitespace so it uses ch.is_whitespace() (or an equivalent
Unicode-aware check) to ensure non-ASCII whitespace characters (e.g., NBSP, em
space) are collapsed as CSS white-space: normal dictates; locate the
collapse_whitespace function and replace the is_ascii_whitespace call with
is_whitespace, keeping the existing logic that emits a single ' ' for runs of
whitespace.
- Around line 2370-2386: The comment and assertion incorrectly expect 3 frames
but the code creates containers for both "html" and "body"; update the
expectation to account for ICB + html + body + h1 = 4 frames by changing the
assert to check icb_count + container_count == 4 and update the assertion
message accordingly (refer to container_count, icb_count, nodes, graph,
Node::Container and Node::InitialContainer to locate the logic and test).
In `@crates/grida-canvas/src/node/schema.rs`:
- Around line 2860-2881: The Rust model MarkdownNodeRec was added but the
FlatBuffers schema and round-trip tests are missing; add a Markdown node table
to the FBS schema mirroring MarkdownNodeRec fields (active, opacity, blend_mode,
effects, mask, transform, size, corner_radius, markdown, fills, layout_child),
add the corresponding node type to the nodes union/enum used by the format (so
the node discriminator includes Markdown), update any generated bindings, then
add round-trip tests: one in the Rust FBS roundtrip tests to
serialize/deserialize a MarkdownNodeRec instance and assert equality, and one in
the JS/TS format-roundtrip test to do the same; finally run flatc to compile the
schema to ensure no schema errors and regenerate bindings used by
crates/packages.
In `@crates/grida-canvas/src/painter/markdown.rs`:
- Around line 654-669: The End handler for Event::End(TagEnd::List(_)) only
decrements list_depth and clears ordered_list_index when depth hits zero, which
loses parent list type context for nested mixed lists; fix by tracking list
stack instead of a single ordered_list_index—on list start push the current list
type/ordered index (e.g., push Some(start_index) for ordered or None for
unordered) and on Event::End(TagEnd::List(_)) pop that stack, set
ordered_list_index to the popped parent value (or None) and decrement
list_depth; update code paths that reference list_depth and ordered_list_index
so nested ordered/unordered lists restore their parent numbering correctly (use
symbols list_depth, ordered_list_index, Event::End(TagEnd::List(_)), and the
list start handler).
- Around line 145-153: The current logic in the markdown painter sets text
decorations separately so set_decoration_type on the TextStyleBuilder (ts)
overwrites previous values when both state.strikethrough and state.link are
true; update the code that builds decorations (the ts.set_decoration_type calls)
to compute a combined decoration using bitwise OR of
textlayout::TextDecoration::LINE_THROUGH and
textlayout::TextDecoration::UNDERLINE when both state.strikethrough and
state.link are set (rather than calling set_decoration_type twice), so the
combined flags are passed once to ts.set_decoration_type.
In `@crates/grida-canvas/src/painter/painter.rs`:
- Around line 2090-2128: The Markdown branch in PainterPictureLayer::Markdown
never applies md_layer.base.blend_mode and when effects are present it also
skips md_layer.base.opacity because draw_shape_with_effects doesn't handle
opacity; fix by routing the draw_content closure through the same blend/opacity
isolation used by Text/Vector: always wrap draw_content with the
with_blendmode_and_opacity (or with_blendmode + with_opacity) helper using
md_layer.base.blend_mode and opacity and the shape rect, and for the effects
case call draw_shape_with_effects(effects, shape, || { draw_content() }) inside
that wrapper so both blend mode and opacity are respected for all branches.
In `@crates/grida-dev/src/main.rs`:
- Around line 338-366: scene_from_markdown_path() is only used for dropped-file
handling; you must wire the same markdown handling into the main loading path so
startup/bench and FileSceneLoader::load use it. Add an "md" / "markdown" branch
in load_scenes_from_source (the function backing FileSceneLoader::load and
run_interactive) that detects markdown input and delegates to
scene_from_markdown_path (or refactor the markdown logic into a shared helper
called by both scene_from_markdown_path and load_scenes_from_source), so
grida-dev README.md no longer falls through to grida_file::decode_all().
- Around line 347-353: The code sets a hard-coded clip height (node.size.height
= 2000.0) in NodeFactory::new -> create_markdown_node which causes imported
Markdown to be clipped; replace the fixed constant with a measured or dynamic
height: compute the rendered markdown height for the given width (e.g., via your
markdown/layout measurement utility or an offscreen layout pass) and assign that
value to node.size.height, or set node.size.height to a sentinel (0.0 or None)
and implement a loader/resize strategy that grows the node after content layout;
update create_markdown_node and any loading path to use the measured size so
large documents aren’t silently truncated.
In `@docs/wg/format/css.md`:
- Around line 281-283: Replace the external markdown link under the "###
Fixture" section to use an inline code path instead of a link: change the linked
path `../../../fixtures/test-html/L0/box-margin.html` to a plain inline code
reference (e.g., `fixtures/test-html/L0/box-margin.html`) in the
docs/wg/format/css.md content, or move the target file into the docs tree and
point to that docs-relative path; update the "Fixture" line accordingly.
- Around line 269-275: The fenced code block showing the CSS/IR examples is
missing a language tag; add a language identifier (use "text") to the opening
triple-backtick so the block becomes ```text and stops markdownlint
warnings—update the fenced example in the CSS/IR snippet (the block containing
"CSS: [A] [B margin-left:auto] [C]" and "IR: [A] [SpacerNode(1)] [B] [C]") to
include the text tag.
---
Nitpick comments:
In `@crates/grida-canvas/src/html/mod.rs`:
- Around line 112-136: The current logic sets all_children_inline to false when
has_element_children is false, which causes text-only nodes (has_text_children
true but no element children) to be handled via emit_text_span instead of
emit_attributed_text; change the computation of all_children_inline (used by the
has_text_children && all_children_inline && !is_structural branch) so that when
has_element_children is false it evaluates to true (i.e., treat “no element
children” as “all children inline”), ensuring text-only elements are routed to
emit_attributed_text rather than emit_text_span; update the boolean expression
around has_element_children / all_children_inline accordingly and keep the
emit_attributed_text, emit_text_span, has_text_children, and is_structural
checks intact.
- Around line 475-482: The code currently uses std::mem::take(builder).push(...)
to call the consuming AttributedStringBuilder::push via a &mut reference (in the
loop handling DemoNodeData::Text and collapse_whitespace), which can leave
builder in a default state if push panics; change AttributedStringBuilder::push
to take &mut self (i.e., fn push(&mut self, text: &str, style: &RunStyle, color:
Color) -> &mut Self) and update this call to simply builder.push(&collapsed,
&run_style, run_color); also update all other call sites of
AttributedStringBuilder::push to the new &mut self signature to remove the
std::mem::take workaround and avoid leaving builder replaced on panic.
- Line 713: The current assignment sets text_style.font_style_italic only when
font.font_style == style::values::computed::FontStyle::ITALIC, which ignores the
OBLIQUE value; update the condition in the code that assigns
text_style.font_style_italic (the line using text_style, font.font_style and
style::values::computed::FontStyle::ITALIC) to also treat FontStyle::OBLIQUE as
italic (e.g., check for == ITALIC || == OBLIQUE or match both variants) so
oblique styles are preserved/synthesized same as italic.
In `@crates/grida-canvas/src/node/factory.rs`:
- Around line 391-394: Introduce a central constant (e.g. DEFAULT_MARKDOWN_SIZE)
and replace the hardcoded Size { width: 400.0, height: 300.0 } in factory.rs
with that constant; define DEFAULT_MARKDOWN_SIZE alongside the other default
constants in the same module (or constants file) so functions/constructors that
use Size can reference it, and update any references in the factory functions
that construct markdown sizes to use DEFAULT_MARKDOWN_SIZE instead of literal
values.
In `@crates/grida-canvas/src/painter/markdown.rs`:
- Around line 367-387: The list prefix logic uses a single ordered_list_index
that gets clobbered by nested lists; change to a stack (e.g., list_stack:
Vec<Option<usize>>) and push the incoming first_item on
Event::Start(Tag::List(first_item)) and pop on Event::End(Tag::List) while still
updating list_depth; then in Event::Start(Tag::Item) read and increment the top
of list_stack (not a single ordered_list_index) to produce the prefix, and fall
back to the bullet string when the top is None. Ensure all references to
ordered_list_index are replaced to use the stack’s top and that End(Tag::List)
decrements list_depth and pops the stack.
In `@crates/grida-canvas/src/resources/mod.rs`:
- Around line 222-224: The Node::Markdown branch inside extract_image_urls
currently only calls collect_image_urls_from_paints(&n.fills, &mut urls) and
therefore doesn't extract inline image URLs from markdown content; either update
that branch to parse the markdown node's content (use the markdown
renderer/parser to walk AST and collect inline image src values and append them
to urls) or explicitly document/annotate the behavior (e.g., add a
TODO/clarifying comment) so callers know inline markdown images are not
preloaded; refer to extract_image_urls, the Node::Markdown arm, and
collect_image_urls_from_paints when making the change.
In `@fixtures/test-markdown/L0/kitchen-sink.md`:
- Around line 94-96: The fixture includes a network-dependent remote image URL
(https://via.placeholder.com/300x200) inside
fixtures/test-markdown/L0/kitchen-sink.md which makes the kitchen-sink sample
non-repo-local; remove or replace that remote-image line and move it into a new
separate sample file (e.g., a new manual-sample markdown) so kitchen-sink.md
only references local assets (like ./image.png) and the remote example lives in
its own repo entry for manual testing.
🪄 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: 39a1524c-3c60-482d-9833-2c34f2627db9
📒 Files selected for processing (27)
crates/csscascade/src/adapter.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/cg/types.rscrates/grida-canvas/src/html/mod.rscrates/grida-canvas/src/layout/engine.rscrates/grida-canvas/src/layout/into_taffy.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/markdown.rscrates/grida-canvas/src/painter/mod.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/src/bench/load_bench.rscrates/grida-dev/src/editor/mutation.rscrates/grida-dev/src/main.rsdocs/wg/format/css.mddocs/wg/format/markdown.mdfixtures/test-html/L0/text-inline-elements.htmlfixtures/test-markdown/L0/kitchen-sink.md
Comment on lines
+139
to
+146
| /// Wrap a DOM [`NodeId`] as an [`HtmlElement`]. | ||
| /// | ||
| /// # Safety (logical) | ||
| /// The caller must ensure the node is actually an element node. | ||
| /// Calling methods on a non-element HtmlElement will panic. | ||
| pub fn from_node_id(id: NodeId) -> Self { | ||
| Self(id) | ||
| } |
There was a problem hiding this comment.
Avoid exposing an unchecked public constructor for HtmlElement.
On Line 144, from_node_id lets callers construct HtmlElement from any NodeId, including non-element nodes; that violates the type invariant and defers failure to panics in element_data() (Line 168). Prefer a checked public API (or make unchecked construction non-public).
Suggested safer API shape
impl HtmlElement {
- pub fn from_node_id(id: NodeId) -> Self {
- Self(id)
- }
+ pub fn try_from_node_id(id: NodeId) -> Option<Self> {
+ matches!(dom().node(id).data, DemoNodeData::Element(_)).then_some(Self(id))
+ }
+
+ pub(crate) fn from_node_id_unchecked(id: NodeId) -> Self {
+ Self(id)
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/csscascade/src/adapter.rs` around lines 139 - 146, The public
constructor HtmlElement::from_node_id exposes an unchecked creation path that
can wrap non-element NodeId values (causing later panics in element_data);
change the API to either (a) make from_node_id non-public (e.g., pub(crate) or
remove pub) and provide a checked public constructor that validates the node
kind and returns Option<HtmlElement>/Result<HtmlElement, Error>, or (b) keep
from_node_id but mark it unsafe and document the precondition, and implement a
new safe method (e.g., try_from_node_id or HtmlElement::from_node_id_checked)
that calls the node-kind check used by element_data and returns None/Err on
failure; update call sites to use the safe checked API or the unsafe constructor
accordingly and reference HtmlElement::from_node_id and element_data when making
these changes.
Comment on lines
+2370
to
+2386
| // h1 has UA margin ~21px top/bottom. With body margin:0, the tree should be: | ||
| // ICB → html → h1(margin merged as padding) → TextSpan | ||
| // Total containers: 3 (ICB + html + h1), no extra wrapper. | ||
| let container_count = nodes | ||
| .iter() | ||
| .filter(|id| matches!(graph.get_node(id).ok(), Some(Node::Container(_)))) | ||
| .count(); | ||
| // ICB is InitialContainer, not Container | ||
| let icb_count = nodes | ||
| .iter() | ||
| .filter(|id| matches!(graph.get_node(id).ok(), Some(Node::InitialContainer(_)))) | ||
| .count(); | ||
| assert_eq!( | ||
| icb_count + container_count, | ||
| 3, | ||
| "ICB + html + h1 = 3 frames, no margin wrapper" | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the test passes and understand the actual tree structure
cd crates/grida-canvas
# Look for how body elements are handled
rg -n "body" src/html/mod.rs | head -20
# Check if there's special handling for structural elements
rg -n "is_structural" src/html/mod.rsRepository: gridaco/grida
Length of output: 804
🏁 Script executed:
# Find the test_h1_margin_no_double_wrap test to see the full HTML
rg -n "test_h1_margin_no_double_wrap" crates/grida-canvas/src/html/mod.rs -A 50 | head -80Repository: gridaco/grida
Length of output: 2219
🏁 Script executed:
# Also check how container creation logic uses is_structural
rg -n "is_structural" crates/grida-canvas/src/html/mod.rs -B 3 -A 3Repository: gridaco/grida
Length of output: 1120
🏁 Script executed:
# Check if there's special flattening or merging for body elements
rg -n "body" crates/grida-canvas/src/html/mod.rs -B 2 -A 2 | grep -A 5 -B 5 "flatten\|merge\|skip"Repository: gridaco/grida
Length of output: 528
🏁 Script executed:
# Look at the emit_container function to see if there's special handling
rg -n "fn emit_container" crates/grida-canvas/src/html/mod.rs -A 20Repository: gridaco/grida
Length of output: 973
🏁 Script executed:
# Also check what dfs_nodes does - does it filter any nodes?
rg -n "fn dfs_nodes" crates/grida-canvas/src/html/mod.rs -A 10Repository: gridaco/grida
Length of output: 530
🏁 Script executed:
# Check if there's any special handling that skips or merges body
rg -n "body" crates/grida-canvas/src/html/mod.rs -C 5 | grep -A 10 -B 10 "skip\|flatten\|parent\|merge"Repository: gridaco/grida
Length of output: 2554
🏁 Script executed:
# Look at the full emit function flow - does structural element handling differ from regular elements?
rg -n "fn emit" crates/grida-canvas/src/html/mod.rs | head -10Repository: gridaco/grida
Length of output: 286
🏁 Script executed:
# Check if there's any code that removes body nodes from the graph after construction
rg -n "root\|children" crates/grida-canvas/src/html/mod.rs | grep -i "body" -A 2 -B 2Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Most importantly - let's see what html_graph actually returns and if it processes the tree
rg -n "fn html_graph" crates/grida-canvas/src/html/mod.rs -A 15Repository: gridaco/grida
Length of output: 652
🏁 Script executed:
# Search for any logic that handles body specifically - maybe parent linking or flattening
rg -n "tag.as_str()" crates/grida-canvas/src/html/mod.rs -B 5 -A 10Repository: gridaco/grida
Length of output: 879
🏁 Script executed:
# Check the from_html_str function to see if it does any post-processing on the tree
rg -n "from_html_str\|pub fn from_html" crates/grida-canvas/src/html/mod.rs -A 30 | head -50Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Check if body gets special parent handling or is skipped during emission
rg -n "html.*body\|body.*skip\|body.*parent" crates/grida-canvas/src/html/mod.rs -C 3Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Look for the from_html_str implementation
rg -n "from_html_str" crates/grida-canvas/src/html/mod.rs -B 2 -A 30Repository: gridaco/grida
Length of output: 8799
🏁 Script executed:
# Search in the entire crate for from_html_str
fd . crates/grida-canvas --type f -name "*.rs" -exec rg -l "from_html_str" {} \;Repository: gridaco/grida
Length of output: 287
🏁 Script executed:
# Check if there's any node filtering or merging after graph construction
rg -n "SceneGraph\|NodeId" crates/grida-canvas/src/html/mod.rs | grep -i "filter\|remove\|skip" -B 2 -A 2Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Look at the build_element function implementation more carefully
rg -n "fn build_element" crates/grida-canvas/src/html/mod.rs -A 50 | head -80Repository: gridaco/grida
Length of output: 2239
🏁 Script executed:
# Check if there's any parent link modification or body-to-html flattening
rg -n "build_element\|build_text" crates/grida-canvas/src/html/mod.rs | head -20Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Let's look at what Parent::Root does vs Parent::NodeId
rg -n "Parent::" crates/grida-canvas/src/html/mod.rs | head -20Repository: gridaco/grida
Length of output: 443
🏁 Script executed:
# Get the full build_element function
rg -n "fn build_element" crates/grida-canvas/src/html/mod.rs -A 100 | head -150Repository: gridaco/grida
Length of output: 4879
🏁 Script executed:
# Also check if there's any body-specific logic elsewhere
rg -n "\"body\"\|'body'" crates/grida-canvas/src/html/mod.rs -B 2 -A 2Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Confirm the exact assertion in the test and what nodes it expects
rg -n "assert_eq!" crates/grida-canvas/src/html/mod.rs | grep -A 2 -B 2 "2382"Repository: gridaco/grida
Length of output: 285
🏁 Script executed:
# Let's check if there's any special logic that skips body node creation
# by looking at append_child implementation
rg -n "fn append_child" crates/grida-canvas/src/ -r -A 20 | head -50Repository: gridaco/grida
Length of output: 471
The comment inaccurately describes the expected tree structure — it omits <body> as a separate container.
The code marks both "html" and "body" as structural elements (line 110), and both trigger container creation via the condition on line 137–139. This means the actual scene graph should include: ICB + html container + body container + h1 container = 4 nodes total. However, the comment claims "ICB + html + h1 = 3 frames" and the assertion expects icb_count + container_count == 3, which implicitly expects only 2 container nodes.
Either the test assertion should be updated to expect 4 total nodes (1 ICB + 3 containers), or the comment should acknowledge that the actual tree includes <body> as a separate container. Clarify which is the intended behavior and update accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/html/mod.rs` around lines 2370 - 2386, The comment
and assertion incorrectly expect 3 frames but the code creates containers for
both "html" and "body"; update the expectation to account for ICB + html + body
+ h1 = 4 frames by changing the assert to check icb_count + container_count == 4
and update the assertion message accordingly (refer to container_count,
icb_count, nodes, graph, Node::Container and Node::InitialContainer to locate
the logic and test).
Comment on lines
+2860
to
+2881
| #[derive(Debug, Clone)] | ||
| pub struct MarkdownNodeRec { | ||
| pub active: bool, | ||
|
|
||
| pub opacity: f32, | ||
| pub blend_mode: LayerBlendMode, | ||
| pub effects: LayerEffects, | ||
| pub mask: Option<LayerMaskType>, | ||
|
|
||
| pub transform: AffineTransform, | ||
| pub size: Size, | ||
| pub corner_radius: RectangularCornerRadius, | ||
|
|
||
| /// The GitHub Flavored Markdown source text. | ||
| pub markdown: String, | ||
|
|
||
| /// Background fills for the markdown container. | ||
| pub fills: Paints, | ||
|
|
||
| /// Layout style for this node when it is a child of a layout container. | ||
| pub layout_child: Option<LayoutChildStyle>, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Markdown is defined in FlatBuffers schema
echo "=== Checking FlatBuffers schema for Markdown ==="
fd -t f 'grida.fbs' --exec grep -l 'Markdown' {} \; || echo "No Markdown found in .fbs files"
# Search for Markdown in any FBS files
echo -e "\n=== Searching for Markdown definition in FBS files ==="
rg -n 'Markdown' --type-add 'fbs:*.fbs' --type fbs || echo "No .fbs files with Markdown found"
# Check for round-trip tests mentioning Markdown
echo -e "\n=== Checking for round-trip tests ==="
rg -n 'Markdown' -g '*roundtrip*' || echo "No round-trip tests mentioning Markdown"Repository: gridaco/grida
Length of output: 306
Add Markdown node definition to FlatBuffers schema and create round-trip tests.
The FlatBuffers schema (format/grida.fbs) is missing the Markdown node definition. Per project learnings, the Rust runtime model in schema.rs must be accompanied by corresponding FBS schema updates. Additionally, round-trip tests must be added in crates/grida-canvas/tests/fbs_roundtrip.rs and packages/grida-canvas-io/__tests__/format-roundtrip.test.ts for the new Markdown node type. Validate that grida.fbs compiles with flatc once schema changes are complete.
🤖 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` around lines 2860 - 2881, The Rust
model MarkdownNodeRec was added but the FlatBuffers schema and round-trip tests
are missing; add a Markdown node table to the FBS schema mirroring
MarkdownNodeRec fields (active, opacity, blend_mode, effects, mask, transform,
size, corner_radius, markdown, fills, layout_child), add the corresponding node
type to the nodes union/enum used by the format (so the node discriminator
includes Markdown), update any generated bindings, then add round-trip tests:
one in the Rust FBS roundtrip tests to serialize/deserialize a MarkdownNodeRec
instance and assert equality, and one in the JS/TS format-roundtrip test to do
the same; finally run flatc to compile the schema to ensure no schema errors and
regenerate bindings used by crates/packages.
Comment on lines
+338
to
+366
| fn scene_from_markdown_path(path: &Path) -> Result<Scene> { | ||
| use cg::cg::prelude::CGColor; | ||
| use cg::node::factory::NodeFactory; | ||
| use cg::node::scene_graph::{Parent, SceneGraph}; | ||
| use cg::node::schema::Node; | ||
|
|
||
| let md_source = std::fs::read_to_string(path) | ||
| .with_context(|| format!("failed to read {}", path.display()))?; | ||
|
|
||
| let nf = NodeFactory::new(); | ||
| let mut node = nf.create_markdown_node(); | ||
| node.markdown = md_source; | ||
| node.size = cg::node::schema::Size { | ||
| width: 800.0, | ||
| height: 2000.0, | ||
| }; | ||
|
|
||
| let mut graph = SceneGraph::new(); | ||
| graph.append_child(Node::Markdown(node), Parent::Root); | ||
|
|
||
| Ok(Scene { | ||
| name: path | ||
| .file_name() | ||
| .map(|n| n.to_string_lossy().into_owned()) | ||
| .unwrap_or_else(|| "Markdown".to_string()), | ||
| graph, | ||
| background_color: Some(CGColor::from_u32(0xFFFFFFFF)), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Wire Markdown through the same loader path used by startup and bench modes.
scene_from_markdown_path() is only reachable from dropped-file dispatch right now. load_scenes_from_source()—which backs FileSceneLoader::load() and the initial run_interactive() startup load—still has no md / markdown branch, so grida-dev README.md will still fall through to grida_file::decode_all() and fail.
♻️ Suggested minimal fix
async fn load_scenes_from_source(source: &str) -> Result<Vec<Scene>> {
if !is_url(source) {
let path = Path::new(source);
if let Some(ext) = path.extension().and_then(|e| e.to_str()) {
match ext.to_ascii_lowercase().as_str() {
"svg" => return scene_from_svg_path(path).map(|s| vec![s]),
"html" | "htm" => return scene_from_html_path(path).map(|s| vec![s]),
+ "md" | "markdown" => return scene_from_markdown_path(path).map(|s| vec![s]),
// Raster images should be loaded via load_raster() by the caller
// so bytes can be registered with the renderer.
ext if is_raster_ext(ext) => return load_raster(path).map(|r| vec![r.scene]),
_ => {}
}
}
}Also applies to: 436-436
🤖 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 338 - 366,
scene_from_markdown_path() is only used for dropped-file handling; you must wire
the same markdown handling into the main loading path so startup/bench and
FileSceneLoader::load use it. Add an "md" / "markdown" branch in
load_scenes_from_source (the function backing FileSceneLoader::load and
run_interactive) that detects markdown input and delegates to
scene_from_markdown_path (or refactor the markdown logic into a shared helper
called by both scene_from_markdown_path and load_scenes_from_source), so
grida-dev README.md no longer falls through to grida_file::decode_all().
Comment on lines
+347
to
+353
| let nf = NodeFactory::new(); | ||
| let mut node = nf.create_markdown_node(); | ||
| node.markdown = md_source; | ||
| node.size = cg::node::schema::Size { | ||
| width: 800.0, | ||
| height: 2000.0, | ||
| }; |
There was a problem hiding this comment.
Avoid baking a 2000px clip height into imported Markdown scenes.
node.size.height becomes the clip bound later in the renderer, so any document taller than 2000px is silently cut off in the dev viewer. This needs measured content height, or at least a loader strategy that can grow the node instead of a fixed constant.
🤖 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 - 353, The code sets a
hard-coded clip height (node.size.height = 2000.0) in NodeFactory::new ->
create_markdown_node which causes imported Markdown to be clipped; replace the
fixed constant with a measured or dynamic height: compute the rendered markdown
height for the given width (e.g., via your markdown/layout measurement utility
or an offscreen layout pass) and assign that value to node.size.height, or set
node.size.height to a sentinel (0.0 or None) and implement a loader/resize
strategy that grows the node after content layout; update create_markdown_node
and any loading path to use the measured size so large documents aren’t silently
truncated.
Comment on lines
+281
to
+283
| ### Fixture | ||
|
|
||
| Visual test fixture for all margin behaviors: [`fixtures/test-html/L0/box-margin.html`](../../../fixtures/test-html/L0/box-margin.html) |
There was a problem hiding this comment.
Use inline code for the fixture path instead of a link outside /docs.
Markdown under docs/ shouldn't link to repo files outside the docs tree. Please keep this as a plain path reference or move the target under docs.
📝 Suggested fix
-Visual test fixture for all margin behaviors: [`fixtures/test-html/L0/box-margin.html`](../../../fixtures/test-html/L0/box-margin.html)
+Visual test fixture for all margin behaviors: `fixtures/test-html/L0/box-margin.html`As per coding guidelines, "Never link outside /docs from docs markdown files; instead reference external paths like crates/ or packages/ as inline code."
📝 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
| ### Fixture | |
| Visual test fixture for all margin behaviors: [`fixtures/test-html/L0/box-margin.html`](../../../fixtures/test-html/L0/box-margin.html) | |
| ### Fixture | |
| Visual test fixture for all margin behaviors: `fixtures/test-html/L0/box-margin.html` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/format/css.md` around lines 281 - 283, Replace the external markdown
link under the "### Fixture" section to use an inline code path instead of a
link: change the linked path `../../../fixtures/test-html/L0/box-margin.html` to
a plain inline code reference (e.g., `fixtures/test-html/L0/box-margin.html`) in
the docs/wg/format/css.md content, or move the target file into the docs tree
and point to that docs-relative path; update the "Fixture" line accordingly.
github-project-automation
Bot
moved this from In Progress
to Done
in Grida Canvas Core Graphics Engine
This was referenced Apr 4, 2026
Merged
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
Node::Markdownvariant that stores GFM source and renders directly to a Skia Picture via pulldown-cmark + Skia Paragraph APIAttributedTextnodes.mdfiles onto the window or pass via CLI; resize via surface handlesMarkdownNode
Renders GFM markdown as a single opaque element on the canvas. Walks pulldown-cmark events and draws directly to Skia — headings (h1–h6 with borders), paragraphs, bold/italic/strikethrough, inline code, fenced code blocks, blockquotes, ordered/unordered/task lists, tables with column alignment, horizontal rules, inline/display math (raw LaTeX fallback), and image placeholders.
Tracked in
docs/wg/format/markdown.md.HTML import
css_margin_to_cg()+ margin wrapper node (tree surgery) for correct margin-outside-border renderingemit_attributed_text()merges adjacent inline elements (<strong>,<em>,<a>, etc.) into a singleAttributedTextnode instead of separateTextSpannodesTest fixtures
fixtures/test-markdown/L0/kitchen-sink.md— comprehensive GFM test covering all supported elementsfixtures/test-html/L0/text-inline-elements.html— inline element merging testTest plan
cargo check -p cg -p grida-canvas-wasm -p grida-devcargo test -p cg— 668 tests passkitchen-sink.mdonto grida-dev window — renders correctly🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
.mdand.markdownfiles directly as scenes.Improvements