From 95832c999ba1076103b8e65327031a6af797d021 Mon Sep 17 00:00:00 2001 From: Oriol Brufau Date: Fri, 23 Jun 2023 19:56:55 +0200 Subject: [PATCH] Layout 2020: Properly calculate clearance calculate_clearance() was not taking into account that adding clearance prevents top margin from collapsing with earlier margins. --- components/layout_2020/flow/float.rs | 68 ++++++++++++++----- components/layout_2020/flow/mod.rs | 25 ++++--- .../clear-on-child-with-margins.html.ini | 2 - .../clear-on-parent-with-margins.html.ini | 2 - ...ive-clearance-after-bottom-margin.html.ini | 2 - ...-clearance-new-formatting-context.html.ini | 2 - 6 files changed, 66 insertions(+), 35 deletions(-) delete mode 100644 tests/wpt/meta/css/CSS2/floats-clear/clear-on-child-with-margins.html.ini delete mode 100644 tests/wpt/meta/css/CSS2/floats-clear/clear-on-parent-with-margins.html.ini delete mode 100644 tests/wpt/meta/css/CSS2/floats-clear/negative-clearance-after-bottom-margin.html.ini delete mode 100644 tests/wpt/meta/css/CSS2/floats-clear/nested-clearance-new-formatting-context.html.ini diff --git a/components/layout_2020/flow/float.rs b/components/layout_2020/flow/float.rs index 0383c23bad7..031ff1883e4 100644 --- a/components/layout_2020/flow/float.rs +++ b/components/layout_2020/flow/float.rs @@ -821,37 +821,73 @@ impl SequentialLayoutState { self.current_margin = CollapsedMargin::zero(); } - /// Returns the amount of clearance that a block with the given `clear` value at the current - /// `bfc_relative_block_position` (with top margin included in `current_margin` if applicable) - /// needs to have. + /// Returns the amount of clearance (if any) that a block with the given `clear` value + /// needs to have at `current_block_position_including_margins()`. + /// `block_start_margin` is the top margin of the block, after collapsing (if possible) + /// with the margin of its contents. This must not be included in `current_margin`, + /// since adding clearance will prevent `current_margin` and `block_start_margin` + /// from collapsing together. /// /// https://www.w3.org/TR/2011/REC-CSS2-20110607/visuren.html#flow-control - pub(crate) fn calculate_clearance(&self, clear_side: ClearSide) -> Option { + pub(crate) fn calculate_clearance( + &self, + clear_side: ClearSide, + block_start_margin: &CollapsedMargin, + ) -> Option { if clear_side == ClearSide::None { return None; } - let hypothetical_block_position = self.current_block_position_including_margins(); + // Calculate the hypothetical position where the element's top border edge + // would have been if the element's `clear` property had been `none`. + let hypothetical_block_position = self.bfc_relative_block_position + + self.current_margin.adjoin(&block_start_margin).solve(); + + // Check if the hypothetical position is past the relevant floats, + // in that case we don't need to add clearance. let clear_position = match clear_side { ClearSide::None => unreachable!(), - ClearSide::Left => self - .floats - .clear_left_position - .max(hypothetical_block_position), - ClearSide::Right => self - .floats - .clear_right_position - .max(hypothetical_block_position), + ClearSide::Left => self.floats.clear_left_position, + ClearSide::Right => self.floats.clear_right_position, ClearSide::Both => self .floats .clear_left_position - .max(self.floats.clear_right_position) - .max(hypothetical_block_position), + .max(self.floats.clear_right_position), }; if hypothetical_block_position >= clear_position { return None; } - Some(clear_position - hypothetical_block_position) + + // We need to add clearance between `current_margin` and `block_start_margin`, + // this prevents them from collapsing. + // Compute the position of the element's top border edge if we added + // a clearance of 0px. + let position_with_zero_clearance = self.bfc_relative_block_position + + self.current_margin.solve() + + block_start_margin.solve(); + + // Set clearance to the amount necessary to place the border edge of the block + // even with the bottom outer edge of the lowest float that is to be cleared. + // Note that CSS2 also allows the alternative option of flooring this amount by + // `hypothetical_block_position - position_with_zero_clearance`, + // but other browser don't seem to do that. + Some(clear_position - position_with_zero_clearance) + } + + /// Helper function that computes the clearance and adds `block_start_margin` accordingly. + /// If there is no clearance, `block_start_margin` can just be adjoined to `current_margin`. + /// But clearance prevents them from collapsing, so `collapse_margins()` is called. + pub(crate) fn calculate_clearance_and_adjoin_margin( + &mut self, + style: &Arc, + block_start_margin: &CollapsedMargin, + ) -> Option { + let clearance = self.calculate_clearance(ClearSide::from_style(style), &block_start_margin); + if clearance.is_some() { + self.collapse_margins(); + } + self.adjoin_assign(&block_start_margin); + clearance } /// Adds a new adjoining margin. diff --git a/components/layout_2020/flow/mod.rs b/components/layout_2020/flow/mod.rs index 121fdbe506a..47fbd9a2c2f 100644 --- a/components/layout_2020/flow/mod.rs +++ b/components/layout_2020/flow/mod.rs @@ -191,7 +191,7 @@ impl BlockFormattingContext { // (https://drafts.csswg.org/css2/#root-height), we implement this by imagining there is // an element with `clear: both` after the actual contents. let clearance = sequential_layout_state.and_then(|sequential_layout_state| { - sequential_layout_state.calculate_clearance(ClearSide::Both) + sequential_layout_state.calculate_clearance(ClearSide::Both, &CollapsedMargin::zero()) }); IndependentLayout { @@ -657,7 +657,7 @@ fn layout_in_flow_non_replaced_block_level( match sequential_layout_state { None => parent_containing_block_position_info = None, Some(ref mut sequential_layout_state) => { - sequential_layout_state.adjoin_assign(&CollapsedMargin::new(margin.block_start)); + let mut block_start_margin = CollapsedMargin::new(margin.block_start); if start_margin_can_collapse_with_children { if let NonReplacedContents::SameFormattingContextBlock( BlockContainer::BlockLevelBoxes(child_boxes), @@ -665,21 +665,20 @@ fn layout_in_flow_non_replaced_block_level( { // The block start margin may collapse with content margins, // compute the resulting one in order to place floats correctly. - let mut future_margins = CollapsedMargin::zero(); BlockLevelBox::find_block_margin_collapsing_with_parent_from_slice( child_boxes, - &mut future_margins, + &mut block_start_margin, WritingMode::empty(), ); - sequential_layout_state.adjoin_assign(&future_margins); } - } else { - sequential_layout_state.collapse_margins(); } // Introduce clearance if necessary. - let clear_side = ClearSide::from_style(style); - clearance = sequential_layout_state.calculate_clearance(clear_side); + clearance = sequential_layout_state + .calculate_clearance_and_adjoin_margin(style, &block_start_margin); + if !start_margin_can_collapse_with_children { + sequential_layout_state.collapse_margins(); + } // NB: This will be a no-op if we're collapsing margins with our children since that // can only happen if we have no block-start padding and border. @@ -844,9 +843,11 @@ fn layout_in_flow_replaced_block_level<'a>( let mut clearance = None; if let Some(ref mut sequential_layout_state) = sequential_layout_state { - sequential_layout_state.adjoin_assign(&CollapsedMargin::new(margin.block_start)); + clearance = sequential_layout_state.calculate_clearance_and_adjoin_margin( + style, + &CollapsedMargin::new(margin.block_start), + ); sequential_layout_state.collapse_margins(); - clearance = sequential_layout_state.calculate_clearance(ClearSide::from_style(style)); sequential_layout_state.advance_block_position( pbm.border.block_sum() + pbm.padding.block_sum() + @@ -943,6 +944,8 @@ impl PlacementState { // Setting `next_in_flow_margin_collapses_with_parent_start_margin` to false // prevents collapsing with the start margin of the parent, and will set // `collapsed_through` to false, preventing the parent from collapsing through. + self.current_block_direction_position += self.current_margin.solve(); + self.current_margin = CollapsedMargin::zero(); self.next_in_flow_margin_collapses_with_parent_start_margin = false; if fragment_block_margins.collapsed_through { self.last_in_flow_margin_collapses_with_parent_end_margin = false; diff --git a/tests/wpt/meta/css/CSS2/floats-clear/clear-on-child-with-margins.html.ini b/tests/wpt/meta/css/CSS2/floats-clear/clear-on-child-with-margins.html.ini deleted file mode 100644 index a7392b97207..00000000000 --- a/tests/wpt/meta/css/CSS2/floats-clear/clear-on-child-with-margins.html.ini +++ /dev/null @@ -1,2 +0,0 @@ -[clear-on-child-with-margins.html] - expected: FAIL diff --git a/tests/wpt/meta/css/CSS2/floats-clear/clear-on-parent-with-margins.html.ini b/tests/wpt/meta/css/CSS2/floats-clear/clear-on-parent-with-margins.html.ini deleted file mode 100644 index 6a41158fa84..00000000000 --- a/tests/wpt/meta/css/CSS2/floats-clear/clear-on-parent-with-margins.html.ini +++ /dev/null @@ -1,2 +0,0 @@ -[clear-on-parent-with-margins.html] - expected: FAIL diff --git a/tests/wpt/meta/css/CSS2/floats-clear/negative-clearance-after-bottom-margin.html.ini b/tests/wpt/meta/css/CSS2/floats-clear/negative-clearance-after-bottom-margin.html.ini deleted file mode 100644 index 85b954771e1..00000000000 --- a/tests/wpt/meta/css/CSS2/floats-clear/negative-clearance-after-bottom-margin.html.ini +++ /dev/null @@ -1,2 +0,0 @@ -[negative-clearance-after-bottom-margin.html] - expected: FAIL diff --git a/tests/wpt/meta/css/CSS2/floats-clear/nested-clearance-new-formatting-context.html.ini b/tests/wpt/meta/css/CSS2/floats-clear/nested-clearance-new-formatting-context.html.ini deleted file mode 100644 index 082ea2659a1..00000000000 --- a/tests/wpt/meta/css/CSS2/floats-clear/nested-clearance-new-formatting-context.html.ini +++ /dev/null @@ -1,2 +0,0 @@ -[nested-clearance-new-formatting-context.html] - expected: FAIL