Skip to content

Replace the layout engine style decode layer with lazy CV views - #10961

Merged
kalenikaliaksandr merged 10 commits into
LadybirdBrowser:masterfrom
kalenikaliaksandr:layout-computed-value-views
Aug 1, 2026
Merged

Replace the layout engine style decode layer with lazy CV views#10961
kalenikaliaksandr merged 10 commits into
LadybirdBrowser:masterfrom
kalenikaliaksandr:layout-computed-value-views

Conversation

@kalenikaliaksandr

Copy link
Copy Markdown
Member

The Rust layout engine still read every style property by eagerly re-encoding the Rust-native computed value payloads into FfiSizeValue — the flat wire format left over from when style reads crossed an FFI callback boundary into C++ — which dragged along a field-selector enum, a central direct_size() dispatcher, and per-read decode work that the C++ engine never needed, because CSS::Size implements its methods directly on the stored representation. This series gives the Rust side the same shape: the stored computed types (ComputedSize, ComputedLengthPercentageOrAuto, ComputedGap, and a borrowed LengthPercentageRef) carry the lazy CSS::Size-style method surface, style accessors become plain payload projections generated from a table, and the one genuinely stateful read — anchor-positioned insets — is modeled explicitly as a typed InsetValue backed by the per-pass store instead of being masked inside the decode. The end state is a ComputedValuesView living beside the computed-value types (the Rust twin of the ComputedValues.h accessor layer), with layout's StyleValues reduced to that view plus pass state, and the entire FfiSizeValue/SizeField/decode machinery deleted.

The Rust layout engine still reads every sizing-shaped property by
eagerly re-encoding the Rust-native group payloads into FfiSizeValue,
the flat wire format left over from the days when style reads crossed
an FFI callback boundary into C++. The C++ side never needed such an
intermediate: CSS::Size implements its methods directly on the stored
ComputedSize representation and resolves lazily per query.

Introduce the same shape on the Rust side. LengthPercentageRef is a
borrowed view over a retained length, percentage or calculated style
value, and ComputedSize, ComputedLengthPercentageOrAuto and ComputedGap
gain the CSS::Size method surface (kind predicates, to_px,
contains_percentage, fit-content argument access) implemented lazily on
the stored payload types. The percentage truncation, length rounding
and Percentage::as_fraction multiplication-order semantics carry over
from the decode path unchanged, and new unit tests pin them, including
the truncate-versus-round split between percentages and lengths.

The shared px-resolution helpers (truncated_css_pixels and the px-basis
calc resolution context) move from style_facts.rs into the css module
so the old decode path and the new views resolve through one
implementation while later changes migrate readers off FfiSizeValue
group by group.
Width, height, the min/max sizes, flex-basis and column-width now come
out of StyleValues as &'static ComputedSize payload references carrying
the lazy CSS::Size-style method surface, instead of being eagerly
re-encoded into FfiSizeValue on every read. The StyleValues reader is
pinned to the 'static payload lifetime the arena already guarantees
for a pass, so the references are as freely storable as the decoded
values were, and the sizing arms of SizeField and direct_size are gone.

Consumers change mechanically: contains_percentage becomes a method
call, the FfiSizeKind comparisons become kind predicates on the stored
kind, the table percentage-contribution paths read the stored
percentage through as_fraction() with the same multiplication order,
and the fit-content keyword-versus-argument distinction is now the
presence of the argument's style value rather than a decoded flag.
Substituted auto sizes in the treat-as-auto replaced sizing paths
share one static auto value. The FfiSizeValue kind predicates lose
their last consumers here and are deleted; the remaining decode users
only need is_auto and to_px.
Margin and padding reads now hand out &'static
ComputedLengthPercentageOrAuto references into the surround style group
payload instead of decoding each side into FfiSizeValue, following the
same shape as the sizing properties. The margin and padding arms of
SizeField and direct_size are gone; only the four inset fields still
route through the decode path, pending the anchor-inset rework.

Every consumer already reads these values through is_auto() and
to_px(), so call sites are unchanged apart from the absolutely
positioned box solver, where the shared resolve_or_auto helper splits
into a margin variant while insets still carry the decoded
representation.
The four inset properties were the last style reads flowing through
size_value: they consult the per-LayoutState anchor-inset store before
the payload, so a plain payload reference cannot represent them. They
now come out of StyleValues as InsetValue, a three-state view that
makes the store's masking rules structural: FromStyle borrows the
stored computed value, BareAnchor borrows the store-owned calculated
wrapper for a bare anchor() inset, and Resolved carries the px-or-auto
value anchor resolution wrote back, which by construction masks both
anchor representations and answers contains_anchor_function() with
false, preserving the abspos engine's early-out on re-entry.

The anchor-inset store now stores that resolved value directly rather
than an FfiSizeValue, indexed by a dedicated InsetField enum, and the
abspos engine reads anchor-bearing calc trees through
anchor_bearing_calculated() instead of a raw pointer field. This
removes size_value, the inset arms of SizeField and direct_size, and
the decode helper for length-percentage-or-auto values.
Row and column gaps now come out of StyleValues as &'static
ComputedGap references, where normal answers is_normal() and resolves
to zero exactly as the decoded auto placeholder did. The SVG x and y
properties, text-indent and the vertical-align offset hand out
borrowed LengthPercentageRef views over their retained style values.

These were the last fields flowing through the SizeField dispatch, so
the enum, direct_size and the accessor macro built around them are
gone, and FfiSizeValue keeps only the constructors and to_px that the
grid track decode still uses.
Grid track breadths and sizing functions were the last holders of
FfiSizeValue: every stored track size was eagerly decoded into the
flat struct, and synthesized fixed tracks (collapsed tracks, gap
tracks, fixed subgrid tracks) forged one from a px value.
GridTrackBreadth is now an enum borrowing the computed size straight
from the grid style group payload, TrackSizingFunction::Fixed carries
that payload reference, and a dedicated FixedPx variant models the
synthesized tracks, which never contain percentages and resolve to
their stored px size exactly as the forged values did. TrackListSource
and the track-expansion helpers now state the 'static payload lifetime
the grid style reader already guarantees.

With the grid decode gone, FfiSizeValue, FfiSizeKind and the
decode_length_percentage/decode_computed_size helpers have no users
left and are deleted: the wire format that once carried style reads
across the FFI callback boundary is fully replaced by lazy views over
the Rust-native computed values. ComputedSize and its handle gain a
Debug derive so the grid track types keep theirs.
Every layout style read now goes through the lazy payload views, so
the dead-code allowance that covered readers landing ahead of their
consumers is gone, along with the few predicate methods that ended up
with no callers outside the unit tests.
With every style read handing out payload references, StyleReader had
become a payload multiplexer that StyleValues merely wrapped, and each
reader-only context (tree building, node facts) juggled a second type
for the same node. StyleValues now owns the payload pointer and the
group accessors directly, generic over the payload lifetime instead of
pinned to 'static: within a pass the payloads still come in 'static
and the anchor-inset store bounds the view; outside a pass,
from_payloads builds a store-less reader whose inset accessors are the
one unavailable surface, matching what StyleReader could not answer
either. The reader-only fact methods (display facts, the
block-formatting-context predicate) move along, and
style_reader_if_styled becomes style_values_if_styled.

The 'static claims that leaked into consumers shrink accordingly:
flex's UsedFlexBasis carries the pass lifetime, and the grid and
sizing helpers take plain references. Grid track expansion keeps its
'static payload references by reading the grid style group through a
store-less reader over the pass-blessed payloads.
The property accessors on StyleValues — which field of which style
group carries width, the margins, tab-size and the rest — are
computed-values knowledge, the Rust twin of the accessor layer in the
C++ ComputedValues.h, so they now live beside the payload types as
ComputedValuesView in computed_value_views.rs. The view owns the group
pointer array, the typed group getters and every pure read, including
the display facts and the style-only block-formatting-context
predicate, with the group indices moving into computed_value_types.rs
where the C++ static asserts keep pinning them through the same
generated header.

StyleValues shrinks to the layout pass state the css module cannot
know about — the anchor-inset store behind the four inset reads and
the line builder's vertical-align keyword substitution — and derefs to
the view for everything else, so call sites are unchanged. This also
undoes the optional-store compromise from folding StyleReader away:
contexts outside a pass (tree building, node facts) now hold a plain
ComputedValuesView with no inset surface at all, and the pass view
carries the store unconditionally again.
The scalar_accessors table already expresses renamed projections, so
the hand-written boolean and scalar accessors that merely rename a
group field (has_column_count, the containment and text-indent flags,
the tab-size triple, the font metrics, flex_basis_is_content and the
natural-aspect-ratio flag) join it instead of restating the same shape
one method at a time. A reference_accessors sibling covers the
by-reference projections — the six sizing properties, column-width,
margins, paddings and gaps — whose returns borrow the group payload
for the view's lifetime, which the scalar macro's unnamed-lifetime
impl block cannot express.

The accessors that remain hand-written all have real bodies: the
computed predicates, the fit-content and flex-basis substitutions, the
expect-carrying length-percentage chains and the unit conversions.
@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds borrowed, lazy computed-style views and typed CSS value readers. Rust layout code migrates from StyleReader and copied FFI values to these views across positioning, sizing, formatting contexts, grid, table, and flex layout.

Changes

Computed style and layout migration

Layer / File(s) Summary
Computed value view foundation
Libraries/LibWeb/Rust/src/css/*
Adds computed-style group indices, lazy length and calculation resolution, typed size and gap accessors, aspect-ratio decoding, layout predicates, and native payload lookup.
Typed inset and anchor resolution
Libraries/LibWeb/Rust/src/layout/style_facts.rs, Libraries/LibWeb/Rust/src/layout/abspos_engine.rs
Replaces generic FFI inset values with InsetValue, explicit inset fields, resolved overrides, and typed margin and anchor resolution.
Layout style lookup migration
Libraries/LibWeb/Rust/src/layout/{formatting_context.rs,layout_state.rs,node_facts.rs,tree_builder.rs,mod.rs}
Constructs and passes ComputedValuesView instances for style classification, formatting-context selection, tree building, and layout state.
Sizing and flex integration
Libraries/LibWeb/Rust/src/layout/{sizing_context.rs,flex_formatting_context.rs,inline_formatting_context.rs}
Updates sizing and flex APIs to borrow ComputedSize values and use typed percentage, automatic-size, intrinsic-sizing, and aspect-ratio methods.
Grid, table, and gap integration
Libraries/LibWeb/Rust/src/layout/{grid_formatting_context.rs,table_formatting_context.rs,block_formatting_context.rs}
Migrates grid and table sizing to typed computed values, adds synthesized fixed-pixel tracks, and treats normal gaps with the font-size or subgrid fallback paths.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The description directly explains the replacement of FfiSizeValue decoding with lazy computed-style views and typed inset handling implemented by the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (1)
Libraries/LibWeb/Rust/src/layout/abspos_engine.rs (1)

1876-1886: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Resolve the inset values lazily.

resolve_opposing calls to_px on both values before it tests is_auto. For a BareAnchor value, to_px runs a full calc resolution. Move each to_px call into the branch that uses it. This removes wasted work and narrows the surface of the BareAnchor resolution concern raised in style_facts.rs.

♻️ Proposed lazy resolution
         let resolve_opposing = |first: InsetValue, second: InsetValue, basis: CssPixels| {
-            let resolved_first = first.to_px(basis);
-            let resolved_second = second.to_px(basis);
             if first.is_auto() && second.is_auto() {
                 (CssPixels::default(), CssPixels::default())
             } else if first.is_auto() {
-                (-resolved_second, resolved_second)
+                let resolved_second = second.to_px(basis);
+                (-resolved_second, resolved_second)
             } else {
-                (resolved_first, -resolved_first)
+                let resolved_first = first.to_px(basis);
+                (resolved_first, -resolved_first)
             }
         };
🤖 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 1876 - 1886,
Update the resolve_opposing closure to resolve inset values lazily: check
is_auto() first, then call to_px only within the branch that uses that value.
Avoid resolving either value in the both-auto case, and preserve the existing
returned inset pairs for all other cases.
🤖 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/css/computed_value_views.rs`:
- Around line 312-328: Update decode_css_preferred_aspect_ratio to reject the
converted denominator when fraction_nearest_values_for produces a zero raw
value, alongside the existing numerator check. Return no_usable_ratio for either
zero fixed-point component; preserve the current validation and successful pair
return otherwise.
- Around line 482-489: Update native_group to use a release-enabled assert! for
the payload non-null check instead of debug_assert!, preserving the existing
unsafe cast while ensuring null group payloads fail before creating a reference.
- Around line 127-140: Update ComputedStyleValueHandle::length_percentage so its
receiver is borrowed as &'a self, binding LengthPercentageRef<'a> to the handle
borrow and preventing callers from selecting an independent longer lifetime.
Preserve the existing null-pointer handling and returned view construction.

In `@Libraries/LibWeb/Rust/src/layout/abspos_engine.rs`:
- Around line 1585-1586: Update the vertical margin resolution calls in the
absolute-positioning layout flow to pass the containing block’s inline-size
baseline, using available_space.inline_size.to_px_or_zero(), for both
style.margin_top() and style.margin_bottom() instead of
containing_block_block_size; document the deviation only if retaining the
current behavior intentionally.

In `@Libraries/LibWeb/Rust/src/layout/style_facts.rs`:
- Around line 110-117: Update InsetValue::to_px so Self::BareAnchor resolves the
anchor() calculation through resolve_calc_with_external_resolutions using the
appropriate anchor resolver before converting to pixels. Ensure unresolved
results are handled safely rather than reaching the resolved-value assertion
when resolve_anchor_insets() returns early.

---

Nitpick comments:
In `@Libraries/LibWeb/Rust/src/layout/abspos_engine.rs`:
- Around line 1876-1886: Update the resolve_opposing closure to resolve inset
values lazily: check is_auto() first, then call to_px only within the branch
that uses that value. Avoid resolving either value in the both-auto case, and
preserve the existing returned inset pairs for all other cases.
🪄 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: 21423f2f-080c-4aad-b617-3e79cb26f2ae

📥 Commits

Reviewing files that changed from the base of the PR and between be8b47e and 29c76c0.

📒 Files selected for processing (16)
  • Libraries/LibWeb/Rust/src/css/computed_value_types.rs
  • Libraries/LibWeb/Rust/src/css/computed_value_views.rs
  • Libraries/LibWeb/Rust/src/css/mod.rs
  • Libraries/LibWeb/Rust/src/layout/abspos_engine.rs
  • Libraries/LibWeb/Rust/src/layout/block_formatting_context.rs
  • Libraries/LibWeb/Rust/src/layout/flex_formatting_context.rs
  • Libraries/LibWeb/Rust/src/layout/formatting_context.rs
  • Libraries/LibWeb/Rust/src/layout/grid_formatting_context.rs
  • Libraries/LibWeb/Rust/src/layout/inline_formatting_context.rs
  • Libraries/LibWeb/Rust/src/layout/layout_state.rs
  • Libraries/LibWeb/Rust/src/layout/mod.rs
  • Libraries/LibWeb/Rust/src/layout/node_facts.rs
  • Libraries/LibWeb/Rust/src/layout/sizing_context.rs
  • Libraries/LibWeb/Rust/src/layout/style_facts.rs
  • Libraries/LibWeb/Rust/src/layout/table_formatting_context.rs
  • Libraries/LibWeb/Rust/src/layout/tree_builder.rs

Comment on lines +127 to +140
impl ComputedStyleValueHandle {
/// The lifetime is the caller's to choose: the referenced style value is
/// retained by whatever owns the handle, not by the handle borrow itself.
pub(crate) fn length_percentage<'a>(&self) -> Option<LengthPercentageRef<'a>> {
if self.pointer.is_null() {
return None;
}
// SAFETY: A non-null handle points at the retained style value owned
// by the node's style group payload, which outlives every reader.
Some(LengthPercentageRef {
value: unsafe { &*self.pointer.cast::<StyleValueData>() },
})
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# List every call site so the tightened lifetime can be checked for compilation.
rg -n -C3 '\.length_percentage\(\)' Libraries/LibWeb/Rust/src

Repository: LadybirdBrowser/ladybird

Length of output: 11774


🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline Libraries/LibWeb/Rust/src/css/computed_value_views.rs --view expanded | sed -n '1,220p'
echo "== relevant computed_value_views.rs =="
sed -n '1,330p' Libraries/LibWeb/Rust/src/css/computed_value_views.rs | cat -n
echo "== relevant LengthPercentageRef definition candidates =="
rg -n -C4 "struct LengthPercentageRef|impl LengthPercentageRef|enum StyleValueData|struct ComputedStyleValueHandle|fn length_percentage" Libraries/LibWeb/Rust/src/css/computed_value_views.rs

Repository: LadybirdBrowser/ladybird

Length of output: 21508


Bind length_percentage’s return lifetime to &'a self.

length_percentage<'a>(&self) lets callers pick an independent lifetime for LengthPercentageRef<'a>, including 'static. Use &'a self so the returned style-value view cannot outlive the handle borrow itself.

🤖 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/css/computed_value_views.rs` around lines 127 -
140, Update ComputedStyleValueHandle::length_percentage so its receiver is
borrowed as &'a self, binding LengthPercentageRef<'a> to the handle borrow and
preventing callers from selecting an independent longer lifetime. Preserve the
existing null-pointer handling and returned view construction.

Comment on lines +312 to +328
fn decode_css_preferred_aspect_ratio(ratio: &ComputedAspectRatio) -> (CssPixels, CssPixels) {
let no_usable_ratio = (CssPixels::default(), CssPixels::default());
if !ratio.has_preferred_ratio {
return no_usable_ratio;
}
let numerator = ratio.preferred_ratio_numerator;
let denominator = ratio.preferred_ratio_denominator;
let is_degenerate = !numerator.is_finite() || numerator == 0.0 || !denominator.is_finite() || denominator == 0.0;
if is_degenerate {
return no_usable_ratio;
}
let (numerator, denominator) = CssPixels::fraction_nearest_values_for(numerator, denominator);
if numerator.raw_value() == 0 {
return no_usable_ratio;
}
(numerator, denominator)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Also reject a denominator that collapses to zero.

fraction_nearest_values_for converts both terms to fixed point. The code checks only numerator.raw_value() == 0 afterwards. A very small but finite denominator can round to raw zero while the numerator stays non-zero. The function then returns a non-sentinel pair with a zero denominator. A consumer that tests only the numerator divides by zero.

🐛 Proposed fix
     let (numerator, denominator) = CssPixels::fraction_nearest_values_for(numerator, denominator);
-    if numerator.raw_value() == 0 {
+    if numerator.raw_value() == 0 || denominator.raw_value() == 0 {
         return no_usable_ratio;
     }
📝 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
fn decode_css_preferred_aspect_ratio(ratio: &ComputedAspectRatio) -> (CssPixels, CssPixels) {
let no_usable_ratio = (CssPixels::default(), CssPixels::default());
if !ratio.has_preferred_ratio {
return no_usable_ratio;
}
let numerator = ratio.preferred_ratio_numerator;
let denominator = ratio.preferred_ratio_denominator;
let is_degenerate = !numerator.is_finite() || numerator == 0.0 || !denominator.is_finite() || denominator == 0.0;
if is_degenerate {
return no_usable_ratio;
}
let (numerator, denominator) = CssPixels::fraction_nearest_values_for(numerator, denominator);
if numerator.raw_value() == 0 {
return no_usable_ratio;
}
(numerator, denominator)
}
fn decode_css_preferred_aspect_ratio(ratio: &ComputedAspectRatio) -> (CssPixels, CssPixels) {
let no_usable_ratio = (CssPixels::default(), CssPixels::default());
if !ratio.has_preferred_ratio {
return no_usable_ratio;
}
let numerator = ratio.preferred_ratio_numerator;
let denominator = ratio.preferred_ratio_denominator;
let is_degenerate = !numerator.is_finite() || numerator == 0.0 || !denominator.is_finite() || denominator == 0.0;
if is_degenerate {
return no_usable_ratio;
}
let (numerator, denominator) = CssPixels::fraction_nearest_values_for(numerator, denominator);
if numerator.raw_value() == 0 || denominator.raw_value() == 0 {
return no_usable_ratio;
}
(numerator, denominator)
}
🤖 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/css/computed_value_views.rs` around lines 312 -
328, Update decode_css_preferred_aspect_ratio to reject the converted
denominator when fraction_nearest_values_for produces a zero raw value,
alongside the existing numerator check. Return no_usable_ratio for either zero
fixed-point component; preserve the current validation and successful pair
return otherwise.

Comment on lines +482 to +489
fn native_group<T>(self, group_index: usize) -> &'a T {
let payload = self.groups[group_index];
debug_assert!(!payload.is_null());
// SAFETY: The payload is the Rust-defined group struct itself; C++
// derives its mirror from the cbindgen twin of the same type, and the
// node's ComputedValues keep it alive while readers exist.
unsafe { &*payload.cast::<T>() }
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Promote the null check to a release assertion.

native_group guards the payload with debug_assert!. In release builds a null or missing payload creates a reference to address zero, which is undefined behavior, and every later field read is unsound. The index also panics only through the slice bound, which gives no group name. Use assert! here. The cost is one branch per style read, and the branch is predictable.

🛡️ Proposed fix
     #[inline]
     fn native_group<T>(self, group_index: usize) -> &'a T {
         let payload = self.groups[group_index];
-        debug_assert!(!payload.is_null());
+        assert!(!payload.is_null(), "style group {group_index} has no payload");
🤖 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/css/computed_value_views.rs` around lines 482 -
489, Update native_group to use a release-enabled assert! for the payload
non-null check instead of debug_assert!, preserving the existing unsafe cast
while ensuring null group payloads fail before creating a reference.

Comment on lines +1585 to +1586
resolve_margin_or_auto(style.margin_top(), containing_block_block_size),
resolve_margin_or_auto(style.margin_bottom(), containing_block_block_size),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Compare with the C++ replaced-element block-axis margin resolution.
rg -nP -C10 'margin_top|margin_bottom' Libraries/LibWeb/Layout/AbsoluteFormattingContext.cpp 2>/dev/null
rg -rn --iglob '*.cpp' -nP -C8 'compute_height_for_absolutely_positioned_replaced|absolutely_positioned_replaced' Libraries/LibWeb/Layout | head -80

Repository: LadybirdBrowser/ladybird

Length of output: 162


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "## candidate file outline/lens"
wc -l Libraries/LibWeb/Rust/src/layout/abspos_engine.rs 2>/dev/null || true
sed -n '1530,1600p' Libraries/LibWeb/Rust/src/layout/abspos_engine.rs 2>/dev/null || true
sed -n '1260,1305p' Libraries/LibWeb/Rust/src/layout/abspos_engine.rs 2>/dev/null || true

echo "## references to resolve_margin_or_auto and available_space.inline_size in Rust layout"
rg -n "resolve_margin_or_auto|containing_block_(inline|block)_size|available_space\.inline_size|containing_block_inline_size" Libraries/LibWeb/Rust/src/layout/abspos_engine.rs

echo "## C++ absolute layout search"
git ls-files 'Libraries/LibWeb/Layout/*' | rg '(AbsoluteFormattingContext|Absolute|abs(?:olute|olutely)|abspos)' | head -80
rg -n -C8 'margin_top|margin_bottom|percentage.*margin|inline_size|block_size' Libraries/LibWeb/Layout/AbsoluteFormattingContext.cpp 2>/dev/null || true
rg -n -C8 'compute_height_for_absolutely_positioned_replaced|absolutely_positioned_replaced|replaced' Libraries/LibWeb/Layout/AbsoluteFormattingContext.cpp 2>/dev/null || true

Repository: LadybirdBrowser/ladybird

Length of output: 9118


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "## repository file names for abspos implementation"
git ls-files | rg '(^|/)(AbsoluteFormattingContext|abspos_engine|Absolute.*Layout|.*Layout.*Absolute).*' || true

echo "## broad C++ source search for abspos/replaced axis margin resolution"
rg -n -C6 'margin_top|margin_bottom|margin-left|margin-right|percentage.*margin|absolute|replaced|solve.*replaced|replaced.*axis' Libraries/LibWeb -g '*.cpp' -g '*.h' -g '*.hpp' | head -240

echo "## targeted CSS text in repo for margin percentages"
rg -n -C3 'margin.*percentage|percentage margin|inline size containing block|margin block' Libraries/LibWeb/Layout -g '*.cpp' -g '*.h' -g '*.hpp' | head -160

echo "## read-only parser behavior probe for percentage margin conversion from source text"
python3 - <<'PY'
from pathlib import Path
p = Path('Libraries/LibWeb/Rust/src/layout/abspos_engine.rs')
s = p.read_text()
def read_block(start, end):
    return s[s.find("fn " + start):s.find("fn " + start, s.find("fn " + start)+1), s.find("fn " + start):s.find("fn " + start, s.find("fn " + start)+1)]
PY
sed -n '820,865p' Libraries/LibWeb/Rust/src/layout/abspos_engine.rs
sed -n '1520,1592p' Libraries/LibWeb/Rust/src/layout/abspos_engine.rs
sed -n '1280,1300p' Libraries/LibWeb/Rust/src/layout/abspos_engine.rs

Repository: LadybirdBrowser/ladybird

Length of output: 25784


🌐 Web query:

CSS Positioned Layout Module percentage margins containing block inline size margin-top margin-bottom

💡 Result:

In CSS, percentage values for margin properties (including margin-top, margin-right, margin-bottom, and margin-left) are calculated with respect to the inline size of the containing block [1][2][3]. The inline size typically corresponds to the width of the containing block in horizontal writing modes [1][2]. This behavior is consistent across all four sides of a box, meaning that even vertical margins (margin-top and margin-bottom) are computed based on the width (inline size) of the containing block, rather than its height (block size) [4][5][2]. This design choice—basing all percentage margins on the inline size—prevents circular dependencies during layout calculation [5][2]. Since the inline size of a block is generally determined by its parent's width, it remains stable regardless of the content's vertical height, allowing browsers to calculate layout without needing to resolve the element's height first [5][2]. For absolutely positioned elements, the containing block is defined by the nearest positioned ancestor (or the initial containing block for fixed positioning) [1][6]. The percentages for their margins, like static or relative elements, continue to be resolved against the inline size of that designated containing block [1][2]. While CSS 2.1 documentation often referred explicitly to the "width" of the containing block [4][6], modern specifications use the term "inline size" to correctly account for different writing modes (e.g., in vertical writing modes, the inline size is the height of the containing block) [2][3].

Citations:


Resolve replaced-block vertical margins against the inline size.

margin_top and margin_bottom must use percentage baselines from the containing block inline size. These calls use containing_block_block_size, so pass available_space.inline_size.to_px_or_zero() here. If this is an intentional compatibility deviation, document it.

🤖 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 1585 - 1586,
Update the vertical margin resolution calls in the absolute-positioning layout
flow to pass the containing block’s inline-size baseline, using
available_space.inline_size.to_px_or_zero(), for both style.margin_top() and
style.margin_bottom() instead of containing_block_block_size; document the
deviation only if retaining the current behavior intentionally.

Comment on lines +110 to 117
pub(crate) fn to_px(self, reference: CssPixels) -> CssPixels {
match self {
Self::FromStyle(value) => value.to_px(reference),
Self::BareAnchor(wrapper) => resolve_calc_to_px(std::ptr::from_ref(wrapper).cast(), reference),
Self::Resolved(resolved) if resolved.is_auto => CssPixels::default(),
Self::Resolved(resolved) => resolved.px,
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect how rust_calc_resolve handles anchor() nodes without external resolutions.
rg -nP -C20 'fn\s+rust_calc_resolve' Libraries/LibWeb/Rust/src/css/calc.rs
rg -nP -C10 'NonMathFunction' Libraries/LibWeb/Rust/src/css/calc.rs | head -100
rg -nP -C6 'resolve_calc_to_px|resolve_calc_with_external_resolutions' Libraries/LibWeb/Rust/src

Repository: LadybirdBrowser/ladybird

Length of output: 14093


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== style_facts InsetValue =="
sed -n '70,150p' Libraries/LibWeb/Rust/src/layout/style_facts.rs
sed -n '251,305p' Libraries/LibWeb/Rust/src/layout/style_facts.rs

echo "== abspos_engine relevant insets and early returns =="
sed -n '560,635p' Libraries/LibWeb/Rust/src/layout/abspos_engine.rs
sed -n '1140,1215p' Libraries/LibWeb/Rust/src/layout/abspos_engine.rs

echo "== resolve_calc_with_external_resolutions implementation =="
sed -n '251,285p' Libraries/LibWeb/Rust/src/layout/style_facts.rs

echo "== rust_calc_resolve implementation sections =="
sed -n '3079,3165p' Libraries/LibWeb/Rust/src/css/calc.rs
sed -n '3165,3260p' Libraries/LibWeb/Rust/src/css/calc.rs

echo "== external resolution helper usages =="
rg -n "FfiCalcExternalResolution|external_resolutions|external_resolution_count|rust_calc_resolve\\(" Libraries/LibWeb/Rust/src/css Libraries/LibWeb/Rust/src/layout -g '*.rs'

Repository: LadybirdBrowser/ladybird

Length of output: 24878


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== computed_value_views px context =="
sed -n '35,58p' Libraries/LibWeb/Rust/src/css/computed_value_views.rs

echo "== px calculation resolution context implementation =="
rg -n "fn px_calc_resolution_context|struct FfiCalcResolutionContext|impl FfiCalcResolutionContext|external_resolutions.*ptr::null|external_resolution_count.*0" Libraries/LibWeb/Rust/src/css Libraries/LibWeb/Rust/src/layout -g '*.rs'

echo "== calc evaluation external resolution resolution =="
sed -n '2450,2785p' Libraries/LibWeb/Rust/src/css/calc.rs

echo "== resolve_simplified_calculation around NonMathFunction =="
rg -n -C20 "NonMathFunction|non_math_function|external_resolutions" Libraries/LibWeb/Rust/src/css/calc.rs

Repository: LadybirdBrowser/ladybird

Length of output: 50380


Resolve bare anchor insets before calling to_px.

InsetValue::to_px() passes the bare anchor() non-math function to resolve_calc_to_px(), and that call has no external resolution callback. It returns an unresolved calculation, so the result.resolved assertion can panic when resolve_anchor_insets() returns early. Use resolve_calc_with_external_resolutions() with the anchor resolver, or handle the unresolved result before the px conversion.

🤖 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/style_facts.rs` around lines 110 - 117,
Update InsetValue::to_px so Self::BareAnchor resolves the anchor() calculation
through resolve_calc_with_external_resolutions using the appropriate anchor
resolver before converting to pixels. Ensure unresolved results are handled
safely rather than reaching the resolved-value assertion when
resolve_anchor_insets() returns early.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant