From ec88b5d752e0ed4aed5fcea2620ca520f271fd99 Mon Sep 17 00:00:00 2001 From: haval0 <56519858+haval0@users.noreply.github.com> Date: Tue, 29 Apr 2025 21:19:31 +0200 Subject: [PATCH] layout: Check if root before establishing containing block (#36360) As per [w3.org/TR/filter-effects-1#FilterProperty](https://www.w3.org/TR/filter-effects-1/#FilterProperty), `filter` shouldn't make the root element establish a containing block for absolute and fixed positioned descendants. `will-change: filter` has matching behavior. This PR adds a check for if we are the root element before establishing such a block. To know if we are the root element, we look at the `FragmentFlags` passed in. Previously for our function, these were dummy flags, always constructed as empty. Thus, this PR also makes sure the correct FragmentFlags are passed down the chain to the function `establishes_containing_block_for_all_descendants`. Testing: - `/css/filter-effects/filtered-html-is-not-container.html` now passes - `/css/css-will-change/will-change-fixedpos-cb-003.html` now passes - Manual tests are working Fixes: #35391 --------- Signed-off-by: haval0 <56519858+haval0@users.noreply.github.com> Signed-off-by: Oriol Brufau Co-authored-by: Oriol Brufau --- components/layout/flexbox/layout.rs | 9 +++-- components/layout/flow/float.rs | 3 +- components/layout/flow/inline/line.rs | 3 +- components/layout/flow/inline/mod.rs | 3 +- components/layout/flow/mod.rs | 4 +-- components/layout/positioned.rs | 36 ++++++++++++------- components/layout/style_ext.rs | 6 ++-- components/layout/table/layout.rs | 6 ++-- components/layout/taffy/layout.rs | 5 +-- .../will-change-fixedpos-cb-003.html.ini | 2 -- .../filtered-html-is-not-container.html.ini | 2 -- 11 files changed, 44 insertions(+), 35 deletions(-) delete mode 100644 tests/wpt/meta/css/css-will-change/will-change-fixedpos-cb-003.html.ini delete mode 100644 tests/wpt/meta/css/filter-effects/filtered-html-is-not-container.html.ini diff --git a/components/layout/flexbox/layout.rs b/components/layout/flexbox/layout.rs index 77069236787..a5540123681 100644 --- a/components/layout/flexbox/layout.rs +++ b/components/layout/flexbox/layout.rs @@ -1774,7 +1774,9 @@ impl FlexItem<'_> { non_stretch_layout_result: Option<&mut FlexItemLayoutResult>, ) -> Option { let containing_block = flex_context.containing_block; - let mut positioning_context = PositioningContext::new_for_style(self.box_.style()) + let independent_formatting_context = &self.box_.independent_formatting_context; + let mut positioning_context = independent_formatting_context + .new_positioning_context() .unwrap_or_else(|| { PositioningContext::new_for_subtree( flex_context @@ -1783,7 +1785,6 @@ impl FlexItem<'_> { ) }); - let independent_formatting_context = &self.box_.independent_formatting_context; let item_writing_mode = independent_formatting_context.style().writing_mode; let item_is_horizontal = item_writing_mode.is_horizontal(); let flex_axis = flex_context.config.flex_axis; @@ -2616,7 +2617,9 @@ impl FlexItemBox { cross_size_stretches_to_container_size: bool, intrinsic_sizing_mode: IntrinsicSizingMode, ) -> Au { - let mut positioning_context = PositioningContext::new_for_style(self.style()) + let mut positioning_context = self + .independent_formatting_context + .new_positioning_context() .unwrap_or_else(|| { PositioningContext::new_for_subtree( flex_context diff --git a/components/layout/flow/float.rs b/components/layout/flow/float.rs index 0570ce0d0f4..dbc50c07603 100644 --- a/components/layout/flow/float.rs +++ b/components/layout/flow/float.rs @@ -913,11 +913,10 @@ impl FloatBox { positioning_context: &mut PositioningContext, containing_block: &ContainingBlock, ) -> BoxFragment { - let style = self.contents.style().clone(); positioning_context.layout_maybe_position_relative_fragment( layout_context, containing_block, - &style, + &self.contents.base, |positioning_context| { self.contents .layout_float_or_atomic_inline( diff --git a/components/layout/flow/inline/line.rs b/components/layout/flow/inline/line.rs index c42f32c9242..e65eaed2367 100644 --- a/components/layout/flow/inline/line.rs +++ b/components/layout/flow/inline/line.rs @@ -326,13 +326,12 @@ impl LineItemLayout<'_, '_> { let inline_box = self.layout.ifc.inline_boxes.get(identifier); let inline_box = &*(inline_box.borrow()); - let style = &inline_box.base.style; let space_above_baseline = inline_box_state.calculate_space_above_baseline(); let block_start_offset = self.calculate_inline_box_block_start(inline_box_state, space_above_baseline); let positioning_context_or_start_offset_in_parent = - match PositioningContext::new_for_style(style) { + match inline_box.base.new_positioning_context() { Some(positioning_context) => Either::Left(positioning_context), None => Either::Right(self.current_positioning_context_mut().len()), }; diff --git a/components/layout/flow/inline/mod.rs b/components/layout/flow/inline/mod.rs index dabb9773410..25fbaa324b1 100644 --- a/components/layout/flow/inline/mod.rs +++ b/components/layout/flow/inline/mod.rs @@ -2004,7 +2004,8 @@ impl IndependentFormattingContext { bidi_level: Level, ) { // We need to know the inline size of the atomic before deciding whether to do the line break. - let mut child_positioning_context = PositioningContext::new_for_style(self.style()) + let mut child_positioning_context = self + .new_positioning_context() .unwrap_or_else(|| PositioningContext::new_for_subtree(true)); let IndependentFloatOrAtomicLayoutResult { mut fragment, diff --git a/components/layout/flow/mod.rs b/components/layout/flow/mod.rs index d983e8592c4..983282dc389 100644 --- a/components/layout/flow/mod.rs +++ b/components/layout/flow/mod.rs @@ -779,7 +779,7 @@ impl BlockLevelBox { ArcRefCell::new(positioning_context.layout_maybe_position_relative_fragment( layout_context, containing_block, - &base.style, + base, |positioning_context| { layout_in_flow_non_replaced_block_level_same_formatting_context( layout_context, @@ -798,7 +798,7 @@ impl BlockLevelBox { positioning_context.layout_maybe_position_relative_fragment( layout_context, containing_block, - independent.style(), + &independent.base, |positioning_context| { independent.layout_in_flow_block_level( layout_context, diff --git a/components/layout/positioned.rs b/components/layout/positioned.rs index 5f08e4e86c5..6bfe2af87ef 100644 --- a/components/layout/positioned.rs +++ b/components/layout/positioned.rs @@ -29,6 +29,7 @@ use crate::geom::{ PhysicalPoint, PhysicalRect, PhysicalSides, PhysicalSize, PhysicalVec, Size, Sizes, ToLogical, ToLogicalWithContainingBlock, }; +use crate::layout_box_base::LayoutBoxBase; use crate::sizing::ContentSizes; use crate::style_ext::{Clamp, ComputedValuesExt, ContentBoxSizesAndPBM, DisplayInside}; use crate::{ @@ -103,6 +104,20 @@ impl AbsolutelyPositionedBox { } } +impl IndependentFormattingContext { + #[inline] + pub(crate) fn new_positioning_context(&self) -> Option { + self.base.new_positioning_context() + } +} + +impl LayoutBoxBase { + #[inline] + pub(crate) fn new_positioning_context(&self) -> Option { + PositioningContext::new_for_style(&self.style, &self.base_fragment_info.flags) + } +} + impl PositioningContext { pub(crate) fn new_for_containing_block_for_all_descendants() -> Self { Self { @@ -130,14 +145,10 @@ impl PositioningContext { self.for_nearest_positioned_ancestor.is_some() } - pub(crate) fn new_for_style(style: &ComputedValues) -> Option { - // NB: We never make PositioningContexts for replaced elements, which is why we always - // pass false here. - if style.establishes_containing_block_for_all_descendants(FragmentFlags::empty()) { + fn new_for_style(style: &ComputedValues, flags: &FragmentFlags) -> Option { + if style.establishes_containing_block_for_all_descendants(*flags) { Some(Self::new_for_containing_block_for_all_descendants()) - } else if style - .establishes_containing_block_for_absolute_descendants(FragmentFlags::empty()) - { + } else if style.establishes_containing_block_for_absolute_descendants(*flags) { Some(Self { for_nearest_positioned_ancestor: Some(Vec::new()), for_nearest_containing_block_for_all_descendants: Vec::new(), @@ -213,12 +224,12 @@ impl PositioningContext { &mut self, layout_context: &LayoutContext, containing_block: &ContainingBlock, - style: &ComputedValues, + base: &LayoutBoxBase, fragment_layout_fn: impl FnOnce(&mut Self) -> BoxFragment, ) -> BoxFragment { // Try to create a context, but if one isn't necessary, simply create the fragment // using the given closure and the current `PositioningContext`. - let mut new_context = match Self::new_for_style(style) { + let mut new_context = match base.new_positioning_context() { Some(new_context) => new_context, None => return fragment_layout_fn(self), }; @@ -229,9 +240,8 @@ impl PositioningContext { // If the new context has any hoisted boxes for the nearest containing block for // pass them up the tree. self.append(new_context); - - if style.clone_position() == Position::Relative { - new_fragment.content_rect.origin += relative_adjustement(style, containing_block) + if base.style.clone_position() == Position::Relative { + new_fragment.content_rect.origin += relative_adjustement(&base.style, containing_block) .to_physical_vector(containing_block.style.writing_mode) } @@ -586,7 +596,7 @@ impl HoistedAbsolutelyPositionedBox { .sizes })); - let mut positioning_context = PositioningContext::new_for_style(&style).unwrap(); + let mut positioning_context = context.new_positioning_context().unwrap(); let mut new_fragment = { let content_size: LogicalVec2; let fragments; diff --git a/components/layout/style_ext.rs b/components/layout/style_ext.rs index 33a22cdf438..023db6b07f1 100644 --- a/components/layout/style_ext.rs +++ b/components/layout/style_ext.rs @@ -845,9 +845,9 @@ impl ComputedValuesExt for ComputedValues { // > A value other than none for the filter property results in the creation of a containing // > block for absolute and fixed positioned descendants unless the element it applies to is // > a document root element in the current browsing context. - // FIXME(#35391): Need to check if this is the root element. - if !self.get_effects().filter.0.is_empty() || - will_change_bits.intersects(WillChangeBits::FIXPOS_CB_NON_SVG) + if !fragment_flags.contains(FragmentFlags::IS_ROOT_ELEMENT) && + (!self.get_effects().filter.0.is_empty() || + will_change_bits.intersects(WillChangeBits::FIXPOS_CB_NON_SVG)) { return true; } diff --git a/components/layout/table/layout.rs b/components/layout/table/layout.rs index 2261f7d165c..0cbe3e9ca76 100644 --- a/components/layout/table/layout.rs +++ b/components/layout/table/layout.rs @@ -1503,7 +1503,7 @@ impl<'a> TableLayout<'a> { layout_context: &LayoutContext, parent_positioning_context: &mut PositioningContext, ) -> BoxFragment { - let mut positioning_context = PositioningContext::new_for_style(caption.context.style()); + let mut positioning_context = caption.context.new_positioning_context(); let containing_block = &ContainingBlock { size: ContainingBlockSize { inline: self.table_width + self.pbm.padding_border_sums.inline, @@ -2325,7 +2325,7 @@ impl<'a> RowFragmentLayout<'a> { Self { row: table_row, rect, - positioning_context: PositioningContext::new_for_style(&table_row.base.style), + positioning_context: table_row.base.new_positioning_context(), containing_block, fragments: Vec::new(), } @@ -2410,7 +2410,7 @@ impl RowGroupFragmentLayout { let row_group = row_group.borrow(); ( dimensions.get_row_group_rect(&row_group), - PositioningContext::new_for_style(&row_group.base.style), + row_group.base.new_positioning_context(), ) }; Self { diff --git a/components/layout/taffy/layout.rs b/components/layout/taffy/layout.rs index a7581136bf2..3777c902053 100644 --- a/components/layout/taffy/layout.rs +++ b/components/layout/taffy/layout.rs @@ -251,8 +251,9 @@ impl taffy::LayoutPartialTree for TaffyContainerContext<'_> { style, }; let layout = { - let mut child_positioning_context = - PositioningContext::new_for_style(style).unwrap_or_else(|| { + let mut child_positioning_context = independent_context + .new_positioning_context() + .unwrap_or_else(|| { PositioningContext::new_for_subtree( self.positioning_context .collects_for_nearest_positioned_ancestor(), diff --git a/tests/wpt/meta/css/css-will-change/will-change-fixedpos-cb-003.html.ini b/tests/wpt/meta/css/css-will-change/will-change-fixedpos-cb-003.html.ini deleted file mode 100644 index 77a5d26e728..00000000000 --- a/tests/wpt/meta/css/css-will-change/will-change-fixedpos-cb-003.html.ini +++ /dev/null @@ -1,2 +0,0 @@ -[will-change-fixedpos-cb-003.html] - expected: FAIL diff --git a/tests/wpt/meta/css/filter-effects/filtered-html-is-not-container.html.ini b/tests/wpt/meta/css/filter-effects/filtered-html-is-not-container.html.ini deleted file mode 100644 index b366310b98b..00000000000 --- a/tests/wpt/meta/css/filter-effects/filtered-html-is-not-container.html.ini +++ /dev/null @@ -1,2 +0,0 @@ -[filtered-html-is-not-container.html] - expected: FAIL