[LFC][BFC] Margin collapsing should not be limited to in-flow non-replaced boxes.
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 Jan 2019 16:18:52 +0000 (16:18 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 Jan 2019 16:18:52 +0000 (16:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=193183

Reviewed by Antti Koivisto.

* layout/FormattingContext.cpp:
(WebCore::Layout::FormattingContext::computeOutOfFlowVerticalGeometry const):
* layout/FormattingContextGeometry.cpp:
(WebCore::Layout::FormattingContext::Geometry::outOfFlowNonReplacedVerticalGeometry):
(WebCore::Layout::FormattingContext::Geometry::outOfFlowReplacedVerticalGeometry):
(WebCore::Layout::FormattingContext::Geometry::complicatedCases):
(WebCore::Layout::FormattingContext::Geometry::floatingNonReplacedWidthAndMargin):
(WebCore::Layout::FormattingContext::Geometry::inlineReplacedHeightAndMargin):
* layout/LayoutUnits.h:
* layout/blockformatting/BlockFormattingContext.cpp:
(WebCore::Layout::BlockFormattingContext::computeHeightAndMargin const):
* layout/blockformatting/BlockFormattingContextGeometry.cpp:
(WebCore::Layout::BlockFormattingContext::Geometry::inFlowNonReplacedHeightAndMargin):
(WebCore::Layout::BlockFormattingContext::Geometry::inFlowHeightAndMargin):
* layout/blockformatting/BlockFormattingContextQuirks.cpp:
(WebCore::Layout::BlockFormattingContext::Quirks::stretchedHeight):
* layout/inlineformatting/InlineFormattingContext.cpp:
(WebCore::Layout::InlineFormattingContext::computeHeightAndMargin const):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@239675 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/layout/FormattingContext.cpp
Source/WebCore/layout/FormattingContextGeometry.cpp
Source/WebCore/layout/LayoutUnits.h
Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
Source/WebCore/layout/blockformatting/BlockFormattingContextGeometry.cpp
Source/WebCore/layout/blockformatting/BlockFormattingContextQuirks.cpp
Source/WebCore/layout/inlineformatting/InlineFormattingContext.cpp

index cd67081..3b718d1 100644 (file)
@@ -1,5 +1,31 @@
 2019-01-07  Zalan Bujtas  <zalan@apple.com>
 
+        [LFC][BFC] Margin collapsing should not be limited to in-flow non-replaced boxes.
+        https://bugs.webkit.org/show_bug.cgi?id=193183
+
+        Reviewed by Antti Koivisto.
+
+        * layout/FormattingContext.cpp:
+        (WebCore::Layout::FormattingContext::computeOutOfFlowVerticalGeometry const):
+        * layout/FormattingContextGeometry.cpp:
+        (WebCore::Layout::FormattingContext::Geometry::outOfFlowNonReplacedVerticalGeometry):
+        (WebCore::Layout::FormattingContext::Geometry::outOfFlowReplacedVerticalGeometry):
+        (WebCore::Layout::FormattingContext::Geometry::complicatedCases):
+        (WebCore::Layout::FormattingContext::Geometry::floatingNonReplacedWidthAndMargin):
+        (WebCore::Layout::FormattingContext::Geometry::inlineReplacedHeightAndMargin):
+        * layout/LayoutUnits.h:
+        * layout/blockformatting/BlockFormattingContext.cpp:
+        (WebCore::Layout::BlockFormattingContext::computeHeightAndMargin const):
+        * layout/blockformatting/BlockFormattingContextGeometry.cpp:
+        (WebCore::Layout::BlockFormattingContext::Geometry::inFlowNonReplacedHeightAndMargin):
+        (WebCore::Layout::BlockFormattingContext::Geometry::inFlowHeightAndMargin):
+        * layout/blockformatting/BlockFormattingContextQuirks.cpp:
+        (WebCore::Layout::BlockFormattingContext::Quirks::stretchedHeight):
+        * layout/inlineformatting/InlineFormattingContext.cpp:
+        (WebCore::Layout::InlineFormattingContext::computeHeightAndMargin const):
+
+2019-01-07  Zalan Bujtas  <zalan@apple.com>
+
         [LFC][BFC] Move MarginCollapse from BlockFormattingContext::Geometry to BlockFormattingContext
         https://bugs.webkit.org/show_bug.cgi?id=193181
 
index 0ccf2d1..a767e93 100644 (file)
@@ -115,12 +115,11 @@ void FormattingContext::computeOutOfFlowVerticalGeometry(const Box& layoutBox) c
     }
 
     auto& displayBox = layoutState.displayBoxForLayoutBox(layoutBox);
-    // Margins of absolutely positioned boxes do not collapse
-    ASSERT(!verticalGeometry.heightAndMargin.usedMargin.hasCollapsedValues());
-    auto nonCollapsedVerticalMargin = verticalGeometry.heightAndMargin.usedMargin.nonCollapsedValues();
+    auto nonCollapsedVerticalMargin = verticalGeometry.heightAndMargin.nonCollapsedMargin;
     displayBox.setTop(verticalGeometry.top + nonCollapsedVerticalMargin.before);
     displayBox.setContentBoxHeight(verticalGeometry.heightAndMargin.height);
-    displayBox.setVerticalMargin(verticalGeometry.heightAndMargin.usedMargin);
+    // Margins of absolutely positioned boxes do not collapse
+    displayBox.setVerticalMargin({ nonCollapsedVerticalMargin, { } });
 }
 
 void FormattingContext::computeBorderAndPadding(const Box& layoutBox) const
index cca6ef9..75e7b7e 100644 (file)
@@ -351,7 +351,7 @@ VerticalGeometry FormattingContext::Geometry::outOfFlowNonReplacedVerticalGeomet
     ASSERT(height);
 
     LOG_WITH_STREAM(FormattingContextLayout, stream << "[Position][Height][Margin] -> out-of-flow non-replaced -> top(" << *top << "px) bottom("  << *bottom << "px) height(" << *height << "px) margin(" << usedVerticalMargin.before << "px, "  << usedVerticalMargin.after << "px) layoutBox(" << &layoutBox << ")");
-    return { *top, *bottom, { *height, { usedVerticalMargin, { } } } };
+    return { *top, *bottom, { *height, usedVerticalMargin } };
 }
 
 HorizontalGeometry FormattingContext::Geometry::outOfFlowNonReplacedHorizontalGeometry(LayoutState& layoutState, const Box& layoutBox, Optional<LayoutUnit> usedWidth)
@@ -558,7 +558,7 @@ VerticalGeometry FormattingContext::Geometry::outOfFlowReplacedVerticalGeometry(
         bottom = containingBlockHeight - (*top + usedVerticalMargin.before + borderTop + paddingTop + height + paddingBottom + borderBottom + usedVerticalMargin.after); 
 
     LOG_WITH_STREAM(FormattingContextLayout, stream << "[Position][Height][Margin] -> out-of-flow replaced -> top(" << *top << "px) bottom("  << *bottom << "px) height(" << height << "px) margin(" << usedVerticalMargin.before << "px, "  << usedVerticalMargin.after << "px) layoutBox(" << &layoutBox << ")");
-    return { *top, *bottom, { height, { usedVerticalMargin, { } } } };
+    return { *top, *bottom, { height, usedVerticalMargin } };
 }
 
 HorizontalGeometry FormattingContext::Geometry::outOfFlowReplacedHorizontalGeometry(const LayoutState& layoutState, const Box& layoutBox, Optional<LayoutUnit> usedWidth)
@@ -682,7 +682,7 @@ HeightAndMargin FormattingContext::Geometry::complicatedCases(const LayoutState&
     ASSERT(height);
 
     LOG_WITH_STREAM(FormattingContextLayout, stream << "[Height][Margin] -> floating non-replaced -> height(" << *height << "px) margin(" << usedVerticalMargin.before << "px, " << usedVerticalMargin.after << "px) -> layoutBox(" << &layoutBox << ")");
-    return HeightAndMargin { *height, { usedVerticalMargin, { } } };
+    return HeightAndMargin { *height, usedVerticalMargin };
 }
 
 WidthAndMargin FormattingContext::Geometry::floatingNonReplacedWidthAndMargin(LayoutState& layoutState, const Box& layoutBox, Optional<LayoutUnit> usedWidth)
@@ -696,16 +696,17 @@ WidthAndMargin FormattingContext::Geometry::floatingNonReplacedWidthAndMargin(La
 
     auto& containingBlock = *layoutBox.containingBlock();
     auto containingBlockWidth = layoutState.displayBoxForLayoutBox(containingBlock).contentBoxWidth();
+    auto computedHorizontalMargin = Geometry::computedHorizontalMargin(layoutState, layoutBox);
 
     // #1
-    auto computedHorizontalMargin = Geometry::computedHorizontalMargin(layoutState, layoutBox);
+    auto usedHorizontallMargin = UsedHorizontalMargin { computedHorizontalMargin.start.valueOr(0), computedHorizontalMargin.end.valueOr(0) };
     // #2
     auto width = computedValueIfNotAuto(usedWidth ? Length { usedWidth.value(), Fixed } : layoutBox.style().logicalWidth(), containingBlockWidth);
     if (!width)
         width = shrinkToFitWidth(layoutState, layoutBox);
 
-    LOG_WITH_STREAM(FormattingContextLayout, stream << "[Width][Margin] -> floating non-replaced -> width(" << *width << "px) margin(" << computedHorizontalMargin.start.valueOr(0_lu) << "px, " << computedHorizontalMargin.end.valueOr(0_lu) << "px) -> layoutBox(" << &layoutBox << ")");
-    return WidthAndMargin { *width, { computedHorizontalMargin.start.valueOr(0_lu), computedHorizontalMargin.end.valueOr(0_lu) }, computedHorizontalMargin };
+    LOG_WITH_STREAM(FormattingContextLayout, stream << "[Width][Margin] -> floating non-replaced -> width(" << *width << "px) margin(" << usedHorizontallMargin.start << "px, " << usedHorizontallMargin.end << "px) -> layoutBox(" << &layoutBox << ")");
+    return WidthAndMargin { *width, usedHorizontallMargin, computedHorizontalMargin };
 }
 
 HeightAndMargin FormattingContext::Geometry::floatingReplacedHeightAndMargin(const LayoutState& layoutState, const Box& layoutBox, Optional<LayoutUnit> usedHeight)
@@ -810,7 +811,7 @@ HeightAndMargin FormattingContext::Geometry::inlineReplacedHeightAndMargin(const
     ASSERT(height);
 
     LOG_WITH_STREAM(FormattingContextLayout, stream << "[Height][Margin] -> inflow replaced -> height(" << *height << "px) margin(" << usedVerticalMargin.before << "px, " << usedVerticalMargin.after << "px) -> layoutBox(" << &layoutBox << ")");
-    return { *height, { usedVerticalMargin, { } } };
+    return { *height, usedVerticalMargin };
 }
 
 WidthAndMargin FormattingContext::Geometry::inlineReplacedWidthAndMargin(const LayoutState& layoutState, const Box& layoutBox,
index a63d723..ea1be90 100644 (file)
@@ -109,7 +109,7 @@ struct WidthAndMargin {
 
 struct HeightAndMargin {
     LayoutUnit height;
-    UsedVerticalMargin usedMargin;
+    UsedVerticalMargin::NonCollapsedValues nonCollapsedMargin;
 };
 
 struct HorizontalGeometry {
index a73a577..2481116 100644 (file)
@@ -358,7 +358,7 @@ void BlockFormattingContext::computeHeightAndMargin(const Box& layoutBox) const
             auto maxHeightAndMargin = compute(maxHeight);
             // Used height should remain the same.
             ASSERT((layoutState.inQuirksMode() && (layoutBox.isBodyBox() || layoutBox.isDocumentBox())) || maxHeightAndMargin.height == *maxHeight);
-            heightAndMargin = { *maxHeight, maxHeightAndMargin.usedMargin };
+            heightAndMargin = { *maxHeight, maxHeightAndMargin.nonCollapsedMargin };
         }
     }
 
@@ -367,17 +367,19 @@ void BlockFormattingContext::computeHeightAndMargin(const Box& layoutBox) const
             auto minHeightAndMargin = compute(minHeight);
             // Used height should remain the same.
             ASSERT((layoutState.inQuirksMode() && (layoutBox.isBodyBox() || layoutBox.isDocumentBox())) || minHeightAndMargin.height == *minHeight);
-            heightAndMargin = { *minHeight, minHeightAndMargin.usedMargin };
+            heightAndMargin = { *minHeight, minHeightAndMargin.nonCollapsedMargin };
         }
     }
 
+    auto collapsedMargin = UsedVerticalMargin::CollapsedValues { MarginCollapse::marginBefore(layoutState, layoutBox), MarginCollapse::marginAfter(layoutState, layoutBox) };
+    auto verticalMargin = UsedVerticalMargin { heightAndMargin.nonCollapsedMargin, collapsedMargin };
     auto& displayBox = layoutState.displayBoxForLayoutBox(layoutBox);
     displayBox.setContentBoxHeight(heightAndMargin.height);
-    displayBox.setVerticalMargin(heightAndMargin.usedMargin);
+    displayBox.setVerticalMargin(verticalMargin);
 
     // If this box has already been moved by the estimated vertical margin, no need to move it again.
     if (layoutBox.isFloatingPositioned() || !displayBox.estimatedMarginBefore())
-        displayBox.moveVertically(heightAndMargin.usedMargin.before());
+        displayBox.moveVertically(verticalMargin.before());
 }
 
 FormattingContext::InstrinsicWidthConstraints BlockFormattingContext::instrinsicWidthConstraints() const
index fc75b53..4cbbc63 100644 (file)
@@ -60,22 +60,21 @@ HeightAndMargin BlockFormattingContext::Geometry::inFlowNonReplacedHeightAndMarg
         auto& displayBox = layoutState.displayBoxForLayoutBox(layoutBox);
         auto computedVerticalMargin = Geometry::computedVerticalMargin(layoutState, layoutBox);
         auto nonCollapsedMargin = UsedVerticalMargin::NonCollapsedValues { computedVerticalMargin.before.valueOr(0), computedVerticalMargin.after.valueOr(0) }; 
-        auto collapsedMargin = UsedVerticalMargin::CollapsedValues { MarginCollapse::marginBefore(layoutState, layoutBox), MarginCollapse::marginAfter(layoutState, layoutBox) };
         auto borderAndPaddingTop = displayBox.borderTop() + displayBox.paddingTop().valueOr(0);
 
         auto height = usedHeight ? usedHeight.value() : computedHeightValue(layoutState, layoutBox, HeightType::Normal);
         if (height)
-            return { height.value(), { nonCollapsedMargin, collapsedMargin } };
+            return { height.value(), nonCollapsedMargin };
 
         if (!is<Container>(layoutBox) || !downcast<Container>(layoutBox).hasInFlowChild())
-            return { 0, { nonCollapsedMargin, collapsedMargin } };
+            return { 0, nonCollapsedMargin };
 
         // 1. the bottom edge of the last line box, if the box establishes a inline formatting context with one or more lines
         if (layoutBox.establishesInlineFormattingContext()) {
             // This is temp and will be replaced by the correct display box once inline runs move over to the display tree.
             auto& inlineRuns = downcast<InlineFormattingState>(layoutState.establishedFormattingState(layoutBox)).inlineRuns();
             auto bottomEdge = inlineRuns.isEmpty() ? LayoutUnit() : inlineRuns.last().logicalBottom();
-            return { bottomEdge, { nonCollapsedMargin, collapsedMargin } };
+            return { bottomEdge, nonCollapsedMargin };
         }
 
         // 2. the bottom edge of the bottom (possibly collapsed) margin of its last in-flow child, if the child's bottom margin...
@@ -83,7 +82,7 @@ HeightAndMargin BlockFormattingContext::Geometry::inFlowNonReplacedHeightAndMarg
         ASSERT(lastInFlowChild);
         if (!MarginCollapse::marginAfterCollapsesWithParentMarginAfter(layoutState, *lastInFlowChild)) {
             auto& lastInFlowDisplayBox = layoutState.displayBoxForLayoutBox(*lastInFlowChild);
-            return { lastInFlowDisplayBox.bottom() + lastInFlowDisplayBox.marginAfter() - borderAndPaddingTop, { nonCollapsedMargin, collapsedMargin } };
+            return { lastInFlowDisplayBox.bottom() + lastInFlowDisplayBox.marginAfter() - borderAndPaddingTop, nonCollapsedMargin };
         }
 
         // 3. the bottom border edge of the last in-flow child whose top margin doesn't collapse with the element's bottom margin
@@ -92,16 +91,16 @@ HeightAndMargin BlockFormattingContext::Geometry::inFlowNonReplacedHeightAndMarg
             inFlowChild = inFlowChild->previousInFlowSibling();
         if (inFlowChild) {
             auto& inFlowDisplayBox = layoutState.displayBoxForLayoutBox(*inFlowChild);
-            return { inFlowDisplayBox.top() + inFlowDisplayBox.borderBox().height() - borderAndPaddingTop, { nonCollapsedMargin, collapsedMargin } };
+            return { inFlowDisplayBox.top() + inFlowDisplayBox.borderBox().height() - borderAndPaddingTop, nonCollapsedMargin };
         }
 
         // 4. zero, otherwise
-        return { 0, { nonCollapsedMargin, collapsedMargin } };
+        return { 0, nonCollapsedMargin };
     };
 
     auto heightAndMargin = compute();
 
-    LOG_WITH_STREAM(FormattingContextLayout, stream << "[Height][Margin] -> inflow non-replaced -> height(" << heightAndMargin.height << "px) margin(" << heightAndMargin.usedMargin.before() << "px, " << heightAndMargin.usedMargin.after() << "px) -> layoutBox(" << &layoutBox << ")");
+    LOG_WITH_STREAM(FormattingContextLayout, stream << "[Height][Margin] -> inflow non-replaced -> height(" << heightAndMargin.height << "px) margin(" << heightAndMargin.nonCollapsedMargin.before << "px, " << heightAndMargin.nonCollapsedMargin.after << "px) -> layoutBox(" << &layoutBox << ")");
     return heightAndMargin;
 }
 
@@ -260,7 +259,7 @@ HeightAndMargin BlockFormattingContext::Geometry::inFlowHeightAndMargin(const La
 
     heightAndMargin = Quirks::stretchedHeight(layoutState, layoutBox, heightAndMargin);
 
-    LOG_WITH_STREAM(FormattingContextLayout, stream << "[Height][Margin] -> inflow non-replaced -> streched to viewport -> height(" << heightAndMargin.height << "px) margin(" << heightAndMargin.usedMargin.before() << "px, " << heightAndMargin.usedMargin.after() << "px) -> layoutBox(" << &layoutBox << ")");
+    LOG_WITH_STREAM(FormattingContextLayout, stream << "[Height][Margin] -> inflow non-replaced -> streched to viewport -> height(" << heightAndMargin.height << "px) margin(" << heightAndMargin.nonCollapsedMargin.before << "px, " << heightAndMargin.nonCollapsedMargin.after << "px) -> layoutBox(" << &layoutBox << ")");
     return heightAndMargin;
 }
 
index f3ebd2d..a107974 100644 (file)
@@ -80,10 +80,9 @@ HeightAndMargin BlockFormattingContext::Quirks::stretchedHeight(const LayoutStat
 
     LayoutUnit totalVerticalMargin;
     if (layoutBox.isDocumentBox()) {
-        auto verticalMargin = heightAndMargin.usedMargin;
         // Document box's margins do not collapse.
-        ASSERT(!verticalMargin.hasCollapsedValues());
-        totalVerticalMargin = verticalMargin.before() + verticalMargin.after();
+        auto verticalMargin = heightAndMargin.nonCollapsedMargin;
+        totalVerticalMargin = verticalMargin.before + verticalMargin.after;
     } else if (layoutBox.isBodyBox()) {
         // Here is the quirky part for body box:
         // Stretch the body using the initial containing block's height and shrink it with document box's margin/border/padding.
@@ -91,14 +90,8 @@ HeightAndMargin BlockFormattingContext::Quirks::stretchedHeight(const LayoutStat
         auto verticalMargin = Geometry::estimatedMarginBefore(layoutState, documentBox) + Geometry::estimatedMarginAfter(layoutState, documentBox);
         strechedHeight -= verticalMargin;
 
-        // This quirk happens when the body height is 0 which means its vertical margins collapse through (top and bottom margins are adjoining).
-        // However now that we stretch the body they don't collapse through anymore, so we need to use the non-collapsed values instead.
-        if (heightAndMargin.height)
-            totalVerticalMargin = heightAndMargin.usedMargin.before() + heightAndMargin.usedMargin.after();
-        else {
-            auto nonCollapsedValues = heightAndMargin.usedMargin.nonCollapsedValues();
-            totalVerticalMargin = nonCollapsedValues.before + nonCollapsedValues.after;
-        }
+        auto nonCollapsedValues = heightAndMargin.nonCollapsedMargin;
+        totalVerticalMargin = nonCollapsedValues.before + nonCollapsedValues.after;
     }
 
     // Stretch but never overstretch with the margins.
index e0aaf59..0c3f5c8 100644 (file)
@@ -362,7 +362,7 @@ void InlineFormattingContext::computeHeightAndMargin(const Box& layoutBox) const
 
     auto& displayBox = layoutState.displayBoxForLayoutBox(layoutBox);
     displayBox.setContentBoxHeight(heightAndMargin.height);
-    displayBox.setVerticalMargin(heightAndMargin.usedMargin);
+    displayBox.setVerticalMargin({ heightAndMargin.nonCollapsedMargin, { } });
 }
 
 void InlineFormattingContext::layoutFormattingContextRoot(const Box& root) const