Lay out absolutely positioned boxes after in-flow layout - #10928
Merged
kalenikaliaksandr merged 2 commits intoJul 30, 2026
Merged
Lay out absolutely positioned boxes after in-flow layout#10928kalenikaliaksandr merged 2 commits into
kalenikaliaksandr merged 2 commits into
Conversation
Absolutely positioned boxes were laid out from inside formatting context completions, in the middle of in-flow layout, with a destructor fallback covering every place that never completed its context. That arrangement hid real problems. Layout ran at points no caller stated explicitly, so a forgotten completion was silently patched instead of caught. A box could resolve its static position against geometry that was not final yet, which is why table cells needed their own layout point after vertical alignment. Anchor resolution saw whatever happened to be laid out at the moment a completion ran. And because laying out an absolutely positioned box required live formatting context objects, there was no way to lay out only the absolutely positioned boxes without re-running everything around them. Now every creator of a formatting context completes it explicitly once its root box has a used size, and completion only records work: it places the block context's floats, registers absolutely positioned children, stores grid-area containing blocks while the grid's tracks are still available, and queues the completed root. The layout entry points run one pass over the queue before committing, when all in-flow geometry is final. The saved-layout replay entry now runs the engine on a bare frame directly, which lets the AbsposReplay context type go away entirely. A foreignObject that is a flex or grid container previously never laid out its absolutely positioned children, because the destructor fallback only handled block contexts; explicit completion fixes that, and the new foreignObject-flex-with-abspos-child layout test fails before this change and passes after it. A second new layout test pins subgrid behavior for an absolutely positioned child of a grid container: no track adoption, since it is not a grid item.
📝 WalkthroughWalkthroughChangesThe PR adds a Rust absolute-positioned layout engine with anchor inset resolution, axis solving, containing-block tracking, explicit layout passes, replay support, and updated formatting-context integration. It also adds grid and SVG regression fixtures. Absolute-positioned layout
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RootLayout
participant FormattingContext
participant LayoutState
participant AbsposEngine
RootLayout->>FormattingContext: run formatting context
FormattingContext->>LayoutState: register and queue abspos children
RootLayout->>FormattingContext: complete root box sizing
RootLayout->>AbsposEngine: run abspos layout pass
AbsposEngine->>LayoutState: consume pending children and overrides
AbsposEngine-->>RootLayout: position abspos elements
Possibly related PRs
🚥 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: 2
🧹 Nitpick comments (4)
Libraries/LibWeb/Rust/src/layout/abspos_engine.rs (3)
1837-1841: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the
assert+unwrapinto anexpect.♻️ Suggested cleanup
- let saved_inputs = self.callbacks.saved_abspos_layout_inputs(node); - let found = saved_inputs.is_some(); - assert!(found); - let mut inputs = saved_inputs.unwrap(); + let mut inputs = self + .callbacks + .saved_abspos_layout_inputs(node) + .expect("abspos replay requires saved layout inputs");🤖 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/abspos_engine.rs` around lines 1837 - 1841, In replay, replace the separate found assertion and saved_inputs.unwrap() with a single expect call on saved_inputs, preserving the assertion failure behavior with a clear message.
1701-1723: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
force_independent_context_run: trueinstead of relying onunreachable!.The
ReenterCurrentarm asserts that every abspos box with contents establishes its own formatting context, but the call passesfalse, so the invariant is enforced only by a panic at runtime. Passingtruemakeslayout_inside_childcreate the independent context itself and turns the panic into dead code.🤖 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/abspos_engine.rs` around lines 1701 - 1723, Update the layout_inside_child call in the abspos child layout flow to pass force_independent_context_run as true instead of false. Preserve the existing ChildLayoutOutcome handling, including the ReenterCurrent arm, while ensuring absolutely positioned children establish their independent formatting context during this call.
437-456: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
anchor_lookuprebuilds the full eligible-node list per call.
used_value_nodes()walks every used-values entry and then a second pass allocates a shellVec. This runs once peranchor()occurrence (including each nested fallback resolution viaresolve_anchor_non_math_function), so cost is O(anchors × nodes) with an allocation each time. Consider computing the shell list once per abspos pass (or perresolve_anchor_insetscall) and threading it throughAnchorCalcCallbackContext.🤖 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/abspos_engine.rs` around lines 437 - 456, Refactor anchor lookup so the eligible shell list is built once per absolute-position resolution pass rather than inside each anchor_lookup call. Cache or thread this precomputed list through AnchorCalcCallbackContext and reuse it from resolve_anchor_insets and nested resolve_anchor_non_math_function calls, while preserving the existing synchronous lookup and invalid-anchor handling.Libraries/LibWeb/Rust/src/layout/formatting_context.rs (1)
2946-2949: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing
// SAFETY:comment on the new unsafe callback.Every other unsafe FFI call in this file documents its invariant (e.g.
node_data,text_content,saved_abspos_layout_inputs). Add the equivalent note here.📝 Suggested addition
pub(crate) fn static_position_containing_block(&self, node: Node) -> Node { + // SAFETY: The callback only reads the live node's box-tree position + // for this synchronous layout pass. unsafe { (self.static_position_containing_block)(self.context, self.shell(node)) } }🤖 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/formatting_context.rs` around lines 2946 - 2949, Add a `// SAFETY:` comment immediately before the unsafe callback invocation in `static_position_containing_block`, documenting the invariant that makes calling `(self.static_position_containing_block)(self.context, self.shell(node))` valid, consistent with the safety notes on `node_data`, `text_content`, and `saved_abspos_layout_inputs`.
🤖 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/Rust/src/layout/grid_formatting_context.rs`:
- Around line 3923-3934: Restrict the override in the closure passed to
override_contained_abspos_child_containing_blocks to children whose
static_position_containing_block is self.grid_container. For non-direct
descendants, leave containing_block_info_override unset so
base_containing_block_info handles them; only compute and return
abspos_containing_block_info with axis-mode adjustments for direct grid
children.
In `@Libraries/LibWeb/Rust/src/layout/layout_state.rs`:
- Around line 1494-1505: Update
override_contained_abspos_child_containing_blocks so
containing_block_info_for_child is invoked before acquiring the mutable borrow
of the target's contained_abspos_children. First collect each child and its
computed AbsposContainingBlockInfo, then borrow the deque mutably and assign the
precomputed overrides, preserving the existing early return and child ordering.
---
Nitpick comments:
In `@Libraries/LibWeb/Rust/src/layout/abspos_engine.rs`:
- Around line 1837-1841: In replay, replace the separate found assertion and
saved_inputs.unwrap() with a single expect call on saved_inputs, preserving the
assertion failure behavior with a clear message.
- Around line 1701-1723: Update the layout_inside_child call in the abspos child
layout flow to pass force_independent_context_run as true instead of false.
Preserve the existing ChildLayoutOutcome handling, including the ReenterCurrent
arm, while ensuring absolutely positioned children establish their independent
formatting context during this call.
- Around line 437-456: Refactor anchor lookup so the eligible shell list is
built once per absolute-position resolution pass rather than inside each
anchor_lookup call. Cache or thread this precomputed list through
AnchorCalcCallbackContext and reuse it from resolve_anchor_insets and nested
resolve_anchor_non_math_function calls, while preserving the existing
synchronous lookup and invalid-anchor handling.
In `@Libraries/LibWeb/Rust/src/layout/formatting_context.rs`:
- Around line 2946-2949: Add a `// SAFETY:` comment immediately before the
unsafe callback invocation in `static_position_containing_block`, documenting
the invariant that makes calling
`(self.static_position_containing_block)(self.context, self.shell(node))` valid,
consistent with the safety notes on `node_data`, `text_content`, and
`saved_abspos_layout_inputs`.
🪄 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: 9025110d-b3fa-4351-af87-3ff379c813f6
📒 Files selected for processing (16)
Libraries/LibWeb/Layout/LayoutRustBridge.cppLibraries/LibWeb/Rust/src/layout/abspos_engine.rsLibraries/LibWeb/Rust/src/layout/block_formatting_context.rsLibraries/LibWeb/Rust/src/layout/flex_formatting_context.rsLibraries/LibWeb/Rust/src/layout/formatting_context.rsLibraries/LibWeb/Rust/src/layout/grid_formatting_context.rsLibraries/LibWeb/Rust/src/layout/inline_formatting_context.rsLibraries/LibWeb/Rust/src/layout/layout_state.rsLibraries/LibWeb/Rust/src/layout/mod.rsLibraries/LibWeb/Rust/src/layout/replaced_with_children_formatting_context.rsLibraries/LibWeb/Rust/src/layout/svg_formatting_context.rsLibraries/LibWeb/Rust/src/layout/table_formatting_context.rsTests/LibWeb/Layout/expected/grid/abspos-subgrid-child-of-grid-container.txtTests/LibWeb/Layout/expected/svg/foreignObject-flex-with-abspos-child.txtTests/LibWeb/Layout/input/grid/abspos-subgrid-child-of-grid-container.htmlTests/LibWeb/Layout/input/svg/foreignObject-flex-with-abspos-child.html
💤 Files with no reviewable changes (4)
- Libraries/LibWeb/Layout/LayoutRustBridge.cpp
- Libraries/LibWeb/Rust/src/layout/inline_formatting_context.rs
- Libraries/LibWeb/Rust/src/layout/flex_formatting_context.rs
- Libraries/LibWeb/Rust/src/layout/table_formatting_context.rs
Comment on lines
+3923
to
+3934
| self.state | ||
| .override_contained_abspos_child_containing_blocks(self.grid_container, |child| { | ||
| let mut info = self.abspos_containing_block_info(child); | ||
| let grid_area_is_childs_static_position = | ||
| self.callbacks.static_position_containing_block(child) == self.grid_container; | ||
| if !grid_area_is_childs_static_position { | ||
| let (inline_axis_mode, block_axis_mode) = axis_modes(self.style(child)); | ||
| info.inline_axis_mode = inline_axis_mode; | ||
| info.block_axis_mode = block_axis_mode; | ||
| } | ||
| info | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm deeper abspos descendants can register against a grid container.
rg -nP -C6 'fn register_contained_abspos_child' Libraries/LibWeb/Rust/src/layout/
# Which node does registration target?
ast-grep run --lang rust --pattern 'crate::layout::register_contained_abspos_child($$$)' Libraries/LibWeb/Rust/src/layout/Repository: LadybirdBrowser/ladybird
Length of output: 4914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the grid formatting context around the override and registration.
sed -n '3890,3965p' Libraries/LibWeb/Rust/src/layout/grid_formatting_context.rs
echo
echo '--- layout_state register_contained_abspos_child ---'
sed -n '1426,1475p' Libraries/LibWeb/Rust/src/layout/layout_state.rs
echo
echo '--- formatting_context register_contained_abspos_child ---'
sed -n '2375,2415p' Libraries/LibWeb/Rust/src/layout/formatting_context.rs
echo
echo '--- search for override_contained_abspos_child_containing_blocks ---'
rg -n "override_contained_abspos_child_containing_blocks|containing_block_info_override|abspos_containing_block_info" Libraries/LibWeb/Rust/src/layout/Repository: LadybirdBrowser/ladybird
Length of output: 8440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the override implementation and the exact data it rewrites.
rg -n -A40 -B10 "fn override_contained_abspos_child_containing_blocks" Libraries/LibWeb/Rust/src/layout/layout_state.rs
echo
echo '--- grid formatting context surrounding the registration + override ---'
sed -n '3900,3955p' Libraries/LibWeb/Rust/src/layout/grid_formatting_context.rsRepository: LadybirdBrowser/ladybird
Length of output: 5478
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how the override is consumed by abspos layout.
sed -n '1800,1885p' Libraries/LibWeb/Rust/src/layout/abspos_engine.rs
echo
echo '--- AbsposContainingBlockInfo definition and related helpers ---'
rg -n -A40 -B10 "struct AbsposContainingBlockInfo|enum AbsposAxisMode|StaticPositionRect|StaticPositionAlignment" Libraries/LibWeb/Rust/src/layout/Repository: LadybirdBrowser/ladybird
Length of output: 50381
Limit the grid-area override to direct abspos grid children.
override_contained_abspos_child_containing_blocks rewrites every pending child under self.grid_container, but abspos_containing_block_info(child) reads grid-track geometry and alignment from the child’s own grid placement. That is only correct when the child’s static-position containing block is the grid container; abspos descendants inside grid items can still land here and will inherit a grid-area-based containing block instead of the generic static-position path. Leave containing_block_info_override unset for non-direct children so base_containing_block_info handles them.
🤖 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/grid_formatting_context.rs` around lines
3923 - 3934, Restrict the override in the closure passed to
override_contained_abspos_child_containing_blocks to children whose
static_position_containing_block is self.grid_container. For non-direct
descendants, leave containing_block_info_override unset so
base_containing_block_info handles them; only compute and return
abspos_containing_block_info with axis-mode adjustments for direct grid
children.
Comment on lines
+1494
to
+1505
| pub(crate) fn override_contained_abspos_child_containing_blocks( | ||
| &self, | ||
| target_box: Node, | ||
| containing_block_info_for_child: impl Fn(Node) -> AbsposContainingBlockInfo, | ||
| ) { | ||
| let Some(children) = self.contained_abspos_children.get(target_box.slot_index()) else { | ||
| return; | ||
| }; | ||
| for entry in children.borrow_mut().iter_mut() { | ||
| entry.containing_block_info_override = Some(containing_block_info_for_child(entry.child_box)); | ||
| } | ||
| let child = children.remove(0); | ||
| Some(PendingAbsposChild { | ||
| child_box: child.child_box, | ||
| static_position_rect: child.static_position_rect, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Callback is invoked while the RefCell is mutably borrowed.
containing_block_info_for_child runs inside children.borrow_mut(). The grid caller's closure reaches into self.abspos_containing_block_info(child), self.style(child) and the static_position_containing_block FFI callback — none of which are provably free of re-entrancy into this same slot's deque. Any such re-entry is an immediate already mutably borrowed panic in a layout pass. Compute the infos outside the borrow.
🛡️ Proposed fix
let Some(children) = self.contained_abspos_children.get(target_box.slot_index()) else {
return;
};
- for entry in children.borrow_mut().iter_mut() {
- entry.containing_block_info_override = Some(containing_block_info_for_child(entry.child_box));
+ let child_boxes = children
+ .borrow()
+ .iter()
+ .map(|entry| entry.child_box)
+ .collect::<Vec<_>>();
+ let infos = child_boxes
+ .into_iter()
+ .map(|child_box| containing_block_info_for_child(child_box))
+ .collect::<Vec<_>>();
+ for (entry, info) in children.borrow_mut().iter_mut().zip(infos) {
+ entry.containing_block_info_override = Some(info);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub(crate) fn override_contained_abspos_child_containing_blocks( | |
| &self, | |
| target_box: Node, | |
| containing_block_info_for_child: impl Fn(Node) -> AbsposContainingBlockInfo, | |
| ) { | |
| let Some(children) = self.contained_abspos_children.get(target_box.slot_index()) else { | |
| return; | |
| }; | |
| for entry in children.borrow_mut().iter_mut() { | |
| entry.containing_block_info_override = Some(containing_block_info_for_child(entry.child_box)); | |
| } | |
| let child = children.remove(0); | |
| Some(PendingAbsposChild { | |
| child_box: child.child_box, | |
| static_position_rect: child.static_position_rect, | |
| }) | |
| } | |
| pub(crate) fn override_contained_abspos_child_containing_blocks( | |
| &self, | |
| target_box: Node, | |
| containing_block_info_for_child: impl Fn(Node) -> AbsposContainingBlockInfo, | |
| ) { | |
| let Some(children) = self.contained_abspos_children.get(target_box.slot_index()) else { | |
| return; | |
| }; | |
| let child_boxes = children | |
| .borrow() | |
| .iter() | |
| .map(|entry| entry.child_box) | |
| .collect::<Vec<_>>(); | |
| let infos = child_boxes | |
| .into_iter() | |
| .map(|child_box| containing_block_info_for_child(child_box)) | |
| .collect::<Vec<_>>(); | |
| for (entry, info) in children.borrow_mut().iter_mut().zip(infos) { | |
| entry.containing_block_info_override = Some(info); | |
| } | |
| } |
🤖 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/layout_state.rs` around lines 1494 - 1505,
Update override_contained_abspos_child_containing_blocks so
containing_block_info_for_child is invoked before acquiring the mutable borrow
of the target's contained_abspos_children. First collect each child and its
computed AbsposContainingBlockInfo, then borrow the deque mutably and assign the
precomputed overrides, preserving the existing early return and child ordering.
kalenikaliaksandr
merged commit Jul 30, 2026
c03a204
into
LadybirdBrowser:master
15 of 16 checks passed
This was referenced Jul 30, 2026
This was referenced Aug 7, 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.
Absolutely positioned boxes were laid out from inside formatting
context completions, in the middle of in-flow layout, with a
destructor fallback covering every place that never completed its
context. That arrangement hid real problems. Layout ran at points no
caller stated explicitly, so a forgotten completion was silently
patched instead of caught. A box could resolve its static position
against geometry that was not final yet, which is why table cells
needed their own layout point after vertical alignment. Anchor
resolution saw whatever happened to be laid out at the moment a
completion ran. And because laying out an absolutely positioned box
required live formatting context objects, there was no way to lay out
only the absolutely positioned boxes without re-running everything
around them.
Now every creator of a formatting context completes it explicitly
once its root box has a used size, and completion only records work:
it places the block context's floats, registers absolutely positioned
children, stores grid-area containing blocks while the grid's tracks
are still available, and queues the completed root. The layout entry
points run one pass over the queue before committing, when all
in-flow geometry is final. The saved-layout replay entry now runs the
engine on a bare frame directly, which lets the AbsposReplay context
type go away entirely.
A foreignObject that is a flex or grid container previously never
laid out its absolutely positioned children, because the destructor
fallback only handled block contexts; explicit completion fixes that,
and the new foreignObject-flex-with-abspos-child layout test fails
before this change and passes after it. A second new layout test pins
subgrid behavior for an absolutely positioned child of a grid
container: no track adoption, since it is not a grid item.