htmlcss: 20+ CSS properties across gradients, filters, clip-path, grid alignment, images#675
Merged
softmarshmallow merged 27 commits intomainfrom Apr 21, 2026
Merged
Conversation
extract_inset was stubbed, so `position: absolute|relative` with explicit offsets silently laid out at auto. Wire up clone_top/right/bottom/left() with px/percent/auto mapping; anchor-positioning variants fall back to auto. Adds probe-style L0 fixtures (layout-position-absolute, layout-position-relative) and unit tests for px and percent cases.
Implements shape / size / position on radial-gradient, from / at on
conic-gradient, and the repeating flag across all three gradient types.
Fixes a correctness bug where conic-gradient(...) with no `from` was
rendered rotated 90° CCW (Skia sweep starts at +x; CSS starts at top).
Collect layer: destructure shape / position / flags from Stylo's
Linear / Radial / Conic variants; wire into new RadialShape,
RadialSize, GradientPosition style types with CSS-default impls.
Shader math (paint.rs):
- Linear/radial: scale extent (line length / radii) by the last-stop
cycle fraction for repeating, via shared `repeat_scale` helper.
- Radial: `radial_radii` resolves CSS extent keywords per spec
(closest-side / farthest-corner / corner variants use √2 scaling of
side distances); unit-circle + local matrix handles circle and
ellipse uniformly.
- Conic: `start = from − 90°` aligns CSS 0° (top) with Skia's +x
origin; always `TileMode::Repeat` since `atan2` returns (−π, π]
and `Clamp` would collapse angles below a negative start onto the
first stop.
Tests: 7 collect-layer extraction tests (shape, size extent / explicit,
position, from, at, repeating flag) + 1 pixel-probe pinning the conic
quadrant→color mapping. `rasterize_rgba` forces RGBA8888 readback to
avoid platform-dependent N32 byte order.
Fixtures: paint-gradient-{radial,conic,repeating}.html cover shape,
size, position, from, at, and all three repeating variants.
Known limitation: px-unit stop positions in repeating gradients
auto-distribute to 0..100% and lose their cycle. Pre-existing in
gradient_items_to_stops — a proper fix preserves raw stop positions
through the data model. Repeating fixture uses % stops to exercise
the working path.
Promote `BackgroundLayer::Image` to carry full CSS geometry (`source`, `size`, `position`, `repeat`, `clip`, `origin`), replacing the previous stretch-to-fill stub. Per-layer longhands cycle per spec when shorter than the image list. - `background-size`: cover, contain, auto, explicit with auto-aspect - `background-position`: %/px/keyword per axis - `background-repeat`: repeat, no-repeat, repeat-x, repeat-y (space / round fall back to repeat — P1 follow-up) - `background-clip` / `background-origin`: border-box, padding-box, content-box; border-box clip respects `border-radius` Shader-based tiling (`Image::to_shader` + `TileMode`) handles both URL images and rasterized gradients uniformly. Tests: 7 collect-layer assertions + 4 pixel probes (no-repeat tile, bottom-right position, repeat-x, padding-box clip). 4 new L0 fixtures drive the golden renderer.
Wire three previously-stubbed text properties from Stylo through paint: - `text-shadow`: new `TextShadow` type, stacked in source order; CSS blur radius is mapped to Skia's Gaussian sigma (blur / 2). Drawn via `TextStyle::add_shadow`. - `text-decoration-color`: extract as `Option<CGColor>`; `currentcolor` stays `None` and paint falls back to the text color. - `text-decoration-style`: map Stylo keyword to Skia's `TextDecorationStyle` (solid/double/dotted/dashed/wavy). Tests: 7 collect-layer assertions (single + stacked + default empty shadow; absolute vs currentcolor decoration color; all 5 style variants) + 1 pixel smoke probe (red shadow pixels present). 2 new L0 fixtures drive the golden renderer. Pre-existing oxfmt inlined some gradient-fixture CSS; folded in.
Extract the standalone `translate`, `rotate`, and `scale` longhands from Stylo and prepend them to the element's transform op list in the spec-required order (translate → rotate → scale → `transform:`). - 2D `rotate()` and `rotate: <angle>` supported; 3D `rotate(x y z a)` is honored only when the axis is (0, 0, z) (collapses to 2D). - `scale: 1.5` / `scale: 2 0.5` cover uniform and non-uniform. - `translate: 50% 25%` resolves percentages against the element box via the existing `TransformOp::Translate` percent resolver. Tests: 5 collect-layer assertions (translate, rotate, scale, ordering vs `transform:` shorthand, default empty). 1 new L0 fixture.
Extract the `z-index` longhand in `collect.rs` (was stubbed as always-`None`) and sort children at paint time so that positioned siblings with an explicit `z-index` obey CSS 2.1 §9.9.1 stacking: negative → in-flow → non-negative, stable within each bucket. Scope limited to sibling z-index ordering. Full stacking-context semantics (opacity, transform, isolation, etc.) still in the backlog. Tests: 4 (z-index auto + integer extraction, pixel probe of overlap wins, source-order default). 1 new L0 fixture.
Thread the element's computed `color` through `convert_image` and the two gradient-item converters so that a `currentcolor` stop — which Stylo surfaces as an unresolvable computed color — falls back to the element's text color instead of transparent. The extraction of `el.color` is reordered to run before `border-image` and `background`, since both now depend on it. Extends to linear, radial, and conic gradients; inherited color propagates naturally because `color` is inherited upstream. Tests: 3 collect-layer assertions (linear, inherited via radial, conic) + 1 pixel probe. 1 new L0 fixture.
Extract Stylo's `color_interpolation_method` on linear/radial/conic gradients and thread it to Skia's `Interpolation` struct (color space + hue method). Covers the full CSS Color 4 palette: sRGB, sRGB-linear, HSL, HWB, Lab, LCH, Oklab, Oklch, Display-P3, Rec2020, A98-RGB, ProphotoRGB. XYZ spaces have no Skia equivalent and fall back to Destination; Stylo's `Specified` hue method collapses to `Shorter`. The explicit `linear-gradient(… in <space>, …)` syntax is gated in Stylo behind the `layout.css.gradient-color-interpolation-method` static pref (default `false` in our workspace). Until that flips, the space is auto-selected via `best_interpolation_between`: sRGB for legacy-only stops, Oklab when any stop uses a modern color function (`oklab()`, `oklch()`, `lab()`, `lch()`, `color()`). The extraction pipeline is correct for both the auto path today and the explicit syntax once the pref flips. Tests: 4 collect-layer assertions (legacy sRGB, oklab() triggers Oklab, oklch() pair, radial inheritance) + 1 pixel probe confirming Skia renders the selected space differently from sRGB. 1 new L0 fixture.
Preserve the raw unit (Px vs Percent) of each gradient stop through extraction by tagging `GradientStop` with an `offset_is_px` flag, and resolve px offsets against the gradient-line length at paint time: `2 * half_len` for linear, `max(rx, ry)` for radial. Conic stops are always angular — no px path. Previously, px-positioned stops went through `position.to_percentage()` which returns `None`, then were auto-distributed to `0..100%`, causing repeating gradients with px stops to collapse to a single cycle spanning the whole box. `repeating-linear-gradient(90deg, red 0 20px, white 20px 40px)` now tiles correctly as 20px red / 20px white stripes. Also introduce a CSS-compliant stop-position clamp at paint time (each stop >= its predecessor). Non-decreasing is a Skia assertion in debug builds and a CSS spec requirement. Tests: 2 extraction (px and percent tagged correctly), 2 pixel probes (repeating px tile, mixed px+% paint). Fixture gains px variants.
Previously these five styles parsed correctly but fell back to `solid` at paint time. Implement them: - `double`: two parallel strokes at 1/3 width with a 1/3 gap between, offset perpendicular to the side's centerline. - `inset` / `groove`: top/left sides painted with the color darkened by 50% toward black; bottom/right lightened 50% toward white. - `outset` / `ridge`: inverse of inset (top/left lighter, bottom/right darker). Refactors `paint_borders` to iterate the four sides uniformly via a new `SidePos` enum and a per-side paint helper. `border_paint` removed (the logic moved to `paint_border_side`). Tests: 3 pixel probes (double three-band pattern, inset asymmetry, outset asymmetry). 1 new L0 fixture covering all 5 new variants.
Stylo's cascade already resolves logical longhands (`inline-size`, `block-size`, `inset-block-*`, `inset-inline-*`, `padding-block-*`, `padding-inline-*`, `margin-block-*`, `margin-inline-*`, `border-block-*`, `border-inline-*`, and their min/max variants) onto our physical extraction slots in the default LTR horizontal-tb writing mode — no code on our side had to do anything. Add 5 collect-layer tests pinning that behavior (sizing, min/max, inset, padding, border) so a future Stylo upgrade or local refactor doesn't silently drop the mapping, plus an L0 fixture documenting the support visually. Update the CSS Property Support tables in `docs/wg/feat-2d/htmlcss.md` to reflect the actual state (and fix a lingering "top/right/bottom/left: Stub in collect.rs" row left over from an earlier fix).
Update `docs/wg/feat-2d/htmlcss.md` status to reflect work since the last docs pass: - Background: size/position/repeat/clip/origin are now ✅ (landed in `0e38d5f2b`); gradient shape/size/position/conic-90° fix, px-unit stops in repeating gradients, currentcolor resolution, and the color-interpolation-method extraction (Stylo-pref-gated on explicit `in <space>` syntax) all noted. - Border: all 9 `border-style` variants paint (groove/ridge/inset/ outset/double), no longer falling back to solid. - Text Decoration: `text-decoration-color` and `-style` are extracted from Stylo (previously flagged as "field defined, not extracted"). - Text Shadow: `text-shadow` is ✅ (was ❌ "not in type schema"). - Transform: standalone `translate` / `rotate` / `scale` longhands ✅. No code changes.
Extract `object-position` from Stylo onto `ReplacedContent` and apply it in `paint_replaced` by overriding the box-fit transform's default (center) translation. Resolution mirrors `background-position`: - Px: raw offset from content box's top-left. - Percent: (box − fitted) × p, so 0% flushes left/top and 100% flushes right/bottom. Works uniformly across `object-fit` values: `fill` is size-invariant and stays unchanged; `contain`/`none`/`scale-down` slide the fitted image within the box; `cover` shifts which part of the oversized fitted image is visible. Tests: 3 collect-layer assertions (default center, keyword corners, px + %). 1 L0 fixture covering 7 position variants across fit modes (renders as placeholders via the golden runner's `NoImages` provider; feature is tested via the unit tests). Docs updated.
Extract the `filter:` chain from Stylo and compose it into a single Skia `ImageFilter` that gets set on the element's layer paint. The layer already exists for `opacity < 1`; filter reuses that path and adds itself to the trigger predicate. Supported functions: - `blur(<px>)` via `image_filters::blur` (Skia sigma = px / 2). - `brightness()` / `contrast()` / `grayscale()` / `hue-rotate()` / `invert()` / `opacity()` / `saturate()` / `sepia()` as 4×5 color matrices wrapped in `image_filters::color_filter`. Filters compose in source order as nested image filters, so `filter: grayscale(1) brightness(1.2) contrast(1.4)` applies left → right. `drop-shadow()` and `url()` (SVG filter refs) are not yet plumbed — their extraction returns None and the chain skips them. Tests: 3 collect-layer (single, chain, all 9 functions parse) + 3 pixel probes (grayscale equalizes RGB, invert black→white, hue-rotate 180° of red → cyan-ish with g≈b). 1 L0 fixture covering 13 variants. Docs updated.
Extend the `FilterFunction` enum with `DropShadow { offset_x, offset_y,
blur, color }` and wire it to `skia_safe::image_filters::drop_shadow`
in the existing filter-chain composer. Reuses Stylo's `SimpleShadow`
representation.
CSS blur radius maps to Skia Gaussian sigma (px / 2), matching the
existing text-shadow and blur() filter conventions. Composes with
other filters in source order — e.g. `filter: grayscale(1)
drop-shadow(4px 4px 2px #000)` applies grayscale to the source first,
then shadows the already-desaturated silhouette.
Tests: extraction of a `drop-shadow(4px 6px 2px #f00)` stop, plus a
pixel probe confirming the offset shadow region paints with the
requested color while the source still renders in place. Adds two
variants to the filter L0 fixture (sharp + soft drop-shadow). Docs
updated.
`url()` SVG filter refs remain the only unsupported filter function.
Add the `ClipPath` enum to `StyledElement` with four concrete shape variants — `inset()`, `circle()`, `ellipse()`, `polygon()` — extract them from Stylo, and apply the resolved clip inside `paint_box` via `canvas.clip_rrect` (inset with optional `round <radius>`) or `clip_path` built through `PathBuilder` (circle/ellipse/polygon). Positions and lengths resolve against the element's border box at paint time; `closest-side`/`farthest-side` on `circle()`/`ellipse()` use the appropriate axis distance. Polygon respects `nonzero` vs `evenodd` fill rules. The clip is popped inside the opacity/filter layer and outside the overflow clip so it participates in element-level stacking but is itself clipped by overflow when present. Out of scope: `url(#…)` SVG references, `path()`, `shape()`, and `<shape-box>`-only forms (all fall back to `None`). Tests: 4 extraction (none default, inset with sides+radius, circle position+radius, polygon points+fill) + 2 pixel probes (inset clips outer ring, circle clips corners). 1 L0 fixture covering 9 variants incl. star polygon. Docs updated.
Two concentric 1/3-width stroked rings separated by a 1/3-width gap, matching the `border-style: double` logic but on the single outline rect. Honors `outline-offset` and `border-radius` (radii are expanded for each ring at its own stroke-center distance via `draw_outline_ring`). Refactors the outline painter to extract `draw_outline_ring` — the solid/dashed/dotted path keeps the existing single-ring call, while `double` invokes it twice at different expansions. Groove/ridge/inset/outset still fall back to solid since the outline is a single rect (no natural per-side decomposition). Tests: one pixel probe scanning outer/gap/inner bands. Extended the L0 outline fixture with `double` and `double + border-radius`. Docs updated.
Expand the overflow clip rect outward by `overflow-clip-margin` when at least one axis uses `overflow: clip`. Per spec, `hidden`, `scroll`, and `auto` ignore the margin and continue to clip exactly at the box edge. Adds a new `overflow_clip_margin: f32` field on `StyledElement`, extracted from Stylo via `style.get_margin().clone_overflow_clip_margin()`. The paint-time clip-rect expansion is conditional on the overflow value so `hidden` semantics remain unchanged. Tests: 2 extraction (explicit px value; default 0) + 2 pixel probes (clip respects the expanded region; hidden still clips at the edge). 1 new L0 fixture covering 5 variants (visible / hidden / clip ± margin / hidden-ignores-margin).
…ontent) Extract three CSS grid alignment properties that were previously stubbed and pass them through to Taffy: - `justify-items` (container): per-cell inline-axis alignment. The default cascade already handled `place-items` — once `justify-items` was wired it propagates via the shorthand automatically. - `justify-self` (child): overrides the container's `justify-items` for a single grid cell. - `align-content` (container): track alignment along the block axis (grid) or cross-axis in flex when `flex-wrap: wrap` splits lines. Also wired `align-self` extraction which was declared on `StyledElement` but never populated from Stylo (previously always `None`). Two new helpers factor the `AlignFlags → AlignItems / JustifyContent` mappings that were inlined in the `align-items` / `justify-content` extraction blocks. Tests: 3 extraction (justify-items, justify-self, align-content) + 1 pixel probe (grid item centers in its cell with `justify-items: center; align-items: center`). 1 new L0 fixture covering 7 variants incl. `align-content: space-between` and per-child align-self/justify-self overrides.
Extract CSS `text-indent` from Stylo as a `CssLength` (preserving px vs percent) on `FontProps`, thread it onto `InlineGroup` and the Taffy `TextMeasure` context, and apply it by prepending a Skia `PlaceholderStyle` to the `ParagraphBuilder` before any glyphs. Because the placeholder sits before any text, Skia's line-breaker only shifts the first visual line — subsequent wrapped lines start at x=0, matching CSS first-line-only semantics. Percent values resolve against the paragraph's layout/available width at measure/paint time. Negative values (hanging indent) clamp to 0 for now since Skia placeholders don't support negative widths; proper hanging indent would need a different mechanism. The `hanging` and `each-line` keyword modifiers on `text-indent` are not honored — indent applies only to the first line of the first paragraph. Tests: 4 extraction (px, %, inherited through cascade, propagated to InlineGroup). 1 L0 fixture covering 5 variants.
Extract CSS `image-rendering` from Stylo as a new `ImageRendering` enum (Auto / CrispEdges / Pixelated) stored on `FontProps` so it inherits through the cascade. Map to Skia `SamplingOptions`: - `auto` → `SamplingOptions::default()` (bilinear filtering). - `pixelated` / `crisp-edges` → `FilterMode::Nearest` (hard pixel edges, ideal for upscaled pixel art). Applied in two paint sites: - `paint_replaced` for `<img>` via `draw_image_with_sampling_options`. - `paint_background_image_layer` for URL-backed background-image via the shader's sampling argument. Tests: 4 extraction (default auto, pixelated, crisp-edges, inherited through cascade). 1 L0 fixture comparing an upscaled checker image under both <img> and background-image with each rendering mode.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
6 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughExtends HTML+CSS pipeline across style collection, layout, paint, types, and fixtures: adds object-fit/object-position, text-indent, z-index ordering, filter and clip-path, overflow-clip-margin, richer background/gradient/image models, text-decoration/shadow, transform longhands, image-rendering, many tests and fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant HTML as "HTML Parser / DOM"
participant Collect as "Style Collector (collect.rs)"
participant Layout as "Layout Engine (layout.rs / Taffy)"
participant Paint as "Paint Pipeline (paint.rs)"
participant Skia as "Skia Renderer / ImageFilter"
HTML->>Collect: compute styles → build StyledElement (backgrounds, filters, clip-path, object-position, z-index, text_indent, etc.)
Collect->>Layout: emit layout IR and styled boxes (text runs with indent, inline groups, replaced content)
Layout->>Paint: provide box tree, painting order (z-index partitioning) and measured text metrics
Paint->>Skia: compose layers, apply ImageFilters, apply clip-paths, draw backgrounds/images/shadows using sampling rules
Skia-->>Paint: rasterized layer result
Paint-->>HTML: final rendered surface
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 9d58267ddc
ℹ️ 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
+2574
to
+2577
| f if f == AlignFlags::SPACE_AROUND => types::JustifyContent::SpaceAround, | ||
| f if f == AlignFlags::SPACE_EVENLY => types::JustifyContent::SpaceEvenly, | ||
| _ => types::JustifyContent::Start, | ||
| } |
There was a problem hiding this comment.
This fallback maps every unhandled AlignFlags value (including normal/stretch) to JustifyContent::Start, so when align-content is not explicitly set, wrapped flex lines and grid tracks get packed to the start instead of using CSS’s default stretch-like behavior. In layouts with extra cross-axis space, this changes placement for many containers that previously relied on default alignment.
Useful? React with 👍 / 👎.
| // circle fall below a negative `start` under `Clamp` and collapse onto | ||
| // the first stop. For non-repeating conic the sweep still covers a full | ||
| // 360° cycle, so `Repeat` produces no visible tiling. | ||
| let gradient = make_gradient(&colors, &positions, TileMode::Repeat, grad.interpolation); |
There was a problem hiding this comment.
The shader is forced to TileMode::Repeat even when grad.repeating is false, which breaks non-repeating conic semantics whenever stop positions do not span the full [0,1] range. For example, gradients like conic-gradient(red 25%, blue 75%) should clamp before/after the stop range, but this path wraps colors around the circle.
Useful? React with 👍 / 👎.
Comment on lines
+2082
to
+2084
| let lp = |v: &style::values::computed::LengthPercentage| -> f32 { | ||
| v.to_length().map(|l| l.px()).unwrap_or(0.0) | ||
| }; |
There was a problem hiding this comment.
clip-path: inset(... round ...) loses percentage corner radii because this conversion only reads absolute lengths and falls back to 0.0 for percentages. As a result, valid values like round 50% render with collapsed (square) corners, so inset round clips are incorrect for responsive radius inputs.
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: 2
♻️ Duplicate comments (3)
crates/grida-canvas/src/htmlcss/collect.rs (2)
2079-2092:⚠️ Potential issue | 🟡 MinorKeep percentage inset-round radii through paint-time resolution.
The helper comment says percentages stay unresolved, but Lines 2090-2091 still turn percentage radii into
0.0.clip-path: inset(0 round 50%)therefore loses its rounded corners; store these radii as length/percentage values and resolve against the final inset clip rect in paint.🤖 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 2079 - 2092, The extract_border_radius_from_generic currently forces percentage radii to 0.0 via the closure lp, losing inset-round percentages like `inset(... round 50%)`; change the code to preserve percentage values instead of collapsing them to zero: make lp (or a new helper) return the actual style::values::computed::LengthPercentage (or a small enum/struct carrying Length vs Percentage) rather than f32, keep percentages intact when mapping GenericBorderRadius in extract_border_radius_from_generic, and update the CornerRadii construction to accept and store these LengthPercentage values so resolution against the final inset clip rect can happen at paint time (refer to extract_border_radius_from_generic and the lp helper).
1830-1836:⚠️ Potential issue | 🟠 MajorPreserve mixed px/% auto stops instead of interpolating raw numbers.
Lines 1873-1876 still interpolate
prevandnextnumerically and copyprev_is_px. For a run between10pxand100%, an auto stop becomes a bogus px value instead of being resolved after the gradient line length is known. Use a unit-preserving/calc-like representation for mixed-unit auto stops, or defer their distribution to paint time.Also applies to: 1840-1877
🤖 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 1830 - 1836, The current auto_distribute_stops_typed -> raw.into_iter().map(...) code converts mixed-unit auto stops into numeric px values by interpolating prev/next and copying prev_is_px, which loses unit information for runs like "10px ... 100%"; update the logic so mixed px/% auto stops are not resolved to raw numbers here but preserve unit information (e.g., keep a unit-preserving representation or mark the stop as deferred) — change the mapping that constructs GradientStop to carry unit metadata (instead of forcing offset= o.unwrap_or(0.0) with offset_is_px copied) and avoid numeric interpolation of prev/next in this pass (defer distribution to paint time or produce a calc-like/enum variant representing mixed-unit interpolation). Ensure the changes touch auto_distribute_stops_typed, the iterator mapping that builds GradientStop, and any code that reads offset_is_px/offset so the deferred/unit-preserving form is propagated correctly.crates/grida-canvas/src/htmlcss/layout.rs (1)
214-230:⚠️ Potential issue | 🟡 MinorHandle or explicitly reject negative
text-indent.
resolve_text_indent()can return negative px values, but Line 222 only applies positive indents; percentage indents are also clamped non-negative on Line 543. CSS allows negative hanging indents, so these values are silently lost unless layout/paint supports them or extraction rejects/documents the unsupported case.Also applies to: 537-545
🤖 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 214 - 230, The code currently ignores negative text-indent values; update the logic around resolve_text_indent(...) and indent_px so negative indents are not silently dropped: if indent_px < 0.0 either implement hanging-indent support (apply the negative offset to subsequent lines via the paragraph/line offset APIs on the builder instead of adding a positive placeholder) or explicitly reject the case by returning/propagating an unsupported-style error (e.g., return an Err/UnsupportedIndent from the surrounding layout function) so callers know negative text-indent isn’t supported; reference resolve_text_indent, indent_px, builder.add_placeholder, and AvailableSpace::Definite/known_dimensions.width when locating where to add the check and handling.
🧹 Nitpick comments (1)
crates/grida-canvas/src/htmlcss/paint.rs (1)
1191-1198:repeat_scaleuses max(position) as the cycle, but CSS repeat period islast − first.Per CSS Images 3 §3.5, the repeat length for
repeating-*-gradientis the distance between the first and last color-stop along the gradient line. When the first stop is not at 0 (e.g.repeating-linear-gradient(red 25%, blue 75%)),cycle = last = 0.75produces a half-period that starts at position 0 of each tile with the first stop mapped to0.25 / 0.75 ≈ 0.33, instead of a 50%-wide tile whose boundary aligns with the first stop.For the common authoring case (
first == 0) the output looks right, which may be why golden tests still pass. Something to consider as a follow-up:let first = *positions.first().unwrap_or(&0.0); let last = *positions.last().unwrap_or(&1.0); let cycle = (last - first).max(1e-6); let scaled = positions.iter().map(|p| (p - first) / cycle).collect();…and shift the gradient start point (
p1, radial origin, or conicstartangle) byfirst * line_length/first * 360°so the pattern still begins at the first stop. Not a blocker for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/paint.rs` around lines 1191 - 1198, The function repeat_scale incorrectly uses the maximum position as the repeat cycle; change it to compute cycle = last - first (where first = positions.first().unwrap_or(&0.0) and last = positions.last().unwrap_or(&1.0)) with a min of 1e-6, then normalize positions as (p - first) / cycle instead of p / cycle so repeating gradients align their tile boundaries with the first and last stops; keep the signature and return (scaled, cycle) and, as a follow-up, ensure callers that position the gradient origin/angle (p1, radial origin, conic start) are shifted by first * line_length or first * 360° so the pattern begins at the original first stop.
🤖 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/paint.rs`:
- Around line 907-1102: The problem is build_filter_chain consumes/loses the
current chain by calling chain.take() inside each match arm so if a sub-filter
returns None (e.g. Blur with sigma<=0 or any image_filters::*/color_filters::*
returning None) the accumulated chain is wiped out; fix by not taking/consuming
chain when building the next filter—pass a non-consuming reference/clone of the
current chain (e.g. chain.as_ref().cloned() or clone the ImageFilter) into
image_filters::blur / image_filters::color_filter / image_filters::drop_shadow
calls and then update the accumulator with chain = next.or(chain) (i.e. only
replace the chain when next.is_some()), so a None sub-filter becomes a no-op and
previously accumulated filters are preserved.
In `@crates/grida-canvas/src/htmlcss/style.rs`:
- Around line 193-196: The doc comment for the object_position field claims the
default is 50% 50% but BackgroundPosition::default() returns 0% 0%, so ensure
images don't accidentally get top-left positioning by either adding a dedicated
center factory on BackgroundPosition (e.g. BackgroundPosition::center()) and
using that when constructing ReplacedContent for <img>, or explicitly set
object_position to a 50%/50% BackgroundPosition in the image extraction path
(the ReplacedContent construction used by the <img> extractor); alternatively
add a short comment on the object_position field pointing to the exact extractor
that supplies the 50%/50% default so future maintainers know the real default.
---
Duplicate comments:
In `@crates/grida-canvas/src/htmlcss/collect.rs`:
- Around line 2079-2092: The extract_border_radius_from_generic currently forces
percentage radii to 0.0 via the closure lp, losing inset-round percentages like
`inset(... round 50%)`; change the code to preserve percentage values instead of
collapsing them to zero: make lp (or a new helper) return the actual
style::values::computed::LengthPercentage (or a small enum/struct carrying
Length vs Percentage) rather than f32, keep percentages intact when mapping
GenericBorderRadius in extract_border_radius_from_generic, and update the
CornerRadii construction to accept and store these LengthPercentage values so
resolution against the final inset clip rect can happen at paint time (refer to
extract_border_radius_from_generic and the lp helper).
- Around line 1830-1836: The current auto_distribute_stops_typed ->
raw.into_iter().map(...) code converts mixed-unit auto stops into numeric px
values by interpolating prev/next and copying prev_is_px, which loses unit
information for runs like "10px ... 100%"; update the logic so mixed px/% auto
stops are not resolved to raw numbers here but preserve unit information (e.g.,
keep a unit-preserving representation or mark the stop as deferred) — change the
mapping that constructs GradientStop to carry unit metadata (instead of forcing
offset= o.unwrap_or(0.0) with offset_is_px copied) and avoid numeric
interpolation of prev/next in this pass (defer distribution to paint time or
produce a calc-like/enum variant representing mixed-unit interpolation). Ensure
the changes touch auto_distribute_stops_typed, the iterator mapping that builds
GradientStop, and any code that reads offset_is_px/offset so the
deferred/unit-preserving form is propagated correctly.
In `@crates/grida-canvas/src/htmlcss/layout.rs`:
- Around line 214-230: The code currently ignores negative text-indent values;
update the logic around resolve_text_indent(...) and indent_px so negative
indents are not silently dropped: if indent_px < 0.0 either implement
hanging-indent support (apply the negative offset to subsequent lines via the
paragraph/line offset APIs on the builder instead of adding a positive
placeholder) or explicitly reject the case by returning/propagating an
unsupported-style error (e.g., return an Err/UnsupportedIndent from the
surrounding layout function) so callers know negative text-indent isn’t
supported; reference resolve_text_indent, indent_px, builder.add_placeholder,
and AvailableSpace::Definite/known_dimensions.width when locating where to add
the check and handling.
---
Nitpick comments:
In `@crates/grida-canvas/src/htmlcss/paint.rs`:
- Around line 1191-1198: The function repeat_scale incorrectly uses the maximum
position as the repeat cycle; change it to compute cycle = last - first (where
first = positions.first().unwrap_or(&0.0) and last =
positions.last().unwrap_or(&1.0)) with a min of 1e-6, then normalize positions
as (p - first) / cycle instead of p / cycle so repeating gradients align their
tile boundaries with the first and last stops; keep the signature and return
(scaled, cycle) and, as a follow-up, ensure callers that position the gradient
origin/angle (p1, radial origin, conic start) are shifted by first * line_length
or first * 360° so the pattern begins at the original first stop.
🪄 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: 91ebc191-b637-4dcd-849a-b22da75ad232
📒 Files selected for processing (6)
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.rsdocs/wg/feat-2d/htmlcss.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/wg/feat-2d/htmlcss.md
Further review feedback from the previous round: - Mixed-unit auto gradient stops (e.g. `red 10px, ..., blue 100%`) no longer try to linearly interpolate raw numeric values across px/% unit boundaries. Interior auto stops in a mixed-unit run now snap to the previous bookend's position, preserving CSS ordering without fabricating a `5.5px`-style halfway value. - `clip-path: inset(... round ...)` radii preserve px vs percent through a new `InsetCornerRadii` type; `apply_clip_path` resolves percentages against the inset clip rect's own width/height at paint time. `round 50%` now produces actual rounded corners. - `build_filter_chain` no longer discards the accumulated chain when a sub-filter returns `None` — `blur(0)` is treated as identity, and Skia factory-call failures fall back to the prior chain. - Add `BackgroundPosition::center()` factory for the `50% 50%` CSS initial value of `object-position`; the `<img>` extractor uses it instead of hand-writing `Percent(0.5)` literals, and the field docstring calls out the difference from `BackgroundPosition:: default()` (`0% 0%`, correct for `background-position`). - Remove the duplicate `paint-text-decoration.html` fixture — the existing `text-decoration-full.html` covers the same ground with more variants and matches the corpus naming convention. - Drop the `-30px (hanging)` case from `text-indent.html` since our current text-indent path only handles positive values.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
crates/grida-canvas/src/htmlcss/mod.rs (1)
3143-3157: Assert the background URL in this regression test.This test exercises
collect_image_urls, but it only verifies<img src>. Add an assertion forbg.pngso the changedBackgroundLayer::Image(...StyleImage::Url...)path is actually covered.🧪 Proposed test assertion
assert!( urls.contains(&"photo.jpg".to_string()), "Should find img src, got {:?}", urls ); - // Note: background-image url() goes through Stylo ComputedUrl resolution. - // With no base URL, Stylo may resolve "bg.png" to an absolute URL. - // The img src is from raw HTML attributes and is preserved as-is. + assert!( + urls.iter().any(|u| u == "bg.png" || u.ends_with("bg.png")), + "Should find background-image url, got {:?}", + urls + );🤖 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 3143 - 3157, The test using collect_image_urls only asserts the img src but misses the background-image URL; update the test in htmlcss::mod.rs to also assert that the collected URLs include "bg.png" so the BackgroundLayer::Image (StyleImage::Url) path is exercised—locate the test snippet where collect_image_urls(html) is called and add an assertion similar to the existing img check that urls.contains(&"bg.png".to_string()) with an informative message.
🤖 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 924-932: The border extraction currently calls
extract_border(style) without providing the element's resolved text color,
causing unresolved `currentcolor` to fall back to CGColor::BLACK; change the
call to pass the resolved color (el.color) into extract_border (e.g.
extract_border(style, el.color)) and update extract_border's
signature/implementation to accept a current_color parameter and use that when
resolving `currentcolor` values instead of defaulting to CGColor::BLACK; mirror
the pattern used by extract_border_image(style, current_color) to ensure border
colors correctly inherit el.color.
- Around line 1053-1076: The AlignFlags bitflags include modifier bits, so exact
equality checks fail when modifiers (e.g. SAFE) are present; replace the eight
`==` comparisons with `.contains(...)` checks so modifiers are masked out.
Specifically, in the `el.align_items` and `el.justify_content` matching blocks
use `as_flags.contains(AlignFlags::CENTER)`/`contains(AlignFlags::START)` etc.
instead of `as_flags == AlignFlags::CENTER` (and likewise for `js_flags`), and
update the helper functions `align_flags_to_items()` and
`align_flags_to_explicit_justify()` to test with
`flags.contains(AlignFlags::...)` rather than `flags == AlignFlags::...` so
values like `CENTER | SAFE` map correctly to Center.
In `@crates/grida-canvas/src/htmlcss/paint.rs`:
- Around line 1708-1718: The per-side border painting loop (using SidePos and
paint_border_side) ignores style.border_radius, causing borders to render
square; update the painting logic to respect border radii by: detect when all
four sides share identical width/style/color and use an RRect fast path that
applies style.border_radius to draw rounded outer/inner rects, otherwise compute
per-side rounded paths that inset/outset using style.border_radius for each
corner and feed those rounded side shapes into paint_border_side (or a new
helper) so per-side strokes match the rounded background; ensure the check for
uniform sides and the new RRect path are performed before the existing per-side
loop (references: SidePos, paint_border_side, style.border_radius,
b.top/bottom/left/right, w, h).
- Around line 2071-2080: The code currently ignores negative values from
resolve_text_indent by only adding a placeholder when indent_px > 0.0; instead
detect indent_px < 0.0 and apply a negative first-line horizontal shift to the
text run rather than dropping it: when indent_px < 0.0, do not call
builder.add_placeholder but adjust the starting x/origin used for the first line
of the run (the initial layout offset for the run produced by the same code
block) by adding indent_px (a negative offset) so the glyphs render hanging
left; keep the positive-path using
builder.add_placeholder(PlaceholderStyle::new(indent_px, ...)) for indent_px >
0.0 and make the same change at the other occurrence (the block around lines
2131-2151) so both code paths (resolve_text_indent → indent_px,
builder.add_placeholder, and the run's start offset) consistently handle
negative text-indent.
- Around line 148-157: The current code uses a single has_clip_axis and a single
margin applied to both axes, causing non-clipped axis to be expanded; change to
compute per-axis margins using style.overflow_x and style.overflow_y (e.g.,
margin_x = if overflow_x==Clip { style.overflow_clip_margin.max(0.0) } else {
0.0 } and similarly margin_y) and pass those into canvas.clip_rect via
Rect::from_xywh(-margin_x, -margin_y, w + margin_x * 2.0, h + margin_y * 2.0) so
only clipped axes get the extra margin.
- Around line 620-642: The inset_radii function currently uses a single shrink
closure that subtracts max(horizontal_inset, vertical_inset) from both radius
components, causing over-shrinking when insets are asymmetric; change it to
shrink each axis independently by subtracting horizontal insets from x radii and
vertical insets from y radii (e.g., use separate shrink_x/shrink_y or a closure
taking separate a and b for x/y) when constructing the four skia_safe::Point
values for r.tl_x/tl_y, r.tr_x/tr_y, r.br_x/br_y, and r.bl_x/bl_y so the x
component uses left_inset/right_inset and the y component uses
top_inset/bottom_inset.
- Around line 922-928: The blur sigma is being halved which makes CSS
blur/drop-shadow too weak; in FilterFunction::Blur replace sigma = (px *
0.5).max(0.0) with sigma = px.max(0.0) and pass that sigma to
image_filters::blur(chain.clone(), ...) and in FilterFunction::DropShadow locate
the sigma computation (where px is multiplied by 0.5) and remove the 0.5 factor
so the computed sigma uses the CSS px directly before calling
image_filters::drop_shadow (and any related offset/blur args) to ensure CSS
blur() and drop-shadow() map 1:1 to Skia sigmas.
---
Nitpick comments:
In `@crates/grida-canvas/src/htmlcss/mod.rs`:
- Around line 3143-3157: The test using collect_image_urls only asserts the img
src but misses the background-image URL; update the test in htmlcss::mod.rs to
also assert that the collected URLs include "bg.png" so the
BackgroundLayer::Image (StyleImage::Url) path is exercised—locate the test
snippet where collect_image_urls(html) is called and add an assertion similar to
the existing img check that urls.contains(&"bg.png".to_string()) with an
informative message.
🪄 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: 0af5d264-6f54-40f4-aefa-818ba45b2e0d
📒 Files selected for processing (5)
crates/grida-canvas/src/htmlcss/collect.rscrates/grida-canvas/src/htmlcss/mod.rscrates/grida-canvas/src/htmlcss/paint.rscrates/grida-canvas/src/htmlcss/style.rsfixtures/test-html/L0/text-indent.html
✅ Files skipped from review due to trivial changes (1)
- fixtures/test-html/L0/text-indent.html
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/grida-canvas/src/htmlcss/style.rs
Comment on lines
+1053
to
+1076
| // justify-items: per-cell alignment on the inline axis for grid. | ||
| let ji_flags = style.clone_justify_items().computed.0.value(); | ||
| el.justify_items = align_flags_to_items(ji_flags); | ||
|
|
||
| // align-content: row-track alignment (grid) or cross-axis content | ||
| // alignment when flex-wrap splits lines. `normal`/`stretch` (the | ||
| // defaults) leave `None` so Taffy uses the layout-method-appropriate | ||
| // default (stretch behavior) instead of packing to start. | ||
| let ac_flags = style.clone_align_content().primary().value(); | ||
| el.align_content = align_flags_to_explicit_justify(ac_flags); | ||
| } | ||
|
|
||
| // Per-child align-self / justify-self. Non-auto values override the | ||
| // container's align-items / justify-items respectively. | ||
| { | ||
| use style::values::specified::align::AlignFlags; | ||
| let as_flags = style.clone_align_self().value(); | ||
| if as_flags != AlignFlags::AUTO { | ||
| el.align_self = Some(align_flags_to_items(as_flags)); | ||
| } | ||
| let js_flags = style.clone_justify_self().value(); | ||
| if js_flags != AlignFlags::AUTO { | ||
| el.justify_self = Some(align_flags_to_items(js_flags)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'AlignFlags::|align_flags_to_items|align_flags_to_explicit_justify' crates/grida-canvas/src/htmlcss/collect.rsRepository: gridaco/grida
Length of output: 3972
🏁 Script executed:
#!/bin/bash
# Find where AlignFlags is defined (likely in Stylo)
fd -t f '*.rs' | xargs rg -l 'enum AlignFlags|struct AlignFlags|bitflags.*AlignFlags' | head -20Repository: gridaco/grida
Length of output: 340
🏁 Script executed:
#!/bin/bash
# Check if AlignFlags uses bitflags and what flags it contains
rg -A 20 'pub.*flags.*AlignFlags|bitflags.*{' | grep -A 20 'AlignFlags' | head -50Repository: gridaco/grida
Length of output: 188
🏁 Script executed:
#!/bin/bash
# Search for safe/unsafe alignment in the codebase
rg -n 'safe.*center|unsafe.*center|AlignFlags.*SAFE|AlignFlags.*UNSAFE' crates/grida-canvas/Repository: gridaco/grida
Length of output: 346
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for align_flags_to_items or align_flags_to_explicit_justify
rg -n 'align_flags_to_items|align_flags_to_explicit_justify' crates/grida-canvas/ | grep -i testRepository: gridaco/grida
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search for AlignFlags definition in the codebase
rg -n 'AlignFlags' crates/grida-canvas/src/htmlcss/collect.rs | head -5Repository: gridaco/grida
Length of output: 478
🏁 Script executed:
#!/bin/bash
# Find what module AlignFlags comes from
rg 'use.*AlignFlags|from.*AlignFlags' crates/grida-canvas/src/htmlcss/collect.rsRepository: gridaco/grida
Length of output: 276
🏁 Script executed:
#!/bin/bash
# Check Stylo documentation or comments about AlignFlags
rg -B 5 -A 15 'style::values::specified::align' crates/grida-canvas/src/htmlcss/collect.rs | head -40Repository: gridaco/grida
Length of output: 1739
🌐 Web query:
Stylo CSS alignment safe unsafe flags AlignFlags
💡 Result:
In Stylo (the Rust CSS engine used by Servo and Firefox), AlignFlags is a bitflags struct defined in style/values/specified/align.rs. It represents constants shared across CSS Box Alignment properties (https://drafts.csswg.org/css-align/). It is a #[repr(C)] pub struct AlignFlags(u8); using bitflags! macro. The lower 5 bits store the alignment keyword enumeration: - AUTO = 0 - NORMAL = 1 - START = 2 - END = 3 - FLEX_START = 4 - FLEX_END = 5 - CENTER = 6 - LEFT = 7 - RIGHT = 8 - BASELINE = 9 - LAST_BASELINE = 10 - STRETCH = 11 - SELF_START = 12 - SELF_END = 13 - SPACE_BETWEEN = 14 (inferred from pattern) - etc. Upper bits (FLAG_BITS = 0b11100000) store modifiers: - LEGACY = 1 << 5 - SAFE = 1 << 6 - UNSAFE = 1 << 7 (mutually exclusive with SAFE) Methods include value to get lower bits, flags for upper bits, with_value to update enumeration while preserving flags. Used in types like ItemPlacement(pub AlignFlags) for align-items/justify-items, SelfAlignment, etc. Supports CSS safe/unsafe keywords (e.g., align-items: safe center falls back to start on overflow). Corresponds to Gecko's StyleAlignFlags.
Citations:
- 1: https://doc.servo.org/src/style/values/specified/align.rs.html
- 2: https://doc.servo.org/style/values/specified/align/struct.ItemPlacement.html
- 3: https://doc.servo.org/layout/taffy/stylo_taffy/stylo/index.html
- 4: https://searchfox.org/firefox-main/source/layout/generic/CSSAlignUtils.h
- 5: https://docs.rs/stylo/latest/style/all.html
🏁 Script executed:
#!/bin/bash
# Check if AlignFlags implements contains() method by searching for bitflags usage patterns
rg -A 5 'flags\.contains' crates/grida-canvas/src/ | head -30Repository: gridaco/grida
Length of output: 520
🏁 Script executed:
#!/bin/bash
# Check if there are other bitflags types using contains() in the codebase for reference
rg 'bitflags\!|impl.*Flags' crates/grida-canvas/src/ | head -20Repository: gridaco/grida
Length of output: 153
🏁 Script executed:
#!/bin/bash
# Look for any CSS test fixtures or examples with "safe" or "unsafe" alignment
rg -i 'safe.*center|unsafe.*center|align.*safe|align.*unsafe' crates/grida-canvas/Repository: gridaco/grida
Length of output: 828
🏁 Script executed:
#!/bin/bash
# Check the actual Stylo dependency to see what version is used
rg '"style"|"stylo"' Cargo.toml | head -10Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Verify contains() is the standard bitflags method by checking the GradientFlags usage pattern
rg -B 2 -A 2 'GradientFlags::REPEATING' crates/grida-canvas/src/htmlcss/collect.rsRepository: gridaco/grida
Length of output: 240
🏁 Script executed:
#!/bin/bash
# Check Cargo.lock or Cargo.toml for stylo/style version
find . -name "Cargo.toml" -o -name "Cargo.lock" | xargs rg -l 'style|stylo' | head -5Repository: gridaco/grida
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Check if there are any integration or unit tests for the collect.rs alignment logic
fd -t f -name '*.rs' crates/grida-canvas/ | xargs rg -l 'align_items|align_content|justify_items' | grep -i testRepository: gridaco/grida
Length of output: 287
🏁 Script executed:
#!/bin/bash
# Search for test files that might cover alignment logic
find crates/grida-canvas -type f -name '*test*.rs' -o -name '*_test.rs'Repository: gridaco/grida
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Check for any tests in the tests directory
ls -la crates/grida-canvas/tests/ 2>/dev/null || echo "No tests directory"Repository: gridaco/grida
Length of output: 1944
🏁 Script executed:
#!/bin/bash
# Look for any mod tests or #[test] in collect.rs
rg '#\[test\]|mod tests' crates/grida-canvas/src/htmlcss/collect.rsRepository: gridaco/grida
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Verify the actual lines and functions affected - get the full align_flags_to_items function
rg -A 10 'fn align_flags_to_items' crates/grida-canvas/src/htmlcss/collect.rsRepository: gridaco/grida
Length of output: 600
🏁 Script executed:
#!/bin/bash
# Check if fbs_roundtrip.rs tests alignment properties
rg -i 'align' crates/grida-canvas/tests/fbs_roundtrip.rs | head -10Repository: gridaco/grida
Length of output: 485
🏁 Script executed:
#!/bin/bash
# Check if there are TypeScript roundtrip tests as mentioned in learnings
find . -path '*/grida-canvas-io/__tests__/*roundtrip*' 2>/dev/nullRepository: gridaco/grida
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Verify the second location (lines 2581-2612) mentioned in the review
rg -A 15 'fn align_flags_to_explicit_justify' crates/grida-canvas/src/htmlcss/collect.rsRepository: gridaco/grida
Length of output: 855
🏁 Script executed:
#!/bin/bash
# Verify one more time that all three locations are indeed using == comparison
echo "=== Lines 1017-1037 inline matches ===" && rg -A 20 'el.align_items = match ai_flags' crates/grida-canvas/src/htmlcss/collect.rs | head -25
echo ""
echo "=== Function align_flags_to_items ===" && rg -A 8 'fn align_flags_to_items' crates/grida-canvas/src/htmlcss/collect.rs
echo ""
echo "=== Function align_flags_to_explicit_justify ===" && rg -A 12 'fn align_flags_to_explicit_justify' crates/grida-canvas/src/htmlcss/collect.rsRepository: gridaco/grida
Length of output: 2727
Use contains() to mask alignment flags instead of exact equality comparisons.
Stylo's AlignFlags is a bitflags type where the lower 5 bits store the alignment keyword and upper bits store modifiers (safe/unsafe/legacy). Exact equality comparisons like f == AlignFlags::CENTER fail when modifiers are present—for example, CSS align-items: safe center produces CENTER | SAFE and falls through to the default Stretch case.
Replace all eight == comparisons with contains() checks in:
el.align_itemsmatch (lines 1019–1022)el.justify_contentmatch (lines 1029–1036)align_flags_to_items()function (lines 2584–2587)align_flags_to_explicit_justify()function (lines 2602–2609)
Proposed fix
fn align_flags_to_items(flags: style::values::specified::align::AlignFlags) -> types::AlignItems {
use style::values::specified::align::AlignFlags;
match flags {
- f if f == AlignFlags::CENTER => types::AlignItems::Center,
- f if f == AlignFlags::FLEX_START || f == AlignFlags::START => types::AlignItems::Start,
- f if f == AlignFlags::FLEX_END || f == AlignFlags::END => types::AlignItems::End,
- f if f == AlignFlags::BASELINE => types::AlignItems::Baseline,
+ f if f.contains(AlignFlags::CENTER) => types::AlignItems::Center,
+ f if f.contains(AlignFlags::FLEX_START) || f.contains(AlignFlags::START) => {
+ types::AlignItems::Start
+ }
+ f if f.contains(AlignFlags::FLEX_END) || f.contains(AlignFlags::END) => {
+ types::AlignItems::End
+ }
+ f if f.contains(AlignFlags::BASELINE) => types::AlignItems::Baseline,
_ => types::AlignItems::Stretch,
}
}
@@
use style::values::specified::align::AlignFlags;
match flags {
- f if f == AlignFlags::CENTER => Some(types::JustifyContent::Center),
- f if f == AlignFlags::FLEX_START || f == AlignFlags::START => {
+ f if f.contains(AlignFlags::CENTER) => Some(types::JustifyContent::Center),
+ f if f.contains(AlignFlags::FLEX_START) || f.contains(AlignFlags::START) => {
Some(types::JustifyContent::Start)
}
- f if f == AlignFlags::FLEX_END || f == AlignFlags::END => Some(types::JustifyContent::End),
- f if f == AlignFlags::SPACE_BETWEEN => Some(types::JustifyContent::SpaceBetween),
- f if f == AlignFlags::SPACE_AROUND => Some(types::JustifyContent::SpaceAround),
- f if f == AlignFlags::SPACE_EVENLY => Some(types::JustifyContent::SpaceEvenly),
+ f if f.contains(AlignFlags::FLEX_END) || f.contains(AlignFlags::END) => {
+ Some(types::JustifyContent::End)
+ }
+ f if f.contains(AlignFlags::SPACE_BETWEEN) => Some(types::JustifyContent::SpaceBetween),
+ f if f.contains(AlignFlags::SPACE_AROUND) => Some(types::JustifyContent::SpaceAround),
+ f if f.contains(AlignFlags::SPACE_EVENLY) => Some(types::JustifyContent::SpaceEvenly),
_ => 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 1053 - 1076, The
AlignFlags bitflags include modifier bits, so exact equality checks fail when
modifiers (e.g. SAFE) are present; replace the eight `==` comparisons with
`.contains(...)` checks so modifiers are masked out. Specifically, in the
`el.align_items` and `el.justify_content` matching blocks use
`as_flags.contains(AlignFlags::CENTER)`/`contains(AlignFlags::START)` etc.
instead of `as_flags == AlignFlags::CENTER` (and likewise for `js_flags`), and
update the helper functions `align_flags_to_items()` and
`align_flags_to_explicit_justify()` to test with
`flags.contains(AlignFlags::...)` rather than `flags == AlignFlags::...` so
values like `CENTER | SAFE` map correctly to Center.
Comment on lines
+2071
to
+2080
| let indent_px = super::layout::resolve_text_indent(run.font.text_indent, width); | ||
| if indent_px > 0.0 { | ||
| builder.add_placeholder(&textlayout::PlaceholderStyle::new( | ||
| indent_px, | ||
| 0.01, | ||
| textlayout::PlaceholderAlignment::Baseline, | ||
| textlayout::TextBaseline::Alphabetic, | ||
| 0.0, | ||
| )); | ||
| } |
There was a problem hiding this comment.
Handle negative text-indent instead of rendering it as zero.
Both text paths only emit the leading placeholder when indent_px > 0.0. Valid negative/hanging indents are silently dropped; either implement the negative first-line shift or clamp/mark it unsupported before it reaches paint.
Also applies to: 2131-2151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/htmlcss/paint.rs` around lines 2071 - 2080, The code
currently ignores negative values from resolve_text_indent by only adding a
placeholder when indent_px > 0.0; instead detect indent_px < 0.0 and apply a
negative first-line horizontal shift to the text run rather than dropping it:
when indent_px < 0.0, do not call builder.add_placeholder but adjust the
starting x/origin used for the first line of the run (the initial layout offset
for the run produced by the same code block) by adding indent_px (a negative
offset) so the glyphs render hanging left; keep the positive-path using
builder.add_placeholder(PlaceholderStyle::new(indent_px, ...)) for indent_px >
0.0 and make the same change at the other occurrence (the block around lines
2131-2151) so both code paths (resolve_text_indent → indent_px,
builder.add_placeholder, and the run's start offset) consistently handle
negative text-indent.
- CSS filter `blur()` / `drop-shadow()` / `text-shadow` / `box-shadow` blur lengths are Gaussian sigma values per the CSS Filter Effects and Backgrounds specs, not "radii" that need halving. Removed the `* 0.5` / `/ 2.0` halving at all four sites. Visibly stronger blurs in the golden renders now match Chromium/Firefox behavior. - Border painting: uniform borders with a non-zero `border-radius` now stroke a single RRect via a new `paint_uniform_rounded_border` fast path. `double` traces two concentric shrunk RRects. Mixed per-side widths/styles/colors still fall back to straight per-side lines (same limitation as before). - `overflow-clip-margin`: resolve and apply per axis instead of expanding both axes whenever either one is `clip`. For `overflow-x: clip; overflow-y: hidden` the clip rect now grows horizontally only, matching the spec's per-axis gating. - `inset_radii` (background padding/content-box rounded clip): shrink each corner's x component by the horizontal inset and y by the vertical inset, not by `max(a, b)` of both. Asymmetric border/ padding no longer over-shrinks the unaffected axis. - `extract_border`: thread `el.color` through so unresolved `currentcolor` border colors fall back to the element's text color instead of opaque black — matches the existing pattern for `extract_border_image` / `extract_filter` / `extract_font`. - `resolve_text_indent`: clamp negative values to 0 explicitly at the resolve site rather than silently dropping them further down. CSS hanging indent is still a known limitation. Review-suggested AlignFlags `contains()` change was not applied — the upstream keyword bits aren't independent (e.g. `SPACE_BETWEEN` bitwise-contains `CENTER`), so `==` against the keyword-only portion (returned by `AlignFlags::value()`, which masks `safe`/`unsafe`/ `legacy`) is the right comparison. Added a doc comment explaining this in both helpers.
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/paint.rs (1)
1144-1183:⚠️ Potential issue | 🟠 MajorNormalize gradient endpoints to [0.0, 1.0] range for consistency with conic gradients.
The
build_conic_gradient_shader()function already adds padding to clamp stops at endpoints (lines 1436–1451), butbuild_linear_gradient_shader()andbuild_radial_gradient_shader()pass positions directly to Skia without normalizing out-of-range values. While Skia handles this gracefully, adopting the same endpoint normalization pattern across all gradient types would improve consistency and explicit control.For non-repeating linear and radial gradients with stops outside [0.0, 1.0] (e.g.,
linear-gradient(red -10px, blue 20px)), add the first color at position 0.0 and the last color at position 1.0, mirroring the conic implementation.Also applies to: 1333–1369, 1371–1406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/paint.rs` around lines 1144 - 1183, The gradient positions for linear/radial shaders must be normalized to [0.0,1.0] like conic gradients; update the code that builds/passes positions (either by enhancing build_gradient_data_with_line_length or in build_linear_gradient_shader and build_radial_gradient_shader) so that for non-repeating gradients you: convert px offsets as currently done, then if the first position > 0.0 insert the first stop color at position 0.0, and if the last position < 1.0 append the last stop color at position 1.0; ensure both the colors Vec and positions Vec are updated in sync before creating the Skia shader so endpoints are explicitly clamped to 0.0 and 1.0.
🤖 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 1730-1738: The helper length_percentage_to_css currently maps
mixed LengthPercentage (e.g., calc(100% - 10px)) to CssLength::Auto which
collapses to 0; update it to preserve mixed values by adding a new enum variant
(e.g., CssLength::Calc { percent: f32, px: f32 } or similar) and return that
variant from length_percentage_to_css when both lp.to_length() and
lp.to_percentage() are present; ensure the new variant is propagated through the
pipeline for later resolution at paint/layout time (update any match arms
handling CssLength in functions that assume Auto/Percent/Px so they accept the
new Calc variant and defer evaluation until painting).
In `@crates/grida-canvas/src/htmlcss/paint.rs`:
- Around line 108-139: The clip-path is currently applied after the
opacity/filter layer is created so filters can bleed outside the clip; move the
clip-path save/apply (canvas.save() and apply_clip_path(...)) to before creating
the layer (before canvas.save_layer(...)) so Skia composites the filtered layer
through the active clip; update both occurrences (the block using layer_paint
and canvas.save_layer and the similar block around lines ~280-290) and ensure
the corresponding canvas.restore() calls still match the new save order.
---
Outside diff comments:
In `@crates/grida-canvas/src/htmlcss/paint.rs`:
- Around line 1144-1183: The gradient positions for linear/radial shaders must
be normalized to [0.0,1.0] like conic gradients; update the code that
builds/passes positions (either by enhancing
build_gradient_data_with_line_length or in build_linear_gradient_shader and
build_radial_gradient_shader) so that for non-repeating gradients you: convert
px offsets as currently done, then if the first position > 0.0 insert the first
stop color at position 0.0, and if the last position < 1.0 append the last stop
color at position 1.0; ensure both the colors Vec and positions Vec are updated
in sync before creating the Skia shader so endpoints are explicitly clamped to
0.0 and 1.0.
🪄 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: 94498e03-8ddf-43dc-ad06-7cc1ff1aa495
📒 Files selected for processing (3)
crates/grida-canvas/src/htmlcss/collect.rscrates/grida-canvas/src/htmlcss/layout.rscrates/grida-canvas/src/htmlcss/paint.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/grida-canvas/src/htmlcss/layout.rs
Two AI review fixes:
- Move `apply_clip_path` outside `save_layer`. Previously the filter
layer was created around the source, then clipped on restore, so the
filter output (blur halos, drop-shadow extents) bled past the clip
shape. Clip-path now wraps the entire opacity/filter layer so all
rendering — including filter output and outline — is shaped by it.
- Preserve mixed `calc()` length-percentage values through extraction
instead of collapsing to `Auto`. Adds `CssLength::Calc { px, percent }`
and decomposes Stylo `LengthPercentage` calc by probing `resolve()` at
two known bases. Paint-time resolvers (`background-size`,
`background-position`, gradient centers, `clip-path` insets, shape
radii, `text-indent`) resolve against the known container basis; Taffy
has no calc variant so mixed values fall back to the dominant term.
This was referenced Apr 22, 2026
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
23 commits filling out
crates/grida-canvas/src/htmlcss/— the HTML+CSS embed renderer. Each commit is a scoped feature with its own L0 fixture, collect-layer tests, and (where feasible) pixel probes. Total delta: +4 tests/iter avg, 758 → 762 cg unit tests, zero regressions.What landed
Paint features
from <angle> at <position>on conic, px-unit stops in repeating gradients,color-interpolation-method(Oklab / sRGB auto-selected; explicitin <space>syntax pref-gated in Stylo),currentcolorin stops, conic 90° rotation fix.background-size/position/repeat/clip/originwith per-layer cycling andborder-radius-aware clipping.blur()+ 8 color-matrix functions (brightness / contrast / grayscale / hue-rotate / invert / opacity / saturate / sepia) +drop-shadow(), composed in source order via SkiaImageFilter.inset()(incl.round <radius>),circle(),ellipse(),polygon()(nonzero + evenodd), resolved against the border box.TextStyle::add_shadow.-colorand-style(solid/double/dotted/dashed/wavy) now extracted and applied.auto→ bilinear,pixelated/crisp-edges→ nearest.Layout features
justify-items/justify-self/align-content/align-selfall plumbed through Taffy;place-*shorthands cascade automatically.translate/rotate/scalelonghands, applied before thetransformshorthand per spec.overflow: clip(spec-compliant carve-out forhidden).object-fitmodes.top/right/bottom/leftproperly extracted (was stubbed); logicalinset-block-*/inset-inline-*verified through Stylo cascade.inline-size/block-size/border-block-*/etc. pinned with tests.Docs
Status tables in⚠️ → ✅.
docs/wg/feat-2d/htmlcss.mdsynced — ~20 rows flipped from ❌/Known blockers (not in this PR)
word-break/overflow-wrap— Skia Paragraph has no per-element break knobs.min-content/max-content/fit-contentsizing — TaffyDimensiondoesn't expose these for width/height (grid tracks only).backdrop-filter,text-overflow: ellipsis, explicitin <colorspace>gradient syntax — all three gated behind Stylolayout.unimplementedpref that defaults to off in ourstylo_static_prefslib.filter: url(#…)SVG ref,clip-path: url()/path()/shape(), hanging (negative)text-indent,order(Taffy doesn't honor it), outline groove/ridge/inset/outset.Test plan
cargo test -p cg— 762 passing, 0 failurescargo check -p cg -p grida-canvas-wasm -p grida-dev— cleancargo run -p cg --example golden_htmlcss— all L0 fixtures renderVisual proof
Hero renders staged at `/tmp/htmlcss-pr-proofs/` on the dev machine (9 PNGs covering gradients, backgrounds, filter chain, clip-path shapes, border variants, text-shadow, grid alignment, transforms, image-rendering) — drag-drop any into this PR body for inline preview.
Per-commit fixtures:
e06546ae4layout-position-absolute.html,layout-position-relative.html69000c149paint-gradient-radial.html,paint-gradient-conic.html,paint-gradient-repeating.html0e38d5f2bpaint-background-size.html,paint-background-position.html,paint-background-repeat.html,paint-background-clip-origin.html82fd657e3paint-text-shadow.html,paint-text-decoration.htmla6127f161transform-individual-props.htmlc7fd92317layout-z-index.html396f40172eb8837107paint-gradient-interpolation.html464637dd0paint-gradient-repeating.html(updated)0c9288181paint-border-style-variants.htmlb8da3b1a1layout-logical-properties.html5bd8f3410paint-object-position.htmled23c7047paint-filter.html08a423708paint-filter.html(extended)124fd9eccpaint-clip-path.html34c0a481apaint-outline.html(extended)db31cac6alayout-overflow-clip-margin.htmle946da779layout-grid-alignment.html9ab39c7dbtext-indent.html9d58267ddpaint-image-rendering.htmlSummary by CodeRabbit