LibWeb: Seal a box's committed metrics when it is placed - #11036
Merged
kalenikaliaksandr merged 4 commits intoAug 7, 2026
Conversation
Block-level boxes and grid items had their relative-position insets resolved by compute_inset() immediately after place_child(), so the inset fields of an already-placed box kept changing after placement. Every other caller (floats, flex items, inline-level boxes) already resolves insets before its boxes are placed, and nothing reads the insets between the two points, so the late resolution was invisible — but it blocks the placement-seal invariant where a placed box's committed metrics are final. Hoist the compute_inset() call above the placement in layout_block_level_box() and in the grid item placement loop. The call's inputs (style insets, the containing block's content sizes, the definiteness walk for percentage insets) are untouched by the statements it crosses in both functions, so committed geometry is unchanged, in measurement passes as well as committing ones.
A block-level box that interrupts an inline formatting context gets a line box of its own, and append_block_level_box() recorded that containing-line-box coordinate on the box after its placement, so the coordinate settled after the box was already placed. Commit is the only reader, so the late write was invisible today — but it blocks the placement-seal invariant where a placed box's committed metrics are final. The box's used values do not exist yet when the interrupting-block wrapper starts, so the writes cannot simply move there. Instead the wrapper reserves the line index up front (the freshly begun line the box will occupy), threads the full coordinate into layout_block_level_box(), which records it right before place_child(), and passes the same index into append_block_level_box(), which now asserts it matches the line it appends to instead of re-deriving and writing it. The reserved index always equals the previously derived one: the last line always exists and is fragment-empty at the wrapper's entry, and nothing during the block's own layout can mutate the outer containing block's line boxes.
A BFC run whose margin state ended nonzero used to overwrite the committed margin_bottom of its last real in-flow child with the run's trailing collapsed margin: an aggregate folding in the margins of every trailing collapse-through sibling and, through nested runs, of descendants' trailing margins. The aggregate only exists at run end, so the overwrite mutated a box that was already placed — the one committed metric that settled after placement — and the root's baselines had to be re-derived afterwards because the write moved that child's margin box. The trailing collapsed margin is now run output, stored on the formatting context together with the child it hangs below, and consumed only by the root's automatic block size, which still extends to the same bottom margin edge as before, so geometry is unchanged everywhere. Committed metrics and baseline derivation read the box's own margins, and the run-tail baseline re-derivation is gone. Observably, the last child of such a run now reports its own used margin-bottom, matching other engines, instead of the aggregate: 46 test expectations change, every one only in a box's reported margin-bottom (45 layout dumps and one getComputedStyle listing); no offset, size, or reference test changes.
"When is this box's geometry final?" used to have a different answer per field and per box class, and nothing checked any of them. Every consumer of placed geometry was correct only by global ordering arguments nobody could verify locally, and a production timing mistake would ship as a silent misrender. Establish the invariant that placement is the seal point: after place_child() returns, no field of that box's UsedValues that commit emits may change again. The sealed set is exactly the FfiCommittedBoxMetrics payload — content offset, content sizes, margins, borders, paddings, insets, and the containing-line-box coordinate. Working state commit never reads (definiteness flags, size constraints, baselines, the collapsing-borders flag) stays freely mutable, and rare data is a separate store with its own rules. A box materialized from a previous paintable is sealed at materialization, which is that box's placement. The seal point is placement, not the end of the box's inside layout: everything the engine finishes between those two moments (float root finalization, abspos alignment insets, table cell stretch) remains legal.
📝 WalkthroughWalkthroughBlock layout now seals committed geometry, preserves line-box fragment coordinates for interrupting blocks, and tracks trailing collapsed margins separately for root automatic block-size calculation. Layout expectations and computed-style expectations update accordingly. ChangesLayout metrics and margin propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Libraries/LibWeb/Rust/src/layout/block_formatting_context.rs (1)
2220-2228: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSkip list marker boxes in the trailing-margin scan.
self.children(self.root)includes list marker boxes. Non-floating list markers return beforecreate_used_values(). If this loop reaches one,margins_collapse_through()callsself.used(node)and panics.Add
facts.is_list_item_marker_box()to the skip condition. This also preserves the intended “last real in-flow child” selection.Proposed fix
- if facts.is_absolutely_positioned() || facts.is_floating() { + if facts.is_list_item_marker_box() || facts.is_absolutely_positioned() || facts.is_floating() { continue; }🤖 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` around lines 2220 - 2228, Update the skip condition in the trailing-margin scan over flow_children_bottom_up to also continue when facts.is_list_item_marker_box() is true. Keep the existing absolutely-positioned, floating, and margins-collapse checks unchanged so list marker boxes never reach margins_collapse_through().
🤖 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.
Outside diff comments:
In `@Libraries/LibWeb/Rust/src/layout/block_formatting_context.rs`:
- Around line 2220-2228: Update the skip condition in the trailing-margin scan
over flow_children_bottom_up to also continue when
facts.is_list_item_marker_box() is true. Keep the existing
absolutely-positioned, floating, and margins-collapse checks unchanged so list
marker boxes never reach margins_collapse_through().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 305033d4-52d2-498e-90b7-766ee21f606d
📒 Files selected for processing (52)
Libraries/LibWeb/Rust/src/layout/block_formatting_context.rsLibraries/LibWeb/Rust/src/layout/formatting_context.rsLibraries/LibWeb/Rust/src/layout/grid_formatting_context.rsLibraries/LibWeb/Rust/src/layout/layout_state.rsLibraries/LibWeb/Rust/src/layout/line_builder.rsLibraries/LibWeb/Rust/src/layout/used_values.rsTests/LibWeb/Layout/expected/abspos-inline-containing-block-first-last-line-rule.txtTests/LibWeb/Layout/expected/block-and-inline/box-with-clearance-and-margin-top.txtTests/LibWeb/Layout/expected/block-and-inline/float-clear-by-line-break-followed-by-block.txtTests/LibWeb/Layout/expected/block-and-inline/float-vertical-offset-by-preceding-float.txtTests/LibWeb/Layout/expected/block-and-inline/forced-break-stops-non-whitespace-sequence.txtTests/LibWeb/Layout/expected/block-and-inline/intrinsic-sizing-stress-test.txtTests/LibWeb/Layout/expected/block-and-inline/list-markers-intruded-by-float.txtTests/LibWeb/Layout/expected/block-and-inline/margin-collapse-1.txtTests/LibWeb/Layout/expected/block-and-inline/relpos-inline-element-js-offsets.txtTests/LibWeb/Layout/expected/block-size.txtTests/LibWeb/Layout/expected/css-counters/basic.txtTests/LibWeb/Layout/expected/css-counters/hidden-elements.txtTests/LibWeb/Layout/expected/css-namespace-rule-matches.txtTests/LibWeb/Layout/expected/css-namespace-rule-no-match.txtTests/LibWeb/Layout/expected/css-text-transform-math-auto.txtTests/LibWeb/Layout/expected/css/content-for-marker-in-list.txtTests/LibWeb/Layout/expected/div_align.txtTests/LibWeb/Layout/expected/document-write-incomplete-tag.txtTests/LibWeb/Layout/expected/empty-list-items.txtTests/LibWeb/Layout/expected/fieldset-legend-variations.txtTests/LibWeb/Layout/expected/flex/list-container-display-contents.txtTests/LibWeb/Layout/expected/grid/justify-content-cols.txtTests/LibWeb/Layout/expected/import-after-namespace.txtTests/LibWeb/Layout/expected/inline-flex-baseline-with-wrapped-hidden-text.txtTests/LibWeb/Layout/expected/inline-size.txtTests/LibWeb/Layout/expected/inside-list-item-content-offset.txtTests/LibWeb/Layout/expected/layout-tree-update/inline-element-position-change.txtTests/LibWeb/Layout/expected/layout-tree-update/transform-longhands-clear.txtTests/LibWeb/Layout/expected/leading-metrics.txtTests/LibWeb/Layout/expected/list-item-marker-content-height.txtTests/LibWeb/Layout/expected/list-marker-rtl.txtTests/LibWeb/Layout/expected/misc/invalid-slotted-selector.txtTests/LibWeb/Layout/expected/multi-code-point-graphemes.txtTests/LibWeb/Layout/expected/ol-render-deep-hybrid-list-item-list.txtTests/LibWeb/Layout/expected/ol-render-item-values.txtTests/LibWeb/Layout/expected/ol-render-style-list-item.txtTests/LibWeb/Layout/expected/ordered-list.txtTests/LibWeb/Layout/expected/pdf-viewer.txtTests/LibWeb/Layout/expected/position-empty-pseudo-elements.txtTests/LibWeb/Layout/expected/quirks/input-in-pre-quirks-mode.txtTests/LibWeb/Layout/expected/set-margin-of-floating-box.txtTests/LibWeb/Layout/expected/table-caption-auto-height.txtTests/LibWeb/Layout/expected/table/table-align-center-with-margin.txtTests/LibWeb/Layout/expected/ul-render.txtTests/LibWeb/Layout/expected/utf-16-be-xhtml-file-should-decode-correctly.txtTests/LibWeb/Text/expected/css/getComputedStyle-print-all.txt
kalenikaliaksandr
merged commit Aug 7, 2026
40e7614
into
LadybirdBrowser:master
14 of 15 checks passed
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.
"When is this box's geometry final?" used to have a different answer
per field and per box class, and nothing checked any of them. Every
consumer of placed geometry was correct only by global ordering
arguments nobody could verify locally, and a production timing mistake
would ship as a silent misrender.
Establish the invariant that placement is the seal point: after
place_child() returns, no field of that box's UsedValues that commit
emits may change again. The sealed set is exactly the
FfiCommittedBoxMetrics payload — content offset, content sizes,
margins, borders, paddings, insets, and the containing-line-box
coordinate. Working state commit never reads (definiteness flags, size
constraints, baselines, the collapsing-borders flag) stays freely
mutable, and rare data is a separate store with its own rules. A box
materialized from a previous paintable is sealed at materialization,
which is that box's placement. The seal point is placement, not the end
of the box's inside layout: everything the engine finishes between
those two moments (float root finalization, abspos alignment insets,
table cell stretch) remains legal.