Properly position floats when subsequent boxes collapse margins with containing block

Margins should be able to collapse through floats when collapsing with
parent blocks (the containing block). To properly place floats in this
situation, we need to look at these subsequent floats to find out how
much of the margin will collapse with the parent.

This initial implementation is very basic and the second step would be
to cache this in order to avoid having to constantly recalculate it.

Fixes #29915.

Co-authored-by: Oriol Brufau <obrufau@igalia.com>

4 files changed, 233 insertions, 105 deletions

diff --git a/components/layout_2020/flow/float.rs b/components/layout_2020/flow/float.rs
index b739ace9014..773c2cdaf7e 100644
--- a/components/layout_2020/flow/float.rs
+++ b/components/layout_2020/flow/float.rs
@@ -10,9 +10,7 @@ use crate::context::LayoutContext;
 use crate::dom::NodeExt;
 use crate::dom_traversal::{Contents, NodeAndStyleInfo};
 use crate::formatting_contexts::IndependentFormattingContext;
-use crate::fragment_tree::{
-    BoxFragment, CollapsedBlockMargins, CollapsedMargin, FloatFragment, Fragment,
-};
+use crate::fragment_tree::{BoxFragment, CollapsedBlockMargins, CollapsedMargin, FloatFragment};
 use crate::geom::flow_relative::{Rect, Vec2};
 use crate::positioned::PositioningContext;
 use crate::style_ext::{ComputedValuesExt, DisplayInside};
@@ -658,34 +656,9 @@ impl FloatBox {
         layout_context: &LayoutContext,
         positioning_context: &mut PositioningContext,
         containing_block: &ContainingBlock,
-        mut sequential_layout_state: Option<&mut SequentialLayoutState>,
-    ) -> Fragment {
-        let sequential_layout_state = sequential_layout_state
-            .as_mut()
-            .expect("Tried to lay out a float with no sequential placement state!");
-
-        // Speculate that the float ceiling will be located at the current block position plus the
-        // result of solving any margins we're building up. This is usually right, but it can be
-        // incorrect if there are more in-flow collapsible margins yet to be seen. An example
-        // showing when this can go wrong:
-        //
-        //      <div style="margin: 5px"></div>
-        //      <div style="float: left"></div>
-        //      <div style="margin: 10px"></div>
-        //
-        // Assuming these are all in-flow, the float should be placed 10px down from the start, not
-        // 5px, but we can't know that because we haven't seen the block after this float yet.
-        //
-        // FIXME(pcwalton): Implement the proper behavior when speculation fails. Either detect it
-        // afterward and fix it up, or detect this situation ahead of time via lookahead and make
-        // sure `current_margin` is accurate before calling this method.
-        sequential_layout_state
-            .floats
-            .lower_ceiling(sequential_layout_state.current_block_position_including_margins());
-
+    ) -> BoxFragment {
         let style = self.contents.style().clone();
-        let float_context = &mut sequential_layout_state.floats;
-        let box_fragment = positioning_context.layout_maybe_position_relative_fragment(
+        positioning_context.layout_maybe_position_relative_fragment(
             layout_context,
             containing_block,
             &style,
@@ -696,7 +669,7 @@ impl FloatBox {
                 let margin = pbm.margin.auto_is(|| Length::zero());
                 let pbm_sums = &(&pbm.padding + &pbm.border) + &margin;
 
-                let (content_size, fragments);
+                let (content_size, children);
                 match self.contents {
                     IndependentFormattingContext::NonReplaced(ref mut non_replaced) => {
                         // Calculate inline size.
@@ -739,7 +712,7 @@ impl FloatBox {
                                 .block
                                 .auto_is(|| independent_layout.content_block_size),
                         };
-                        fragments = independent_layout.fragments;
+                        children = independent_layout.fragments;
                     },
                     IndependentFormattingContext::Replaced(ref replaced) => {
                         // https://drafts.csswg.org/css2/#float-replaced-width
@@ -750,40 +723,32 @@ impl FloatBox {
                             None,
                             &pbm,
                         );
-                        fragments = replaced
+                        children = replaced
                             .contents
                             .make_fragments(&replaced.style, content_size.clone());
                     },
                 };
-                let margin_box_start_corner = float_context.add_float(&PlacementInfo {
-                    size: &content_size + &pbm_sums.sum(),
-                    side: FloatSide::from_style(&style).expect("Float box wasn't floated!"),
-                    clear: ClearSide::from_style(&style),
-                });
 
                 let content_rect = Rect {
-                    start_corner: &margin_box_start_corner + &pbm_sums.start_offset(),
-                    size: content_size.clone(),
+                    start_corner: Vec2::zero(),
+                    size: content_size,
                 };
 
-                // Clearance is handled internally by the float placement logic, so there's no need
-                // to store it explicitly in the fragment.
-                let clearance = Length::zero();
-
                 BoxFragment::new(
                     self.contents.base_fragment_info(),
                     style.clone(),
-                    fragments,
+                    children,
                     content_rect,
                     pbm.padding,
                     pbm.border,
                     margin,
-                    clearance,
+                    // Clearance is handled internally by the float placement logic, so there's no need
+                    // to store it explicitly in the fragment.
+                    Length::zero(), // clearance
                     CollapsedBlockMargins::zero(),
                 )
             },
-        );
-        Fragment::Float(box_fragment)
+        )
     }
 }
 
@@ -890,4 +855,52 @@ impl SequentialLayoutState {
     pub(crate) fn adjoin_assign(&mut self, margin: &CollapsedMargin) {
         self.current_margin.adjoin_assign(margin)
     }
+
+    /// Get the offset of the current containing block and any uncollapsed margins.
+    pub(crate) fn current_containing_block_offset(&self) -> CSSPixelLength {
+        self.floats.containing_block_info.block_start +
+            self.floats
+                .containing_block_info
+                .block_start_margins_not_collapsed
+                .solve()
+    }
+
+    /// This function places a Fragment that has been created for a FloatBox.
+    pub(crate) fn place_float_fragment(
+        &mut self,
+        box_fragment: &mut BoxFragment,
+        margins_collapsing_with_parent_containing_block: CollapsedMargin,
+        block_offset_from_containining_block_top: CSSPixelLength,
+    ) {
+        let block_start_of_containing_block_in_bfc = self.floats.containing_block_info.block_start +
+            self.floats
+                .containing_block_info
+                .block_start_margins_not_collapsed
+                .adjoin(&margins_collapsing_with_parent_containing_block)
+                .solve();
+
+        self.floats.lower_ceiling(
+            block_start_of_containing_block_in_bfc + block_offset_from_containining_block_top,
+        );
+
+        let pbm_sums = &(&box_fragment.padding + &box_fragment.border) + &box_fragment.margin;
+        let margin_box_start_corner = self.floats.add_float(&PlacementInfo {
+            size: &box_fragment.content_rect.size + &pbm_sums.sum(),
+            side: FloatSide::from_style(&box_fragment.style).expect("Float box wasn't floated!"),
+            clear: ClearSide::from_style(&box_fragment.style),
+        });
+
+        // This is the position of the float in the float-containing block formatting context. We add the
+        // existing start corner here because we may have already gotten some relative positioning offset.
+        let new_position_in_bfc = &(&margin_box_start_corner + &pbm_sums.start_offset()) +
+            &box_fragment.content_rect.start_corner;
+
+        // This is the position of the float relative to the containing block start.
+        let new_position_in_containing_block = Vec2 {
+            inline: new_position_in_bfc.inline - self.floats.containing_block_info.inline_start,
+            block: new_position_in_bfc.block - block_start_of_containing_block_in_bfc,
+        };
+
+        box_fragment.content_rect.start_corner = new_position_in_containing_block;
+    }
 }
diff --git a/components/layout_2020/flow/inline.rs b/components/layout_2020/flow/inline.rs
index e713d9c2a8e..58f8884473e 100644
--- a/components/layout_2020/flow/inline.rs
+++ b/components/layout_2020/flow/inline.rs
@@ -8,8 +8,8 @@ use crate::flow::float::{FloatBox, SequentialLayoutState};
 use crate::flow::FlowLayout;
 use crate::formatting_contexts::IndependentFormattingContext;
 use crate::fragment_tree::{
-    AnonymousFragment, BaseFragmentInfo, BoxFragment, CollapsedBlockMargins, FontMetrics, Fragment,
-    TextFragment,
+    AnonymousFragment, BaseFragmentInfo, BoxFragment, CollapsedBlockMargins, CollapsedMargin,
+    FontMetrics, Fragment, TextFragment,
 };
 use crate::geom::flow_relative::{Rect, Sides, Vec2};
 use crate::positioned::{
@@ -349,30 +349,30 @@ impl InlineFormattingContext {
                             .fragments_so_far
                             .push(Fragment::AbsoluteOrFixedPositioned(hoisted_fragment));
                     },
-                    InlineLevelBox::OutOfFlowFloatBox(box_) => {
-                        let mut fragment = box_.layout(
+                    InlineLevelBox::OutOfFlowFloatBox(float_box) => {
+                        let mut box_fragment = float_box.layout(
                             layout_context,
                             ifc.positioning_context,
                             containing_block,
-                            ifc.sequential_layout_state.as_mut().map(|c| &mut **c),
                         );
-                        if let Some(state) = &ifc.sequential_layout_state {
-                            let offset_from_formatting_context_to_containing_block = Vec2 {
-                                inline: state.floats.containing_block_info.inline_start,
-                                block: state.floats.containing_block_info.block_start +
-                                    state
-                                        .floats
-                                        .containing_block_info
-                                        .block_start_margins_not_collapsed
-                                        .solve(),
-                            };
-                            if let Fragment::Float(ref mut box_fragment) = &mut fragment {
-                                box_fragment.content_rect.start_corner =
-                                    &box_fragment.content_rect.start_corner -
-                                        &offset_from_formatting_context_to_containing_block;
-                            }
-                        }
-                        ifc.current_nesting_level.fragments_so_far.push(fragment);
+
+                        let state = ifc
+                            .sequential_layout_state
+                            .as_mut()
+                            .expect("Tried to lay out a float with no sequential placement state!");
+
+                        let block_offset_from_containining_block_top = state
+                            .current_block_position_including_margins() -
+                            state.current_containing_block_offset();
+                        state.place_float_fragment(
+                            &mut box_fragment,
+                            CollapsedMargin::zero(),
+                            block_offset_from_containining_block_top,
+                        );
+
+                        ifc.current_nesting_level
+                            .fragments_so_far
+                            .push(Fragment::Float(box_fragment));
                     },
                 }
             } else
diff --git a/components/layout_2020/flow/mod.rs b/components/layout_2020/flow/mod.rs
index 03c50e5a807..7050913e98a 100644
--- a/components/layout_2020/flow/mod.rs
+++ b/components/layout_2020/flow/mod.rs
@@ -26,7 +26,7 @@ use style::computed_values::clear::T as Clear;
 use style::computed_values::float::T as Float;
 use style::logical_geometry::WritingMode;
 use style::properties::ComputedValues;
-use style::values::computed::{Length, LengthOrAuto};
+use style::values::computed::{CSSPixelLength, Length, LengthOrAuto};
 use style::Zero;
 
 mod construct;
@@ -61,6 +61,90 @@ pub(crate) enum BlockLevelBox {
     Independent(IndependentFormattingContext),
 }
 
+impl BlockLevelBox {
+    fn find_block_margin_collapsing_with_parent_for_floats(
+        &self,
+        collected_margin: &mut CollapsedMargin,
+        containing_block_writing_mode: WritingMode,
+    ) -> bool {
+        // TODO(mrobinson,Loirooriol): Cache margins here so that we don't constantly
+        // have to keep looking forward when dealing with sequences of floats.
+        let style = match self {
+            BlockLevelBox::SameFormattingContextBlock { ref style, .. } => &style,
+            BlockLevelBox::OutOfFlowAbsolutelyPositionedBox(_) |
+            BlockLevelBox::OutOfFlowFloatBox(_) => return true,
+            BlockLevelBox::Independent(ref context) => context.style(),
+        };
+
+        let margin = style.margin(containing_block_writing_mode);
+        let border = style.border_width(containing_block_writing_mode);
+        let padding = style.padding(containing_block_writing_mode);
+
+        if style.get_box().clear != Clear::None {
+            return false;
+        }
+
+        let start_margin = margin
+            .block_start
+            .percentage_relative_to(CSSPixelLength::zero())
+            .auto_is(CSSPixelLength::zero);
+        collected_margin.adjoin_assign(&CollapsedMargin::new(start_margin));
+
+        let start_padding_is_zero = padding.block_start.is_zero();
+        let start_border_is_zero = border.block_start.is_zero();
+        if !start_border_is_zero || !start_padding_is_zero {
+            return false;
+        }
+
+        let contents = match self {
+            BlockLevelBox::SameFormattingContextBlock { ref contents, .. } => contents,
+            _ => return false,
+        };
+        match contents {
+            BlockContainer::BlockLevelBoxes(boxes) => {
+                if !Self::find_block_margin_collapsing_with_parent_for_floats_from_slice(
+                    &boxes,
+                    collected_margin,
+                    style.writing_mode,
+                ) {
+                    return false;
+                }
+            },
+            BlockContainer::InlineFormattingContext(_) => return false,
+        }
+
+        let block_size_zero =
+            style.content_block_size().is_definitely_zero() || style.content_block_size().is_auto();
+        if !style.min_block_size().is_definitely_zero() ||
+            !block_size_zero ||
+            !border.block_end.is_zero() ||
+            !padding.block_end.is_zero()
+        {
+            return false;
+        }
+
+        let end_margin = margin
+            .block_end
+            .percentage_relative_to(CSSPixelLength::zero())
+            .auto_is(CSSPixelLength::zero);
+        collected_margin.adjoin_assign(&CollapsedMargin::new(end_margin));
+
+        true
+    }
+
+    fn find_block_margin_collapsing_with_parent_for_floats_from_slice(
+        boxes: &[ArcRefCell<BlockLevelBox>],
+        margin: &mut CollapsedMargin,
+        writing_mode: WritingMode,
+    ) -> bool {
+        boxes.iter().all(|block_level_box| {
+            block_level_box
+                .borrow()
+                .find_block_margin_collapsing_with_parent_for_floats(margin, writing_mode)
+        })
+    }
+}
+
 struct FlowLayout {
     pub fragments: Vec<Fragment>,
     pub content_block_size: Length,
@@ -337,7 +421,8 @@ fn layout_block_level_children_sequentially(
     // tracks every float encountered so far (again in tree order).
     let fragments = child_boxes
         .iter()
-        .map(|child_box| {
+        .enumerate()
+        .map(|(index, child_box)| {
             let mut child_positioning_context =
                 PositioningContext::new_for_subtree(collects_for_nearest_positioned_ancestor);
             let mut fragment = child_box.borrow_mut().layout(
@@ -347,9 +432,15 @@ fn layout_block_level_children_sequentially(
                 Some(&mut *sequential_layout_state),
             );
 
-            placement_state.place_fragment(&mut fragment, Some(sequential_layout_state));
+            let sequential_info = PlacementStateSequentialInfo {
+                sequential_layout_state,
+                following_boxes: &child_boxes[index..],
+            };
+            placement_state.place_fragment(&mut fragment, Some(sequential_info));
+
             child_positioning_context.adjust_static_position_of_hoisted_fragments(&fragment);
             positioning_context.append(child_positioning_context);
+
             fragment
         })
         .collect();
@@ -442,12 +533,11 @@ impl BlockLevelBox {
                 positioning_context.push(hoisted_box);
                 Fragment::AbsoluteOrFixedPositioned(hoisted_fragment)
             },
-            BlockLevelBox::OutOfFlowFloatBox(box_) => box_.layout(
+            BlockLevelBox::OutOfFlowFloatBox(float_box) => Fragment::Float(float_box.layout(
                 layout_context,
                 positioning_context,
                 containing_block,
-                sequential_layout_state,
-            ),
+            )),
         }
     }
 
@@ -777,11 +867,23 @@ fn solve_inline_margins_for_in_flow_block_level(
     }
 }
 
-// State that we maintain when placing blocks.
-//
-// In parallel mode, this placement is done after all child blocks are laid out. In sequential
-// mode, this is done right after each block is laid out.
-pub(crate) struct PlacementState {
+/// Information passed to [PlacementState::place_fragment] when operating in sequential
+/// mode.
+struct PlacementStateSequentialInfo<'a> {
+    /// The sequential layout state of the current layout.
+    sequential_layout_state: &'a mut SequentialLayoutState,
+
+    /// The boxes that follow the one currently being placed. This is used to try
+    /// to calculate margins after the current box that will collapse with the
+    /// parent, if this current box is floating.
+    following_boxes: &'a [ArcRefCell<BlockLevelBox>],
+}
+
+/// State that we maintain when placing blocks.
+///
+/// In parallel mode, this placement is done after all child blocks are laid out. In
+/// sequential mode, this is done right after each block is laid out.
+struct PlacementState {
     next_in_flow_margin_collapses_with_parent_start_margin: bool,
     last_in_flow_margin_collapses_with_parent_end_margin: bool,
     start_margin: CollapsedMargin,
@@ -803,10 +905,12 @@ impl PlacementState {
         }
     }
 
+    /// Place a single [Fragment] in a block level context using the state so far and
+    /// information gathered from the [Fragment] itself.
     fn place_fragment(
         &mut self,
         fragment: &mut Fragment,
-        sequential_layout_state: Option<&mut SequentialLayoutState>,
+        sequential_info: Option<PlacementStateSequentialInfo>,
     ) {
         match fragment {
             Fragment::Box(fragment) => {
@@ -832,6 +936,7 @@ impl PlacementState {
                 } else if !fragment_block_margins.collapsed_through {
                     self.last_in_flow_margin_collapses_with_parent_end_margin = true;
                 }
+
                 if self.next_in_flow_margin_collapses_with_parent_start_margin {
                     debug_assert_eq!(self.current_margin.solve(), Length::zero());
                     self.start_margin
@@ -845,19 +950,21 @@ impl PlacementState {
                     self.current_margin
                         .adjoin_assign(&fragment_block_margins.start);
                 }
+
                 fragment.content_rect.start_corner.block +=
                     self.current_margin.solve() + self.current_block_direction_position;
+
                 if fragment_block_margins.collapsed_through {
                     // `fragment_block_size` is typically zero when collapsing through,
                     // but we still need to consider it in case there is clearance.
-                    self.current_block_direction_position += fragment_block_size;
+                    self.current_block_direction_position += fragment.clearance;
                     self.current_margin
                         .adjoin_assign(&fragment_block_margins.end);
-                    return;
+                } else {
+                    self.current_block_direction_position +=
+                        self.current_margin.solve() + fragment_block_size;
+                    self.current_margin = fragment_block_margins.end;
                 }
-                self.current_block_direction_position +=
-                    self.current_margin.solve() + fragment_block_size;
-                self.current_margin = fragment_block_margins.end;
             },
             Fragment::AbsoluteOrFixedPositioned(fragment) => {
                 let offset = Vec2 {
@@ -867,22 +974,32 @@ impl PlacementState {
                 fragment.borrow_mut().adjust_offsets(offset);
             },
             Fragment::Float(box_fragment) => {
-                // When Float fragments are created in block flows, they are positioned
-                // relative to the float containing independent block formatting context.
-                // Once we place a float's containing block, this function can be used to
-                // fix up the float position to be relative to the containing block.
-                let sequential_layout_state = sequential_layout_state
-                    .expect("Found float fragment without SequentialLayoutState");
-                let containing_block_info = &sequential_layout_state.floats.containing_block_info;
-                let margin = containing_block_info
-                    .block_start_margins_not_collapsed
-                    .adjoin(&self.start_margin);
-                let parent_fragment_offset_in_formatting_context = Vec2 {
-                    inline: containing_block_info.inline_start,
-                    block: containing_block_info.block_start + margin.solve(),
-                };
-                box_fragment.content_rect.start_corner = &box_fragment.content_rect.start_corner -
-                    &parent_fragment_offset_in_formatting_context;
+                let info = sequential_info
+                    .expect("Tried to lay out a float with no sequential placement state!");
+
+                let mut margins_collapsing_with_parent_containing_block = self.start_margin;
+                if self.next_in_flow_margin_collapses_with_parent_start_margin {
+                    // If block start margins are still collapsing from the parent, margins of elements
+                    // that follow this float in tree order might collapse, pushing this float down
+                    // and changing the offset of the containing block of the float. We try to look
+                    // ahead to determine which margins from future elements will collapse with the
+                    // parent.
+                    let mut future_margins = CollapsedMargin::zero();
+                    BlockLevelBox::find_block_margin_collapsing_with_parent_for_floats_from_slice(
+                        info.following_boxes,
+                        &mut future_margins,
+                        WritingMode::empty(),
+                    );
+                    margins_collapsing_with_parent_containing_block.adjoin_assign(&future_margins)
+                }
+
+                let block_offset_from_containing_block_top =
+                    self.current_block_direction_position + self.current_margin.solve();
+                info.sequential_layout_state.place_float_fragment(
+                    box_fragment,
+                    margins_collapsing_with_parent_containing_block,
+                    block_offset_from_containing_block_top,
+                );
             },
             Fragment::Anonymous(_) => {},
             _ => unreachable!(),
diff --git a/tests/wpt/meta/css/CSS2/floats-clear/clear-float-003.xht.ini b/tests/wpt/meta/css/CSS2/floats-clear/clear-float-003.xht.ini
deleted file mode 100644
index 2a3dc9e10f6..00000000000
--- a/tests/wpt/meta/css/CSS2/floats-clear/clear-float-003.xht.ini
+++ /dev/null
@@ -1,2 +0,0 @@
-[clear-float-003.xht]
-  expected: FAIL