Attachment 358469 Details for Bug 193183 – Patch

[patch] Patch

Patch.txt (text/plain), 18.97 KB, created by zalan on 2019-01-06 17:23:04 PST

(hide)

Description:

Filename:

MIME Type:

Creator: zalan

Created: 2019-01-06 17:23:04 PST

Size: 18.97 KB

patch

obsolete

>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 3d455e56c4e..d84c7de2453 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,29 @@
>+2019-01-06  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 NOBODY (OOPS!).
>+
>+        * 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-06  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC][BFC] Move MarginCollapse from BlockFormattingContext::Geometry to BlockFormattingContext
>diff --git a/Source/WebCore/layout/FormattingContext.cpp b/Source/WebCore/layout/FormattingContext.cpp
>index 0ccf2d19eb2..a767e93e95f 100644
>--- a/Source/WebCore/layout/FormattingContext.cpp
>+++ b/Source/WebCore/layout/FormattingContext.cpp
>@@ -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
>diff --git a/Source/WebCore/layout/FormattingContextGeometry.cpp b/Source/WebCore/layout/FormattingContextGeometry.cpp
>index cca6ef9a1d9..75e7b7ee69c 100644
>--- a/Source/WebCore/layout/FormattingContextGeometry.cpp
>+++ b/Source/WebCore/layout/FormattingContextGeometry.cpp
>@@ -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,
>diff --git a/Source/WebCore/layout/LayoutUnits.h b/Source/WebCore/layout/LayoutUnits.h
>index a63d723d7cc..ea1be90a9f6 100644
>--- a/Source/WebCore/layout/LayoutUnits.h
>+++ b/Source/WebCore/layout/LayoutUnits.h
>@@ -109,7 +109,7 @@ struct WidthAndMargin {
> 
> struct HeightAndMargin {
>     LayoutUnit height;
>-    UsedVerticalMargin usedMargin;
>+    UsedVerticalMargin::NonCollapsedValues nonCollapsedMargin;
> };
> 
> struct HorizontalGeometry {
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>index a73a5771c04..2481116afd7 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>@@ -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
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContextGeometry.cpp b/Source/WebCore/layout/blockformatting/BlockFormattingContextGeometry.cpp
>index fc75b5307c3..4cbbc633040 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContextGeometry.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContextGeometry.cpp
>@@ -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;
> }
> 
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContextQuirks.cpp b/Source/WebCore/layout/blockformatting/BlockFormattingContextQuirks.cpp
>index f3ebd2d7cf1..a107974d07a 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContextQuirks.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContextQuirks.cpp
>@@ -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.
>diff --git a/Source/WebCore/layout/inlineformatting/InlineFormattingContext.cpp b/Source/WebCore/layout/inlineformatting/InlineFormattingContext.cpp
>index e0aaf59defd..0c3f5c8d77a 100644
>--- a/Source/WebCore/layout/inlineformatting/InlineFormattingContext.cpp
>+++ b/Source/WebCore/layout/inlineformatting/InlineFormattingContext.cpp
>@@ -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

Flags:

koivisto: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193183: 358469