From 3d2f0d1be5f58d147416d833c1d34f76365f3308 Mon Sep 17 00:00:00 2001 From: Oriol Brufau Date: Tue, 29 Jul 2025 11:59:54 +0200 Subject: [PATCH] layout: Fix `depends_on_block_constraints` logic (#38318) The logic was wrong, sometimes we weren't setting it to true on flex containers that needed it, and then as a workaround we were setting it to to true on flex items that didn't need it. For example, this testcase had 5 cache misses when stretching the items, now we will avoid laying them out again: ```html
``` Also, the workaround wasn't always working, e.g. it failed to stretch the green element here: ```html
``` Testing: Adding new test Fixes: #38023 Signed-off-by: Oriol Brufau --- components/layout/flexbox/layout.rs | 19 ++++++++++++++----- components/layout/flow/mod.rs | 9 +-------- components/layout/flow/root.rs | 1 - components/layout/formatting_contexts.rs | 10 +--------- components/layout/positioned.rs | 1 - components/layout/replaced.rs | 3 +-- components/layout/table/layout.rs | 10 +--------- components/layout/taffy/layout.rs | 1 - tests/wpt/meta/MANIFEST.json | 13 +++++++++++++ ...stretched-child-in-nested-flexbox-003.html | 19 +++++++++++++++++++ 10 files changed, 50 insertions(+), 36 deletions(-) create mode 100644 tests/wpt/tests/css/css-flexbox/stretched-child-in-nested-flexbox-003.html diff --git a/components/layout/flexbox/layout.rs b/components/layout/flexbox/layout.rs index 265e6b8a530..15ccbac7147 100644 --- a/components/layout/flexbox/layout.rs +++ b/components/layout/flexbox/layout.rs @@ -627,7 +627,6 @@ impl FlexContainer { layout_context: &LayoutContext, positioning_context: &mut PositioningContext, containing_block: &ContainingBlock, - depends_on_block_constraints: bool, lazy_block_size: &LazySize, ) -> CacheableLayoutResult { let mut flex_context = FlexContext { @@ -953,6 +952,20 @@ impl FlexContainer { .or(all_baselines.last), }; + // TODO: `depends_on_block_constraints` could be false in some corner cases + // in order to improve performance. + // - In a single-line column container where all items have the grow and shrink + // factors set to zero and the flex basis doesn't depend on block constraints, + // and `justify-content` is `start` or equivalent. + // This is unlikely because the flex shrink factor defaults to 1. + // - In a single-line row container where all items have `align-self: start` or + // equivalent, and the cross size doesn't depend on block constraints. + // This is unlikely because `align-self` stretches by default. + // - In a multi-line row container where `align-content` is `start` or equivalent, + // and no item cross size depends on block constraints. + // This is unlikely because `align-content` defaults to `stretch`. + let depends_on_block_constraints = true; + CacheableLayoutResult { fragments, content_block_size, @@ -1880,9 +1893,6 @@ impl FlexItem<'_> { &item_as_containing_block, containing_block, self.preferred_aspect_ratio, - flex_axis == FlexAxis::Column || - self.cross_size_stretches_to_line || - self.depends_on_block_constraints, &lazy_block_size, ); let CacheableLayoutResult { @@ -2597,7 +2607,6 @@ impl FlexItemBox { &item_as_containing_block, flex_context.containing_block, preferred_aspect_ratio, - false, /* depends_on_block_constraints */ &LazySize::intrinsic(), ) .content_block_size diff --git a/components/layout/flow/mod.rs b/components/layout/flow/mod.rs index 604eb590e7c..e7a0ae7f908 100644 --- a/components/layout/flow/mod.rs +++ b/components/layout/flow/mod.rs @@ -346,7 +346,6 @@ impl OutsideMarker { layout_context, positioning_context, &containing_block_for_children, - false, /* depends_on_block_constraints */ ); let max_inline_size = @@ -429,7 +428,6 @@ impl BlockFormattingContext { layout_context: &LayoutContext, positioning_context: &mut PositioningContext, containing_block: &ContainingBlock, - depends_on_block_constraints: bool, ) -> CacheableLayoutResult { let mut sequential_layout_state = if self.contains_floats || !layout_context.use_rayon { Some(SequentialLayoutState::new(containing_block.size.inline)) @@ -470,8 +468,7 @@ impl BlockFormattingContext { clearance.unwrap_or_default(), content_inline_size_for_table: None, baselines: flow_layout.baselines, - depends_on_block_constraints: depends_on_block_constraints || - flow_layout.depends_on_block_constraints, + depends_on_block_constraints: flow_layout.depends_on_block_constraints, specific_layout_info: None, collapsible_margins_in_children: CollapsedBlockMargins::zero(), } @@ -1241,7 +1238,6 @@ impl IndependentFormattingContext { &containing_block_for_children, containing_block, preferred_aspect_ratio, - false, /* depends_on_block_constraints */ &lazy_block_size, ); @@ -1432,7 +1428,6 @@ impl IndependentFormattingContext { }, containing_block, preferred_aspect_ratio, - false, /* depends_on_block_constraints */ &lazy_block_size, ); @@ -1498,7 +1493,6 @@ impl IndependentFormattingContext { }, containing_block, preferred_aspect_ratio, - false, /* depends_on_block_constraints */ &lazy_block_size, ); @@ -2326,7 +2320,6 @@ impl IndependentFormattingContext { &containing_block_for_children, containing_block, preferred_aspect_ratio, - false, /* depends_on_block_constraints */ &lazy_block_size, ); diff --git a/components/layout/flow/root.rs b/components/layout/flow/root.rs index ce7e41cd2b1..4fb6a9c62a1 100644 --- a/components/layout/flow/root.rs +++ b/components/layout/flow/root.rs @@ -225,7 +225,6 @@ impl BoxTree { layout_context, &mut positioning_context, &(&initial_containing_block).into(), - false, /* depends_on_block_constraints */ ); let mut root_fragments = independent_layout.fragments.into_iter().collect::>(); diff --git a/components/layout/formatting_contexts.rs b/components/layout/formatting_contexts.rs index 8c91e90fd2b..577e0afbd50 100644 --- a/components/layout/formatting_contexts.rs +++ b/components/layout/formatting_contexts.rs @@ -254,7 +254,6 @@ impl IndependentFormattingContext { containing_block_for_children: &ContainingBlock, containing_block: &ContainingBlock, preferred_aspect_ratio: Option, - depends_on_block_constraints: bool, lazy_block_size: &LazySize, ) -> CacheableLayoutResult { match &self.contents { @@ -263,20 +262,17 @@ impl IndependentFormattingContext { containing_block_for_children, preferred_aspect_ratio, &self.base, - depends_on_block_constraints, lazy_block_size, ), IndependentFormattingContextContents::Flow(bfc) => bfc.layout( layout_context, positioning_context, containing_block_for_children, - depends_on_block_constraints, ), IndependentFormattingContextContents::Flex(fc) => fc.layout( layout_context, positioning_context, containing_block_for_children, - depends_on_block_constraints, lazy_block_size, ), IndependentFormattingContextContents::Grid(fc) => fc.layout( @@ -290,7 +286,6 @@ impl IndependentFormattingContext { positioning_context, containing_block_for_children, containing_block, - depends_on_block_constraints, ), } } @@ -307,7 +302,6 @@ impl IndependentFormattingContext { containing_block_for_children: &ContainingBlock, containing_block: &ContainingBlock, preferred_aspect_ratio: Option, - depends_on_block_constraints: bool, lazy_block_size: &LazySize, ) -> CacheableLayoutResult { if let Some(cache) = self.base.cached_layout_result.borrow().as_ref() { @@ -316,8 +310,7 @@ impl IndependentFormattingContext { containing_block_for_children.size.inline && (cache.containing_block_for_children_size.block == containing_block_for_children.size.block || - !(cache.result.depends_on_block_constraints || - depends_on_block_constraints)) + !cache.result.depends_on_block_constraints) { positioning_context.append(cache.positioning_context.clone()); return cache.result.clone(); @@ -337,7 +330,6 @@ impl IndependentFormattingContext { containing_block_for_children, containing_block, preferred_aspect_ratio, - depends_on_block_constraints, lazy_block_size, ); diff --git a/components/layout/positioned.rs b/components/layout/positioned.rs index b6dbf01ffab..b7cb5bff16a 100644 --- a/components/layout/positioned.rs +++ b/components/layout/positioned.rs @@ -584,7 +584,6 @@ impl HoistedAbsolutelyPositionedBox { &containing_block_for_children, containing_block, preferred_aspect_ratio, - false, /* depends_on_block_constraints */ &lazy_block_size, ); diff --git a/components/layout/replaced.rs b/components/layout/replaced.rs index 05c5bdf3c40..254ca0bfe3e 100644 --- a/components/layout/replaced.rs +++ b/components/layout/replaced.rs @@ -451,7 +451,6 @@ impl ReplacedContents { containing_block_for_children: &ContainingBlock, preferred_aspect_ratio: Option, base: &LayoutBoxBase, - depends_on_block_constraints: bool, lazy_block_size: &LazySize, ) -> CacheableLayoutResult { let writing_mode = base.style.writing_mode; @@ -472,7 +471,7 @@ impl ReplacedContents { collapsible_margins_in_children: CollapsedBlockMargins::zero(), content_block_size, content_inline_size_for_table: None, - depends_on_block_constraints, + depends_on_block_constraints: false, fragments: self.make_fragments(layout_context, &base.style, size), specific_layout_info: None, } diff --git a/components/layout/table/layout.rs b/components/layout/table/layout.rs index 9e80f322610..9190c6932f7 100644 --- a/components/layout/table/layout.rs +++ b/components/layout/table/layout.rs @@ -1114,7 +1114,6 @@ impl<'a> TableLayout<'a> { layout_context, &mut positioning_context, &containing_block_for_children, - false, /* depends_on_block_constraints */ ); Some(CellLayout { @@ -1545,15 +1544,10 @@ impl<'a> TableLayout<'a> { positioning_context: &mut PositioningContext, containing_block_for_children: &ContainingBlock, containing_block_for_table: &ContainingBlock, - depends_on_block_constraints: bool, ) -> CacheableLayoutResult { let table_writing_mode = containing_block_for_children.style.writing_mode; self.compute_border_collapse(table_writing_mode); let layout_style = self.table.layout_style(Some(&self)); - let depends_on_block_constraints = depends_on_block_constraints || - layout_style - .content_box_sizes_and_padding_border_margin(&containing_block_for_table.into()) - .depends_on_block_constraints; self.pbm = layout_style .padding_border_margin_with_writing_mode_and_containing_block_inline_size( @@ -1592,7 +1586,7 @@ impl<'a> TableLayout<'a> { content_block_size: Zero::zero(), content_inline_size_for_table: None, baselines: Baselines::default(), - depends_on_block_constraints, + depends_on_block_constraints: true, specific_layout_info: Some(SpecificLayoutInfo::TableWrapper), collapsible_margins_in_children: CollapsedBlockMargins::zero(), }; @@ -2700,14 +2694,12 @@ impl Table { positioning_context: &mut PositioningContext, containing_block_for_children: &ContainingBlock, containing_block_for_table: &ContainingBlock, - depends_on_block_constraints: bool, ) -> CacheableLayoutResult { TableLayout::new(self).layout( layout_context, positioning_context, containing_block_for_children, containing_block_for_table, - depends_on_block_constraints, ) } } diff --git a/components/layout/taffy/layout.rs b/components/layout/taffy/layout.rs index fa40f2547a2..7672b317434 100644 --- a/components/layout/taffy/layout.rs +++ b/components/layout/taffy/layout.rs @@ -205,7 +205,6 @@ impl taffy::LayoutPartialTree for TaffyContainerContext<'_> { &content_box_size_override, containing_block, preferred_aspect_ratio, - false, /* depends_on_block_constraints */ &lazy_block_size, ); diff --git a/tests/wpt/meta/MANIFEST.json b/tests/wpt/meta/MANIFEST.json index 69db7787804..abb70ae5e86 100644 --- a/tests/wpt/meta/MANIFEST.json +++ b/tests/wpt/meta/MANIFEST.json @@ -183398,6 +183398,19 @@ {} ] ], + "stretched-child-in-nested-flexbox-003.html": [ + "7f8cd10da3afd7812353ac39eff15194f328058a", + [ + null, + [ + [ + "/css/reference/ref-filled-green-200px-square.html", + "==" + ] + ], + {} + ] + ], "stretching-orthogonal-flows.html": [ "acaa787faf657b76a5b213fb9203088d5d28f4c4", [ diff --git a/tests/wpt/tests/css/css-flexbox/stretched-child-in-nested-flexbox-003.html b/tests/wpt/tests/css/css-flexbox/stretched-child-in-nested-flexbox-003.html new file mode 100644 index 00000000000..7f8cd10da3a --- /dev/null +++ b/tests/wpt/tests/css/css-flexbox/stretched-child-in-nested-flexbox-003.html @@ -0,0 +1,19 @@ + +css-flexbox: stretching of flex item in nested flexbox + + + + + +

Test passes if there is a filled green square and no red.

+
+
+
+
+
+
+