feat(htmlcss): image support — <img>, background-image, ImageProvider#654
Merged
softmarshmallow merged 7 commits intomainfrom Apr 9, 2026
Merged
feat(htmlcss): image support — <img>, background-image, ImageProvider#654softmarshmallow merged 7 commits intomainfrom
softmarshmallow merged 7 commits intomainfrom
Conversation
…ageProvider trait Introduce a host-agnostic image loading architecture for the htmlcss renderer, inspired by Chromium's StyleImage / ImageResourceContent pattern. The module never blocks on image loads — missing images render as placeholders. **Core abstractions (Chromium-inspired, self-contained in htmlcss):** - `ImageProvider` trait — host implements get(url) → Option<&Image> - `NoImages` — zero-cost null provider for image-free rendering - `PreloadedImages` — HashMap-backed provider for pre-resolved flow - `StyleImage` enum — polymorphic CSS image (Url | gradient variants) - `BackgroundLayer` refactored to Solid | Image(StyleImage) - `ObjectFit` enum delegating to math2::BoxFit::calculate_transform **<img> element support:** - Detected via detect_widget, goes through full Stylo cascade - CSS width/height takes priority over HTML attrs over intrinsic size - aspect_ratio set on Taffy style for proportional auto-sizing - Painted with object-fit via BoxFit transform + content-box clip **background-image: url() support:** - Extracts from both ComputedUrl::Valid and ComputedUrl::Invalid (Stylo servo-mode can't resolve relative URLs without a base) - Painted as image shader in background layer stack **grida-dev integration:** - collect_image_urls() scans HTML for referenced image URLs - load_html_images() resolves local paths + fetches remote in parallel - Images registered via Renderer::add_image_by_url() (arbitrary keys) - HTML files from CLI always use embed mode with image preloading **Test fixtures:** - replaced-img.html — <img> element variants and object-fit - paint-background-image.html — background-image: url() specimens - mixed-images.html — cards combining both image types Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdd image support to the htmlcss pipeline: introduce an ImageProvider trait with Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Editor/Client
participant Renderer as Renderer
participant HTMLCSS as htmlcss (collect → layout → paint)
participant Images as ImageProvider / ImageRepository
Client->>Renderer: request render(html, fonts, width, images)
Renderer->>HTMLCSS: collect styled tree
HTMLCSS->>Images: collect_image_urls() / get_size(url)
Images-->>HTMLCSS: size info or None
HTMLCSS->>HTMLCSS: compute_layout(..., images) (apply_replaced_intrinsic_size)
HTMLCSS->>Images: paint -> get(url) / resolve StyleImage
Images-->>HTMLCSS: skia_safe::Image or None
HTMLCSS->>Renderer: return Picture (with images or placeholders)
Renderer-->>Client: display Picture
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)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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: d07e73fab6
ℹ️ 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
+134
to
+137
| 0.0, | ||
| 0.0, | ||
| w, | ||
| h, |
There was a problem hiding this comment.
into content box instead of border box
paint_box passes the full element rect (0,0,w,h) into paint_replaced, and paint_replaced clips/draws the bitmap to that rect. Because borders are painted just before this call, <img> content can overpaint the border/padding area (e.g. img { border: 8px solid }), so borders are visually lost or incorrect. Replaced content should be inset to the content box (subtract border + padding) before applying object-fit/clipping.
Useful? React with 👍 / 👎.
Comment on lines
+307
to
+310
| } else if h_is_auto { | ||
| // No size info at all → 300×150 fallback | ||
| style.size.width = Dimension::length(300.0); | ||
| style.size.height = Dimension::length(150.0); |
There was a problem hiding this comment.
The 300×150 fallback is only applied when both axes are auto (w_is_auto && h_is_auto). If width is specified (CSS or width attribute) but intrinsic image size is unavailable, height stays auto and this empty replaced node can collapse to 0px in Taffy, breaking layout for unresolved/missing images (for example <img style="width:120px">). The fallback/default-object-size logic needs to also handle the single-auto-axis case when no intrinsic ratio/size exists.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/grida-dev/src/editor/document.rs (1)
200-205:⚠️ Potential issue | 🟠 MajorAuto-height resize path ignores available image dimensions
Line 204 and Line 212 pass
NoImages, so width-driven remeasurement for HTML/Markdown embeds uses fallback image sizing instead of intrinsic sizes. This can produce incorrect heights during resize and content clipping. Please use the renderer-backed image provider in this path.Also applies to: 208-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/editor/document.rs` around lines 200 - 205, The auto-height path calls cg::htmlcss::measure_content_height with cg::htmlcss::NoImages, causing image sizes to fall back and produce incorrect heights; replace NoImages with the renderer-backed image provider (obtainable from the existing renderer instance used for fonts, e.g., the same app.renderer() used for app.renderer().fonts) so that cg::htmlcss::measure_content_height receives real intrinsic image dimensions instead of NoImages.
🧹 Nitpick comments (5)
crates/grida-canvas/src/layout/tree.rs (1)
275-280: Thread a realImageProviderinto markdown measurement pathLine 279 hardwires
NoImages, so markdown auto-measurement ignores intrinsic image sizes and can drift from paint-time layout for<img>content. Consider passing an actual provider when available to keep measurement/render parity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/layout/tree.rs` around lines 275 - 280, The markdown measurement currently hardcodes crate::htmlcss::NoImages so intrinsic image sizes are ignored; change the call to crate::htmlcss::measure_content_height(&ctx.styled_html, width, provider.fonts, /* real image provider */) by passing the real ImageProvider from your existing provider (e.g. provider or provider.image_provider()) instead of NoImages, and update the surrounding types/signature if necessary so that measure_content_height accepts that ImageProvider implementation; keep references to measured_height, ctx.styled_html, width, provider.fonts and crate::htmlcss::measure_content_height when making the change.crates/grida-canvas/examples/golden_htmlcss.rs (1)
27-28: Consider image-aware mode for this golden tool
NoImageskeeps the example simple, but it also prevents visual validation of the new image pipeline. Consider an optional preload/provider path for fixtures with image URLs.🤖 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 27 - 28, The example always uses htmlcss::NoImages which disables image rendering; update the golden example so htmlcss::render can accept an image-aware provider when desired (e.g. via an optional CLI flag or env var). Locate the call to htmlcss::render and the use of htmlcss::NoImages and add a conditional that constructs and passes an image provider/preloader (a fixture-mapping provider or local file loader) when the flag/env var is set, falling back to htmlcss::NoImages otherwise; ensure the provider supports the same trait expected by htmlcss::render so the example can validate the image pipeline when enabled.crates/grida-canvas/src/runtime/image_repository.rs (1)
88-103: Consider extracting shared decode+insert logic to avoid drift.
insertandinsert_bytesnow duplicate the same decode/mipmap/tracking sequence. A shared helper would make future behavior changes safer.Refactor sketch
impl ImageRepository { + fn insert_decoded(&mut self, src: String, image: Image) -> Option<(u32, u32)> { + let width = image.width() as u32; + let height = image.height() as u32; + let mipmapped = image.with_default_mipmaps().unwrap_or(image); + self.images.insert(src.clone(), mipmapped); + self.missing_refs.borrow_mut().remove(&src); + self.reported_refs.remove(&src); + Some((width, height)) + } + pub fn insert(&mut self, src: String, hash: u64) -> Option<(u32, u32)> { if let Some(bytes) = self.store.lock().unwrap().get(hash) { let data = skia_safe::Data::new_copy(bytes); - if let Some(image) = Image::from_encoded(data) { - let width = image.width() as u32; - let height = image.height() as u32; - let mipmapped = image.with_default_mipmaps().unwrap_or(image); - self.images.insert(src.clone(), mipmapped); - self.missing_refs.borrow_mut().remove(&src); - self.reported_refs.remove(&src); - return Some((width, height)); - } + if let Some(image) = Image::from_encoded(data) { + return self.insert_decoded(src, image); + } } None } pub fn insert_bytes(&mut self, src: String, bytes: &[u8]) -> Option<(u32, u32)> { let data = skia_safe::Data::new_copy(bytes); let image = Image::from_encoded(data)?; - let width = image.width() as u32; - let height = image.height() as u32; - let mipmapped = image.with_default_mipmaps().unwrap_or(image); - self.images.insert(src.clone(), mipmapped); - self.missing_refs.borrow_mut().remove(&src); - self.reported_refs.remove(&src); - Some((width, height)) + self.insert_decoded(src, image) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/runtime/image_repository.rs` around lines 88 - 103, Both insert and insert_bytes duplicate the decode -> mipmap -> store -> tracking update sequence; extract that shared logic into a private helper (e.g., a method like insert_decoded_image(&mut self, src: String, image: skia_safe::Image) -> Option<(u32,u32)> or insert_from_data(&mut self, src: String, data: skia_safe::Data) -> Option<(u32,u32)>). Update insert to call the helper after decoding from ByteStore and update insert_bytes to decode with skia_safe::Data::new_copy then call the helper; ensure the helper applies with_default_mipmaps().unwrap_or(image), inserts into self.images, removes from self.missing_refs and self.reported_refs, and returns (width,height) matching the current behavior and error handling.crates/grida-canvas/src/htmlcss/mod.rs (2)
39-45: Return an owned image handle fromget().Line 44’s
Option<&skia_safe::Image>only works for providers that can hand out long-lived borrows from stable storage. That makes common host-backed implementations awkward or impossible —Mutex/RwLockcaches,RefCellstores, or on-demand decoders can’t let the reference outlive the guard or temporary. Returning an owned handle here (skia_safe::ImageorArc<_>) keeps the API aligned with the “host-agnostic” contract in the doc comment.🤖 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 39 - 45, The ImageProvider trait's get method currently returns a borrowed Option<&skia_safe::Image>, which forces implementations to expose long-lived borrows; change the signature of fn get(&self, url: &str) -> Option<&skia_safe::Image> in the ImageProvider trait to return an owned handle instead (e.g. Option<skia_safe::Image> or Option<std::sync::Arc<skia_safe::Image>>) and update the doc comment to state callers receive ownership of an image handle; then update all implementations of ImageProvider and any call sites to construct/clone/arc the image into the returned owned type (e.g. take the decoded image out of a Mutex/RwLock or clone/wrap it in Arc) so temporary guards are not required to outlive the return value.
1209-1242: Tighten the new image tests so they hit the provider-specific branches.Right now these tests still pass if
background-imageURLs stop being collected or if the renderer falls back to the placeholder path:test_collect_image_urls()never asserts thebg.pngcase, andtest_img_render_with_provider()ignoresinsert_bytes()success, hard-codeswidth/height, and only checksis_ok(). Please add one normalization-tolerant assertion for the background URL (for example,ends_with("bg.png")) and one intrinsic-size case with no HTML dimensions that proves the measured/rendered size comes from the preloaded image instead of the 300×150 fallback.
🤖 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/collect.rs`:
- Around line 313-314: The current check in collect_inline_items skips nodes
with child.replaced, causing inline <img> to remain as StyledNode::Element and
break InlineGroup assembly; update collect_inline_items (or the caller branch
that currently does "if is_inline && !child.widget.is_widget() &&
child.replaced.is_none()") to treat inline replaced elements specially: detect
replaced inline elements (e.g., <img> / nodes where child.replaced.is_some() and
they are inline-level) and create a dedicated inline-replaced item and push it
into pending_inline instead of skipping, so layout.rs can consume them as part
of InlineGroup rather than as separate box children. Ensure the logic still
skips replaced block-level widgets but handles inline replaced elements as
inline items.
In `@crates/grida-canvas/src/htmlcss/layout.rs`:
- Around line 300-323: The code only applies the 300×150 fallback when both
width and height are still auto; to fix, when img_size (result of get_size()) is
None apply the default object dimensions/ratio to any single axis still auto so
the leaf Taffy node can be measured: if img_size.is_none() and (w_is_auto ||
h_is_auto) set the missing axis to Dimension::length(300.0) or 150.0
respectively (unless aspect_ratio logic should override), and ensure the height
branch that checks style.aspect_ratio still respects this fallback; update the
branches around style.size.width/height, w_is_auto/h_is_auto and img_size to
implement this single-axis fallback when get_size() returns None.
In `@crates/grida-canvas/src/htmlcss/paint.rs`:
- Around line 338-349: The code currently builds dest_rect from the full
width/height (Rect::from_xywh(0.0, 0.0, w, h)) and clips to that, which lets
padding/border become part of the drawable area; change the clipping to use the
element's inner/content rect (the content-box) instead of dest_rect so
replaced-element painting is limited to the content area. Locate the painting
block around dest_rect, border_radius, clip_rrect and clip_rect and replace the
rect used for clipping with the computed inner/content rect (accounting for
padding and border) and still apply border_radius via RRect::set_rect_radii when
non-zero.
In `@crates/grida-canvas/src/window/application.rs`:
- Around line 1406-1419: Currently prefixed sources (msg.src starting with
"res://" or "system://") are being passed to renderer.add_image which rekeys
them; instead detect when msg.src is a non-empty prefixed key and call
renderer.add_image_by_url(&msg.src, &msg.data) so the original res:// or
system:// key is preserved; only set updated = true when add_image or
add_image_by_url actually succeeded (i.e., when add_image returns a URL/hash or
add_image_by_url returns Some((w,h))). Update the branches around
renderer.add_image, renderer.add_image_by_url, msg.src, msg.data and updated to
reflect this logic.
In `@crates/grida-dev/src/main.rs`:
- Around line 431-455: The fetch currently accepts any HTTP response and uses
the default reqwest::get() with no timeout; change this to create a configured
reqwest::Client via Client::builder().timeout(Duration::from_secs(...)).build()
(clone the client into the async closure) and replace reqwest::get(&url).await
handling with a client.get(&url).send().await flow that calls
resp.error_for_status() before reading bytes so non-2xx responses are treated as
errors; also consider wrapping the closure body with a Semaphore/permit if you
need to limit concurrency when pushing into remote_futures (refer to the
remote_futures push site and the url variable inside the async move).
In `@docs/wg/feat-2d/htmlcss.md`:
- Around line 619-621: The docs overstate behavior for CSS backgrounds; update
the paragraph so the placeholder claim only applies to <img> elements and not
CSS background images by clarifying that ImageProvider-provided images for <img
src> render placeholders when missing, whereas background-image: url() layers
(handled in crates/grida-canvas/src/htmlcss/paint.rs) are skipped entirely
rather than rendering placeholders; reference the ImageProvider trait and the
paint.rs background-image handling to guide the wording change.
In `@docs/wg/research/chromium/external-resource-loading.md`:
- Around line 19-27: The fenced code blocks in this markdown (e.g., the block
starting with "Resource (abstract base)" and other similar fenced regions) lack
language specifiers and trigger MD040; update each triple-backtick fence (```)
to include an appropriate language tag (for ASCII diagrams use "text", for C++
examples use "cpp", etc.) so every fenced block is annotated (apply the same fix
for the other fenced regions noted in the review).
- Around line 1-4: The frontmatter for the new document is incomplete: add the
required keys by updating the existing frontmatter block (which currently
includes title) to also include a descriptive "description" string, a "keywords"
array (e.g., ["chromium","resource-loading","images","css"]), and add "format:
md" to indicate plain Markdown; keep the existing title unchanged and ensure the
frontmatter YAML stays at the top of the file.
In `@docs/wg/research/chromium/index.md`:
- Line 41: This WG page (index.md) is missing required YAML frontmatter; add a
top-of-file frontmatter block containing title (matching the page header),
description (brief summary of the page contents), keywords (comma-separated list
of relevant terms), and format: md so the docs build and SEO fields are present;
ensure the frontmatter keys are exactly title, description, keywords, and
format: md and place it before any markdown content.
---
Outside diff comments:
In `@crates/grida-dev/src/editor/document.rs`:
- Around line 200-205: The auto-height path calls
cg::htmlcss::measure_content_height with cg::htmlcss::NoImages, causing image
sizes to fall back and produce incorrect heights; replace NoImages with the
renderer-backed image provider (obtainable from the existing renderer instance
used for fonts, e.g., the same app.renderer() used for app.renderer().fonts) so
that cg::htmlcss::measure_content_height receives real intrinsic image
dimensions instead of NoImages.
---
Nitpick comments:
In `@crates/grida-canvas/examples/golden_htmlcss.rs`:
- Around line 27-28: The example always uses htmlcss::NoImages which disables
image rendering; update the golden example so htmlcss::render can accept an
image-aware provider when desired (e.g. via an optional CLI flag or env var).
Locate the call to htmlcss::render and the use of htmlcss::NoImages and add a
conditional that constructs and passes an image provider/preloader (a
fixture-mapping provider or local file loader) when the flag/env var is set,
falling back to htmlcss::NoImages otherwise; ensure the provider supports the
same trait expected by htmlcss::render so the example can validate the image
pipeline when enabled.
In `@crates/grida-canvas/src/htmlcss/mod.rs`:
- Around line 39-45: The ImageProvider trait's get method currently returns a
borrowed Option<&skia_safe::Image>, which forces implementations to expose
long-lived borrows; change the signature of fn get(&self, url: &str) ->
Option<&skia_safe::Image> in the ImageProvider trait to return an owned handle
instead (e.g. Option<skia_safe::Image> or
Option<std::sync::Arc<skia_safe::Image>>) and update the doc comment to state
callers receive ownership of an image handle; then update all implementations of
ImageProvider and any call sites to construct/clone/arc the image into the
returned owned type (e.g. take the decoded image out of a Mutex/RwLock or
clone/wrap it in Arc) so temporary guards are not required to outlive the return
value.
In `@crates/grida-canvas/src/layout/tree.rs`:
- Around line 275-280: The markdown measurement currently hardcodes
crate::htmlcss::NoImages so intrinsic image sizes are ignored; change the call
to crate::htmlcss::measure_content_height(&ctx.styled_html, width,
provider.fonts, /* real image provider */) by passing the real ImageProvider
from your existing provider (e.g. provider or provider.image_provider()) instead
of NoImages, and update the surrounding types/signature if necessary so that
measure_content_height accepts that ImageProvider implementation; keep
references to measured_height, ctx.styled_html, width, provider.fonts and
crate::htmlcss::measure_content_height when making the change.
In `@crates/grida-canvas/src/runtime/image_repository.rs`:
- Around line 88-103: Both insert and insert_bytes duplicate the decode ->
mipmap -> store -> tracking update sequence; extract that shared logic into a
private helper (e.g., a method like insert_decoded_image(&mut self, src: String,
image: skia_safe::Image) -> Option<(u32,u32)> or insert_from_data(&mut self,
src: String, data: skia_safe::Data) -> Option<(u32,u32)>). Update insert to call
the helper after decoding from ByteStore and update insert_bytes to decode with
skia_safe::Data::new_copy then call the helper; ensure the helper applies
with_default_mipmaps().unwrap_or(image), inserts into self.images, removes from
self.missing_refs and self.reported_refs, and returns (width,height) matching
the current behavior and error handling.
🪄 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: b2595049-b72a-409d-8009-fb1d4cc93a60
📒 Files selected for processing (21)
crates/grida-canvas/examples/golden_htmlcss.rscrates/grida-canvas/src/cache/geometry.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/tree.rscrates/grida-canvas/src/painter/painter.rscrates/grida-canvas/src/runtime/image_repository.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/src/window/application.rscrates/grida-dev/src/editor/document.rscrates/grida-dev/src/main.rsdocs/wg/feat-2d/htmlcss.mddocs/wg/research/chromium/external-resource-loading.mddocs/wg/research/chromium/index.mdfixtures/test-html/L0/mixed-images.htmlfixtures/test-html/L0/paint-background-image.htmlfixtures/test-html/L0/replaced-img.html
Comment on lines
+313
to
314
| if is_inline && !child.widget.is_widget() && child.replaced.is_none() { | ||
| collect_inline_items(&child, &mut pending_inline); |
There was a problem hiding this comment.
Inline <img>s still don't have an inline layout path.
By skipping child.replaced here, default inline images become standalone StyledNode::Elements. layout.rs only keeps text/inlines inside InlineGroup, so foo <img> bar will turn into separate box children instead of sharing one line. You'll need a dedicated inline-replaced item before taking images out of the flattener.
🤖 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 313 - 314, The
current check in collect_inline_items skips nodes with child.replaced, causing
inline <img> to remain as StyledNode::Element and break InlineGroup assembly;
update collect_inline_items (or the caller branch that currently does "if
is_inline && !child.widget.is_widget() && child.replaced.is_none()") to treat
inline replaced elements specially: detect replaced inline elements (e.g., <img>
/ nodes where child.replaced.is_some() and they are inline-level) and create a
dedicated inline-replaced item and push it into pending_inline instead of
skipping, so layout.rs can consume them as part of InlineGroup rather than as
separate box children. Ensure the logic still skips replaced block-level widgets
but handles inline replaced elements as inline items.
Comment on lines
+300
to
+323
| // If still auto after HTML attrs, use natural image dimensions | ||
| let w_is_auto = style.size.width == Dimension::auto(); | ||
| let h_is_auto = style.size.height == Dimension::auto(); | ||
|
|
||
| if w_is_auto { | ||
| if let Some((nw, _)) = img_size { | ||
| style.size.width = Dimension::length(nw as f32); | ||
| } else if h_is_auto { | ||
| // No size info at all → 300×150 fallback | ||
| style.size.width = Dimension::length(300.0); | ||
| style.size.height = Dimension::length(150.0); | ||
| } | ||
| // else: width auto + height set + aspect_ratio → Taffy resolves | ||
| } | ||
|
|
||
| if style.size.height == Dimension::auto() { | ||
| if let Some((_, nh)) = img_size { | ||
| // Only set natural height if width wasn't explicitly set to | ||
| // something different (aspect_ratio handles that case) | ||
| if style.aspect_ratio.is_none() { | ||
| style.size.height = Dimension::length(nh as f32); | ||
| } | ||
| } | ||
| // else: height auto + width set + aspect_ratio → Taffy resolves |
There was a problem hiding this comment.
Single-axis fallback sizing is still missing for unloaded images.
The 300×150 fallback only runs when both axes remain auto. For width: 100%, HTML width=..., CSS height, etc. without decoded image size, the other axis stays auto, and this leaf Taffy node has nothing to measure from, so it collapses to 0. Use the default object ratio/dimensions when get_size() returns None.
🤖 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 300 - 323, The code
only applies the 300×150 fallback when both width and height are still auto; to
fix, when img_size (result of get_size()) is None apply the default object
dimensions/ratio to any single axis still auto so the leaf Taffy node can be
measured: if img_size.is_none() and (w_is_auto || h_is_auto) set the missing
axis to Dimension::length(300.0) or 150.0 respectively (unless aspect_ratio
logic should override), and ensure the height branch that checks
style.aspect_ratio still respects this fallback; update the branches around
style.size.width/height, w_is_auto/h_is_auto and img_size to implement this
single-axis fallback when get_size() returns None.
| | [node-data-layout.md](./node-data-layout.md) | Node data layout: DOM RareData, compositor property trees, ECS comparison | | ||
| | [svg-pattern.md](./svg-pattern.md) | SVG `<pattern>` paint server semantics, Chromium/resvg/Skia comparison | | ||
| | [blink-rendering-pipeline.md](./blink-rendering-pipeline.md) | Blink Style → Layout → Paint pipeline, ComputedStyle groups, LayoutNG, inline layout, list markers | | ||
| | [external-resource-loading.md](./external-resource-loading.md) | Resource fetch lifecycle, ImageResource observer pattern, CSS background-image/img loading pipeline | |
There was a problem hiding this comment.
Add required docs frontmatter on this edited WG page
Since this page was meaningfully edited, please add missing description, keywords, and format: md fields in frontmatter.
As per coding guidelines, "Include SEO frontmatter with title, description, and keywords when adding or meaningfully editing actively maintained doc pages" and "Add format: md frontmatter to Markdown pages in docs/wg/ and docs/reference/ that do not use MDX/JSX features."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/research/chromium/index.md` at line 41, This WG page (index.md) is
missing required YAML frontmatter; add a top-of-file frontmatter block
containing title (matching the page header), description (brief summary of the
page contents), keywords (comma-separated list of relevant terms), and format:
md so the docs build and SEO fields are present; ensure the frontmatter keys are
exactly title, description, keywords, and format: md and place it before any
markdown content.
…preservation - paint_replaced: clip/draw into content-box (inset by border+padding), not border-box. Fixes image overpainting borders/padding. - apply_replaced_intrinsic_size: set default 2:1 aspect ratio when no image data exists, preventing 0px collapse on single-axis auto. - process_image_queue: preserve res:// keys via add_image_with_rid instead of re-hashing; only set updated on successful registration. - load_html_images: add 15s timeout via reqwest::Client, reject non-2xx responses via error_for_status(). - ImageRepository: extract shared insert_decoded() helper to eliminate decode→mipmap→store duplication between insert/insert_bytes. - Resolve merge conflict with main (outline support). - Doc frontmatter: add description/keywords/format to research doc. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Run oxfmt on 6 files (md + html fixtures) to pass CI check - Clarify docs: missing <img> → placeholder rect, missing background-image → silently skipped (not placeholder) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
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/htmlcss/collect.rs (1)
1234-1289:⚠️ Potential issue | 🟠 MajorPreserve CSS background stacking order here.
paint_background()drawsstyle.backgroundin vector order. Appending image layers as-is means multi-image backgrounds are stored top-first instead of bottom-to-top, so later authored layers end up covering earlier ones.🛠️ Suggested fix
- for image in bg.background_image.0.iter() { + for image in bg.background_image.0.iter().rev() {🤖 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 1234 - 1289, The background image loop currently iterates bg.background_image.0 in author order and pushes into layers, which stores images top-first while paint_background() expects vector order bottom-to-top; fix by iterating in reverse (or otherwise inserting so earlier-authored layers end up later in the layers Vec) so that when you push BackgroundLayer::Image (StyleImage::LinearGradient/ RadialGradient/ ConicGradient/ Url) the final layers vector matches paint_background()'s bottom-to-top stacking; update the loop over bg.background_image.0 (and any ComputedUrl handling) to use reversed iteration or push_front semantics so stacking order is preserved.
♻️ Duplicate comments (3)
crates/grida-canvas/src/window/application.rs (1)
1414-1418:⚠️ Potential issue | 🟡 MinorDon't report the content-addressed path as a guaranteed load.
The new RID/URL branches use an
Optionresult, butRenderer::add_image()incrates/grida-canvas/src/runtime/scene.rsstill collapses a failed or no-op insert to(0, 0). This branch can therefore log success and flipokeven when it didn't create a usable image entry. Please deriveokfrom the returned dimensions here, or makeadd_image()returnOptionlike the other two APIs.🐛 Proposed fix
- } else if msg.src.is_empty() { - // No key — content-addressed (hash → res://images/{hash}) - let (hash, url, _, _, _) = self.renderer.add_image(&msg.data); - println!("📝 Registered image: {} ({})", hash, url); - true + } else if msg.src.is_empty() { + // No key — content-addressed (hash → res://images/{hash}) + let (hash, url, w, h, _) = self.renderer.add_image(&msg.data); + if w > 0 && h > 0 { + println!("📝 Registered image: {} ({})", hash, url); + true + } else { + false + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/window/application.rs` around lines 1414 - 1418, The branch handling content-addressed images (msg.src.is_empty()) currently assumes add_image succeeded because it destructures let (hash, url, _, _, _) = self.renderer.add_image(&msg.data) and logs/returns true; instead derive ok from the returned dimensions (e.g., the width/height fields returned by renderer.add_image) and only log "Registered image" and return true when those dimensions indicate a real image (non-zero), otherwise do not log success and return false; update the branch around self.renderer.add_image(&msg.data) to inspect the returned size tuple (or change add_image to return Option and handle None) so success reflects an actual usable image entry.docs/wg/research/chromium/external-resource-loading.md (1)
43-45:⚠️ Potential issue | 🟡 MinorAnnotate this fenced block with a language.
This is still a plain triple-backtick fence, so markdownlint MD040 will keep flagging the page.
textfits here.🛠️ Suggested fix
-``` +```text kNotStarted → kPending → kCached | kLoadError | kDecodeError</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/wg/research/chromium/external-resource-loading.mdaround lines 43 - 45,
The fenced code block containing the state line "kNotStarted → kPending →
kCached | kLoadError | kDecodeError" is missing a language annotation; update
the triple-backtick fence to usetext (i.e., changeto ```text) so
markdownlint MD040 stops flagging it and the block is treated as plain text.</details> </blockquote></details> <details> <summary>crates/grida-canvas/src/htmlcss/collect.rs (1)</summary><blockquote> `313-314`: _⚠️ Potential issue_ | _🟠 Major_ **Mixed text + inline `<img>` still gets split out of the inline sequence.** This guard turns `foo <img> bar` into separate siblings instead of one inline formatting context. Since `InlineRunItem` still has no replaced/atomic-inline variant, the collector cannot preserve normal paragraph wrapping/alignment semantics across inline images. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/collect.rs` around lines 313 - 314, The current guard in collect.rs prevents replaced inline elements from being added to the inline sequence (the condition "is_inline && !child.widget.is_widget() && child.replaced.is_none()"), which splits "foo <img> bar" into separate siblings; remove the replaced.is_none() check so collect_inline_items(&child, &mut pending_inline) is called for replaced inline elements too (i.e., change the condition to "is_inline && !child.widget.is_widget()"), ensuring images remain inside the inline run collected by collect_inline_items and preserving paragraph wrapping/alignment semantics. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>fixtures/test-html/L0/replaced-img.html (1)</summary><blockquote> `69-90`: **Keep this fixture hermetic.** These `picsum.photos` cases make a baseline fixture depend on network availability and third-party content stability. That will cause flaky renders and offline failures. Please keep this fixture on checked-in assets and move remote-loading coverage to a dedicated integration path. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@fixtures/test-html/L0/replaced-img.html` around lines 69 - 90, The fixture includes remote image URLs (the <img> elements with src="https://picsum.photos/id/237/200/120" and src="https://picsum.photos/id/1025/160/160") which makes the test non-hermetic; replace these remote <img> tags with checked-in static assets (point src to local fixture image files committed to the repo and keep width/height/alt intact) and move any tests that require remote-loading behavior into a separate integration test path so the baseline fixture remains offline-stable. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@crates/grida-canvas/src/window/application.rs:
- Around line 1428-1431: The image loading path currently only marks
ChangeFlags::IMAGE_LOADED in process_image_queue() (via
self.renderer.mark_changed(ChangeFlags::IMAGE_LOADED)) but does not trigger a
winit redraw; update the HostEvent::ImageLoaded handler (the code that calls
notify_resource_loaded()) to call self.window.request_redraw() immediately after
notify_resource_loaded() so the frame() path runs and the newly-registered
images are painted, mirroring the pattern used in the HostEvent::LoadScene
handler.
Outside diff comments:
In@crates/grida-canvas/src/htmlcss/collect.rs:
- Around line 1234-1289: The background image loop currently iterates
bg.background_image.0 in author order and pushes into layers, which stores
images top-first while paint_background() expects vector order bottom-to-top;
fix by iterating in reverse (or otherwise inserting so earlier-authored layers
end up later in the layers Vec) so that when you push BackgroundLayer::Image
(StyleImage::LinearGradient/ RadialGradient/ ConicGradient/ Url) the final
layers vector matches paint_background()'s bottom-to-top stacking; update the
loop over bg.background_image.0 (and any ComputedUrl handling) to use reversed
iteration or push_front semantics so stacking order is preserved.
Duplicate comments:
In@crates/grida-canvas/src/htmlcss/collect.rs:
- Around line 313-314: The current guard in collect.rs prevents replaced inline
elements from being added to the inline sequence (the condition "is_inline &&
!child.widget.is_widget() && child.replaced.is_none()"), which splits "foo
bar" into separate siblings; remove the replaced.is_none() check so
collect_inline_items(&child, &mut pending_inline) is called for replaced inline
elements too (i.e., change the condition to "is_inline &&
!child.widget.is_widget()"), ensuring images remain inside the inline run
collected by collect_inline_items and preserving paragraph wrapping/alignment
semantics.In
@crates/grida-canvas/src/window/application.rs:
- Around line 1414-1418: The branch handling content-addressed images
(msg.src.is_empty()) currently assumes add_image succeeded because it
destructures let (hash, url, _, _, _) = self.renderer.add_image(&msg.data) and
logs/returns true; instead derive ok from the returned dimensions (e.g., the
width/height fields returned by renderer.add_image) and only log "Registered
image" and return true when those dimensions indicate a real image (non-zero),
otherwise do not log success and return false; update the branch around
self.renderer.add_image(&msg.data) to inspect the returned size tuple (or change
add_image to return Option and handle None) so success reflects an actual usable
image entry.In
@docs/wg/research/chromium/external-resource-loading.md:
- Around line 43-45: The fenced code block containing the state line
"kNotStarted → kPending → kCached | kLoadError | kDecodeError" is missing a
language annotation; update the triple-backtick fence to usetext (i.e., changeto ```text) so markdownlint MD040 stops flagging it and the block is
treated as plain text.
Nitpick comments:
In@fixtures/test-html/L0/replaced-img.html:
- Around line 69-90: The fixture includes remote image URLs (the
elements
with src="https://picsum.photos/id/237/200/120" and
src="https://picsum.photos/id/1025/160/160") which makes the test non-hermetic;
replace these remotetags with checked-in static assets (point src to
local fixture image files committed to the repo and keep width/height/alt
intact) and move any tests that require remote-loading behavior into a separate
integration test path so the baseline fixture remains offline-stable.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `88c75226-4307-4253-bafb-107de54b63c1` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d07e73fab6a9d87ad7542e003e89966e17d0b332 and fa0c67a2bb11897981f7b21c33ac958173e7aa2f. </details> <details> <summary>📒 Files selected for processing (13)</summary> * `crates/grida-canvas/src/htmlcss/collect.rs` * `crates/grida-canvas/src/htmlcss/layout.rs` * `crates/grida-canvas/src/htmlcss/paint.rs` * `crates/grida-canvas/src/htmlcss/style.rs` * `crates/grida-canvas/src/runtime/image_repository.rs` * `crates/grida-canvas/src/window/application.rs` * `crates/grida-dev/src/main.rs` * `docs/wg/feat-2d/htmlcss.md` * `docs/wg/research/chromium/external-resource-loading.md` * `docs/wg/research/chromium/index.md` * `fixtures/test-html/L0/mixed-images.html` * `fixtures/test-html/L0/paint-background-image.html` * `fixtures/test-html/L0/replaced-img.html` </details> <details> <summary>✅ Files skipped from review due to trivial changes (3)</summary> * docs/wg/research/chromium/index.md * fixtures/test-html/L0/paint-background-image.html * fixtures/test-html/L0/mixed-images.html </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (4)</summary> * crates/grida-canvas/src/htmlcss/layout.rs * docs/wg/feat-2d/htmlcss.md * crates/grida-dev/src/main.rs * crates/grida-canvas/src/runtime/image_repository.rs </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+1428
to
1431
| updated |= ok; | ||
| } | ||
| if updated { | ||
| self.renderer.mark_changed(ChangeFlags::IMAGE_LOADED); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find where image-loaded notifications are handled.
rg -n -C5 'HostEvent::ImageLoaded|notify_resource_loaded\s*\(|resource_loaded\s*\(' --type rust
echo
# 2) Check whether those paths also schedule a repaint/invalidation.
rg -n -C5 'request_redraw\s*\(|redraw_requested\s*\(|window\.request_redraw|queue\(' --type rustRepository: gridaco/grida
Length of output: 50369
🏁 Script executed:
# Get the full context around lines 1428-1431 in application.rs
sed -n '1400,1500p' crates/grida-canvas/src/window/application.rs | cat -nRepository: gridaco/grida
Length of output: 4604
🏁 Script executed:
# Check what happens after notify_resource_loaded() is called in native_application.rs
# and see if there's a redraw request after it
sed -n '735,760p' crates/grida-dev/src/platform/native_application.rs | cat -nRepository: gridaco/grida
Length of output: 1235
🏁 Script executed:
# Check what queue() does - it appears to be the method that should request redraw
# Let me search for the queue() method definition
rg -n 'fn queue\(' crates/grida-canvas/src/window/application.rs | head -5Repository: gridaco/grida
Length of output: 87
🏁 Script executed:
# Get the queue() method implementation
sed -n '1268,1280p' crates/grida-canvas/src/window/application.rs | cat -nRepository: gridaco/grida
Length of output: 662
🏁 Script executed:
# Now let's verify: after notify_resource_loaded() in native_application.rs (lines 741-745),
# is there any explicit queue() or request_redraw() call?
# Let me check the full context around the image/font loaded handlers
sed -n '738,750p' crates/grida-dev/src/platform/native_application.rs | cat -nRepository: gridaco/grida
Length of output: 690
🏁 Script executed:
# Let me check if resource_loaded() or its callees call queue() or request_redraw()
sed -n '1508,1515p' crates/grida-canvas/src/window/application.rs | cat -nRepository: gridaco/grida
Length of output: 331
🏁 Script executed:
# Let me verify the complete flow - check if there's ANYTHING after notify_resource_loaded()
# that triggers invalidation. Let me search for where notify_resource_loaded is called
# and what happens in the event loop
rg -n 'notify_resource_loaded|resource_loaded' crates/grida-dev/src/platform/native_application.rsRepository: gridaco/grida
Length of output: 166
🏁 Script executed:
# Let me check if there's an event loop that processes mark_changed automatically
# or if queue()/request_redraw() needs to be called explicitly after mark_changed()
rg -n 'mark_changed.*IMAGE_LOADED' crates/grida-canvas/src/ --type rustRepository: gridaco/grida
Length of output: 436
🏁 Script executed:
# Check the emscripten version to see if it calls request_redraw() after mark_changed()
sed -n '420,440p' crates/grida-canvas/src/window/application_emscripten.rs | cat -nRepository: gridaco/grida
Length of output: 1070
🏁 Script executed:
# Let me verify what happens with mark_changed() - does it trigger a redraw automatically?
# Check if there's a polling mechanism that checks mark_changed flags
rg -n 'mark_changed|ChangeFlags::IMAGE_LOADED' crates/grida-canvas/src/runtime/ --type rust -A 2 -B 2 | head -50Repository: gridaco/grida
Length of output: 3804
🏁 Script executed:
# Check the frame() method to see if it's automatically called or if queue()/request_redraw is needed
rg -n 'fn frame\(' crates/grida-canvas/src/window/application.rsRepository: gridaco/grida
Length of output: 110
🏁 Script executed:
# Get the frame() method implementation to see if it's called automatically
sed -n '1285,1320p' crates/grida-canvas/src/window/application.rs | cat -nRepository: gridaco/grida
Length of output: 1666
🏁 Script executed:
# Let me check how the winit event loop works - does it call frame() on every iteration
# or does it only call it after request_redraw() is called?
rg -n 'frame\(' crates/grida-dev/src/platform/native_application.rs | head -10Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Check how the native application event loop works - look for MainEventsCleared or other event loop patterns
rg -n 'MainEventsCleared|about_to_wait|redraw_requested|frame\(' crates/grida-dev/src/platform/native_application.rs | head -20Repository: gridaco/grida
Length of output: 101
🏁 Script executed:
# Get context around line 392 to see the event loop structure
sed -n '380,410p' crates/grida-dev/src/platform/native_application.rs | cat -nRepository: gridaco/grida
Length of output: 1188
🏁 Script executed:
# Let me verify the complete call chain. Check what redraw_requested() does
sed -n '301,310p' crates/grida-canvas/src/window/application.rs | cat -nRepository: gridaco/grida
Length of output: 541
Add request_redraw() after image registration to ensure immediate visual updates.
The process_image_queue() method only marks ChangeFlags::IMAGE_LOADED but does not trigger a redraw. Since frame() is only invoked when the RedrawRequested event fires from winit, async <img> and background-image updates will remain as placeholders until an unrelated paint event occurs.
The HostEvent::ImageLoaded handler in native_application.rs should call self.window.request_redraw() after notify_resource_loaded(), following the pattern used in the HostEvent::LoadScene handler which explicitly requests redraw.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/window/application.rs` around lines 1428 - 1431, The
image loading path currently only marks ChangeFlags::IMAGE_LOADED in
process_image_queue() (via
self.renderer.mark_changed(ChangeFlags::IMAGE_LOADED)) but does not trigger a
winit redraw; update the HostEvent::ImageLoaded handler (the code that calls
notify_resource_loaded()) to call self.window.request_redraw() immediately after
notify_resource_loaded() so the frame() path runs and the newly-registered
images are painted, mirroring the pattern used in the HostEvent::LoadScene
handler.
…comments Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/grida-canvas/src/htmlcss/collect.rs (1)
253-267: Consider simplifying the redundant option check.The outer
if el.replaced.is_some()guard and the innerif let Some(ref mut replaced) = el.replacedare redundant. You can combine them into a singleif letblock.♻️ Suggested simplification
- // Extract object-fit from Stylo for replaced elements (<img>) - if el.replaced.is_some() { + // Extract object-fit from Stylo for replaced elements (<img>) + if let Some(ref mut replaced) = el.replaced { use style::properties::longhands::object_fit::computed_value::T as StyloObjectFit; let of = style.get_position().clone_object_fit(); let object_fit = match of { StyloObjectFit::Fill => types::ObjectFit::Fill, StyloObjectFit::Contain => types::ObjectFit::Contain, StyloObjectFit::Cover => types::ObjectFit::Cover, StyloObjectFit::None => types::ObjectFit::None, StyloObjectFit::ScaleDown => types::ObjectFit::ScaleDown, }; - if let Some(ref mut replaced) = el.replaced { - replaced.object_fit = object_fit; - } + replaced.object_fit = object_fit; }🤖 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 253 - 267, The code redundantly checks el.replaced twice; replace the outer if el.replaced.is_some() and inner if let Some(ref mut replaced) = el.replaced with a single pattern match: use if let Some(ref mut replaced) = el.replaced { ... } to both test and bind the mutable replaced value, then perform the style.get_position().clone_object_fit() mapping and assign replaced.object_fit; keep the StyloObjectFit import and mapping logic intact but remove the extra is_some() guard so only the single if let is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/grida-canvas/src/htmlcss/collect.rs`:
- Around line 253-267: The code redundantly checks el.replaced twice; replace
the outer if el.replaced.is_some() and inner if let Some(ref mut replaced) =
el.replaced with a single pattern match: use if let Some(ref mut replaced) =
el.replaced { ... } to both test and bind the mutable replaced value, then
perform the style.get_position().clone_object_fit() mapping and assign
replaced.object_fit; keep the StyloObjectFit import and mapping logic intact but
remove the extra is_some() guard so only the single if let is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9c096e3-93bd-4a96-a164-99c6dbe0cc04
📒 Files selected for processing (1)
crates/grida-canvas/src/htmlcss/collect.rs
…ox-sizing fix **border-image (9-slice algorithm):** - `BorderImage` struct + `BorderImageRepeat` enum (Chromium: NinePieceImage) - `extract_border_image()` from Stylo — all 5 longhands (source, slice, width, outset, repeat) - `paint_border_image()` — 4 corners (scaled), 4 edges (stretch/repeat/ round/space), optional center fill - `border-image-width: Number(n)` correctly resolved as n× border-width **resolve_style_image() — unified image resolution:** - Single entry point for StyleImage → skia::Image (Chromium: StyleImage::GetImage) - URL images resolved via ImageProvider, gradients rasterized to temp surface - Eliminates variant branching in paint_background and paint_border_image - Gradient border-images now work (rasterize → 9-slice uniformly) **convert_image() — shared Stylo extraction:** - Extracted GenericImage → StyleImage conversion used by both extract_background and extract_border_image (was copy-pasted) - extract_background body reduced from 55 lines to 4 **box-sizing fix (pre-existing bug):** - Taffy defaults to BorderBox, CSS defaults to content-box - Now extracted from Stylo and mapped to Taffy style - Fixes all elements with borders/padding (most visible with thick borders) **Fixture:** paint-border-image.html using MDN border-diamonds.png 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 (1)
crates/grida-canvas/src/htmlcss/mod.rs (1)
210-242:collect_image_urlsmay miss images inInlineGroupchildren.The recursive traversal only descends into
StyledNode::Element. If an inline element with a background-image URL were inside anInlineGroup, it would be missed. However, sinceInlineGroupcontainsInlineRunItem(text runs and box markers) rather than fullStyledElements, this is likely not a practical concern.Consider documenting this assumption if inline elements with image backgrounds are not expected to appear in InlineGroups.
🤖 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 210 - 242, collect_urls_from_element currently only recurses into children when a child matches style::StyledNode::Element, so images that might live inside InlineGroup/InlineRunItem nodes are skipped; either extend the traversal to inspect InlineGroup/InlineRunItem variants (e.g., descend into InlineGroup to walk its InlineRunItem entries and extract any StyledElement or style::StyleImage::Url occurrences) or add a clear comment above collect_urls_from_element documenting the assumption that InlineGroup/InlineRunItem will not contain image-bearing elements; update the function to handle style::StyledNode::InlineGroup (and its run items) if you want to be robust, referencing collect_urls_from_element, style::StyledNode::Element, style::StyledNode::InlineGroup, and InlineRunItem to locate where to add the additional traversal or the explanatory comment.
🤖 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/collect.rs`:
- Around line 1208-1223: The current code treats any unresolved (auto) side from
resolve_bisw as 0.0 via unwrap_or(0.0), but per the review we should not default
auto to 0. Implement Option 1: only construct and return Some(EdgeInsets) when
all four sides are Some; if any of t, r, bv, or l is None return None so
paint-time can apply the border-image-slice fallback. Modify the block that uses
resolve_bisw on biw.0..3 (and border_widths) to check t.is_some() && r.is_some()
&& bv.is_some() && l.is_some() before calling unwrap on each to build
EdgeInsets; otherwise set width to None.
---
Nitpick comments:
In `@crates/grida-canvas/src/htmlcss/mod.rs`:
- Around line 210-242: collect_urls_from_element currently only recurses into
children when a child matches style::StyledNode::Element, so images that might
live inside InlineGroup/InlineRunItem nodes are skipped; either extend the
traversal to inspect InlineGroup/InlineRunItem variants (e.g., descend into
InlineGroup to walk its InlineRunItem entries and extract any StyledElement or
style::StyleImage::Url occurrences) or add a clear comment above
collect_urls_from_element documenting the assumption that
InlineGroup/InlineRunItem will not contain image-bearing elements; update the
function to handle style::StyledNode::InlineGroup (and its run items) if you
want to be robust, referencing collect_urls_from_element,
style::StyledNode::Element, style::StyledNode::InlineGroup, and InlineRunItem to
locate where to add the additional traversal or the explanatory comment.
🪄 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: 74315fc3-ee12-4435-8d99-9e9298aac6e5
⛔ Files ignored due to path filters (1)
fixtures/images/border-diamonds.pngis excluded by!**/*.png
📒 Files selected for processing (8)
crates/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.rsdocs/wg/feat-2d/htmlcss.mdfixtures/test-html/L0/paint-border-image.html
✅ Files skipped from review due to trivial changes (1)
- fixtures/test-html/L0/paint-border-image.html
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/grida-canvas/src/htmlcss/types.rs
- docs/wg/feat-2d/htmlcss.md
- crates/grida-canvas/src/htmlcss/layout.rs
Comment on lines
+1208
to
+1223
| let width = { | ||
| let t = resolve_bisw(&biw.0, border_widths[0]); | ||
| let r = resolve_bisw(&biw.1, border_widths[1]); | ||
| let bv = resolve_bisw(&biw.2, border_widths[2]); | ||
| let l = resolve_bisw(&biw.3, border_widths[3]); | ||
| if t.is_some() || r.is_some() || bv.is_some() || l.is_some() { | ||
| Some(EdgeInsets { | ||
| top: t.unwrap_or(0.0), | ||
| right: r.unwrap_or(0.0), | ||
| bottom: bv.unwrap_or(0.0), | ||
| left: l.unwrap_or(0.0), | ||
| }) | ||
| } else { | ||
| None | ||
| } | ||
| }; |
There was a problem hiding this comment.
border-image-width: auto fallback may produce incorrect values.
When border-image-width has auto for some sides, the code sets those to 0.0. Per CSS spec, auto should fall back to the corresponding border-image-slice value. Currently, if only some sides are explicit, the auto sides become 0, which would produce no border rendering on those edges.
Consider either:
- Returning
Nonefor the wholewidthwhen any side isauto(let paint-time handle fallback) - Substituting the slice value for
autosides here
🔧 Suggested fix (Option 1: keep None semantics consistent)
let width = {
let t = resolve_bisw(&biw.0, border_widths[0]);
let r = resolve_bisw(&biw.1, border_widths[1]);
let bv = resolve_bisw(&biw.2, border_widths[2]);
let l = resolve_bisw(&biw.3, border_widths[3]);
- if t.is_some() || r.is_some() || bv.is_some() || l.is_some() {
+ // Only produce explicit widths if ALL sides are specified (no auto)
+ if t.is_some() && r.is_some() && bv.is_some() && l.is_some() {
Some(EdgeInsets {
- top: t.unwrap_or(0.0),
- right: r.unwrap_or(0.0),
- bottom: bv.unwrap_or(0.0),
- left: l.unwrap_or(0.0),
+ top: t.unwrap(),
+ right: r.unwrap(),
+ bottom: bv.unwrap(),
+ left: l.unwrap(),
})
} else {
None
}
};🤖 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 1208 - 1223, The
current code treats any unresolved (auto) side from resolve_bisw as 0.0 via
unwrap_or(0.0), but per the review we should not default auto to 0. Implement
Option 1: only construct and return Some(EdgeInsets) when all four sides are
Some; if any of t, r, bv, or l is None return None so paint-time can apply the
border-image-slice fallback. Modify the block that uses resolve_bisw on biw.0..3
(and border_widths) to check t.is_some() && r.is_some() && bv.is_some() &&
l.is_some() before calling unwrap on each to build EdgeInsets; otherwise set
width to None.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grida-dev/src/image_loader.rs`:
- Around line 73-84: The synchronous std::fs::read call inside the image-loading
loop blocks Tokio worker threads; replace it with the async
tokio::fs::read(...).await to perform non-blocking file I/O. Locate the read
call (std::fs::read(&resolved)) in the image loading function (e.g.,
load_images) and change the match to await tokio::fs::read(&resolved).await,
keeping the existing Ok/Err handling and the eprintln for errors so
local_results.push((url.clone(), bytes)) receives the same Option<Vec<u8>>
result.
🪄 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: 1f9c507c-791c-428e-8b1c-b4938ce3391e
📒 Files selected for processing (3)
crates/grida-dev/src/image_loader.rscrates/grida-dev/src/lib.rscrates/grida-dev/src/main.rs
✅ Files skipped from review due to trivial changes (1)
- crates/grida-dev/src/lib.rs
Comment on lines
+73
to
+84
| let bytes = match std::fs::read(&resolved) { | ||
| Ok(b) => Some(b), | ||
| Err(e) => { | ||
| eprintln!( | ||
| " [img] read failed {} (resolved: {}): {e}", | ||
| url, | ||
| resolved.display() | ||
| ); | ||
| None | ||
| } | ||
| }; | ||
| local_results.push((url.clone(), bytes)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check its contents
cat -n crates/grida-dev/src/image_loader.rs | head -120Repository: gridaco/grida
Length of output: 5140
🏁 Script executed:
# Check if Tokio is in dependencies for grida-dev crate
cat crates/grida-dev/Cargo.toml | grep -A 20 "\[dependencies\]"Repository: gridaco/grida
Length of output: 607
🏁 Script executed:
# Verify the function signature and full context
sed -n '29,103p' crates/grida-dev/src/image_loader.rs | cat -nRepository: gridaco/grida
Length of output: 3303
Avoid blocking disk I/O inside async image loading.
std::fs::read at line 73 executes synchronously in the loop, blocking the entire async function. Even though local files are read before the remote .await point, if load_images is called from a Tokio context, it will block a worker thread. Tokio's fs feature is already enabled in dependencies; use tokio::fs::read().await to make local file reads properly async.
Proposed fix
use std::path::Path;
+use tokio::fs as async_fs;
@@
- let bytes = match std::fs::read(&resolved) {
+ let bytes = match async_fs::read(&resolved).await {
Ok(b) => Some(b),
Err(e) => {
eprintln!(📝 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
| let bytes = match std::fs::read(&resolved) { | |
| Ok(b) => Some(b), | |
| Err(e) => { | |
| eprintln!( | |
| " [img] read failed {} (resolved: {}): {e}", | |
| url, | |
| resolved.display() | |
| ); | |
| None | |
| } | |
| }; | |
| local_results.push((url.clone(), bytes)); | |
| let bytes = match async_fs::read(&resolved).await { | |
| Ok(b) => Some(b), | |
| Err(e) => { | |
| eprintln!( | |
| " [img] read failed {} (resolved: {}): {e}", | |
| url, | |
| resolved.display() | |
| ); | |
| None | |
| } | |
| }; | |
| local_results.push((url.clone(), bytes)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-dev/src/image_loader.rs` around lines 73 - 84, The synchronous
std::fs::read call inside the image-loading loop blocks Tokio worker threads;
replace it with the async tokio::fs::read(...).await to perform non-blocking
file I/O. Locate the read call (std::fs::read(&resolved)) in the image loading
function (e.g., load_images) and change the match to await
tokio::fs::read(&resolved).await, keeping the existing Ok/Err handling and the
eprintln for errors so local_results.push((url.clone(), bytes)) receives the
same Option<Vec<u8>> result.
This was referenced Apr 21, 2026
Merged
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
ImageProvidertrait — host-agnostic image resolution for the htmlcss renderer, inspired by Chromium'sStyleImage/ImageResourceContentpattern<img>element rendering — full Stylo cascade, correct CSS/HTML sizing priority,object-fitviamath2::BoxFit, content-box clippingbackground-image: url()support — extracts from bothComputedUrl::ValidandComputedUrl::Invalid(Stylo servo-mode relative URL handling)collect_image_urls()→ parallel local+remote loading →PreloadedImagesfor measurement →add_image_by_url()for renderer registrationArchitecture
Sizing priority (HTML spec)
width/height(from Stylo)width/heightattributes (presentational hints)ImageProvider::get_size())Image data always contributes
aspect_ratioon the Taffy style.Test plan
cargo check -p cg -p grida-dev -p grida-canvas-wasm— 0 errors, 0 warningscargo test -p cg— 717 pass, 0 failures (5 new tests: img collection, img render with provider, img placeholder, background-image placeholder, URL extraction)grida-dev fixtures/test-html/L0/replaced-img.html— local images render at correct sizegrida-dev fixtures/test-html/L0/mixed-images.html— cards with local + remote imagesgrida-dev fixtures/test-html/L0/paint-background-image.html— background-image specimens🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests/Fixtures