feat(htmlcss): background-repeat space/round, direction, list-item markers#677
Merged
softmarshmallow merged 5 commits intomainfrom Apr 22, 2026
Merged
feat(htmlcss): background-repeat space/round, direction, list-item markers#677softmarshmallow merged 5 commits intomainfrom
softmarshmallow merged 5 commits intomainfrom
Conversation
Previously both fell back to plain `repeat`. Now: - `round` scales tile size so an integer number fits exactly along each axis. - `space` distributes whitespace between edge-pinned copies; when fewer than two fit, falls back to no-repeat per spec. - Mixed per-axis modes (e.g. `space round`) supported via a draw-loop path triggered only when an axis is `space`; shader path remains for all other combinations. Adds L0 fixture with dimensions chosen to make offsets visible.
Three independent additions bundled together: - CSS `direction: ltr | rtl` — extracted from Stylo, threaded through FontProps/InlineGroup to Skia's ParagraphStyle for bidi reordering. Also resolves logical `text-align: start/end` against direction. - HTML `<ol type="1|a|A|i|I">` — reads the attribute, overrides the CSS list-style-type at marker generation. Routes around Stylo's servo build not parsing `list-style-type: lower-roman/upper-roman`. - HTML `<ol start="N">` + `<li value="N">` — initialize and override the list counter per HTML §4.4.5/§4.4.8 (negative starts allowed).
disc/circle/square were emitted as Unicode characters (•◦▪), leaving their visual size at the mercy of the system font's glyph metrics — ▪ in particular was noticeably larger than •. Now painted geometrically, matching Chromium's TextFragmentPainter::PaintSymbol: - new InlineRunItem::SymbolMarker variant carries kind + color + font_size - collect.rs splits marker output into Symbol(kind) vs Text(string) following Chromium's kSymbol/kLanguage category split - layout.rs reserves a Skia placeholder sized from the font (bullet ≈ ascent/3 ≈ font_size/4, with trailing inline-end gap) - paint.rs looks up the placeholder rect via get_rects_for_placeholders() and draws via draw_oval (filled for disc, 1px stroked for circle) or draw_rect (square) Ordinal markers (1./a./iv.) still flow through the text path per Chromium's kLanguage category. Also adds HTML <ul type="disc|circle|square"> attribute support, mirroring the prior <ol type> work.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
WalkthroughExtended list marker handling to support HTML Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: 15e9572579
ℹ️ 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
+320
to
+322
| if let Some(ov) = type_override { | ||
| marker_output_for_type(ov, ordinal) | ||
| } else { |
There was a problem hiding this comment.
<ol type> is present
This branch makes the HTML type attribute unconditionally override the computed list-style-type, so author CSS on ol/li (for example list-style-type: none or decimal) is ignored whenever type exists. That regresses browser behavior where type is only a presentational hint and CSS should still be able to override marker style, so mixed attribute+CSS lists will render the wrong markers.
Useful? React with 👍 / 👎.
Comment on lines
+576
to
+577
| if n < 2 { | ||
| return vec![0.0]; |
There was a problem hiding this comment.
space when only one tile fits
When background-repeat: space cannot fit at least two copies, this hard-codes the single tile offset to 0.0, which drops the requested background-position. In that one-tile case the axis should behave like no-repeat positioning, so layouts such as a 60px area with a 50px tile and background-position: 100% currently render at the start edge instead of the end edge.
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: 4
🧹 Nitpick comments (1)
fixtures/test-html/L0/list-ol-type.html (1)
76-88: Consider adding edge case coverage for Roman numerals and alpha ranges.The current fixture tests Roman numerals up to X (10), which covers basic conversion. However, the implementation in
crates/grida-canvas/src/htmlcss/collect.rs:153-180supports Roman numerals up to 3999 with subtractive notation (IV, IX, XL, XC, CD, CM) and handles the alpha fallback for ordinals beyond 26. Consider adding test cases for:
- Alpha types with ordinal > 26 (should fall back to numeric)
- Roman numerals with subtractive notation (e.g., 40=XL, 90=XC, 400=CD, 900=CM)
- Edge case: ordinal ≤ 0 for Roman numerals
This is optional refinement for more comprehensive edge-case validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fixtures/test-html/L0/list-ol-type.html` around lines 76 - 88, Add additional fixture cases alongside the existing type="I" ordered list to cover the reported edge cases: add an ordered list with type="a" where the start/ordinal exceeds 26 to verify the alpha->numeric fallback, add several type="I" lists with start values that exercise subtractive Roman numerals (e.g., 40→XL, 90→XC, 400→CD, 900→CM) and a case with start <= 0 to assert how non-positive ordinals are handled; these changes will exercise the Roman conversion logic in collect.rs (the Roman numeral conversion used by the HTML/CSS collector) and ensure the fallback behavior for alpha ordinals beyond 26 is tested.
🤖 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 269-290: The unordered-list branch in collect.rs initializes a
ListCounter with value 0 causing numeric list-style-types on <ul> to start at 0;
change the ListCounter instantiation in the "ul" || "menu" branch (the
Some(ListCounter { ... }) code) to initialize value: 1 and keep type_override
logic the same so numeric counters begin at 1 for <ul> elements.
- Around line 132-148: The alpha helper and decimal-leading-zero handling are
wrong: implement full CSS counter formatting for LowerAlpha/UpperAlpha by
converting ordinal into a base-26 alphabetic sequence (e.g., 27 -> "aa", 28 ->
"ab") using b'a' or b'A' as the base in the alpha function (replace the current
single-character logic in the closure alpha used by LowerAlpha/UpperAlpha), and
change the L::DecimalLeadingZero arm to render numbers with a leading zero for
values < 10 (e.g., format!("0{}.", ordinal)) while preserving normal decimal
formatting for others; update MarkerOutput::Text return values accordingly so
LowerAlpha/UpperAlpha call the new alpha sequence and DecimalLeadingZero outputs
"01. " etc.
In `@crates/grida-canvas/src/htmlcss/paint.rs`:
- Around line 551-580: space_axis_positions currently ignores
background-position by returning 0.0 when fewer than two tiles fit; update it to
accept the positioning offset and return that value so the "space" fallback
behaves like "no-repeat". Change the signature to space_axis_positions(area:
f32, tile: f32, pos: f32) -> Vec<f32>, update the call in axis_tile_positions to
pass pos (BackgroundRepeatKeyword::Space => space_axis_positions(area, tile,
pos)), and in space_axis_positions replace the n < 2 branch to return vec![pos]
instead of vec![0.0]; keep the existing spacing logic for n >= 2.
In `@crates/grida-canvas/src/htmlcss/style.rs`:
- Around line 278-284: The bullet_rect placement always anchors to
placeholder.left; update bullet_rect to accept a direction parameter (e.g., an
enum/boolean for RTL) and compute the horizontal anchor based on that direction:
for LTR use placeholder.left + gap_offset (current left + 1.0), for RTL compute
from placeholder.right minus the gap and the bullet width so the marker sits
between the text and the gap (e.g., x = placeholder.right - gap_offset -
bullet). Also update the caller in paint_inline_group to pass group.direction
into bullet_rect (or into the marker paint call) so the marker placement is
direction-aware.
---
Nitpick comments:
In `@fixtures/test-html/L0/list-ol-type.html`:
- Around line 76-88: Add additional fixture cases alongside the existing
type="I" ordered list to cover the reported edge cases: add an ordered list with
type="a" where the start/ordinal exceeds 26 to verify the alpha->numeric
fallback, add several type="I" lists with start values that exercise subtractive
Roman numerals (e.g., 40→XL, 90→XC, 400→CD, 900→CM) and a case with start <= 0
to assert how non-positive ordinals are handled; these changes will exercise the
Roman conversion logic in collect.rs (the Roman numeral conversion used by the
HTML/CSS collector) and ensure the fallback behavior for alpha ordinals beyond
26 is tested.
🪄 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: 7cc986ed-5faa-4b70-9251-609790a17a65
📒 Files selected for processing (12)
crates/grida-canvas/src/htmlcss/collect.rscrates/grida-canvas/src/htmlcss/layout.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/list-ol-start-value.htmlfixtures/test-html/L0/list-ol-type.htmlfixtures/test-html/L0/list-ul-type.htmlfixtures/test-html/L0/paint-background-repeat-space-round.htmlfixtures/test-html/L0/paint-background-repeat.htmlfixtures/test-html/L0/text-direction.html
Comment on lines
+278
to
+284
| pub fn bullet_rect(&self, placeholder: skia_safe::Rect) -> skia_safe::Rect { | ||
| let bullet = self.font_size * 0.25; | ||
| let cy = (placeholder.top + placeholder.bottom) * 0.5; | ||
| let left = placeholder.left + 1.0; | ||
| let top = cy - bullet * 0.5; | ||
| skia_safe::Rect::from_xywh(left, top, bullet, bullet) | ||
| } |
There was a problem hiding this comment.
Make symbol marker placement direction-aware.
Line 281 always anchors the bullet to the placeholder’s left edge. In RTL inline groups, that puts the gap on the outside edge instead of between the marker and the text.
Suggested direction-aware geometry
- pub fn bullet_rect(&self, placeholder: skia_safe::Rect) -> skia_safe::Rect {
+ pub fn bullet_rect(
+ &self,
+ placeholder: skia_safe::Rect,
+ direction: super::types::Direction,
+ ) -> skia_safe::Rect {
let bullet = self.font_size * 0.25;
let cy = (placeholder.top + placeholder.bottom) * 0.5;
- let left = placeholder.left + 1.0;
+ let left = match direction {
+ super::types::Direction::Ltr => placeholder.left + 1.0,
+ super::types::Direction::Rtl => placeholder.right - bullet - 1.0,
+ };
let top = cy - bullet * 0.5;
skia_safe::Rect::from_xywh(left, top, bullet, bullet)
}Also pass group.direction from the marker paint call in paint_inline_group.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/htmlcss/style.rs` around lines 278 - 284, The
bullet_rect placement always anchors to placeholder.left; update bullet_rect to
accept a direction parameter (e.g., an enum/boolean for RTL) and compute the
horizontal anchor based on that direction: for LTR use placeholder.left +
gap_offset (current left + 1.0), for RTL compute from placeholder.right minus
the gap and the bullet width so the marker sits between the text and the gap
(e.g., x = placeholder.right - gap_offset - bullet). Also update the caller in
paint_inline_group to pass group.direction into bullet_rect (or into the marker
paint call) so the marker placement is direction-aware.
Four correctness fixes flagged by automated review: - Base-26 alpha sequence for lower-alpha/upper-alpha past ordinal 26 (y, z, aa, ab, …). Previously fell back to numeric above 26, so <ol type="a" start="27"> rendered "27." instead of "aa.". - decimal-leading-zero pads to two digits (01., 02., …, 10., 11., …) matching CSS and browser behavior. Previously aliased to plain decimal. - <ul> counter seeds at 1 instead of 0, so <ul style="list-style-type: decimal"> renders items from 1. - background-repeat: space honors background-position when fewer than two tiles fit (the no-repeat fallback per CSS Backgrounds §3.4). - Bullet placement is direction-aware: anchored to the placeholder's inline-start edge, so RTL lists put the bullet on the right.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/grida-canvas/src/htmlcss/paint.rs (1)
671-717: Mixedrepeat+spacefalls off the shader fast path.When only one axis is
Space, both axes now use per-tiledraw_image_rect_with_sampling_options. For the companion axis beingRepeatwith a small tile relative to the clip region, this can emit a large number of draw calls where the previous shader-based path would have been a single clipped rect. Correctness is fine; just worth watching for pathological cases (e.g. small patterns withrepeat, space) — a future optimization could compose aRepeat-shader-per-row when only one axis isSpace.
🤖 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 75-115: The function comment for parse_stylo_list_style_type is
stale: it claims `lower-roman`/`upper-roman` are not recognized, but the
function already checks for "LowerRoman" and "UpperRoman"; update the docstring
to reflect that Roman list-style types are now handled (or if those match arms
are truly unnecessary, remove them). Locate parse_stylo_list_style_type and
either (a) change the comment to state that it also recognizes LowerRoman and
UpperRoman (mentioning the Debug string checks for "LowerRoman"/"UpperRoman"),
or (b) remove the two if branches for "LowerRoman"/"UpperRoman" and add a
clarifying comment if they were retained for defensive reasons.
🪄 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: 659429ab-654c-4417-84da-de92d672f487
📒 Files selected for processing (4)
crates/grida-canvas/src/htmlcss/collect.rscrates/grida-canvas/src/htmlcss/paint.rscrates/grida-canvas/src/htmlcss/style.rsfixtures/test-html/L0/list-ol-start-value.html
✅ Files skipped from review due to trivial changes (1)
- fixtures/test-html/L0/list-ol-start-value.html
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/grida-canvas/src/htmlcss/style.rs
Comment on lines
+75
to
+115
| /// Parse Stylo's generated `ListStyleType` enum into our typed | ||
| /// [`types::ListStyleType`] via `Debug` string matching. Stylo's | ||
| /// servo-mode enum is generated and not structurally accessible, so we | ||
| /// fall back to string inspection. Unrecognized keywords return `None` | ||
| /// and callers fall back to `disc`. Does not recognize `lower-roman` | ||
| /// or `upper-roman` — servo Stylo parses them as invalid. | ||
| fn parse_stylo_list_style_type<T: std::fmt::Debug>(lst: &T) -> Option<types::ListStyleType> { | ||
| use types::ListStyleType as L; | ||
| let debug = format!("{:?}", lst); | ||
|
|
||
| if debug.contains("None") { | ||
| return None; | ||
| return Some(L::None); | ||
| } | ||
|
|
||
| // Symbol markers (Chromium: ListStyleCategory::kSymbol) | ||
| if debug.contains("Disc") { | ||
| return Some("\u{2022} ".to_string()); // • | ||
| return Some(L::Disc); | ||
| } | ||
| if debug.contains("Circle") { | ||
| return Some("\u{25E6} ".to_string()); // ◦ | ||
| return Some(L::Circle); | ||
| } | ||
| if debug.contains("Square") { | ||
| return Some("\u{25AA} ".to_string()); // ▪ | ||
| return Some(L::Square); | ||
| } | ||
| if debug.contains("DecimalLeadingZero") { | ||
| return Some(L::DecimalLeadingZero); | ||
| } | ||
|
|
||
| // Ordinal markers (Chromium: ListStyleCategory::kLanguage) | ||
| if debug.contains("Decimal") { | ||
| return Some(format!("{}. ", ordinal)); | ||
| return Some(L::Decimal); | ||
| } | ||
| if debug.contains("LowerAlpha") { | ||
| if (1..=26).contains(&ordinal) { | ||
| let ch = (b'a' + (ordinal - 1) as u8) as char; | ||
| return Some(format!("{}. ", ch)); | ||
| } | ||
| return Some(format!("{}. ", ordinal)); | ||
| return Some(L::LowerAlpha); | ||
| } | ||
| if debug.contains("UpperAlpha") { | ||
| if (1..=26).contains(&ordinal) { | ||
| let ch = (b'A' + (ordinal - 1) as u8) as char; | ||
| return Some(format!("{}. ", ch)); | ||
| } | ||
| return Some(format!("{}. ", ordinal)); | ||
| return Some(L::UpperAlpha); | ||
| } | ||
| if debug.contains("LowerRoman") { | ||
| return Some(format!("{}. ", to_roman(ordinal).to_lowercase())); | ||
| return Some(L::LowerRoman); | ||
| } | ||
| if debug.contains("UpperRoman") { | ||
| return Some(format!("{}. ", to_roman(ordinal))); | ||
| return Some(L::UpperRoman); | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
Stale docstring: code contradicts the "does not recognize lower-roman/upper-roman" note.
The function-level comment says servo Stylo parses lower-roman/upper-roman as invalid and so this parser doesn't recognize them, but the body explicitly matches "LowerRoman" and "UpperRoman" in the Debug string. Either the comment is stale (and should be updated now that Roman cases are handled), or the two match arms are defensive/dead code (and should either be removed or have a note explaining when they fire). Right now a reader has to reconcile two contradictory signals.
Suggested comment cleanup
/// Parse Stylo's generated `ListStyleType` enum into our typed
/// [`types::ListStyleType`] via `Debug` string matching. Stylo's
/// servo-mode enum is generated and not structurally accessible, so we
/// fall back to string inspection. Unrecognized keywords return `None`
-/// and callers fall back to `disc`. Does not recognize `lower-roman`
-/// or `upper-roman` — servo Stylo parses them as invalid.
+/// and callers fall back to `disc`. `lower-roman`/`upper-roman` are
+/// matched here defensively; servo Stylo currently parses them as
+/// invalid so the HTML `<ol type="i|I">` override path
+/// (`marker_output_for_type`) is the normal way these styles are
+/// reached today.🤖 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 75 - 115, The
function comment for parse_stylo_list_style_type is stale: it claims
`lower-roman`/`upper-roman` are not recognized, but the function already checks
for "LowerRoman" and "UpperRoman"; update the docstring to reflect that Roman
list-style types are now handled (or if those match arms are truly unnecessary,
remove them). Locate parse_stylo_list_style_type and either (a) change the
comment to state that it also recognizes LowerRoman and UpperRoman (mentioning
the Debug string checks for "LowerRoman"/"UpperRoman"), or (b) remove the two if
branches for "LowerRoman"/"UpperRoman" and add a clarifying comment if they were
retained for defensive reasons.
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
Three independent additions to the HTML/CSS embed renderer (
crates/grida-canvas/src/htmlcss/), each commit self-contained:b1cc08e3f—background-repeat: spaceandround(previously fell back to plainrepeat)90abe699e— CSSdirection: ltr | rtl+ HTML<ol type/start>,<li value>attributes15e957257— Geometric list-item markers (disc/circle/square) mirroring Chromium'sTextFragmentPainter::PaintSymbolbackground-repeat: spaceandroundround: scales tile so an integer number fits exactly along each axis (n = max(1, round(area/tile)); new_tile = area/n).space: pins first/last to edges, distributes whitespace evenly between copies; falls back to no-repeat when fewer than two copies fit (per CSS Backgrounds §3.4).Direction + HTML ordered-list attributes
direction: ltr | rtl— extracted from Stylo, threaded throughFontProps/InlineGroupto Skia'sParagraphStyle. Enables correct bidi reordering of Arabic/Hebrew. Also resolves logicaltext-align: start/endagainst direction.HTML
<ol type="1|a|A|i|I">— reads the attribute, overrides CSSlist-style-typeat marker generation. Routes around Stylo's servo build not parsinglist-style-type: lower-roman/upper-roman.HTML
<ol start="N">+<li value="N">— initialize and override the list counter per HTML §4.4.5/§4.4.8 (negative starts allowed).New L0 fixtures: text-direction.html, list-ol-type.html, list-ol-start-value.html.
Geometric list-item markers (Chromium-style)
Previously
disc/circle/squaremarkers were emitted as Unicode characters (• ◦ ▪), leaving their visual size at the mercy of the system font's glyph metrics —▪in particular rendered noticeably larger than•, and◦was often indistinguishable from•.Now painted geometrically, mirroring Chromium's
TextFragmentPainter::PaintSymbol:InlineRunItem::SymbolMarkervariant carries{ kind, color, font_size }.collect.rssplits marker output intoSymbol(kind)vsText(string)following Chromium'skSymbol/kLanguagecategory split.layout.rsreserves a Skia placeholder sized from the font (bullet ≈ascent / 3≈font_size / 4, with trailing inline-end gap baked in).paint.rslooks up the placeholder rect viaget_rects_for_placeholders()and draws viadraw_oval(filled disc / 1px-stroked circle) ordraw_rect(filled square).Ordinal markers (
1.,a.,iv.) still flow through the text path per Chromium'skLanguagecategory. Also adds HTML<ul type="disc|circle|square">attribute support, mirroring the prior<ol type>work.New L0 fixture: list-ul-type.html.
What we do differently from Chromium
::markerpseudo-element — marker emitted as an inline item attached to the first content of the<li>(Chromium uses an actual layout pseudo-element).ascent ≈ 0.75 × font_size) rather than querying font metrics at collect time — ±20% error on unusual fonts, acceptable for static canvas embed.Doc updates
docs/wg/feat-2d/htmlcss.md:background-repeatdirection❌ → ✅lower-roman, upper-roman❌ →<ol type>)Test plan
cargo check -p cgcleancargo clippy -p cg --no-deps --libcleancargo fmt --allappliedcargo test -p cg --lib htmlcss— 149 passed, 0 failedcargo run -p cg --example golden_htmlcssVisuals
Summary by CodeRabbit
<ol>) now supporttypeandstartattributes, plus<li value>for custom numbering.<ul>) now supporttypeattribute for bullet styles (disc, circle, square).background-repeat: spaceandbackground-repeat: roundmodes now fully supported.directionproperty (ltr/rtl) now supported for text rendering.