Auto merge of #29939 - mrobinson:fix-float-placement-with-future-margins, r=mrobinson

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>

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #29915.
- [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

@@ -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::{
+use crate::fragment_tree::{BoxFragment, CollapsedBlockMargins, CollapsedMargin, FloatFragment};
-    BoxFragment, CollapsedBlockMargins, CollapsedMargin, FloatFragment, Fragment,
-};
 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>,
+    ) -> BoxFragment {
-    ) -> 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());
-
         let style = self.contents.style().clone();
-        let float_context = &mut sequential_layout_state.floats;
+        positioning_context.layout_maybe_position_relative_fragment(
-        let box_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(),
+                    start_corner: Vec2::zero(),
-                    size: content_size.clone(),
+                    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;
+    }
 }

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,
+    AnonymousFragment, BaseFragmentInfo, BoxFragment, CollapsedBlockMargins, CollapsedMargin,
-    TextFragment,
+    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_) => {
+                    InlineLevelBox::OutOfFlowFloatBox(float_box) => {
-                        let mut fragment = box_.layout(
+                        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 {
+                        let state = ifc
-                                inline: state.floats.containing_block_info.inline_start,
+                            .sequential_layout_state
-                                block: state.floats.containing_block_info.block_start +
+                            .as_mut()
-                                    state
+                            .expect("Tried to lay out a float with no sequential placement state!");
-                                        .floats
+
-                                        .containing_block_info
+                        let block_offset_from_containining_block_top = state
-                                        .block_start_margins_not_collapsed
+                            .current_block_position_including_margins() -
-                                        .solve(),
+                            state.current_containing_block_offset();
-                            };
+                        state.place_float_fragment(
-                            if let Fragment::Float(ref mut box_fragment) = &mut fragment {
+                            &mut box_fragment,
-                                box_fragment.content_rect.start_corner =
+                            CollapsedMargin::zero(),
-                                    &box_fragment.content_rect.start_corner -
+                            block_offset_from_containining_block_top,
-                                        &offset_from_formatting_context_to_containing_block;
+                        );
-                            }
+
-                        }
+                        ifc.current_nesting_level
-                        ifc.current_nesting_level.fragments_so_far.push(fragment);
+                            .fragments_so_far
+                            .push(Fragment::Float(box_fragment));
                     },
                 }
             } else

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.
+/// Information passed to [PlacementState::place_fragment] when operating in sequential
-//
+/// mode.
-// In parallel mode, this placement is done after all child blocks are laid out. In sequential
+struct PlacementStateSequentialInfo<'a> {
-// mode, this is done right after each block is laid out.
+    /// The sequential layout state of the current layout.
-pub(crate) struct PlacementState {
+    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;
+                }
             },
             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
+                let info = sequential_info
-                // relative to the float containing independent block formatting context.
+                    .expect("Tried to lay out a float with no sequential placement state!");
-                // 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 mut margins_collapsing_with_parent_containing_block = self.start_margin;
-                let sequential_layout_state = sequential_layout_state
+                if self.next_in_flow_margin_collapses_with_parent_start_margin {
-                    .expect("Found float fragment without SequentialLayoutState");
+                    // If block start margins are still collapsing from the parent, margins of elements
-                let containing_block_info = &sequential_layout_state.floats.containing_block_info;
+                    // that follow this float in tree order might collapse, pushing this float down
-                let margin = containing_block_info
+                    // and changing the offset of the containing block of the float. We try to look
-                    .block_start_margins_not_collapsed
+                    // ahead to determine which margins from future elements will collapse with the
-                    .adjoin(&self.start_margin);
+                    // parent.
-                let parent_fragment_offset_in_formatting_context = Vec2 {
+                    let mut future_margins = CollapsedMargin::zero();
-                    inline: containing_block_info.inline_start,
+                    BlockLevelBox::find_block_margin_collapsing_with_parent_for_floats_from_slice(
-                    block: containing_block_info.block_start + margin.solve(),
+                        info.following_boxes,
-                };
+                        &mut future_margins,
-                box_fragment.content_rect.start_corner = &box_fragment.content_rect.start_corner -
+                        WritingMode::empty(),
-                    &parent_fragment_offset_in_formatting_context;
+                    );
+                    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!(),

tests/wpt/meta/css/CSS2/floats-clear/clear-float-003.xht.ini

@@ -1,2 +0,0 @@
-[clear-float-003.xht]
-  expected: FAIL