From 0dd6abe0230c69df72b1e31b4793ffa548fc9300 Mon Sep 17 00:00:00 2001 From: Oriol Brufau Date: Wed, 21 Jun 2023 11:04:29 +0200 Subject: [PATCH] Fix interaction of clearance with margin collapse In #29897 I did the simple naive thing, but it wasn't entirely correct. This patch tries to address the problems. In particular: - Clearance should prevent margins from collapsing through if it happens between them, as opposed to on the element that owns them. - The margins of an element with clearance can still collapse through, and collapse with other siblings as normal, but the resulting margin can't collapse with the bottom margin of the parent. --- components/layout_2020/flow/mod.rs | 53 ++++++++++++++----- ...float-nested-forced-clearance-002.html.ini | 2 - .../floats-clear/margin-collapse-033.xht.ini | 2 - .../floats-clear/margin-collapse-034.xht.ini | 2 - .../floats-clear/margin-collapse-035.xht.ini | 2 - .../margin-collapse-clear-012.xht.ini | 2 - .../margin-collapse-clear-013.xht.ini | 2 - 7 files changed, 41 insertions(+), 24 deletions(-) delete mode 100644 tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/adjoining-float-nested-forced-clearance-002.html.ini delete mode 100644 tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-033.xht.ini delete mode 100644 tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-034.xht.ini delete mode 100644 tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-035.xht.ini delete mode 100644 tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-clear-012.xht.ini delete mode 100644 tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-clear-013.xht.ini diff --git a/components/layout_2020/flow/mod.rs b/components/layout_2020/flow/mod.rs index 1ded969ea0a..82d4fa2efc9 100644 --- a/components/layout_2020/flow/mod.rs +++ b/components/layout_2020/flow/mod.rs @@ -306,10 +306,11 @@ fn layout_block_level_children_in_parallel( }) .collect(); + let (content_block_size, collapsible_margins_in_children) = placement_state.finish(); FlowLayout { fragments, - content_block_size: placement_state.current_block_direction_position, - collapsible_margins_in_children: placement_state.collapsible_margins_in_children(), + content_block_size, + collapsible_margins_in_children, } } @@ -349,10 +350,11 @@ fn layout_block_level_children_sequentially( }) .collect(); + let (content_block_size, collapsible_margins_in_children) = placement_state.finish(); FlowLayout { fragments, - content_block_size: placement_state.current_block_direction_position, - collapsible_margins_in_children: placement_state.collapsible_margins_in_children(), + content_block_size, + collapsible_margins_in_children, } } @@ -641,7 +643,6 @@ fn layout_in_flow_non_replaced_block_level( collapsible_margins_in_children.collapsed_through && block_is_same_formatting_context && pbm.padding_border_sums.block == Length::zero() && - clearance == Length::zero() && block_size.auto_is(|| Length::zero()) == Length::zero() && min_box_size.block == Length::zero(); }, @@ -775,6 +776,7 @@ fn solve_inline_margins_for_in_flow_block_level( // mode, this is done right after each block is laid out. pub(crate) 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, current_margin: CollapsedMargin, current_block_direction_position: Length, @@ -787,6 +789,7 @@ impl PlacementState { PlacementState { next_in_flow_margin_collapses_with_parent_start_margin: collapsible_with_parent_start_margin.0, + last_in_flow_margin_collapses_with_parent_end_margin: true, start_margin: CollapsedMargin::zero(), current_margin: CollapsedMargin::zero(), current_block_direction_position: Length::zero(), @@ -801,7 +804,23 @@ impl PlacementState { fragment.padding.block_sum() + fragment.border.block_sum() + fragment.content_rect.size.block; - + // We use `last_in_flow_margin_collapses_with_parent_end_margin` to implement + // this quote from https://drafts.csswg.org/css2/#collapsing-margins + // > If the top and bottom margins of an element with clearance are adjoining, + // > its margins collapse with the adjoining margins of following siblings but that + // > resulting margin does not collapse with the bottom margin of the parent block. + if fragment.clearance != Length::zero() { + // Margins can't be adjoining if they are separated by clearance. + // Setting `next_in_flow_margin_collapses_with_parent_start_margin` to false + // prevents collapsing with the start margin of the parent, and will set + // `collapsed_through` to false, preventing the parent from collapsing through. + self.next_in_flow_margin_collapses_with_parent_start_margin = false; + if fragment_block_margins.collapsed_through { + self.last_in_flow_margin_collapses_with_parent_end_margin = false; + } + } 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 @@ -818,6 +837,9 @@ impl PlacementState { 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_margin .adjoin_assign(&fragment_block_margins.end); return; @@ -838,12 +860,19 @@ impl PlacementState { } } - fn collapsible_margins_in_children(&self) -> CollapsedBlockMargins { - CollapsedBlockMargins { - collapsed_through: self.next_in_flow_margin_collapses_with_parent_start_margin, - start: self.start_margin, - end: self.current_margin, + fn finish(mut self) -> (Length, CollapsedBlockMargins) { + if !self.last_in_flow_margin_collapses_with_parent_end_margin { + self.current_block_direction_position += self.current_margin.solve(); + self.current_margin = CollapsedMargin::zero(); } + ( + self.current_block_direction_position, + CollapsedBlockMargins { + collapsed_through: self.next_in_flow_margin_collapses_with_parent_start_margin, + start: self.start_margin, + end: self.current_margin, + }, + ) } /// When Float fragments are created in block flows, they are positioned @@ -865,7 +894,7 @@ impl PlacementState { // 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( + let collapsed_margins = self.start_margin.adjoin( &sequential_layout_state .floats .containing_block_info diff --git a/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/adjoining-float-nested-forced-clearance-002.html.ini b/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/adjoining-float-nested-forced-clearance-002.html.ini deleted file mode 100644 index a8a9b5c830c..00000000000 --- a/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/adjoining-float-nested-forced-clearance-002.html.ini +++ /dev/null @@ -1,2 +0,0 @@ -[adjoining-float-nested-forced-clearance-002.html] - expected: FAIL diff --git a/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-033.xht.ini b/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-033.xht.ini deleted file mode 100644 index 1023cc94695..00000000000 --- a/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-033.xht.ini +++ /dev/null @@ -1,2 +0,0 @@ -[margin-collapse-033.xht] - expected: FAIL diff --git a/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-034.xht.ini b/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-034.xht.ini deleted file mode 100644 index 818ff0893c9..00000000000 --- a/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-034.xht.ini +++ /dev/null @@ -1,2 +0,0 @@ -[margin-collapse-034.xht] - expected: FAIL diff --git a/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-035.xht.ini b/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-035.xht.ini deleted file mode 100644 index fc29e78ad3a..00000000000 --- a/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-035.xht.ini +++ /dev/null @@ -1,2 +0,0 @@ -[margin-collapse-035.xht] - expected: FAIL diff --git a/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-clear-012.xht.ini b/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-clear-012.xht.ini deleted file mode 100644 index a14c5dc9d93..00000000000 --- a/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-clear-012.xht.ini +++ /dev/null @@ -1,2 +0,0 @@ -[margin-collapse-clear-012.xht] - expected: FAIL diff --git a/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-clear-013.xht.ini b/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-clear-013.xht.ini deleted file mode 100644 index cf23903e90b..00000000000 --- a/tests/wpt/metadata-layout-2020/css/CSS2/floats-clear/margin-collapse-clear-013.xht.ini +++ /dev/null @@ -1,2 +0,0 @@ -[margin-collapse-clear-013.xht] - expected: FAIL