Auto merge of #29949 - Loirooriol:proper-clearance, r=mrobinson

Layout 2020: Properly calculate clearance

<!-- Please describe your changes on the following line: -->
calculate_clearance() was not taking into account that adding clearance prevents top margin from collapsing with earlier margins.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #29885 and #29919
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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<Length> {
+    pub(crate) fn calculate_clearance(
+        &self,
+        clear_side: ClearSide,
+        block_start_margin: &CollapsedMargin,
+    ) -> Option<Length> {
         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<ComputedValues>,
+        block_start_margin: &CollapsedMargin,
+    ) -> Option<Length> {
+        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.

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;

tests/wpt/meta/css/CSS2/floats-clear/clear-on-child-with-margins.html.ini

@@ -1,2 +0,0 @@
-[clear-on-child-with-margins.html]
-  expected: FAIL

tests/wpt/meta/css/CSS2/floats-clear/clear-on-parent-with-margins.html.ini

@@ -1,2 +0,0 @@
-[clear-on-parent-with-margins.html]
-  expected: FAIL

tests/wpt/meta/css/CSS2/floats-clear/negative-clearance-after-bottom-margin.html.ini

@@ -1,2 +0,0 @@
-[negative-clearance-after-bottom-margin.html]
-  expected: FAIL

tests/wpt/meta/css/CSS2/floats-clear/nested-clearance-new-formatting-context.html.ini

@@ -1,2 +0,0 @@
-[nested-clearance-new-formatting-context.html]
-  expected: FAIL