Skip to content

Commit a1dfadc

Browse files
authored
Auto merge of servo#29951 - Loirooriol:margin-collapse-use-computed, r=mrobinson
Check computed (min-)block-size when determining margin collapse To collapse margins through, CSS2 requires "zero or auto computed height" and "zero computed min-height" (the latter should also also include auto, which was introduced in CSS3 as the new initial value). Similarly, "auto computed height" is required to collapse the bottom margin with the bottom margin of the contents. Therefore this patch stops collapsing when the used height is clamped to zero by max-height, but the computed value is not zero. Same for non-zero percentages that are indefinite or that resolve to 0px. Note that 0% and calc(0% + 0px) are still considered to be zero (so they allow margin collapse), this may a bit inconsistent but matches Firefox (for the collapsing through case). This also matches the heuristics in find_block_margin_collapsing_with_parent(). We could change the heuristics instead, but then they would have to track block sizes in order to be able to resolve percentages. The change makes margin-collapse-through-percentage-height-block.html fail, that test is only passing on Blink, which did a different interpretation of the spec. To be fair, various places of CSS2 loosely consider that indefinite percentages compute to auto, and even Firefox does so when determining whether to collapse the top margin with the contents. Since the spec isn't particularly clear and interoperability is lacking, I filed w3c/csswg-drafts#8919. <!-- 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 servo#29907 - [X] There are tests for these changes (1 test fails, see above) <!-- 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. -->
2 parents c41334e + 7d8122b commit a1dfadc

File tree

2 files changed

+8
-4
lines changed

2 files changed

+8
-4
lines changed

components/layout_2020/flow/mod.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -656,13 +656,14 @@ fn layout_in_flow_non_replaced_block_level(
656656
NonReplacedContents::EstablishesAnIndependentFormattingContext(_) => false,
657657
};
658658

659+
let computed_block_size = style.content_block_size();
659660
let start_margin_can_collapse_with_children = block_is_same_formatting_context &&
660661
pbm.padding.block_start == Length::zero() &&
661662
pbm.border.block_start == Length::zero();
662663
let end_margin_can_collapse_with_children = block_is_same_formatting_context &&
663664
pbm.padding.block_end == Length::zero() &&
664665
pbm.border.block_end == Length::zero() &&
665-
block_size == LengthOrAuto::Auto;
666+
computed_block_size.is_auto();
666667

667668
let mut clearance = None;
668669
let parent_containing_block_position_info;
@@ -763,12 +764,13 @@ fn layout_in_flow_non_replaced_block_level(
763764
} else {
764765
content_block_size += collapsible_margins_in_children.end.solve();
765766
}
767+
let computed_min_block_size = style.min_block_size();
766768
block_margins_collapsed_with_children.collapsed_through =
767769
collapsible_margins_in_children.collapsed_through &&
768-
block_is_same_formatting_context &&
769770
pbm.padding_border_sums.block == Length::zero() &&
770-
block_size.auto_is(|| Length::zero()) == Length::zero() &&
771-
min_box_size.block == Length::zero();
771+
(computed_block_size.is_definitely_zero() || computed_block_size.is_auto()) &&
772+
(computed_min_block_size.is_definitely_zero() ||
773+
computed_min_block_size.is_auto());
772774
},
773775
NonReplacedContents::EstablishesAnIndependentFormattingContext(non_replaced) => {
774776
let independent_layout = non_replaced.layout(
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
[margin-collapse-through-percentage-height-block.html]
2+
expected: FAIL

0 commit comments

Comments
 (0)