LibWeb: Render list markers through generated content - #11017
Merged
kalenikaliaksandr merged 3 commits intoAug 6, 2026
Merged
Conversation
📝 WalkthroughWalkthroughList markers now resolve images or generated text and use standard inline layout. Legacy marker facts and painting code are removed. Filled ellipse display-list commands are removed, and the scroll indicator uses rounded-rectangle filling. ChangesList marker migration
Ellipse display-list cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TreeBuilder
participant ListItemMarkerBox
participant RustLayout
participant PaintableWithLines
TreeBuilder->>ListItemMarkerBox: resolve text or list-style image
ListItemMarkerBox->>RustLayout: provide inline marker child
RustLayout->>PaintableWithLines: produce marker geometry
PaintableWithLines-->>TreeBuilder: expose painted marker output
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Libraries/LibWeb/Layout/TreeBuilder.cpp`:
- Around line 331-334: Update the marker handling around list_style_image() so
it does not return image content until the image has successfully decoded; when
GeneratedContentImageProvider::decoded_image_data() is unavailable or null,
continue to the existing list-style-type/counter-text generation path. Add a
layout test using an invalid list-style-image URL and verify the list-style-type
marker remains rendered.
In `@Libraries/LibWeb/Rust/src/layout/block_formatting_context.rs`:
- Line 1742: Update the float-intrusion estimate near estimated_block_size to
use the laid-out marker’s actual block geometry and baseline, matching the final
placement logic around marker positioning, rather than
normal_line_height(marker_style). Ensure natural-size image markers and
non-default line heights produce a conservative estimate that includes floats
between estimated and final marker positions, and add a regression covering an
image marker with a floated list-item child.
🪄 Autofix
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 Plus
Run ID: 5b2573ab-2d78-4eb8-89ed-f14399e185c2
⛔ Files ignored due to path filters (1)
Tests/LibWeb/Screenshot/expected/details-open-then-closed.pngis excluded by!**/*.png
📒 Files selected for processing (42)
Libraries/LibWeb/CMakeLists.txtLibraries/LibWeb/Layout/LayoutRustBridge.cppLibraries/LibWeb/Layout/ListItemMarkerBox.cppLibraries/LibWeb/Layout/ListItemMarkerBox.hLibraries/LibWeb/Layout/TreeBuilder.cppLibraries/LibWeb/Painting/DisplayListCommand.cppLibraries/LibWeb/Painting/DisplayListCommand.hLibraries/LibWeb/Painting/DisplayListPlayerSkia.cppLibraries/LibWeb/Painting/DisplayListRecorder.cppLibraries/LibWeb/Painting/DisplayListRecorder.hLibraries/LibWeb/Painting/MarkerPaintable.cppLibraries/LibWeb/Painting/MarkerPaintable.hLibraries/LibWeb/Painting/Paintable.cppLibraries/LibWeb/Rust/src/layout/block_formatting_context.rsLibraries/LibWeb/Rust/src/layout/inline_formatting_context.rsLibraries/LibWeb/Rust/src/layout/layout_state.rsLibraries/LibWeb/Rust/src/layout/node_facts.rsTests/LibWeb/Layout/expected/block-and-inline/list-markers-intruded-by-float.txtTests/LibWeb/Layout/expected/block-in-inline-in-floated-fieldset.txtTests/LibWeb/Layout/expected/css-pseudo-element-display-list-item.txtTests/LibWeb/Layout/expected/details-closed.txtTests/LibWeb/Layout/expected/details-open.txtTests/LibWeb/Layout/expected/details-summary-default-ua-style.txtTests/LibWeb/Layout/expected/inside-list-item-content-offset.txtTests/LibWeb/Layout/expected/list-item-marker-content-height.txtTests/LibWeb/Layout/expected/list-item-marker-pseudo-placement.txtTests/LibWeb/Layout/expected/list-item-specified-height-not-floored-by-marker.txtTests/LibWeb/Layout/expected/list-item-with-floated-child.txtTests/LibWeb/Layout/expected/list-marker-in-abspos-list-item.txtTests/LibWeb/Layout/expected/list-marker-in-flex-item-list-item.txtTests/LibWeb/Layout/expected/list-marker-in-floating-list-item.txtTests/LibWeb/Layout/expected/list-marker-in-flow-root-and-overflow-hidden-list-item.txtTests/LibWeb/Layout/expected/list-marker-rtl.txtTests/LibWeb/Layout/expected/list-render-no-list.txtTests/LibWeb/Layout/expected/list-style-image-marker.txtTests/LibWeb/Layout/expected/pseudo-element-content-replacement-list-item.txtTests/LibWeb/Layout/expected/ul-render.txtTests/LibWeb/Layout/input/list-render-list-owner-is-document.htmlTests/LibWeb/Layout/input/list-render-list-owner-not-ol.htmlTests/LibWeb/Layout/input/list-style-image-marker.htmlTests/LibWeb/Layout/input/ol-render-item-values.htmlTests/LibWeb/Layout/input/ul-render.html
💤 Files with no reviewable changes (12)
- Libraries/LibWeb/CMakeLists.txt
- Libraries/LibWeb/Painting/MarkerPaintable.h
- Libraries/LibWeb/Painting/DisplayListRecorder.h
- Libraries/LibWeb/Painting/DisplayListPlayerSkia.cpp
- Libraries/LibWeb/Rust/src/layout/inline_formatting_context.rs
- Libraries/LibWeb/Painting/MarkerPaintable.cpp
- Libraries/LibWeb/Layout/ListItemMarkerBox.cpp
- Libraries/LibWeb/Layout/ListItemMarkerBox.h
- Libraries/LibWeb/Painting/DisplayListCommand.cpp
- Libraries/LibWeb/Painting/DisplayListCommand.h
- Libraries/LibWeb/Rust/src/layout/layout_state.rs
- Libraries/LibWeb/Painting/DisplayListRecorder.cpp
Comment on lines
+331
to
+334
| if (auto const* list_style_image = marker.list_style_image()) { | ||
| content.data.append(NonnullRefPtr { const_cast<CSS::AbstractImageStyleValue&>(*list_style_image) }); | ||
| return content; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Retain the list-style-type fallback for unavailable images.
Line 331 selects list_style_image before Lines 381-385 load the resource. If the image fails to decode or load, GeneratedContentImageProvider::decoded_image_data() returns null and this marker has no generated counter text. The list item then renders without a marker instead of falling back to list-style-type.
Keep resolved counter text as a fallback for an unavailable list-style-image. Add a layout test with an invalid image URL.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Libraries/LibWeb/Layout/TreeBuilder.cpp` around lines 331 - 334, Update the
marker handling around list_style_image() so it does not return image content
until the image has successfully decoded; when
GeneratedContentImageProvider::decoded_image_data() is unavailable or null,
continue to the existing list-style-type/counter-text generation path. Add a
layout test using an invalid list-style-image URL and verify the list-style-type
marker remains rendered.
| } else { | ||
| normal_line_height(marker_style) | ||
| }; | ||
| let estimated_block_size = normal_line_height(marker_style); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use marker geometry when estimating float intrusion.
Line 1742 uses the marker font’s normal line height. Final placement at Lines 1448-1464 uses the measured marker block size and baseline. These values differ for natural-size images and non-default line heights.
If a float starts between the estimated and final marker positions, inline_space_used_before_children_formatted omits that intrusion. The outside marker can then overlap the float or use the wrong inline offset.
Derive this estimate from the laid-out marker geometry, or use a conservative range. Add a regression with a natural-size image marker and a floated list-item child.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Libraries/LibWeb/Rust/src/layout/block_formatting_context.rs` at line 1742,
Update the float-intrusion estimate near estimated_block_size to use the
laid-out marker’s actual block geometry and baseline, matching the final
placement logic around marker positioning, rather than
normal_line_height(marker_style). Ensure natural-size image markers and
non-default line heights produce a conservative estimate that includes floats
between estimated and final marker positions, and add a regression covering an
image marker with a floated list-item child.
Symbolic counter styles and list-style-image markers bypassed generated content and inline layout. Their custom painting, sizing, and spacing were coupled across C++ and Rust. This was redundant because Default.css already defines each symbolic style with the required glyph and suffix. Resolve normal marker content to either the list-style image or the counter-style text, then lay both out as inline generated content. Remove MarkerPaintable along with the symbolic sizing facts and layout branches. Inside and outside markers now share ordinary replaced or text layout, with their positions determined through baseline alignment. Rebaseline affected layout and screenshot expectations for the new glyph-based markers, then add coverage for natural-size list-style images as generated content in both inside and outside marker positions.
MarkerPaintable was the main user of FillEllipse. Once list markers moved to text rendering, only the middle-button scroll indicator emitted the command, leaving dedicated recorder and player plumbing for one use. Fill the indicator with the existing rounded-rectangle command, using half its diameter for every corner radius to produce the same circle. This lets us remove the command, recorder API, dumper, and Skia player.
List marker layout still fetched marker linkage and position through a dedicated C++ callback and per-pass Rust cache after marker content had become ordinary layout children, leaving the fact bundle redundant. Find each marker in its list item child tree and record inside position on its node data. Remove the C++ WeakPtr, callback, cache, and duplicate list-style fields from the marker box. A layout regression covers inside markers under anonymous wrappers and independently nested list markers.
kalenikaliaksandr
force-pushed
the
remove-symbolic-list-markers
branch
from
August 6, 2026 04:56
80b56ba to
9089430
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Libraries/LibWeb/Layout/ListItemMarkerBox.h`:
- Line 19: Restore access to the list-style image used by
TreeBuilder::resolve_normal_marker_content(): either retain a compatible
list_style_image() accessor on ListItemMarkerBox or update the resolver to read
the image from list_box.computed_values(). Ensure the existing
marker.list_style_image() call has a valid declared implementation and the C++
build succeeds.
🪄 Autofix
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 Plus
Run ID: fe15afb7-478f-4ad9-961d-71d8acaa4f8c
⛔ Files ignored due to path filters (1)
Tests/LibWeb/Screenshot/expected/details-open-then-closed.pngis excluded by!**/*.png
📒 Files selected for processing (49)
Libraries/LibWeb/CMakeLists.txtLibraries/LibWeb/Layout/LayoutRustBridge.cppLibraries/LibWeb/Layout/ListItemBox.cppLibraries/LibWeb/Layout/ListItemBox.hLibraries/LibWeb/Layout/ListItemMarkerBox.cppLibraries/LibWeb/Layout/ListItemMarkerBox.hLibraries/LibWeb/Layout/TreeBuilder.cppLibraries/LibWeb/Painting/DisplayListCommand.cppLibraries/LibWeb/Painting/DisplayListCommand.hLibraries/LibWeb/Painting/DisplayListPlayerSkia.cppLibraries/LibWeb/Painting/DisplayListRecorder.cppLibraries/LibWeb/Painting/DisplayListRecorder.hLibraries/LibWeb/Painting/MarkerPaintable.cppLibraries/LibWeb/Painting/MarkerPaintable.hLibraries/LibWeb/Painting/Paintable.cppLibraries/LibWeb/Rust/src/layout/block_formatting_context.rsLibraries/LibWeb/Rust/src/layout/formatting_context.rsLibraries/LibWeb/Rust/src/layout/inline_formatting_context.rsLibraries/LibWeb/Rust/src/layout/inline_level_iterator.rsLibraries/LibWeb/Rust/src/layout/layout_state.rsLibraries/LibWeb/Rust/src/layout/node_data.rsLibraries/LibWeb/Rust/src/layout/node_facts.rsTests/LibWeb/Layout/expected/block-and-inline/list-markers-intruded-by-float.txtTests/LibWeb/Layout/expected/block-in-inline-in-floated-fieldset.txtTests/LibWeb/Layout/expected/css-pseudo-element-display-list-item.txtTests/LibWeb/Layout/expected/details-closed.txtTests/LibWeb/Layout/expected/details-open.txtTests/LibWeb/Layout/expected/details-summary-default-ua-style.txtTests/LibWeb/Layout/expected/inside-list-item-content-offset.txtTests/LibWeb/Layout/expected/list-item-marker-content-height.txtTests/LibWeb/Layout/expected/list-item-marker-pseudo-placement.txtTests/LibWeb/Layout/expected/list-item-marker-tree-linkage.txtTests/LibWeb/Layout/expected/list-item-specified-height-not-floored-by-marker.txtTests/LibWeb/Layout/expected/list-item-with-floated-child.txtTests/LibWeb/Layout/expected/list-marker-in-abspos-list-item.txtTests/LibWeb/Layout/expected/list-marker-in-flex-item-list-item.txtTests/LibWeb/Layout/expected/list-marker-in-floating-list-item.txtTests/LibWeb/Layout/expected/list-marker-in-flow-root-and-overflow-hidden-list-item.txtTests/LibWeb/Layout/expected/list-marker-rtl.txtTests/LibWeb/Layout/expected/list-render-no-list.txtTests/LibWeb/Layout/expected/list-style-image-marker.txtTests/LibWeb/Layout/expected/pseudo-element-content-replacement-list-item.txtTests/LibWeb/Layout/expected/ul-render.txtTests/LibWeb/Layout/input/list-item-marker-tree-linkage.htmlTests/LibWeb/Layout/input/list-render-list-owner-is-document.htmlTests/LibWeb/Layout/input/list-render-list-owner-not-ol.htmlTests/LibWeb/Layout/input/list-style-image-marker.htmlTests/LibWeb/Layout/input/ol-render-item-values.htmlTests/LibWeb/Layout/input/ul-render.html
💤 Files with no reviewable changes (14)
- Libraries/LibWeb/Painting/MarkerPaintable.h
- Libraries/LibWeb/Rust/src/layout/inline_formatting_context.rs
- Libraries/LibWeb/Painting/DisplayListPlayerSkia.cpp
- Libraries/LibWeb/Painting/DisplayListRecorder.h
- Libraries/LibWeb/CMakeLists.txt
- Libraries/LibWeb/Painting/DisplayListCommand.cpp
- Libraries/LibWeb/Layout/ListItemBox.cpp
- Libraries/LibWeb/Painting/MarkerPaintable.cpp
- Libraries/LibWeb/Rust/src/layout/formatting_context.rs
- Libraries/LibWeb/Painting/DisplayListCommand.h
- Libraries/LibWeb/Layout/LayoutRustBridge.cpp
- Libraries/LibWeb/Rust/src/layout/node_facts.rs
- Libraries/LibWeb/Painting/DisplayListRecorder.cpp
- Libraries/LibWeb/Layout/ListItemBox.h
🚧 Files skipped from review as they are similar to previous changes (28)
- Tests/LibWeb/Layout/input/ul-render.html
- Libraries/LibWeb/Painting/Paintable.cpp
- Tests/LibWeb/Layout/expected/details-open.txt
- Tests/LibWeb/Layout/expected/inside-list-item-content-offset.txt
- Tests/LibWeb/Layout/input/list-render-list-owner-is-document.html
- Tests/LibWeb/Layout/expected/list-render-no-list.txt
- Tests/LibWeb/Layout/input/list-render-list-owner-not-ol.html
- Tests/LibWeb/Layout/expected/block-in-inline-in-floated-fieldset.txt
- Tests/LibWeb/Layout/input/list-style-image-marker.html
- Tests/LibWeb/Layout/expected/details-closed.txt
- Tests/LibWeb/Layout/expected/list-item-specified-height-not-floored-by-marker.txt
- Tests/LibWeb/Layout/expected/list-marker-in-abspos-list-item.txt
- Tests/LibWeb/Layout/expected/details-summary-default-ua-style.txt
- Tests/LibWeb/Layout/input/ol-render-item-values.html
- Tests/LibWeb/Layout/expected/list-item-marker-content-height.txt
- Tests/LibWeb/Layout/expected/list-marker-in-floating-list-item.txt
- Tests/LibWeb/Layout/expected/block-and-inline/list-markers-intruded-by-float.txt
- Tests/LibWeb/Layout/expected/list-marker-rtl.txt
- Tests/LibWeb/Layout/expected/ul-render.txt
- Tests/LibWeb/Layout/expected/pseudo-element-content-replacement-list-item.txt
- Tests/LibWeb/Layout/expected/list-style-image-marker.txt
- Tests/LibWeb/Layout/expected/list-item-marker-pseudo-placement.txt
- Tests/LibWeb/Layout/expected/list-marker-in-flex-item-list-item.txt
- Tests/LibWeb/Layout/expected/css-pseudo-element-display-list-item.txt
- Tests/LibWeb/Layout/expected/list-item-with-floated-child.txt
- Tests/LibWeb/Layout/expected/list-marker-in-flow-root-and-overflow-hidden-list-item.txt
- Libraries/LibWeb/Layout/TreeBuilder.cpp
- Libraries/LibWeb/Rust/src/layout/block_formatting_context.rs
| static bool counter_style_is_rendered_with_custom_image(RefPtr<CSS::CounterStyle const> const& counter_style); | ||
|
|
||
| explicit ListItemMarkerBox(DOM::Document&, CSS::ListStyleType, CSS::ListStylePosition, NonnullRefPtr<CSS::ComputedValues const>); | ||
| explicit ListItemMarkerBox(DOM::Document&, bool is_inside, NonnullRefPtr<CSS::ComputedValues const>); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Restore the list-style image source.
Line 19 removes the marker list-style API. Libraries/LibWeb/Layout/TreeBuilder.cpp:326 still calls marker.list_style_image(). That call has no declared target after this change and stops the C++ build. Read the image from list_box.computed_values() in resolve_normal_marker_content(), or retain a compatible marker accessor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Libraries/LibWeb/Layout/ListItemMarkerBox.h` at line 19, Restore access to
the list-style image used by TreeBuilder::resolve_normal_marker_content():
either retain a compatible list_style_image() accessor on ListItemMarkerBox or
update the resolver to read the image from list_box.computed_values(). Ensure
the existing marker.list_style_image() call has a valid declared implementation
and the C++ build succeeds.
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.
Symbolic counter styles and list-style-image markers bypassed generated
content and inline layout. Their custom painting, sizing, and spacing
were coupled across C++ and Rust. This was redundant because Default.css
already defines each symbolic style with the required glyph and suffix.
Resolve normal marker content to either the list-style image or the
counter-style text, then lay both out as inline generated content.
Remove MarkerPaintable along with the symbolic sizing facts and layout
branches. Inside and outside markers now share ordinary replaced or text
layout, with their positions determined through baseline alignment.
Rebaseline affected layout and screenshot expectations for the new
glyph-based markers, then add coverage for natural-size list-style
images as generated content in both inside and outside marker positions.