LibWeb: Drop the needs_inset_resolution layout callback - #11092
Conversation
compute_inset opened with a callback into C++ that answered whether the box is relatively positioned or carries anchor() insets. Both facts are already in the node's style payload, which the code right after the gate reads anyway, so the call was redundant work on a path taken once per in-flow box, once per float, and once per inline node entry while building lines. Its C++ side was also the expensive half: it walked to the box's abstract element and could rebuild four computed style values just to look for anchor() functions. Compute the gate from the style payload instead. The anchor test stays ahead of the position check, because an anchor-bearing box that is not relatively positioned still has to run anchor resolution, which clears a stale default scroll shift left behind by an earlier pass.
📝 WalkthroughWalkthroughThe formatting-context FFI callback table no longer exposes 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.
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/Rust/src/layout/abspos_engine.rs`:
- Around line 1814-1822: Update the early-return flow in the
relative-positioning logic so nodes that are relative, including those without
anchor insets, always call resolve_anchor_insets. Retain the early return only
when the node is neither relative nor anchor-bearing, ensuring stale default
scroll-shift state is cleared for ordinary relative nodes.
🪄 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: a9cc2803-b5a4-4da8-a0be-c2389dfdf3d8
📒 Files selected for processing (3)
Libraries/LibWeb/Layout/LayoutRustBridge.cppLibraries/LibWeb/Rust/src/layout/abspos_engine.rsLibraries/LibWeb/Rust/src/layout/formatting_context.rs
💤 Files with no reviewable changes (2)
- Libraries/LibWeb/Layout/LayoutRustBridge.cpp
- Libraries/LibWeb/Rust/src/layout/formatting_context.rs
| if !has_anchor_insets && initial_style.position() != positioning::RELATIVE { | ||
| return; | ||
| } | ||
| // Anchor resolution also refreshes the box's default scroll shift, so | ||
| // an anchor-bearing box resolves its insets even when it turns out not | ||
| // to be relatively positioned. | ||
| let resolved = if has_anchor_insets { | ||
| self.resolve_anchor_insets(node, None, NodeSlotId::INVALID) | ||
| } else { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve stale scroll-shift cleanup for relative nodes.
When a relative node has no anchor() inset, this branch skips resolve_anchor_insets. That helper clears the existing default scroll shift before checking for anchor insets at Lines 558-568. If a node changes from anchor-bearing to ordinary relative positioning, stale scroll-shift state can remain and produce incorrect layout.
Keep the early return for nodes that are neither relative nor anchor-bearing, but call resolve_anchor_insets for every node that passes that gate.
Proposed fix
- let resolved = if has_anchor_insets {
- self.resolve_anchor_insets(node, None, NodeSlotId::INVALID)
- } else {
- None
- };
+ let resolved = self.resolve_anchor_insets(node, None, NodeSlotId::INVALID);📝 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.
| if !has_anchor_insets && initial_style.position() != positioning::RELATIVE { | |
| return; | |
| } | |
| // Anchor resolution also refreshes the box's default scroll shift, so | |
| // an anchor-bearing box resolves its insets even when it turns out not | |
| // to be relatively positioned. | |
| let resolved = if has_anchor_insets { | |
| self.resolve_anchor_insets(node, None, NodeSlotId::INVALID) | |
| } else { | |
| if !has_anchor_insets && initial_style.position() != positioning::RELATIVE { | |
| return; | |
| } | |
| // Anchor resolution also refreshes the box's default scroll shift, so | |
| // an anchor-bearing box resolves its insets even when it turns out not | |
| // to be relatively positioned. | |
| let resolved = self.resolve_anchor_insets(node, None, NodeSlotId::INVALID); |
🤖 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 1814 - 1822,
Update the early-return flow in the relative-positioning logic so nodes that are
relative, including those without anchor insets, always call
resolve_anchor_insets. Retain the early return only when the node is neither
relative nor anchor-bearing, ensuring stale default scroll-shift state is
cleared for ordinary relative nodes.
1 participant
compute_inset opened with a callback into C++ that answered whether the box is relatively positioned or carries anchor() insets. Both facts are already in the node's style payload, which the code right after the gate reads anyway, so the call was redundant work on a path taken once per in-flow box, once per float, and once per inline node entry while building lines. Its C++ side was also the expensive half: it walked to the box's abstract element and could rebuild four computed style values just to look for anchor() functions.
Compute the gate from the style payload instead. The anchor test stays ahead of the position check, because an anchor-bearing box that is not relatively positioned still has to run anchor resolution, which clears a stale default scroll shift left behind by an earlier pass.