LibWeb: Prepare Layout::TreeBuilder for a Rust port - #10824
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughThe PR adds shared CSS Rust FFI and display modules, refactors selector and style computation integration, reorganizes layout-tree construction and pseudo-element handling, and exposes layout-tree build confinement statistics through Internals with a covering text test. ChangesCSS Rust FFI and display integration
Layout-tree construction
Layout-tree diagnostics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LayoutTreeUpdateTest
participant Internals
participant Document
participant TreeBuilder
LayoutTreeUpdateTest->>Internals: call layoutTreeBuildStats()
Internals->>Document: read layout_tree_build_stats()
Document->>TreeBuilder: record rebuild results
TreeBuilder-->>Document: return rebuilt roots and escape status
Document-->>Internals: return statistics object
Internals-->>LayoutTreeUpdateTest: return diagnostic values
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
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: 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/CSS/Rust/src/display.rs`:
- Around line 31-32: Update the byte decoding in Display::from_raw to use
little-endian ordering via to_le_bytes instead of native-endian conversion,
matching the packing performed by encoded() and preserving correct variant
selection across architectures.
🪄 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 Plus
Run ID: 92d47071-366e-4656-8fa0-441039538de9
📒 Files selected for processing (18)
Libraries/LibWeb/CSS/Rust/build.rsLibraries/LibWeb/CSS/Rust/src/css_enums.rsLibraries/LibWeb/CSS/Rust/src/display.rsLibraries/LibWeb/CSS/Rust/src/ffi_support.rsLibraries/LibWeb/CSS/Rust/src/lib.rsLibraries/LibWeb/CSS/Rust/src/selector_engine.rsLibraries/LibWeb/CSS/Rust/src/style_compute.rsLibraries/LibWeb/DOM/Document.cppLibraries/LibWeb/DOM/Document.hLibraries/LibWeb/Internals/Internals.cppLibraries/LibWeb/Internals/Internals.hLibraries/LibWeb/Internals/Internals.idlLibraries/LibWeb/Layout/Node.cppLibraries/LibWeb/Layout/Node.hLibraries/LibWeb/Layout/TreeBuilder.cppLibraries/LibWeb/Layout/TreeBuilder.hTests/LibWeb/Text/expected/layout-tree-update/tree-build-confinement.txtTests/LibWeb/Text/input/layout-tree-update/tree-build-confinement.html
Move RetainedCxxPointer, FfiCallScope, the borrowed DOM string view types, and the UTF-16 comparison helpers into a new ffi_support module so upcoming Rust ports can use the C++/Rust boundary toolkit without reaching into the selector engine. Pure code motion; SelectorRustFFI.h now also generates from the new module so the DomStringView layout stays in the selector FFI namespace.
The Rust style computation carried a private FfiDisplay with only the predicates that box type transformation needed. Move it into its own display module, complete the predicate set to mirror CSS::Display (table internals, list-item, flow/flow-root/table/ruby inside), and add table and inline-table constructors, so upcoming ports that are display-predicate heavy get a shared, tested vocabulary. The generated keyword and enum constants move into a css_enums module that both style_compute and display use; style_compute re-exports them so existing paths keep resolving. New unit tests cover the encoding round-trip and every predicate.
The display:contents update path carried near-copies of the main walk's shadow root iteration, DOM child iteration, and assigned slottable handling, and the top layer detach helper had a third copy of the slottable cleanup loop. Extract each into a shared helper so the walk logic exists once. No behavior change.
TreeBuilder adjusted freshly created boxes through ad-hoc modify_computed_values lambdas in ten places, each rebuilding the computed values to change one or two properties. Add set_display, set_content, and set_overflow to NodeWithStyle and use them at those sites, so every computed-value adjustment during tree building is a named operation. No behavior change.
Pull four pieces of machinery out of the tree-building control flow into named helpers: list item marker creation and attachment (which existed twice, inlined), the first-letter DOM text slicing, the transfer of saved layout state from a replaced box to its in-place replacement, and the two button layout wrapper constructions. The walk logic now reads as tree restructuring built from named operations. No behavior change.
The rebuild-confinement report (which subtrees an incremental layout tree build rebuilt in place, and whether any mutation escaped them) feeds partial relayout eligibility, but nothing could assert on it, so a mistake there surfaces only as rare underinvalidation. Record the report on the document after every build and expose it through internals.layoutTreeBuildStats(). Add a test pinning the confinement behavior for the interesting mutation shapes: plain child insertion, batched independent mutations, display:none reveal, block-into-inline restructuring, display:contents subtrees, top layer entry and exit, shadow root attachment, and out-of-flow insertion. Of these only top layer entry escapes today; the test exists so any change to that answer is a deliberate one.
TreeBuilder resolved a pseudo-element's content property twice: once before creating the box, because the box-generation decision needs the content type, and again after resolve_counters() so counter() items see real values. But the type of the resolved content depends only on the shape of the computed value, which computed_content() already stores, so the existence checks can read that directly and the full resolution can happen exactly once, after the box is inserted and counters are resolved. This also stops a pseudo-element whose display type yields no layout node from consuming open-quote/close-quote nesting levels, since resolution no longer runs for boxes that never get created, and it removes the unreachable TODO() for the two resolutions disagreeing on type.
update_layout_tree decided and created the layout node for elements, documents, and text inline in one long branch, with the top layer ::backdrop handling and the lazy style recovery for bypass paths mixed into the surrounding flow. Extract each into a named function: create_layout_node_for_element (content replacement, SVG resource box overrides, and the element's own layout node factory), the text node creation with its display:contents style wrapper, the ::backdrop creation for top layer members, and the style update for elements reached through bypass paths. The walk now reads as its actual phases: decide, create, insert or replace, then children. No behavior change.
This was referenced Jul 23, 2026
This was referenced Jul 31, 2026
This was referenced Aug 10, 2026
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.
This is groundwork for rewriting Layout::TreeBuilder in Rust, following the same FFI approach as the selector engine and style computation: the DOM stays in C++, the tree building logic moves to Rust, and the existing C++ layout tree is built through opaque handles. No Rust tree builder yet; these commits reshape both sides so the port becomes a mechanical translation instead of a design problem.
On the Rust side, the boundary toolkit (RetainedCxxPointer, FfiCallScope, the borrowed DOM string views and UTF-16 helpers) moves out of the selector engine into a shared ffi_support module, and FfiDisplay moves into a shared display module that mirrors the complete CSS::Display predicate set (table internals, list-item, flow/flow-root/table/ruby inside) with unit tests for every predicate and the encoding round-trip. Tree building is display-predicate heavy, so this pre-tests the riskiest translation category.
On the C++ side, TreeBuilder is refactored so every operation a future bridge must expose is already a named function with an FFI-shaped signature:
Two changes go beyond code motion. Pseudo-element content is now resolved once, after counters, instead of twice: box existence is decided from computed_content().type, which is equivalent by construction since the computed style value for content is reconstructed from it. As a side effect, a pseudo-element whose display type yields no layout node no longer consumes open-quote/close-quote nesting levels. And the rebuild-confinement report that feeds partial relayout eligibility is now recorded per build and exposed as internals.layoutTreeBuildStats(), with a test pinning which mutation shapes stay confined and which escape (today only top layer entry escapes). Mistakes in that contract previously surfaced only as rare underinvalidation; now they fail a test.