Fix interaction of margins and clearance for PlacementAmongFloats (#30038)

Consumers of PlacementAmongFloats weren't handling margins properly.
They were assuming that they would either get a positive adjustment,
or zero for no-op.

However, just like the regular clearance triggered by 'clear', the
clearance added onto blocks that establish an independent FC can be
zero or negative, and the effect is different than having no clearance.

components/layout_2020/flow/float.rs

@@ -905,15 +905,24 @@ impl SequentialLayoutState {
         self.current_margin = CollapsedMargin::zero();
     }
 
-    /// Returns the amount of clearance (if any) that a block with the given `clear` value
+    /// Computes the position of the block-start border edge of an element
-    /// needs to have at `current_block_position_including_margins()`.
+    /// with the provided `block_start_margin`, assuming no clearance.
-    /// `block_start_margin` is the top margin of the block, after collapsing (if possible)
+    fn position_without_clearance(&self, block_start_margin: &CollapsedMargin) -> Length {
-    /// with the margin of its contents. This must not be included in `current_margin`,
+        // Adjoin `current_margin` and `block_start_margin` since there is no clearance.
-    /// since adding clearance will prevent `current_margin` and `block_start_margin`
+        self.bfc_relative_block_position + self.current_margin.adjoin(&block_start_margin).solve()
-    /// from collapsing together.
+    }
-    ///
+
-    /// https://www.w3.org/TR/2011/REC-CSS2-20110607/visuren.html#flow-control
+    /// Computes the position of the block-start border edge of an element
-    pub(crate) fn calculate_clearance(
+    /// with the provided `block_start_margin`, assuming a clearance of 0px.
+    fn position_with_zero_clearance(&self, block_start_margin: &CollapsedMargin) -> Length {
+        // Clearance prevents `current_margin` and `block_start_margin` from being
+        // adjoining, so we need to solve them separately and then sum.
+        self.bfc_relative_block_position + self.current_margin.solve() + block_start_margin.solve()
+    }
+
+    /// Returns the block-end outer edge of the lowest float that is to be cleared (if any)
+    /// by an element with the provided `clear` and `block_start_margin`.
+    fn calculate_clear_position(
         &self,
         clear: Clear,
         block_start_margin: &CollapsedMargin,
@@ -924,8 +933,7 @@ impl SequentialLayoutState {
 
         // 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 +
+        let hypothetical_block_position = self.position_without_clearance(&block_start_margin);
-            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.
@@ -939,23 +947,60 @@ impl SequentialLayoutState {
                 .max(self.floats.clear_right_position),
         };
         if hypothetical_block_position >= clear_position {
-            return None;
+            None
+        } else {
+            Some(clear_position)
         }
+    }
 
-        // We need to add clearance between `current_margin` and `block_start_margin`,
+    /// Returns the amount of clearance (if any) that a block with the given `clear` value
-        // this prevents them from collapsing.
+    /// needs to have at `current_block_position_including_margins()`.
-        // Compute the position of the element's top border edge if we added
+    /// `block_start_margin` is the top margin of the block, after collapsing (if possible)
-        // a clearance of 0px.
+    /// with the margin of its contents. This must not be included in `current_margin`,
-        let position_with_zero_clearance = self.bfc_relative_block_position +
+    /// since adding clearance will prevent `current_margin` and `block_start_margin`
-            self.current_margin.solve() +
+    /// from collapsing together.
-            block_start_margin.solve();
+    ///
+    /// <https://www.w3.org/TR/2011/REC-CSS2-20110607/visuren.html#flow-control>
+    pub(crate) fn calculate_clearance(
+        &self,
+        clear: Clear,
+        block_start_margin: &CollapsedMargin,
+    ) -> Option<Length> {
+        return self
+            .calculate_clear_position(clear, &block_start_margin)
+            .map(|offset| offset - self.position_with_zero_clearance(&block_start_margin));
+    }
 
-        // Set clearance to the amount necessary to place the border edge of the block
+    /// A block that is replaced or establishes an independent formatting context can't overlap floats,
-        // even with the bottom outer edge of the lowest float that is to be cleared.
+    /// it has to be placed next to them, and may get some clearance if there isn't enough space.
-        // Note that CSS2 also allows the alternative option of flooring this amount by
+    /// Given such a block with the provided 'clear', 'block_start_margin' and 'object_size',
-        // `hypothetical_block_position - position_with_zero_clearance`,
+    /// this method finds an area that is big enough and doesn't overlap floats.
-        // but other browser don't seem to do that.
+    /// It returns a tuple with:
-        Some(clear_position - position_with_zero_clearance)
+    /// - The clearance amount (if any), which includes both the effect of 'clear'
+    ///   and the extra space to avoid floats.
+    /// - The inline offset from the containing block that we need to avoid floats.
+    pub(crate) fn calculate_clearance_and_inline_adjustment(
+        &self,
+        clear: Clear,
+        block_start_margin: &CollapsedMargin,
+        object_size: Vec2<Length>,
+    ) -> (Option<Length>, Length) {
+        // First compute the clear position required by the 'clear' property.
+        // The code below may then add extra clearance when the element can't fit
+        // next to floats not covered by 'clear'.
+        let clear_position = self.calculate_clear_position(clear, &block_start_margin);
+        let ceiling =
+            clear_position.unwrap_or_else(|| self.position_without_clearance(&block_start_margin));
+        let mut placement = PlacementAmongFloats::new(&self.floats, ceiling, object_size);
+        let position = placement.place();
+        let has_clearance = clear_position.is_some() || position.block > ceiling;
+        let clearance = if has_clearance {
+            Some(position.block - self.position_with_zero_clearance(&block_start_margin))
+        } else {
+            None
+        };
+        let inline_adjustment = position.inline - self.floats.containing_block_info.inline_start;
+        (clearance, inline_adjustment)
     }
 
     /// Adds a new adjoining margin.

components/layout_2020/flow/mod.rs

@@ -4,7 +4,6 @@
 
 //! Flow layout, also known as block-and-inline layout.
 
-use self::float::PlacementAmongFloats;
 use crate::cell::ArcRefCell;
 use crate::context::LayoutContext;
 use crate::flow::float::{ContainingBlockPositionInfo, FloatBox, SequentialLayoutState};
@@ -94,7 +93,11 @@ impl BlockLevelBox {
             BlockLevelBox::SameFormattingContextBlock { ref style, .. } => &style,
             BlockLevelBox::OutOfFlowAbsolutelyPositionedBox(_) |
             BlockLevelBox::OutOfFlowFloatBox(_) => return true,
-            BlockLevelBox::Independent(ref context) => context.style(),
+            BlockLevelBox::Independent(ref context) => {
+                // FIXME: If the element doesn't fit next to floats, it will get clearance.
+                // In that case this should be returning false.
+                context.style()
+            },
         };
 
         // FIXME: This should only return false when 'clear' causes clearance.
@@ -879,10 +882,6 @@ impl NonReplacedFormattingContext {
             &self.style,
         );
 
-        let block_start_margin = CollapsedMargin::new(margin.block_start);
-        let clearance = sequential_layout_state
-            .calculate_clearance(self.style.get_box().clear, &block_start_margin);
-
         let layout = self.layout(
             layout_context,
             positioning_context,
@@ -904,70 +903,35 @@ impl NonReplacedFormattingContext {
         //  sufficient space. They may even make the border box of said element narrower
         //  than defined by section 10.3.3. CSS 2 does not define when a UA may put said
         //  element next to the float or by how much said element may become narrower."
-        let mut adjustment_from_floats = Vec2::zero();
+        let clearance;
-        adjustment_from_floats.block = clearance.unwrap_or_else(Length::zero);
+        let inline_adjustment_from_floats;
-
         let inline_size_is_auto = self
             .style
             .box_size(containing_block.style.writing_mode)
             .inline
             .is_auto();
-        if !inline_size_is_auto {
+        let block_start_margin = CollapsedMargin::new(margin.block_start);
-            // We calculate a hypothetical value for `bfc_relative_block_position`,
+        if inline_size_is_auto {
-            // assuming that there was no adjustment from floats. The real value will
+            // TODO: Use PlacementAmongFloats to avoid overlapping floats.
-            // depend on whether or not there was adjustment.
+            clearance = sequential_layout_state
-            let hypothetical_bfc_relative_block_position = if clearance.is_some() {
+                .calculate_clearance(self.style.get_box().clear, &block_start_margin);
-                sequential_layout_state.bfc_relative_block_position +
+            inline_adjustment_from_floats = Length::zero();
-                    sequential_layout_state.current_margin.solve() +
+        } else {
-                    block_start_margin.solve()
-            } else {
-                sequential_layout_state.bfc_relative_block_position +
-                    sequential_layout_state
-                        .current_margin
-                        .adjoin(&block_start_margin)
-                        .solve()
-            };
-
             let size = &Vec2 {
                 inline: containing_block_for_children.inline_size,
                 block: block_size,
             } + &pbm.padding_border_sums;
-            let placement = PlacementAmongFloats::new(
+            (clearance, inline_adjustment_from_floats) = sequential_layout_state
-                &sequential_layout_state.floats,
+                .calculate_clearance_and_inline_adjustment(
-                hypothetical_bfc_relative_block_position + clearance.unwrap_or_else(Length::zero),
+                    self.style.get_box().clear,
-                size.clone(),
+                    &block_start_margin,
-            )
+                    size.clone(),
-            .place();
+                );
-
-            // This placement is in the coordinates of the float-containing block formatting
-            // context, but we need to calculate an offset to use for placing this replaced
-            // element.
-            adjustment_from_floats = &placement -
-                &Vec2 {
-                    inline: sequential_layout_state
-                        .floats
-                        .containing_block_info
-                        .inline_start,
-                    block: hypothetical_bfc_relative_block_position,
-                };
         }
 
-        // Clearance and any adjustment from float should prevent margin collapse, so it's
+        // Clearance prevents margin collapse between this block and previous ones,
-        // important to make sure that it is non-None even when it is zero. Yet, when we
+        // so in that case collapse margins before adjoining them below.
-        // didn't have clearance or any adjustment from placing next to floats, we want the
+        if clearance.is_some() {
-        // value of clearance on the Fragment to be None, so margin collapse still works
-        // properly.
-        let effective_clearance = if clearance.is_some() || !adjustment_from_floats.block.is_zero()
-        {
-            Some(adjustment_from_floats.block)
-        } else {
-            None
-        };
-
-        // If there was effective clearance, it prevent margins collapse between this
-        // block and previous ones, so in that case collapse margins before adjoining
-        // them below.
-        if effective_clearance.is_some() {
             sequential_layout_state.collapse_margins();
         }
         sequential_layout_state.adjoin_assign(&block_start_margin);
@@ -975,7 +939,7 @@ impl NonReplacedFormattingContext {
         // Margins can never collapse into independent formatting contexts.
         sequential_layout_state.collapse_margins();
         sequential_layout_state.advance_block_position(
-            pbm.padding_border_sums.block + adjustment_from_floats.block + block_size,
+            pbm.padding_border_sums.block + block_size + clearance.unwrap_or_else(Length::zero),
         );
         sequential_layout_state.adjoin_assign(&CollapsedMargin::new(margin.block_end));
 
@@ -988,11 +952,11 @@ impl NonReplacedFormattingContext {
             start_corner: Vec2 {
                 block: pbm.padding.block_start +
                     pbm.border.block_start +
-                    adjustment_from_floats.block,
+                    clearance.unwrap_or_else(Length::zero),
                 inline: pbm.padding.inline_start +
                     pbm.border.inline_start +
                     margin.inline_start +
-                    adjustment_from_floats.inline,
+                    inline_adjustment_from_floats,
             },
             size: Vec2 {
                 block: block_size,
@@ -1008,7 +972,7 @@ impl NonReplacedFormattingContext {
             pbm.padding,
             pbm.border,
             margin,
-            effective_clearance,
+            clearance,
             block_margins_collapsed_with_children,
         )
     }
@@ -1037,27 +1001,10 @@ fn layout_in_flow_replaced_block_level<'a>(
     };
     let fragments = replaced.make_fragments(style, size.clone());
 
-    let mut effective_clearance = None;
+    let clearance;
-    let mut adjustment_from_floats = Vec2::zero();
+    let inline_adjustment_from_floats;
     if let Some(ref mut sequential_layout_state) = sequential_layout_state {
         let block_start_margin = CollapsedMargin::new(margin.block_start);
-        let clearance =
-            sequential_layout_state.calculate_clearance(style.get_box().clear, &block_start_margin);
-
-        // We calculate a hypothetical value for `bfc_relative_block_position`,
-        // assuming that there was no adjustment from floats. The real value will
-        // depend on whether or not there was adjustment.
-        let hypothetical_bfc_relative_block_position = if clearance.is_some() {
-            sequential_layout_state.bfc_relative_block_position +
-                sequential_layout_state.current_margin.solve() +
-                block_start_margin.solve()
-        } else {
-            sequential_layout_state.bfc_relative_block_position +
-                sequential_layout_state
-                    .current_margin
-                    .adjoin(&block_start_margin)
-                    .solve()
-        };
 
         // From https://drafts.csswg.org/css2/#floats:
         // "The border box of a table, a block-level replaced element, or an element in
@@ -1069,40 +1016,16 @@ fn layout_in_flow_replaced_block_level<'a>(
         //  sufficient space. They may even make the border box of said element narrower
         //  than defined by section 10.3.3. CSS 2 does not define when a UA may put said
         //  element next to the float or by how much said element may become narrower."
-        let placement_among_floats = PlacementAmongFloats::new(
+        (clearance, inline_adjustment_from_floats) = sequential_layout_state
-            &sequential_layout_state.floats,
+            .calculate_clearance_and_inline_adjustment(
-            hypothetical_bfc_relative_block_position + clearance.unwrap_or_else(Length::zero),
+                style.get_box().clear,
-            &size + &pbm.padding_border_sums,
+                &block_start_margin,
-        )
+                &size + &pbm.padding_border_sums,
-        .place();
+            );
 
-        // This placement is in the coordinates of the float-containing block formatting
+        // Clearance prevents margin collapse between this block and previous ones,
-        // context, but we need to calculate an offset to use for placing this replaced
+        // so in that case collapse margins before adjoining them below.
-        // element.
+        if clearance.is_some() {
-        adjustment_from_floats = &placement_among_floats -
-            &Vec2 {
-                inline: sequential_layout_state
-                    .floats
-                    .containing_block_info
-                    .inline_start,
-                block: hypothetical_bfc_relative_block_position,
-            };
-
-        // Clearance and any adjustment from float should prevent margin collapse, so it's
-        // important to make sure that it is non-None even when it is zero. Yet, when we
-        // didn't have clearance or any adjustment from placing next to floats, we want the
-        // value of clearance on the Fragment to be None, so margin collapse still works
-        // properly.
-        effective_clearance = if clearance.is_some() || !adjustment_from_floats.block.is_zero() {
-            Some(adjustment_from_floats.block)
-        } else {
-            None
-        };
-
-        // If there was effective clearance, it prevent margins collapse between this
-        // block and previous ones, so in that case collapse margins before adjoining
-        // them below.
-        if effective_clearance.is_some() {
             sequential_layout_state.collapse_margins();
         }
         sequential_layout_state.adjoin_assign(&block_start_margin);
@@ -1110,17 +1033,22 @@ fn layout_in_flow_replaced_block_level<'a>(
         // Margins can never collapse into replaced elements.
         sequential_layout_state.collapse_margins();
         sequential_layout_state.advance_block_position(
-            pbm.padding_border_sums.block + size.block + adjustment_from_floats.block,
+            pbm.padding_border_sums.block + size.block + clearance.unwrap_or_else(Length::zero),
         );
         sequential_layout_state.adjoin_assign(&CollapsedMargin::new(margin.block_end));
+    } else {
+        clearance = None;
+        inline_adjustment_from_floats = Length::zero();
     };
 
     let start_corner = Vec2 {
-        block: pbm.padding.block_start + pbm.border.block_start + adjustment_from_floats.block,
+        block: pbm.padding.block_start +
+            pbm.border.block_start +
+            clearance.unwrap_or_else(Length::zero),
         inline: pbm.padding.inline_start +
             pbm.border.inline_start +
             margin.inline_start +
-            adjustment_from_floats.inline,
+            inline_adjustment_from_floats,
     };
 
     let content_rect = Rect { start_corner, size };
@@ -1133,7 +1061,7 @@ fn layout_in_flow_replaced_block_level<'a>(
         pbm.padding,
         pbm.border,
         margin,
-        effective_clearance,
+        clearance,
         block_margins_collapsed_with_children,
     )
 }

tests/wpt/meta/css/CSS2/floats/new-fc-separates-from-float-2.html.ini

@@ -1,2 +0,0 @@
-[new-fc-separates-from-float-2.html]
-  expected: FAIL

tests/wpt/meta/css/CSS2/floats/new-fc-separates-from-float-3.html.ini

@@ -1,2 +0,0 @@
-[new-fc-separates-from-float-3.html]
-  expected: FAIL

tests/wpt/meta/css/CSS2/floats/new-fc-separates-from-float.html.ini

@@ -1,2 +0,0 @@
-[new-fc-separates-from-float.html]
-  expected: FAIL