Attachment 358895 Details for Bug 193346 – Patch

[patch] Patch

bug-193346-20190111074517.patch (text/plain), 18.41 KB, created by zalan on 2019-01-11 07:45:29 PST

(hide)

Description:

Filename:

MIME Type:

Creator: zalan

Created: 2019-01-11 07:45:29 PST

Size: 18.41 KB

patch

obsolete

>Subversion Revision: 239862
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 76b62769033d12153a576529ab23321418a49b44..7522bd076351ee53eaae6109db0baf27614f2557 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,30 @@
>+2019-01-10  Zalan Bujtas  <zalan@apple.com>
>+
>+        [LFC][BFC][MarginCollapsing] Adjust vertical position when box margin collapses through.
>+        https://bugs.webkit.org/show_bug.cgi?id=193346
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        If the top and bottom margins of a box are adjoining, then it is possible for margins to collapse through it.
>+        In this case, the position of the element depends on its relationship with the other elements whose margins are being collapsed.
>+
>+        1. If the element's margins are collapsed with its parent's top margin, the top border edge of the box is defined to be the same as the parent's.
>+        2. Otherwise, either the element's parent is not taking part in the margin collapsing, or only the parent's bottom margin is involved.
>+           The position of the element's top border edge is the same as it would have been if the element had a non-zero bottom border.
>+        
>+        Test: fast/block/block-only/collapsed-through-with-parent.html
>+
>+        * layout/MarginTypes.h:
>+        (WebCore::Layout::EstimatedMarginBefore::usedValue const):
>+        * layout/blockformatting/BlockFormattingContext.cpp:
>+        (WebCore::Layout::BlockFormattingContext::computeEstimatedMarginBefore const):
>+        (WebCore::Layout::BlockFormattingContext::computeHeightAndMargin const):
>+        (WebCore::Layout::BlockFormattingContext::adjustedVerticalPositionAfterMarginCollapsing const):
>+        * layout/blockformatting/BlockFormattingContext.h:
>+        * layout/blockformatting/BlockMarginCollapse.cpp:
>+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::estimatedMarginBefore):
>+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::marginBeforeIgnoringCollapsingThrough):
>+
> 2019-01-10  Myles C. Maxfield  <mmaxfield@apple.com>
> 
>         [WHLSL] Include the standard library
>diff --git a/Source/WebCore/layout/MarginTypes.h b/Source/WebCore/layout/MarginTypes.h
>index ac4aa8bc5eedb9e571e1adae0ab3c6cdfbc1075d..67b68ab074ac6f8b29a405083e8a079467869249 100644
>--- a/Source/WebCore/layout/MarginTypes.h
>+++ b/Source/WebCore/layout/MarginTypes.h
>@@ -75,7 +75,10 @@ struct UsedHorizontalMargin {
> };
> 
> struct EstimatedMarginBefore {
>-    LayoutUnit usedValue;
>+    LayoutUnit usedValue() const { return collapsedValue.valueOr(nonCollapsedValue); }
>+    LayoutUnit nonCollapsedValue;
>+    Optional<LayoutUnit> collapsedValue;
>+    bool isCollapsedThrough { false };
> };
> 
> struct PositiveAndNegativeVerticalMargin {
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>index ecd8f87437b4804e9e407dbfdde1b19bc16e5d8f..f36414d8ebc8b329ec2c34f9896d2870bfd21896 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>@@ -191,7 +191,10 @@ void BlockFormattingContext::computeEstimatedMarginBefore(const Box& layoutBox)
>     blockFormattingState().setHasEstimatedMarginBefore(layoutBox);
> 
>     auto& displayBox = layoutState.displayBoxForLayoutBox(layoutBox);
>-    displayBox.setTop(adjustedVerticalPositionAfterMarginCollapsing(layoutBox, estimatedMarginBefore.usedValue));
>+    auto nonCollapsedValues = UsedVerticalMargin::NonCollapsedValues { estimatedMarginBefore.nonCollapsedValue, { } };
>+    auto collapsedValues = UsedVerticalMargin::CollapsedValues { estimatedMarginBefore.collapsedValue, { }, estimatedMarginBefore.isCollapsedThrough };
>+    auto verticalMargin = UsedVerticalMargin { nonCollapsedValues, collapsedValues };
>+    displayBox.setTop(adjustedVerticalPositionAfterMarginCollapsing(layoutBox, verticalMargin));
> #if !ASSERT_DISABLED
>     displayBox.setEstimatedMarginBefore(estimatedMarginBefore);
> #endif
>@@ -372,21 +375,33 @@ void BlockFormattingContext::computeHeightAndMargin(const Box& layoutBox) const
>         }
>     }
> 
>+    // 1. Compute collapsed margins.
>+    // 2. Adjust vertical position using the collaped values
>+    // 3. Adjust previous in-flow sibling margin after using this margin.
>     auto collapsedMargin = MarginCollapse::collapsedVerticalValues(layoutState, layoutBox, heightAndMargin.nonCollapsedMargin);
>     auto verticalMargin = UsedVerticalMargin { heightAndMargin.nonCollapsedMargin, collapsedMargin };
>     auto& displayBox = layoutState.displayBoxForLayoutBox(layoutBox);
>+    
>+    // Out of flow boxes don't need vertical adjustment after margin collapsing.
>+    if (layoutBox.isOutOfFlowPositioned()) {
>+        displayBox.setContentBoxHeight(heightAndMargin.height);
>+        displayBox.setVerticalMargin(verticalMargin);
>+        return;
>+    }
>+
>     auto& formattingState = this->blockFormattingState();
>     if (formattingState.hasEstimatedMarginBefore(layoutBox)) {
>         formattingState.clearHasEstimatedMarginBefore(layoutBox);
>-        ASSERT(displayBox.estimatedMarginBefore()->usedValue == verticalMargin.before());
>+        ASSERT(displayBox.estimatedMarginBefore()->usedValue() == verticalMargin.before());
>     } else
>-        displayBox.setTop(adjustedVerticalPositionAfterMarginCollapsing(layoutBox, verticalMargin.before()));
>+        displayBox.setTop(adjustedVerticalPositionAfterMarginCollapsing(layoutBox, verticalMargin));
> 
>-    displayBox.setContentBoxHeight(heightAndMargin.height);
>-    displayBox.setVerticalMargin(verticalMargin);
>     // Adjust the previous sibling's margin bottom now that this box's vertical margin is computed.
>     if (MarginCollapse::marginBeforeCollapsesWithPreviousSiblingMarginAfter(layoutState, layoutBox))
>         MarginCollapse::updateCollapsedMarginAfter(layoutState, *layoutBox.previousInFlowSibling(), verticalMargin);
>+
>+    displayBox.setContentBoxHeight(heightAndMargin.height);
>+    displayBox.setVerticalMargin(verticalMargin);
> }
> 
> FormattingContext::InstrinsicWidthConstraints BlockFormattingContext::instrinsicWidthConstraints() const
>@@ -453,8 +468,9 @@ FormattingContext::InstrinsicWidthConstraints BlockFormattingContext::instrinsic
>     return instrinsicWidthConstraints;
> }
> 
>-LayoutUnit BlockFormattingContext::adjustedVerticalPositionAfterMarginCollapsing(const Box& layoutBox, LayoutUnit marginBefore) const
>+LayoutUnit BlockFormattingContext::adjustedVerticalPositionAfterMarginCollapsing(const Box& layoutBox, const UsedVerticalMargin& verticalMargin) const
> {
>+    ASSERT(!layoutBox.isOutOfFlowPositioned());
>     // Now that we've computed the final margin before, let's shift the box's vertical position.
>     // 1. Check if the margin before collapses with the previous box's margin after. if not -> return previous box's bottom inlcuding margin after + marginBefore
>     // 2. Check if the previous box's margins collapse through. If not -> return previous box' bottom excluding margin after + marginBefore (they are supposed to be equal)
>@@ -467,26 +483,45 @@ LayoutUnit BlockFormattingContext::adjustedVerticalPositionAfterMarginCollapsing
>         auto& previousInFlowSibling = *currentLayoutBox->previousInFlowSibling();
>         if (!MarginCollapse::marginBeforeCollapsesWithPreviousSiblingMarginAfter(layoutState, *currentLayoutBox)) {
>             auto& previousDisplayBox = layoutState.displayBoxForLayoutBox(previousInFlowSibling);
>-            return previousDisplayBox.rectWithMargin().bottom() + marginBefore;
>+            return previousDisplayBox.rectWithMargin().bottom() + verticalMargin.before();
>         }
> 
>         if (!MarginCollapse::marginsCollapseThrough(layoutState, previousInFlowSibling)) {
>             auto& previousDisplayBox = layoutState.displayBoxForLayoutBox(previousInFlowSibling);
>-            return previousDisplayBox.bottom() + marginBefore;
>+            return previousDisplayBox.bottom() + verticalMargin.before();
>         }
>         currentLayoutBox = &previousInFlowSibling;
>     }
> 
>     auto& containingBlock = *layoutBox.containingBlock();
>     auto containingBlockContentBoxTop = layoutState.displayBoxForLayoutBox(containingBlock).contentBoxTop();
>-    // If we reached the parent through collapsed sibling(s), this box (may) collapses the before margin indirectly with the parent.  
>-    auto* collapsingCandidate = layoutBox.previousInFlowSibling() ? containingBlock.firstInFlowChild() : &layoutBox;
>-    auto marginBeforeCollapsesWithParent = MarginCollapse::marginBeforeCollapsesWithParentMarginBefore(layoutState, *collapsingCandidate) 
>-        || MarginCollapse::marginBeforeCollapsesWithParentMarginAfter(layoutState, *collapsingCandidate);
>-    if (marginBeforeCollapsesWithParent)
>+    // Adjust vertical position depending whether this box directly or indirectly adjoins with its parent.
>+    auto directlyAdjoinsParent = !layoutBox.previousInFlowSibling(); 
>+    if (directlyAdjoinsParent) {
>+        // If the top and bottom margins of a box are adjoining, then it is possible for margins to collapse through it.
>+        // In this case, the position of the element depends on its relationship with the other elements whose margins are being collapsed.
>+        if (verticalMargin.collapsedValues().isCollapsedThrough) {
>+            // If the element's margins are collapsed with its parent's top margin, the top border edge of the box is defined to be the same as the parent's.
>+            if (MarginCollapse::marginBeforeCollapsesWithParentMarginBefore(layoutState, layoutBox))
>+                return containingBlockContentBoxTop;
>+            // Otherwise, either the element's parent is not taking part in the margin collapsing, or only the parent's bottom margin is involved.
>+            // The position of the element's top border edge is the same as it would have been if the element had a non-zero bottom border.
>+            auto beforeMarginWithBottomBorder = MarginCollapse::marginBeforeIgnoringCollapsingThrough(layoutState, layoutBox, verticalMargin.nonCollapsedValues());
>+            return containingBlockContentBoxTop + beforeMarginWithBottomBorder;
>+        }
>+        // Non-collapsed through box vertical position depending whether the margin collapses.
>+        if (MarginCollapse::marginBeforeCollapsesWithParentMarginBefore(layoutState, layoutBox))
>+            return containingBlockContentBoxTop;
>+
>+        return containingBlockContentBoxTop + verticalMargin.before();
>+    }
>+    // At this point this box indirectly (via collapsed through previous in-flow siblings) adjoins the parent. Let's check if it margin collapses with the parent.
>+    ASSERT(containingBlock.firstInFlowChild());
>+    ASSERT(containingBlock.firstInFlowChild() != &layoutBox);
>+    if (MarginCollapse::marginBeforeCollapsesWithParentMarginBefore(layoutState, *containingBlock.firstInFlowChild()))
>         return containingBlockContentBoxTop;
> 
>-    return containingBlockContentBoxTop + marginBefore;
>+    return containingBlockContentBoxTop + verticalMargin.before();
> }
> 
> }
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContext.h b/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>index 500d0649c25e237ec00de72fbdeb92b05e758ed5..a2cfdad2017fae77103e8081c311e6f556608e10 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>@@ -69,7 +69,7 @@ private:
>     void precomputeVerticalPositionForFormattingRootIfNeeded(const Box&) const;
> 
>     InstrinsicWidthConstraints instrinsicWidthConstraints() const override;
>-    LayoutUnit adjustedVerticalPositionAfterMarginCollapsing(const Box&, LayoutUnit marginBefore) const;
>+    LayoutUnit adjustedVerticalPositionAfterMarginCollapsing(const Box&, const UsedVerticalMargin&) const;
> 
>     // This class implements positioning and sizing for boxes participating in a block formatting context.
>     class Geometry : public FormattingContext::Geometry {
>@@ -95,6 +95,7 @@ private:
>         static UsedVerticalMargin::CollapsedValues collapsedVerticalValues(const LayoutState&, const Box&, const UsedVerticalMargin::NonCollapsedValues&);
> 
>         static EstimatedMarginBefore estimatedMarginBefore(const LayoutState&, const Box&);
>+        static LayoutUnit marginBeforeIgnoringCollapsingThrough(const LayoutState&, const Box&, const UsedVerticalMargin::NonCollapsedValues&);
>         static void updateCollapsedMarginAfter(const LayoutState&, const Box&, const UsedVerticalMargin& nextSiblingVerticalMargin);
> 
>         static bool marginBeforeCollapsesWithParentMarginBefore(const LayoutState&, const Box&);
>diff --git a/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp b/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
>index f9ab127f9aa48c20d0a7f4fa1e7b4c445ad44c1f..f10ba4641dfe239a00cef9f8fa248315ec7489ad 100644
>--- a/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
>@@ -593,13 +593,21 @@ EstimatedMarginBefore BlockFormattingContext::MarginCollapse::estimatedMarginBef
>     ASSERT(!layoutBox.establishesBlockFormattingContext());
>     
>     auto computedVerticalMargin = Geometry::computedVerticalMargin(layoutState, layoutBox);
>-    auto nonCollapsedMargin = UsedVerticalMargin::NonCollapsedValues { computedVerticalMargin.before.valueOr(0), computedVerticalMargin.after.valueOr(0) }; 
>+    auto nonCollapsedMargin = UsedVerticalMargin::NonCollapsedValues { computedVerticalMargin.before.valueOr(0), computedVerticalMargin.after.valueOr(0) };
>+    auto marginsCollapseThrough = MarginCollapse::marginsCollapseThrough(layoutState, layoutBox);
>+    auto positiveNegativeMarginBefore = MarginCollapse::positiveNegativeMarginBefore(layoutState, layoutBox, nonCollapsedMargin);
> 
>-    if (!marginsCollapseThrough(layoutState, layoutBox))
>-        return { marginValue(positiveNegativeMarginBefore(layoutState, layoutBox, nonCollapsedMargin)).valueOr(0) };
>-    
>-    return { marginValue(computedPositiveAndNegativeMargin(positiveNegativeMarginBefore(layoutState, layoutBox, nonCollapsedMargin),
>-        positiveNegativeMarginAfter(layoutState, layoutBox, nonCollapsedMargin))).valueOr(0) };
>+    auto collapsedMarginBefore = marginValue(!marginsCollapseThrough ? positiveNegativeMarginBefore
>+        : computedPositiveAndNegativeMargin(positiveNegativeMarginBefore, positiveNegativeMarginAfter(layoutState, layoutBox, nonCollapsedMargin)));
>+
>+    return { nonCollapsedMargin.before, collapsedMarginBefore, marginsCollapseThrough };
>+}
>+
>+LayoutUnit BlockFormattingContext::MarginCollapse::marginBeforeIgnoringCollapsingThrough(const LayoutState& layoutState, const Box& layoutBox, const UsedVerticalMargin::NonCollapsedValues& nonCollapsedValues)
>+{
>+    ASSERT(!layoutBox.isAnonymous());
>+    ASSERT(layoutBox.isBlockLevelBox());
>+    return marginValue(positiveNegativeMarginBefore(layoutState, layoutBox, nonCollapsedValues)).valueOr(nonCollapsedValues.before); 
> }
> 
> UsedVerticalMargin::CollapsedValues BlockFormattingContext::MarginCollapse::collapsedVerticalValues(const LayoutState& layoutState, const Box& layoutBox, const UsedVerticalMargin::NonCollapsedValues& nonCollapsedValues)
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 2e3a51c702ebebdbb8a3a13739c60922f35c8c36..03e98ad0f255163e845cf8ef55957599842a1570 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,12 @@
>+2019-01-10  Zalan Bujtas  <zalan@apple.com>
>+
>+        [LFC][BFC][MarginCollapsing] Adjust vertical position when box margin collapses through.
>+        https://bugs.webkit.org/show_bug.cgi?id=193346
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * LayoutReloaded/misc/LFC-passing-tests.txt:
>+
> 2019-01-10  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, fix typo that breaks dashboard link.
>diff --git a/Tools/LayoutReloaded/misc/LFC-passing-tests.txt b/Tools/LayoutReloaded/misc/LFC-passing-tests.txt
>index e3c8518416d32bde445b4d2ab9ecd97016906d2b..d7d55a646428beb28fb214787e7b4ca4c12fe842 100644
>--- a/Tools/LayoutReloaded/misc/LFC-passing-tests.txt
>+++ b/Tools/LayoutReloaded/misc/LFC-passing-tests.txt
>@@ -64,6 +64,7 @@ fast/block/block-only/margin-propagation-simple-content-height.html
> fast/block/block-only/margin-sibling-collapse-propagated.html
> fast/block/block-only/margin-simple.html
> fast/block/block-only/collapsed-through-siblings.html
>+fast/block/block-only/collapsed-through-with-parent.html
> fast/block/block-only/min-max-height-percentage.html
> fast/block/block-only/negative-margin-simple.html
> fast/block/block-only/padding-nested.html
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index a6e591f84fdc65f7ad0699f850d637b0a1af4794..a79322d4297cd841ac4456cb554dd61287f211c5 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,13 @@
>+2019-01-10  Zalan Bujtas  <zalan@apple.com>
>+
>+        [LFC][BFC][MarginCollapsing] Adjust vertical position when box margin collapses through.
>+        https://bugs.webkit.org/show_bug.cgi?id=193346
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * fast/block/block-only/collapsed-through-with-parent-expected.txt: Added.
>+        * fast/block/block-only/collapsed-through-with-parent.html: Added.
>+
> 2019-01-11  Carlos Garcia Campos  <cgarcia@igalia.com>
> 
>         Unreviewed GTK gardening. Rebaseline several tests after r239822.
>diff --git a/LayoutTests/fast/block/block-only/collapsed-through-with-parent-expected.txt b/LayoutTests/fast/block/block-only/collapsed-through-with-parent-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..61050a58e40aa6b46ba6c4e2d71e39eeced08842
>--- /dev/null
>+++ b/LayoutTests/fast/block/block-only/collapsed-through-with-parent-expected.txt
>@@ -0,0 +1,8 @@
>+layer at (0,0) size 800x600
>+  RenderView at (0,0) size 800x600
>+layer at (0,0) size 800x600
>+  RenderBlock {HTML} at (0,0) size 800x600
>+    RenderBody {BODY} at (8,8) size 784x584
>+      RenderBlock {DIV} at (0,0) size 402x402 [border: (1px solid #FF0000)]
>+        RenderBlock {DIV} at (1,11) size 100x0
>+        RenderBlock {DIV} at (1,81) size 100x0
>diff --git a/LayoutTests/fast/block/block-only/collapsed-through-with-parent.html b/LayoutTests/fast/block/block-only/collapsed-through-with-parent.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..0a5d4d89133de7dd170266ff79b3af359463c06a
>--- /dev/null
>+++ b/LayoutTests/fast/block/block-only/collapsed-through-with-parent.html
>@@ -0,0 +1,4 @@
>+<div style="border: 1px solid red; width: 400px; height: 400px">
>+	<div style="outline: 2px solid red; width: 100px; margin-top: 10px; margin-bottom: 70px"></div>
>+	<div style="outline: 1px solid green; width: 100px; margin-top: 80px"></div>
>+</div>

Flags:

koivisto: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193346: 358876 | 358895