Do not hoist floated fragments

Instead of hoisting floated fragments to be siblings of the fragment
created by their containing block formatting context, keep them in
"normal" fragment tree position and adjust their positioning to be
relative to the containing block. This means that float fragments follow
the existing invariants of the fragment tree and properly handle hit
testing, painting order, and relative positioning.

The tradeoff here is more complexity tracking the containing block
offsets from the block formatting context (including handling collapsed
margins), but less complexity dealing with hoisting / shared ownership
in addition to the correctness benefits.

Some tests are failing now because this change revealed some additional
shortcomings with clearing block formatting context content size past
the end of their contained floats. This will be fixed in a followup
change.

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

components/layout_2020/display_list/mod.rs

@@ -193,15 +193,13 @@ impl Fragment {
         section: StackingContextSection,
     ) {
         match self {
-            Fragment::Box(b) => match b.style.get_inherited_box().visibility {
+            Fragment::Box(b) | Fragment::Float(b) => match b.style.get_inherited_box().visibility {
                 Visibility::Visible => {
                     BuilderForBoxFragment::new(b, containing_block).build(builder, section)
                 },
                 Visibility::Hidden => (),
                 Visibility::Collapse => (),
             },
-            Fragment::HoistedFloat(_) => {},
-            Fragment::Float => {},
             Fragment::AbsoluteOrFixedPositioned(_) => {},
             Fragment::Anonymous(_) => {},
             Fragment::Image(i) => match i.style.get_inherited_box().visibility {

components/layout_2020/display_list/stacking_context.rs

@@ -7,7 +7,7 @@ use crate::cell::ArcRefCell;
 use crate::display_list::conversions::ToWebRender;
 use crate::display_list::DisplayListBuilder;
 use crate::fragment_tree::ContainingBlockManager;
-use crate::fragments::{AnonymousFragment, BoxFragment, Fragment, HoistedFloatFragment};
+use crate::fragments::{AnonymousFragment, BoxFragment, Fragment};
 use crate::geom::PhysicalRect;
 use crate::style_ext::ComputedValuesExt;
 use crate::FragmentTree;
@@ -531,7 +531,7 @@ impl Fragment {
     ) {
         let containing_block = containing_block_info.get_containing_block_for_fragment(self);
         match self {
-            Fragment::Box(fragment) => {
+            Fragment::Box(fragment) | Fragment::Float(fragment) => {
                 if mode == StackingContextBuildMode::SkipHoisted &&
                     fragment.style.clone_position().is_absolutely_positioned()
                 {
@@ -554,14 +554,6 @@ impl Fragment {
                     stacking_context,
                 );
             },
-            Fragment::HoistedFloat(fragment) => {
-                fragment.build_stacking_context_tree(
-                    display_list,
-                    containing_block_info,
-                    stacking_context,
-                );
-            },
-            Fragment::Float => {},
             Fragment::AbsoluteOrFixedPositioned(fragment) => {
                 let shared_fragment = fragment.borrow();
                 let fragment_ref = match shared_fragment.fragment.as_ref() {
@@ -1103,20 +1095,3 @@ impl AnonymousFragment {
         }
     }
 }
-
-impl HoistedFloatFragment {
-    fn build_stacking_context_tree(
-        &self,
-        display_list: &mut DisplayList,
-        containing_block_info: &ContainingBlockInfo,
-        stacking_context: &mut StackingContext,
-    ) {
-        self.fragment.borrow().build_stacking_context_tree(
-            &self.fragment,
-            display_list,
-            containing_block_info,
-            stacking_context,
-            StackingContextBuildMode::SkipHoisted,
-        );
-    }
-}

components/layout_2020/flow/float.rs

@@ -6,22 +6,22 @@
 //!
 //! See CSS 2.1 § 9.5.1: https://www.w3.org/TR/CSS2/visuren.html#float-position
 
-use crate::cell::ArcRefCell;
 use crate::context::LayoutContext;
 use crate::dom::NodeExt;
 use crate::dom_traversal::{Contents, NodeAndStyleInfo};
 use crate::formatting_contexts::IndependentFormattingContext;
-use crate::fragments::{BoxFragment, CollapsedBlockMargins, CollapsedMargin, FloatFragment};
+use crate::fragments::{
-use crate::fragments::{Fragment, HoistedFloatFragment};
+    BoxFragment, CollapsedBlockMargins, CollapsedMargin, FloatFragment, Fragment,
+};
 use crate::geom::flow_relative::{Rect, Vec2};
 use crate::positioned::PositioningContext;
 use crate::style_ext::{ComputedValuesExt, DisplayInside};
 use crate::ContainingBlock;
 use euclid::num::Zero;
 use servo_arc::Arc;
-use std::f32;
 use std::fmt::{Debug, Formatter, Result as FmtResult};
 use std::ops::Range;
+use std::{f32, mem};
 use style::computed_values::clear::T as ClearProperty;
 use style::computed_values::float::T as FloatProperty;
 use style::properties::ComputedValues;
@@ -35,12 +35,49 @@ pub(crate) struct FloatBox {
     pub contents: IndependentFormattingContext,
 }
 
+/// `FloatContext` positions floats relative to the independent block formatting
+/// context which contains the floating elements. The Fragment tree positions
+/// elements relative to their containing blocks. This data structure is used to
+/// help map between these two coordinate systems.
+#[derive(Clone, Copy, Debug)]
+pub(crate) struct ContainingBlockPositionInfo {
+    /// The distance from the block start of the independent block formatting
+    /// context that contains the floats and the block start of the current
+    /// containing block, excluding uncollapsed block start margins. Note that
+    /// this does not include uncollapsed block start margins because we don't
+    /// know the value of collapsed margins until we lay out children.
+    pub block_start: Length,
+    /// Any uncollapsed block start margins that we have collected between the
+    /// block start of the float containing independent block formatting context
+    /// and this containing block, including for this containing block.
+    pub block_start_margins_not_collapsed: CollapsedMargin,
+    /// The distance from the inline start position of the float containing
+    /// independent formatting context and the inline start of this containing
+    /// block.
+    pub inline_start: Length,
+    /// The offset from the inline start position of the float containing
+    /// independent formatting context to the inline end of this containing
+    /// block.
+    pub inline_end: Length,
+}
+
+impl ContainingBlockPositionInfo {
+    fn new() -> ContainingBlockPositionInfo {
+        ContainingBlockPositionInfo {
+            block_start: Length::zero(),
+            block_start_margins_not_collapsed: CollapsedMargin::zero(),
+            inline_start: Length::zero(),
+            inline_end: Length::new(f32::INFINITY),
+        }
+    }
+}
+
 /// Data kept during layout about the floats in a given block formatting context.
 ///
 /// This is a persistent data structure. Each float has its own private copy of the float context,
 /// although such copies may share portions of the `bands` tree.
 #[derive(Clone, Debug)]
-pub struct FloatContext {
+pub(crate) struct FloatContext {
     /// A persistent AA tree of float bands.
     ///
     /// This tree is immutable; modification operations return the new tree, which may share nodes
@@ -49,27 +86,16 @@ pub struct FloatContext {
     /// The current (logically) vertical position. No new floats may be placed (logically) above
     /// this line.
     pub ceiling: Length,
-    /// Distances from the logical left side of the block formatting context to the logical sides
+    /// Details about the position of the containing block relative to the
-    /// of the current containing block.
+    /// independent block formatting context that contains all of the floats
-    pub walls: InlineWalls,
+    /// this `FloatContext` positions.
+    pub containing_block_info: ContainingBlockPositionInfo,
     /// The (logically) lowest margin edge of the last left float.
     pub clear_left_position: Length,
     /// The (logically) lowest margin edge of the last right float.
     pub clear_right_position: Length,
 }
 
-/// Distances from the logical left side of the block formatting context to the logical sides of
-/// the current containing block.
-#[derive(Clone, Copy, Debug)]
-pub struct InlineWalls {
-    /// The distance from the logical left side of the block formatting context to the logical
-    /// left side of the current containing block.
-    pub left: Length,
-    /// The distance from the logical *left* side of the block formatting context to the logical
-    /// right side of this object's containing block.
-    pub right: Length,
-}
-
 impl FloatContext {
     /// Returns a new float context representing a containing block with the given content
     /// inline-size.
@@ -88,17 +114,12 @@ impl FloatContext {
         FloatContext {
             bands,
             ceiling: Length::zero(),
-            walls: InlineWalls::new(),
+            containing_block_info: ContainingBlockPositionInfo::new(),
             clear_left_position: Length::zero(),
             clear_right_position: Length::zero(),
         }
     }
 
-    /// Returns the current ceiling value. No new floats may be placed (logically) above this line.
-    pub fn ceiling(&self) -> Length {
-        self.ceiling
-    }
-
     /// (Logically) lowers the ceiling to at least `new_ceiling` units.
     ///
     /// If the ceiling is already logically lower (i.e. larger) than this, does nothing.
@@ -124,7 +145,7 @@ impl FloatContext {
 
         // Find the first band this float fits in.
         let mut first_band = self.bands.find(ceiling).unwrap();
-        while !first_band.object_fits(&object, &self.walls) {
+        while !first_band.object_fits(&object, &self.containing_block_info) {
             let next_band = self.bands.find_next(first_band.top).unwrap();
             if next_band.top.px().is_infinite() {
                 break;
@@ -136,8 +157,8 @@ impl FloatContext {
         match object.side {
             FloatSide::Left => {
                 let left_object_edge = match first_band.left {
-                    Some(band_left) => band_left.max(self.walls.left),
+                    Some(band_left) => band_left.max(self.containing_block_info.inline_start),
-                    None => self.walls.left,
+                    None => self.containing_block_info.inline_start,
                 };
                 Vec2 {
                     inline: left_object_edge,
@@ -146,8 +167,8 @@ impl FloatContext {
             },
             FloatSide::Right => {
                 let right_object_edge = match first_band.right {
-                    Some(band_right) => band_right.min(self.walls.right),
+                    Some(band_right) => band_right.min(self.containing_block_info.inline_end),
-                    None => self.walls.right,
+                    None => self.containing_block_info.inline_end,
                 };
                 Vec2 {
                     inline: right_object_edge - object.size.inline,
@@ -210,15 +231,6 @@ impl FloatContext {
     }
 }
 
-impl InlineWalls {
-    fn new() -> InlineWalls {
-        InlineWalls {
-            left: Length::zero(),
-            right: Length::new(f32::INFINITY),
-        }
-    }
-}
-
 /// Information needed to place an object so that it doesn't collide with existing floats.
 #[derive(Clone, Debug)]
 pub struct PlacementInfo {
@@ -291,18 +303,18 @@ impl ClearSide {
 
 impl FloatBand {
     // Determines whether an object fits in a band.
-    fn object_fits(&self, object: &PlacementInfo, walls: &InlineWalls) -> bool {
+    fn object_fits(&self, object: &PlacementInfo, walls: &ContainingBlockPositionInfo) -> bool {
         match object.side {
             FloatSide::Left => {
                 // Compute a candidate left position for the object.
                 let candidate_left = match self.left {
-                    None => walls.left,
+                    None => walls.inline_start,
-                    Some(left) => left.max(walls.left),
+                    Some(left) => left.max(walls.inline_start),
                 };
 
                 // If this band has an existing left float in it, then make sure that the object
                 // doesn't stick out past the right edge (rule 7).
-                if self.left.is_some() && candidate_left + object.size.inline > walls.right {
+                if self.left.is_some() && candidate_left + object.size.inline > walls.inline_end {
                     return false;
                 }
 
@@ -317,13 +329,14 @@ impl FloatBand {
             FloatSide::Right => {
                 // Compute a candidate right position for the object.
                 let candidate_right = match self.right {
-                    None => walls.right,
+                    None => walls.inline_end,
-                    Some(right) => right.min(walls.right),
+                    Some(right) => right.min(walls.inline_end),
                 };
 
                 // If this band has an existing right float in it, then make sure that the new
                 // object doesn't stick out past the left edge (rule 7).
-                if self.right.is_some() && candidate_right - object.size.inline < walls.left {
+                if self.right.is_some() && candidate_right - object.size.inline < walls.inline_start
+                {
                     return false;
                 }
 
@@ -634,7 +647,7 @@ impl FloatBox {
         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!");
@@ -654,10 +667,9 @@ impl FloatBox {
         // 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
-            sequential_layout_state.bfc_relative_block_position +
+            .floats
-                sequential_layout_state.current_margin.solve(),
+            .lower_ceiling(sequential_layout_state.current_block_position_including_margins());
-        );
 
         let style = match self.contents {
             IndependentFormattingContext::Replaced(ref replaced) => replaced.style.clone(),
@@ -766,42 +778,12 @@ impl FloatBox {
                 )
             },
         );
-        sequential_layout_state.push_float_fragment(ArcRefCell::new(Fragment::Box(box_fragment)));
+        Fragment::Float(box_fragment)
     }
 }
 
 // Float fragment storage
 
-// A persistent linked list that stores float fragments that need to be hoisted to their nearest
-// ancestor containing block.
-#[derive(Clone)]
-struct FloatFragmentList {
-    root: FloatFragmentLink,
-}
-
-// A single link in the float fragment list.
-#[derive(Clone)]
-struct FloatFragmentLink(Option<Arc<FloatFragmentNode>>);
-
-// A single node in the float fragment list.
-#[derive(Clone)]
-struct FloatFragmentNode {
-    // The fragment.
-    fragment: ArcRefCell<Fragment>,
-    // The next fragment (previous in document order).
-    next: FloatFragmentLink,
-}
-
-impl FloatFragmentList {
-    fn new() -> FloatFragmentList {
-        FloatFragmentList {
-            root: FloatFragmentLink(None),
-        }
-    }
-}
-
-// Sequential layout state
-
 // Layout state that we maintain when doing sequential traversals of the box tree in document
 // order.
 //
@@ -816,18 +798,15 @@ impl FloatFragmentList {
 pub(crate) struct SequentialLayoutState {
     // Holds all floats in this block formatting context.
     pub(crate) floats: FloatContext,
-    // A list of all float fragments in this block formatting context. These are gathered up and
-    // hoisted to the top of the BFC.
-    bfc_float_fragments: FloatFragmentList,
     // The (logically) bottom border edge or top padding edge of the last in-flow block. Floats
     // cannot be placed above this line.
     //
     // This is often, but not always, the same as the float ceiling. The float ceiling can be lower
     // than this value because this value is calculated based on in-flow boxes only, while
     // out-of-flow floats can affect the ceiling as well (see CSS 2.1 § 9.5.1 rule 6).
-    bfc_relative_block_position: Length,
+    pub(crate) bfc_relative_block_position: Length,
     // Any collapsible margins that we've encountered after `bfc_relative_block_position`.
-    current_margin: CollapsedMargin,
+    pub(crate) current_margin: CollapsedMargin,
 }
 
 impl SequentialLayoutState {
@@ -837,7 +816,6 @@ impl SequentialLayoutState {
             floats: FloatContext::new(),
             current_margin: CollapsedMargin::zero(),
             bfc_relative_block_position: Length::zero(),
-            bfc_float_fragments: FloatFragmentList::new(),
         }
     }
 
@@ -849,6 +827,18 @@ impl SequentialLayoutState {
         self.floats.lower_ceiling(self.bfc_relative_block_position);
     }
 
+    pub(crate) fn update_all_containing_block_offsets(
+        &mut self,
+        mut new_distance: ContainingBlockPositionInfo,
+    ) -> ContainingBlockPositionInfo {
+        mem::swap(&mut new_distance, &mut self.floats.containing_block_info);
+        new_distance
+    }
+
+    pub(crate) fn current_block_position_including_margins(&self) -> Length {
+        self.bfc_relative_block_position + self.current_margin.solve()
+    }
+
     // Collapses margins, moving the block position down by the collapsed value of `current_margin`
     // and resetting `current_margin` to zero.
     //
@@ -869,8 +859,7 @@ impl SequentialLayoutState {
             return Length::zero();
         }
 
-        let hypothetical_block_position =
+        let hypothetical_block_position = self.current_block_position_including_margins();
-            self.bfc_relative_block_position + self.current_margin.solve();
         let clear_position = match clear_side {
             ClearSide::None => unreachable!(),
             ClearSide::Left => self
@@ -894,25 +883,4 @@ impl SequentialLayoutState {
     pub(crate) fn adjoin_assign(&mut self, margin: &CollapsedMargin) {
         self.current_margin.adjoin_assign(margin)
     }
-
-    /// Adds the float fragment to this list.
-    pub(crate) fn push_float_fragment(&mut self, new_float_fragment: ArcRefCell<Fragment>) {
-        self.bfc_float_fragments.root.0 = Some(Arc::new(FloatFragmentNode {
-            fragment: new_float_fragment,
-            next: FloatFragmentLink(self.bfc_float_fragments.root.0.take()),
-        }));
-    }
-
-    /// Adds the float fragments we've been building up to the given vector.
-    pub(crate) fn add_float_fragments_to_list(&self, fragment_list: &mut Vec<Fragment>) {
-        let start_index = fragment_list.len();
-        let mut link = &self.bfc_float_fragments.root;
-        while let Some(ref node) = link.0 {
-            fragment_list.push(Fragment::HoistedFloat(HoistedFloatFragment {
-                fragment: node.fragment.clone(),
-            }));
-            link = &node.next;
-        }
-        fragment_list[start_index..].reverse();
-    }
 }

components/layout_2020/flow/inline.rs

@@ -352,15 +352,29 @@ impl InlineFormattingContext {
                             .push(Fragment::AbsoluteOrFixedPositioned(hoisted_fragment));
                     },
                     InlineLevelBox::OutOfFlowFloatBox(box_) => {
-                        box_.layout(
+                        let mut fragment = box_.layout(
                             layout_context,
                             ifc.positioning_context,
                             containing_block,
                             ifc.sequential_layout_state.as_mut().map(|c| &mut **c),
                         );
-                        ifc.current_nesting_level
+                        if let Some(state) = &ifc.sequential_layout_state {
-                            .fragments_so_far
+                            let offset_from_formatting_context_to_containing_block = Vec2 {
-                            .push(Fragment::Float);
+                                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);
                     },
                 }
             } else

components/layout_2020/flow/mod.rs

@@ -4,9 +4,11 @@
 
 //! Flow layout, also known as block-and-inline layout.
 
+use std::ops::DerefMut;
+
 use crate::cell::ArcRefCell;
 use crate::context::LayoutContext;
-use crate::flow::float::{ClearSide, FloatBox, SequentialLayoutState};
+use crate::flow::float::{ClearSide, ContainingBlockPositionInfo, FloatBox, SequentialLayoutState};
 use crate::flow::inline::InlineFormattingContext;
 use crate::formatting_contexts::{
     IndependentFormattingContext, IndependentLayout, NonReplacedFormattingContext,
@@ -82,7 +84,7 @@ impl BlockFormattingContext {
             None
         };
 
-        let mut flow_layout = self.contents.layout(
+        let flow_layout = self.contents.layout(
             layout_context,
             positioning_context,
             containing_block,
@@ -96,11 +98,6 @@ impl BlockFormattingContext {
                 .collapsed_through
         );
 
-        // FIXME(pcwalton): Relative positioning of ancestors should affect descendant floats.
-        if let Some(ref sequential_layout_state) = sequential_layout_state {
-            sequential_layout_state.add_float_fragments_to_list(&mut flow_layout.fragments);
-        }
-
         IndependentLayout {
             fragments: flow_layout.fragments,
             content_block_size: flow_layout.content_block_size +
@@ -267,7 +264,10 @@ fn layout_block_level_children_sequentially(
                     tree_rank,
                     Some(&mut *sequential_layout_state),
                 );
+
                 placement_state.place_fragment(&mut fragment);
+                placement_state
+                    .adjust_positions_of_float_children(&mut fragment, sequential_layout_state);
                 fragment
             })
             .collect()
@@ -364,15 +364,12 @@ impl BlockLevelBox {
                 positioning_context.push(hoisted_box);
                 Fragment::AbsoluteOrFixedPositioned(hoisted_fragment)
             },
-            BlockLevelBox::OutOfFlowFloatBox(box_) => {
+            BlockLevelBox::OutOfFlowFloatBox(box_) => box_.layout(
-                box_.layout(
+                layout_context,
-                    layout_context,
+                positioning_context,
-                    positioning_context,
+                containing_block,
-                    containing_block,
+                sequential_layout_state,
-                    sequential_layout_state,
+            ),
-                );
-                Fragment::Float
-            },
         }
     }
 
@@ -489,9 +486,9 @@ fn layout_in_flow_non_replaced_block_level(
         min_box_size.block == Length::zero();
 
     let mut clearance = Length::zero();
-    let old_inline_walls;
+    let parent_containing_block_position_info;
     match sequential_layout_state {
-        None => old_inline_walls = None,
+        None => parent_containing_block_position_info = None,
         Some(ref mut sequential_layout_state) => {
             sequential_layout_state.adjoin_assign(&CollapsedMargin::new(margin.block_start));
             if !start_margin_can_collapse_with_children {
@@ -508,12 +505,26 @@ fn layout_in_flow_non_replaced_block_level(
                 pbm.padding.block_start + pbm.border.block_start + clearance,
             );
 
-            // Store our old inline walls so we can reset them later.
+            // We are about to lay out children. Update the offset between the block formatting
-            old_inline_walls = Some(sequential_layout_state.floats.walls);
+            // context and the containing block that we create for them. This offset is used to
-            sequential_layout_state.floats.walls.left +=
+            // ajust BFC relative coordinates to coordinates that are relative to our content box.
-                pbm.padding.inline_start + pbm.border.inline_start + margin.inline_start;
+            // Our content box establishes the containing block for non-abspos children, including
-            sequential_layout_state.floats.walls.right =
+            // floats.
-                sequential_layout_state.floats.walls.left + inline_size;
+            let inline_start = sequential_layout_state
+                .floats
+                .containing_block_info
+                .inline_start +
+                pbm.padding.inline_start +
+                pbm.border.inline_start +
+                margin.inline_start;
+            let new_cb_offsets = ContainingBlockPositionInfo {
+                block_start: sequential_layout_state.bfc_relative_block_position,
+                block_start_margins_not_collapsed: sequential_layout_state.current_margin,
+                inline_start,
+                inline_end: inline_start + inline_size,
+            };
+            parent_containing_block_position_info =
+                Some(sequential_layout_state.update_all_containing_block_offsets(new_cb_offsets));
         },
     };
 
@@ -579,8 +590,10 @@ fn layout_in_flow_non_replaced_block_level(
     });
 
     if let Some(ref mut sequential_layout_state) = sequential_layout_state {
-        // Now that we're done laying out our children, we can restore the old inline walls.
+        // Now that we're done laying out our children, we can restore the
-        sequential_layout_state.floats.walls = old_inline_walls.unwrap();
+        // parent's containing block position information.
+        sequential_layout_state
+            .update_all_containing_block_offsets(parent_containing_block_position_info.unwrap());
 
         // Account for padding and border. We also might have to readjust the
         // `bfc_relative_block_position` if it was different from the content size (i.e. was
@@ -749,7 +762,7 @@ impl PlacementState {
                 };
                 fragment.borrow_mut().adjust_offsets(offset);
             },
-            Fragment::Anonymous(_) | Fragment::Float => {},
+            Fragment::Anonymous(_) | Fragment::Float(_) => {},
             _ => unreachable!(),
         }
     }
@@ -761,4 +774,53 @@ impl PlacementState {
             end: self.current_margin,
         }
     }
+
+    /// 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.
+    fn adjust_positions_of_float_children(
+        &self,
+        fragment: &mut Fragment,
+        sequential_layout_state: &mut SequentialLayoutState,
+    ) {
+        let fragment = match fragment {
+            Fragment::Box(ref mut fragment) => fragment,
+            _ => return,
+        };
+
+        // TODO(mrobinson): Will these margins be accurate if this fragment
+        // collapses through. Can a fragment collapse through when it has a
+        // non-zero sized float inside? The float won't be positioned correctly
+        // anyway (see the comment in `floats.rs` about margin collapse), but
+        // this might make the result even worse.
+        let collapsed_margins = self.collapsible_margins_in_children().start.adjoin(
+            &sequential_layout_state
+                .floats
+                .containing_block_info
+                .block_start_margins_not_collapsed,
+        );
+
+        let parent_fragment_offset_in_cb = &fragment.content_rect.start_corner;
+        let parent_fragment_offset_in_formatting_context = Vec2 {
+            inline: sequential_layout_state
+                .floats
+                .containing_block_info
+                .inline_start +
+                parent_fragment_offset_in_cb.inline,
+            block: sequential_layout_state
+                .floats
+                .containing_block_info
+                .block_start +
+                collapsed_margins.solve() +
+                parent_fragment_offset_in_cb.block,
+        };
+
+        for child_fragment in fragment.children.iter_mut() {
+            if let Fragment::Float(box_fragment) = child_fragment.borrow_mut().deref_mut() {
+                box_fragment.content_rect.start_corner = &box_fragment.content_rect.start_corner -
+                    &parent_fragment_offset_in_formatting_context;
+            }
+        }
+    }
 }

components/layout_2020/flow/root.rs

@@ -440,14 +440,12 @@ impl FragmentTree {
             }
 
             let fragment_relative_rect = match fragment {
-                Fragment::Box(fragment) => fragment
+                Fragment::Box(fragment) | Fragment::Float(fragment) => fragment
                     .border_rect()
                     .to_physical(fragment.style.writing_mode, &containing_block),
                 Fragment::Text(fragment) => fragment
                     .rect
                     .to_physical(fragment.parent_style.writing_mode, &containing_block),
-                Fragment::HoistedFloat(_) |
-                Fragment::Float |
                 Fragment::AbsoluteOrFixedPositioned(_) |
                 Fragment::Image(_) |
                 Fragment::IFrame(_) |
@@ -521,15 +519,13 @@ impl FragmentTree {
             }
 
             scroll_area = match fragment {
-                Fragment::Box(fragment) => fragment
+                Fragment::Box(fragment) | Fragment::Float(fragment) => fragment
                     .scrollable_overflow(&containing_block)
                     .translate(containing_block.origin.to_vector()),
                 Fragment::Text(_) |
                 Fragment::AbsoluteOrFixedPositioned(_) |
                 Fragment::Image(_) |
                 Fragment::IFrame(_) |
-                Fragment::Float |
-                Fragment::HoistedFloat(_) |
                 Fragment::Anonymous(_) => return None,
             };
             None::<()>

components/layout_2020/fragments.rs

@@ -26,11 +26,12 @@ use webrender_api::{FontInstanceKey, ImageKey};
 #[derive(Serialize)]
 pub(crate) enum Fragment {
     Box(BoxFragment),
-    // The original document position of a float in the document tree.
+    /// Floating content. A floated fragment is very similar to a normal
-    Float,
+    /// [BoxFragment] but it isn't positioned using normal in block flow
-    // A float hoisted up from its original position (where a placeholder `Fragment::Float` is) to
+    /// positioning rules (margin collapse, etc). Instead, they are laid out by
-    // its containing block.
+    /// the [SequentialLayoutState] of their float containing block formatting
-    HoistedFloat(HoistedFloatFragment),
+    /// context.
+    Float(BoxFragment),
     Anonymous(AnonymousFragment),
     /// Absolute and fixed position fragments are hoisted up so that they
     /// are children of the BoxFragment that establishes their containing
@@ -45,13 +46,6 @@ pub(crate) enum Fragment {
     IFrame(IFrameFragment),
 }
 
-// A float hoisted up from its original position (where a placeholder `Fragment::Float` is) to its
-// containing block.
-#[derive(Serialize)]
-pub(crate) struct HoistedFloatFragment {
-    pub fragment: ArcRefCell<Fragment>,
-}
-
 #[derive(Serialize)]
 pub(crate) struct BoxFragment {
     pub base: BaseFragment,
@@ -171,9 +165,7 @@ impl Fragment {
     pub fn offset_inline(&mut self, offset: &Length) {
         let position = match self {
             Fragment::Box(f) => &mut f.content_rect.start_corner,
-            Fragment::HoistedFloat(_) |
+            Fragment::Float(_) | Fragment::AbsoluteOrFixedPositioned(_) => return,
-            Fragment::Float |
-            Fragment::AbsoluteOrFixedPositioned(_) => return,
             Fragment::Anonymous(f) => &mut f.rect.start_corner,
             Fragment::Text(f) => &mut f.rect.start_corner,
             Fragment::Image(f) => &mut f.rect.start_corner,
@@ -187,12 +179,11 @@ impl Fragment {
         Some(match self {
             Fragment::Box(fragment) => &fragment.base,
             Fragment::Text(fragment) => &fragment.base,
-            Fragment::Float => return None,
             Fragment::AbsoluteOrFixedPositioned(_) => return None,
             Fragment::Anonymous(fragment) => &fragment.base,
             Fragment::Image(fragment) => &fragment.base,
             Fragment::IFrame(fragment) => &fragment.base,
-            Fragment::HoistedFloat(_) => return None,
+            Fragment::Float(fragment) => &fragment.base,
         })
     }
 
@@ -203,8 +194,11 @@ impl Fragment {
     pub fn print(&self, tree: &mut PrintTree) {
         match self {
             Fragment::Box(fragment) => fragment.print(tree),
-            Fragment::HoistedFloat(fragment) => fragment.print(tree),
+            Fragment::Float(fragment) => {
-            Fragment::Float => tree.add_item(format!("Float")),
+                tree.new_level(format!("Float"));
+                fragment.print(tree);
+                tree.end_level();
+            },
             Fragment::AbsoluteOrFixedPositioned(_) => {
                 tree.add_item("AbsoluteOrFixedPositioned".to_string());
             },
@@ -220,11 +214,10 @@ impl Fragment {
         containing_block: &PhysicalRect<Length>,
     ) -> PhysicalRect<Length> {
         match self {
-            Fragment::Box(fragment) => fragment.scrollable_overflow_for_parent(&containing_block),
+            Fragment::Box(fragment) | Fragment::Float(fragment) => {
-            Fragment::HoistedFloat(fragment) => {
+                fragment.scrollable_overflow_for_parent(&containing_block)
-                (*fragment.fragment.borrow()).scrollable_overflow(&containing_block)
             },
-            Fragment::Float | Fragment::AbsoluteOrFixedPositioned(_) => PhysicalRect::zero(),
+            Fragment::AbsoluteOrFixedPositioned(_) => PhysicalRect::zero(),
             Fragment::Anonymous(fragment) => fragment.scrollable_overflow.clone(),
             Fragment::Text(fragment) => fragment
                 .rect
@@ -250,7 +243,7 @@ impl Fragment {
         }
 
         match self {
-            Fragment::Box(fragment) => {
+            Fragment::Box(fragment) | Fragment::Float(fragment) => {
                 let content_rect = fragment
                     .content_rect
                     .to_physical(fragment.style.writing_mode, containing_block)
@@ -294,15 +287,6 @@ impl Fragment {
     }
 }
 
-impl HoistedFloatFragment {
-    #[allow(dead_code)]
-    pub fn print(&self, tree: &mut PrintTree) {
-        tree.new_level(format!("HoistedFloatFragment"));
-        self.fragment.borrow().print(tree);
-        tree.end_level();
-    }
-}
-
 impl AnonymousFragment {
     pub fn new(rect: Rect<Length>, children: Vec<Fragment>, mode: WritingMode) -> Self {
         // FIXME(mrobinson, bug 25564): We should be using the containing block
@@ -450,19 +434,17 @@ impl BoxFragment {
                 \ncontent={:?}\
                 \npadding rect={:?}\
                 \nborder rect={:?}\
+                \nclearance={:?}\
                 \nscrollable_overflow={:?}\
-                \noverflow={:?} / {:?}\
+                \noverflow={:?} / {:?}",
-                \noverconstrained={:?}
-                \nstyle={:p}",
             self.base,
             self.content_rect,
             self.padding_rect(),
             self.border_rect(),
+            self.clearance,
             self.scrollable_overflow(&PhysicalRect::zero()),
             self.style.get_box().overflow_x,
             self.style.get_box().overflow_y,
-            self.overconstrained,
-            self.style,
         ));
 
         for child in &self.children {

components/layout_2020/positioned.rs

@@ -794,7 +794,7 @@ fn adjust_static_positions(
 
         let child_fragment_rect = match &child_fragments[original_tree_rank] {
             Fragment::Box(b) => &b.content_rect,
-            Fragment::AbsoluteOrFixedPositioned(_) | Fragment::Float => continue,
+            Fragment::AbsoluteOrFixedPositioned(_) | Fragment::Float(_) => continue,
             Fragment::Anonymous(a) => &a.rect,
             _ => unreachable!(),
         };

components/layout_2020/query.rs

@@ -425,7 +425,7 @@ fn process_offset_parent_query_inner(
             //
             // [1]: https://github.com/w3c/csswg-drafts/issues/4541
             let fragment_relative_rect = match fragment {
-                Fragment::Box(fragment) => fragment
+                Fragment::Box(fragment) | Fragment::Float(fragment) => fragment
                     .border_rect()
                     .to_physical(fragment.style.writing_mode, &containing_block),
                 Fragment::Text(fragment) => fragment
@@ -434,8 +434,6 @@ fn process_offset_parent_query_inner(
                 Fragment::AbsoluteOrFixedPositioned(_) |
                 Fragment::Image(_) |
                 Fragment::IFrame(_) |
-                Fragment::Float |
-                Fragment::HoistedFloat(_) |
                 Fragment::Anonymous(_) => unreachable!(),
             };
             let border_box = fragment_relative_rect.translate(containing_block.origin.to_vector());
@@ -485,7 +483,7 @@ fn process_offset_parent_query_inner(
         } else {
             // Record the paths of the nodes being traversed.
             let parent_node_address = match fragment {
-                Fragment::Box(fragment) => {
+                Fragment::Box(fragment) | Fragment::Float(fragment) => {
                     let is_eligible_parent =
                         match (is_body_element, fragment.style.get_box().position) {
                             // Spec says the element is eligible as `offsetParent` if any of
@@ -512,8 +510,6 @@ fn process_offset_parent_query_inner(
                 Fragment::Text(_) |
                 Fragment::Image(_) |
                 Fragment::IFrame(_) |
-                Fragment::Float |
-                Fragment::HoistedFloat(_) |
                 Fragment::Anonymous(_) => None,
             };
 
@@ -544,29 +540,28 @@ fn process_offset_parent_query_inner(
             fragment_tree
                 .find(|fragment, _, containing_block| {
                     match fragment {
-                        Fragment::Box(fragment)
+                        Fragment::Box(fragment) | Fragment::Float(fragment) => {
-                            if fragment.base.tag == Some(offset_parent_node_tag) =>
+                            if fragment.base.tag == Some(offset_parent_node_tag) {
-                        {
+                                // Again, take the *first* associated CSS layout box.
-                            // Again, take the *first* associated CSS layout box.
+                                let padding_box_corner = fragment
-                            let padding_box_corner = fragment
+                                    .padding_rect()
-                                .padding_rect()
+                                    .to_physical(fragment.style.writing_mode, &containing_block)
-                                .to_physical(fragment.style.writing_mode, &containing_block)
+                                    .origin
-                                .origin
+                                    .to_vector() +
-                                .to_vector() +
+                                    containing_block.origin.to_vector();
-                                containing_block.origin.to_vector();
+                                let padding_box_corner = Vector2D::new(
-                            let padding_box_corner = Vector2D::new(
+                                    Au::from_f32_px(padding_box_corner.x.px()),
-                                Au::from_f32_px(padding_box_corner.x.px()),
+                                    Au::from_f32_px(padding_box_corner.y.px()),
-                                Au::from_f32_px(padding_box_corner.y.px()),
+                                );
-                            );
+                                Some(padding_box_corner)
-                            Some(padding_box_corner)
+                            } else {
+                                None
+                            }
                         },
                         Fragment::AbsoluteOrFixedPositioned(_) |
-                        Fragment::Box(_) |
                         Fragment::Text(_) |
                         Fragment::Image(_) |
                         Fragment::IFrame(_) |
-                        Fragment::Float |
-                        Fragment::HoistedFloat(_) |
                         Fragment::Anonymous(_) => None,
                     }
                 })

components/layout_2020/tests/floats.rs

@@ -9,7 +9,7 @@ extern crate lazy_static;
 
 use euclid::num::Zero;
 use layout::flow::float::{ClearSide, FloatBand, FloatBandNode, FloatBandTree, FloatContext};
-use layout::flow::float::{FloatSide, InlineWalls, PlacementInfo};
+use layout::flow::float::{ContainingBlockOffsets, FloatSide, PlacementInfo};
 use layout::geom::flow_relative::{Rect, Vec2};
 use quickcheck::{Arbitrary, Gen};
 use std::f32;
@@ -339,7 +339,7 @@ struct FloatInput {
     ceiling: u32,
     /// Distances from the logical left side of the block formatting context to the logical sides
     /// of the current containing block.
-    walls: InlineWalls,
+    cb_offset: ContainingBlockOffsets,
 }
 
 impl Arbitrary for FloatInput {
@@ -366,7 +366,8 @@ impl Arbitrary for FloatInput {
                 clear: new_clear_side(clear),
             },
             ceiling,
-            walls: InlineWalls {
+            cb_offset: ContainingBlockOffsets {
+                top: Length::zero(),
                 left: Length::new(left as f32),
                 right: Length::new(right as f32),
             },
@@ -388,12 +389,12 @@ impl Arbitrary for FloatInput {
             this.info.clear = new_clear_side(clear_side);
             shrunk = true;
         }
-        if let Some(left) = self.walls.left.px().shrink().next() {
+        if let Some(left) = self.cb_offset.left.px().shrink().next() {
-            this.walls.left = Length::new(left);
+            this.cb_offset.left = Length::new(left);
             shrunk = true;
         }
-        if let Some(right) = self.walls.right.px().shrink().next() {
+        if let Some(right) = self.cb_offset.right.px().shrink().next() {
-            this.walls.right = Length::new(right);
+            this.cb_offset.right = Length::new(right);
             shrunk = true;
         }
         if let Some(ceiling) = self.ceiling.shrink().next() {
@@ -429,7 +430,7 @@ struct PlacedFloat {
     origin: Vec2<Length>,
     info: PlacementInfo,
     ceiling: Length,
-    walls: InlineWalls,
+    walls: ContainingBlockOffsets,
 }
 
 impl Drop for FloatPlacement {
@@ -470,12 +471,12 @@ impl FloatPlacement {
         for float in floats {
             let ceiling = Length::new(float.ceiling as f32);
             float_context.lower_ceiling(ceiling);
-            float_context.walls = float.walls;
+            float_context.cb_bfc_distance = float.cb_offset;
             placed_floats.push(PlacedFloat {
                 origin: float_context.add_float(&float.info),
                 info: float.info,
                 ceiling,
-                walls: float.walls,
+                walls: float.cb_offset,
             })
         }
         FloatPlacement {

tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/clear-on-child-with-margins-2.html.ini

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

tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/remove-block-before-self-collapsing-sibling-with-clearance.html.ini

@@ -0,0 +1,2 @@
+[remove-block-before-self-collapsing-sibling-with-clearance.html]
+  expected: FAIL

tests/wpt/metadata-layout-2020/css/CSS2/floats/float-no-interpolation.html.ini

@@ -8,18 +8,6 @@
   [CSS Transitions with transition: all: property <float> from [initial\] to [right\] at (0.3) should be [right\]]
     expected: FAIL
 
-  [CSS Animations: property <float> from [initial\] to [right\] at (0.5) should be [right\]]
-    expected: FAIL
-
-  [CSS Animations: property <float> from [initial\] to [right\] at (0.6) should be [right\]]
-    expected: FAIL
-
-  [CSS Animations: property <float> from [initial\] to [right\] at (1) should be [right\]]
-    expected: FAIL
-
-  [CSS Animations: property <float> from [initial\] to [right\] at (1.5) should be [right\]]
-    expected: FAIL
-
   [Web Animations: property <float> from [initial\] to [right\] at (-0.3) should be [initial\]]
     expected: FAIL
 

tests/wpt/metadata-layout-2020/css/CSS2/floats/float-root.html.ini

@@ -1,2 +1,2 @@
 [float-root.html]
-  expected: TIMEOUT
+  expected: FAIL

tests/wpt/metadata-layout-2020/css/CSS2/floats/float-under-flatten-under-preserve-3d.html.ini

@@ -1,2 +0,0 @@
-[float-under-flatten-under-preserve-3d.html]
-  expected: FAIL

tests/wpt/metadata-layout-2020/css/CSS2/floats/hit-test-floats-001.html.ini

@@ -1,3 +0,0 @@
-[hit-test-floats-001.html]
-  [hit-test-floats-001]
-    expected: FAIL

tests/wpt/metadata-layout-2020/css/CSS2/floats/hit-test-floats-004.html.ini

@@ -1,3 +0,0 @@
-[hit-test-floats-004.html]
-  [Miss float below something else]
-    expected: FAIL

tests/wpt/metadata-layout-2020/css/CSS2/floats/hit-test-floats-005.html.ini

@@ -1,3 +0,0 @@
-[hit-test-floats-005.html]
-  [Miss clipped float]
-    expected: FAIL

tests/wpt/metadata-layout-2020/css/CSS2/floats/new-fc-beside-adjoining-float.html.ini

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

tests/wpt/metadata-layout-2020/css/CSS2/floats/new-fc-separates-from-float.html.ini

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

tests/wpt/metadata-layout-2020/css/CSS2/normal-flow/block-in-inline-hittest-float-001.html.ini

@@ -1,3 +0,0 @@
-[block-in-inline-hittest-float-001.html]
-  [block-in-inline-hittest-float-001]
-    expected: FAIL

tests/wpt/metadata-layout-2020/css/CSS2/normal-flow/block-in-inline-hittest-float-002.html.ini

@@ -1,15 +0,0 @@
-[block-in-inline-hittest-float-002.html]
-  [block-in-inline-hittest-float-002]
-    expected: FAIL
-
-  [with background]
-    expected: FAIL
-
-  [with padding]
-    expected: FAIL
-
-  [floats before block-in-inline]
-    expected: FAIL
-
-  [floats before block-in-inline with background]
-    expected: FAIL