[LFC][MarginCollapsing][Quirks] Quirk margin values get propagated through margin...
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 28 Jan 2019 16:20:17 +0000 (16:20 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 28 Jan 2019 16:20:17 +0000 (16:20 +0000)
https://bugs.webkit.org/show_bug.cgi?id=193896

Reviewed by Antti Koivisto.

Source/WebCore:

This patch implements quirk margin value collapsing. There are a few "quirk" rules when it comes to margin collapsing.

1. Collapsed quirk margin values are ignored on quirk containers

    <body>
      <p> p elements have 1em vertical (top/bottom) quirk margin
    </body>

    In quirk mode, <p> and <body> (quirk container) collapses their vertical margins but <p>'s quirk values(1qem -> 16px) are ignored.
    Used vertical margin values on the <body> are top: 8px bottom: 8px.

2. Quirk margin values are turned into non-quirk values when collapsed with non-zero, non-quirk margins.

    <body>
      <div style="margin-top: 1px">
        <p> p elements have 1em vertical (top/bottom) quirk margin
      </div>
    </body>

    When <p>'s vertical margin collapses with the parent <div>,
    - the collapsed before value becomes 16px (max(1qem, 1px)) and this collapsed value is now considered as a non-quirk value
    - the collapsed after value stays 1qem quirk value.

    When <div> collapses with <body>
    - the collapsed before value becomes 16px (max(16px, 8px))
    - the <div>'s quirk after value gets ignored and the collapsed after value stays 8px.
    Used vertical margin values on the <body> are top: 16px (1em) bottom: 8px.

* layout/MarginTypes.h:
(WebCore::Layout::PositiveAndNegativeVerticalMargin::Values::isNonZero const):
* layout/blockformatting/BlockFormattingContext.h:
* layout/blockformatting/BlockFormattingContextQuirks.cpp:
(WebCore::Layout::BlockFormattingContext::Quirks::shouldIgnoreCollapsedQuirkMargin):
(WebCore::Layout::hasMarginBeforeQuirkValue): Deleted.
(WebCore::Layout::BlockFormattingContext::Quirks::shouldIgnoreMarginBefore): Deleted.
(WebCore::Layout::BlockFormattingContext::Quirks::shouldIgnoreMarginAfter): Deleted.
* layout/blockformatting/BlockMarginCollapse.cpp:
(WebCore::Layout::BlockFormattingContext::MarginCollapse::marginBeforeCollapsesWithPreviousSiblingMarginAfter):
(WebCore::Layout::BlockFormattingContext::MarginCollapse::marginBeforeCollapsesWithFirstInFlowChildMarginBefore):
(WebCore::Layout::BlockFormattingContext::MarginCollapse::marginAfterCollapsesWithLastInFlowChildMarginAfter):
(WebCore::Layout::computedPositiveAndNegativeMargin):
(WebCore::Layout::BlockFormattingContext::MarginCollapse::positiveNegativeMarginBefore):
(WebCore::Layout::BlockFormattingContext::MarginCollapse::positiveNegativeMarginAfter):

Tools:

* LayoutReloaded/misc/LFC-passing-tests.txt:

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

Source/WebCore/ChangeLog
Source/WebCore/layout/MarginTypes.h
Source/WebCore/layout/blockformatting/BlockFormattingContext.h
Source/WebCore/layout/blockformatting/BlockFormattingContextQuirks.cpp
Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
Tools/ChangeLog
Tools/LayoutReloaded/misc/LFC-passing-tests.txt

index a07bfdf..0fa3a3b 100644 (file)
@@ -1,5 +1,56 @@
 2019-01-28  Zalan Bujtas  <zalan@apple.com>
 
+        [LFC][MarginCollapsing][Quirks] Quirk margin values get propagated through margin collapsing
+        https://bugs.webkit.org/show_bug.cgi?id=193896
+
+        Reviewed by Antti Koivisto.
+
+        This patch implements quirk margin value collapsing. There are a few "quirk" rules when it comes to margin collapsing.
+
+        1. Collapsed quirk margin values are ignored on quirk containers
+
+            <body>
+              <p> p elements have 1em vertical (top/bottom) quirk margin
+            </body>
+
+            In quirk mode, <p> and <body> (quirk container) collapses their vertical margins but <p>'s quirk values(1qem -> 16px) are ignored.
+            Used vertical margin values on the <body> are top: 8px bottom: 8px.
+
+        2. Quirk margin values are turned into non-quirk values when collapsed with non-zero, non-quirk margins.
+
+            <body>
+              <div style="margin-top: 1px">
+                <p> p elements have 1em vertical (top/bottom) quirk margin
+              </div>
+            </body>
+
+            When <p>'s vertical margin collapses with the parent <div>,
+            - the collapsed before value becomes 16px (max(1qem, 1px)) and this collapsed value is now considered as a non-quirk value
+            - the collapsed after value stays 1qem quirk value.
+
+            When <div> collapses with <body>
+            - the collapsed before value becomes 16px (max(16px, 8px))
+            - the <div>'s quirk after value gets ignored and the collapsed after value stays 8px.
+            Used vertical margin values on the <body> are top: 16px (1em) bottom: 8px.
+
+        * layout/MarginTypes.h:
+        (WebCore::Layout::PositiveAndNegativeVerticalMargin::Values::isNonZero const):
+        * layout/blockformatting/BlockFormattingContext.h:
+        * layout/blockformatting/BlockFormattingContextQuirks.cpp:
+        (WebCore::Layout::BlockFormattingContext::Quirks::shouldIgnoreCollapsedQuirkMargin):
+        (WebCore::Layout::hasMarginBeforeQuirkValue): Deleted.
+        (WebCore::Layout::BlockFormattingContext::Quirks::shouldIgnoreMarginBefore): Deleted.
+        (WebCore::Layout::BlockFormattingContext::Quirks::shouldIgnoreMarginAfter): Deleted.
+        * layout/blockformatting/BlockMarginCollapse.cpp:
+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::marginBeforeCollapsesWithPreviousSiblingMarginAfter):
+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::marginBeforeCollapsesWithFirstInFlowChildMarginBefore):
+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::marginAfterCollapsesWithLastInFlowChildMarginAfter):
+        (WebCore::Layout::computedPositiveAndNegativeMargin):
+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::positiveNegativeMarginBefore):
+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::positiveNegativeMarginAfter):
+
+2019-01-28  Zalan Bujtas  <zalan@apple.com>
+
         [LFC][BFC] Remove redundant vertical positioning in BlockFormattingContext::computeFloatingPosition
         https://bugs.webkit.org/show_bug.cgi?id=193872
 
index 5254f44..e42492b 100644 (file)
@@ -84,8 +84,11 @@ struct EstimatedMarginBefore {
 
 struct PositiveAndNegativeVerticalMargin {
     struct Values {
+        bool isNonZero() const { return positive.valueOr(0) || negative.valueOr(0); }
+
         Optional<LayoutUnit> positive;
         Optional<LayoutUnit> negative;
+        bool isQuirk { false };
     };
     Values before;
     Values after;
index 572ca26..3e99c42 100644 (file)
@@ -123,6 +123,7 @@ private:
         static bool needsStretching(const LayoutState&, const Box&);
         static HeightAndMargin stretchedInFlowHeight(const LayoutState&, const Box&, HeightAndMargin);
 
+        static bool shouldIgnoreCollapsedQuirkMargin(const LayoutState&, const Box&);
         static bool shouldIgnoreMarginBefore(const LayoutState&, const Box&);
         static bool shouldIgnoreMarginAfter(const LayoutState&, const Box&);
     };
index a125b64..c29f491 100644 (file)
@@ -48,11 +48,6 @@ static bool isQuirkContainer(const Box& layoutBox)
     return layoutBox.isBodyBox() || layoutBox.isDocumentBox() || layoutBox.isTableCell();
 }
 
-static bool hasMarginBeforeQuirkValue(const Box& layoutBox)
-{
-    return layoutBox.style().hasMarginBeforeQuirk();
-}
-
 bool BlockFormattingContext::Quirks::needsStretching(const LayoutState& layoutState, const Box& layoutBox)
 {
     // In quirks mode, body stretches to html and html to the initial containing block (height: auto only).
@@ -104,26 +99,9 @@ HeightAndMargin BlockFormattingContext::Quirks::stretchedInFlowHeight(const Layo
     return heightAndMargin;
 }
 
-bool BlockFormattingContext::Quirks::shouldIgnoreMarginBefore(const LayoutState& layoutState, const Box& layoutBox)
-{
-    if (!layoutBox.parent())
-        return false;
-
-    return layoutState.inQuirksMode() && isQuirkContainer(*layoutBox.parent()) && hasMarginBeforeQuirkValue(layoutBox);
-}
-
-bool BlockFormattingContext::Quirks::shouldIgnoreMarginAfter(const LayoutState& layoutState, const Box& layoutBox)
+bool BlockFormattingContext::Quirks::shouldIgnoreCollapsedQuirkMargin(const LayoutState& layoutState, const Box& layoutBox)
 {
-    // Ignore maring after when the ignored margin before collapses through.
-    if (!shouldIgnoreMarginBefore(layoutState, layoutBox))
-        return false;
-
-    ASSERT(layoutBox.parent());
-    auto& parent = *layoutBox.parent();
-    if (parent.firstInFlowOrFloatingChild() != &layoutBox || parent.firstInFlowOrFloatingChild() != parent.lastInFlowOrFloatingChild())
-        return false;
-
-    return MarginCollapse::marginsCollapseThrough(layoutState, layoutBox);
+    return layoutState.inQuirksMode() && isQuirkContainer(layoutBox);
 }
 
 }
index 4424fb2..e18cb29 100644 (file)
@@ -155,10 +155,6 @@ bool BlockFormattingContext::MarginCollapse::marginBeforeCollapsesWithPreviousSi
         return false;
 
     auto& previousInFlowSibling = *layoutBox.previousInFlowSibling();
-
-    if (Quirks::shouldIgnoreMarginAfter(layoutState, previousInFlowSibling))
-        return false;
-
     // Margins between a floated box and any other box do not collapse.
     if (layoutBox.isFloatingPositioned() || previousInFlowSibling.isFloatingPositioned())
         return false;
@@ -203,9 +199,6 @@ bool BlockFormattingContext::MarginCollapse::marginBeforeCollapsesWithFirstInFlo
         return false;
 
     auto& firstInFlowChild = *downcast<Container>(layoutBox).firstInFlowChild();
-    if (Quirks::shouldIgnoreMarginBefore(layoutState, firstInFlowChild))
-        return false;
-
     if (!firstInFlowChild.isBlockLevelBox())
         return false;
 
@@ -326,9 +319,6 @@ bool BlockFormattingContext::MarginCollapse::marginAfterCollapsesWithLastInFlowC
         return false;
 
     auto& lastInFlowChild = *downcast<Container>(layoutBox).lastInFlowChild();
-    if (Quirks::shouldIgnoreMarginAfter(layoutState, lastInFlowChild))
-        return false;
-
     if (!lastInFlowChild.isBlockLevelBox())
         return false;
 
@@ -452,19 +442,14 @@ static PositiveAndNegativeVerticalMargin::Values computedPositiveAndNegativeMarg
     else
         computedValues.negative = a.negative ? a.negative : b.negative;
 
-    return computedValues;
-}
+    if (a.isNonZero() && b.isNonZero())
+        computedValues.isQuirk = a.isQuirk && b.isQuirk;
+    else if (a.isNonZero())
+        computedValues.isQuirk = a.isQuirk;
+    else
+        computedValues.isQuirk = b.isQuirk;
 
-static PositiveAndNegativeVerticalMargin::Values computedPositiveAndNegativeMargin(PositiveAndNegativeVerticalMargin::Values a, Optional<LayoutUnit> marginValue)
-{
-    if (!marginValue || !marginValue.value())
-        return a;
-    if (*marginValue > 0)
-        return computedPositiveAndNegativeMargin(a, { marginValue, { } });
-    if (*marginValue < 0)
-        return computedPositiveAndNegativeMargin(a, { { }, marginValue });
-    ASSERT_NOT_REACHED();
-    return { };
+    return computedValues;
 }
 
 static Optional<LayoutUnit> marginValue(PositiveAndNegativeVerticalMargin::Values marginValues)
@@ -560,7 +545,16 @@ PositiveAndNegativeVerticalMargin::Values BlockFormattingContext::MarginCollapse
     // 2. Gather positive and negative margin values from previous inflow sibling if margins are adjoining.
     // 3. Compute min/max positive and negative collapsed margin values using non-collpased computed margin before.
     auto collapsedMarginBefore = computedPositiveAndNegativeMargin(firstChildCollapsedMarginBefore(), previouSiblingCollapsedMarginAfter());
-    return computedPositiveAndNegativeMargin(collapsedMarginBefore, nonCollapsedValues.before);
+    if (collapsedMarginBefore.isQuirk && Quirks::shouldIgnoreCollapsedQuirkMargin(layoutState, layoutBox))
+        collapsedMarginBefore = { };
+
+    PositiveAndNegativeVerticalMargin::Values nonCollapsedBefore;
+    if (nonCollapsedValues.before > 0)
+        nonCollapsedBefore = { nonCollapsedValues.before, { }, layoutBox.style().hasMarginBeforeQuirk() };
+    else if (nonCollapsedValues.before < 0)
+        nonCollapsedBefore = { { }, nonCollapsedValues.before, layoutBox.style().hasMarginBeforeQuirk() };
+
+    return computedPositiveAndNegativeMargin(collapsedMarginBefore, nonCollapsedBefore);
 }
 
 PositiveAndNegativeVerticalMargin::Values BlockFormattingContext::MarginCollapse::positiveNegativeMarginAfter(const LayoutState& layoutState, const Box& layoutBox, const UsedVerticalMargin::NonCollapsedValues& nonCollapsedValues)
@@ -573,7 +567,13 @@ PositiveAndNegativeVerticalMargin::Values BlockFormattingContext::MarginCollapse
 
     // We don't know yet the margin before value of the next sibling. Let's just pretend it does not have one and 
     // update it later when we compute the next sibling's margin before. See updateCollapsedMarginAfter.
-    return computedPositiveAndNegativeMargin(lastChildCollapsedMarginAfter(), nonCollapsedValues.after);
+    PositiveAndNegativeVerticalMargin::Values nonCollapsedAfter;
+    if (nonCollapsedValues.after > 0)
+        nonCollapsedAfter = { nonCollapsedValues.after, { }, layoutBox.style().hasMarginAfterQuirk() };
+    else if (nonCollapsedValues.after < 0)
+        nonCollapsedAfter = { { }, nonCollapsedValues.after, layoutBox.style().hasMarginAfterQuirk() };
+
+    return computedPositiveAndNegativeMargin(lastChildCollapsedMarginAfter(), nonCollapsedAfter);
 }
 
 EstimatedMarginBefore BlockFormattingContext::MarginCollapse::estimatedMarginBefore(const LayoutState& layoutState, const Box& layoutBox)
index 7f994b5..e40de57 100644 (file)
@@ -1,3 +1,12 @@
+2019-01-28  Zalan Bujtas  <zalan@apple.com>
+
+        [LFC][MarginCollapsing][Quirks] Quirk margin values get propagated through margin collapsing
+        https://bugs.webkit.org/show_bug.cgi?id=193896
+
+        Reviewed by Antti Koivisto.
+
+        * LayoutReloaded/misc/LFC-passing-tests.txt:
+
 2018-12-15  Darin Adler  <darin@apple.com>
 
         Replace many uses of String::format with more type-safe alternatives
index ebf5693..135934e 100644 (file)
@@ -101,12 +101,15 @@ fast/block/basic/percent-height-inside-anonymous-block.html
 fast/block/basic/quirk-mode-percent-height.html
 fast/block/basic/quirk-percent-height-grandchild.html
 fast/block/float/001.html
+fast/block/float/003.html
 fast/block/float/007.html
 fast/block/float/008.html
 fast/block/float/009.html
 fast/block/float/013.html
 fast/block/float/019.html
+fast/block/float/023.html
 fast/block/float/030.html
+fast/block/float/033.html
 fast/block/float/negative-margin-clear.html
 fast/block/float/overhanging-after-height-decrease-offsets.html
 fast/block/float/overhanging-after-height-decrease.html
@@ -125,8 +128,16 @@ fast/block/float/float-in-descendant-formatting-context.html
 fast/block/float/floats-with-negative-horizontal-margin.html
 fast/block/float/float-forced-below-other-floats.html
 fast/block/float/float-first-child-and-clear-sibling.html
+fast/block/float/intruding-float-sibling-with-margin.html
+fast/block/float/positioned-float-crash.html
+fast/block/float/previous-sibling-abspos-001.html
+fast/block/float/previous-sibling-abspos-002.html
+fast/block/float/previous-sibling-float-001.html
+fast/block/float/previous-sibling-float-002.html
 fast/block/margin-collapse/002.html
 fast/block/margin-collapse/003.html
+# webkit bug negative margin, document renderer size is invalid.
+#fast/block/margin-collapse/004.html
 fast/block/margin-collapse/026.html
 fast/block/margin-collapse/027.html
 fast/block/margin-collapse/028.html
@@ -134,7 +145,9 @@ fast/block/margin-collapse/029.html
 fast/block/margin-collapse/035.html
 fast/block/margin-collapse/039.html
 fast/block/margin-collapse/040.html
+fast/block/margin-collapse/043.html
 fast/block/margin-collapse/044.html
+fast/block/margin-collapse/063.html
 fast/block/margin-collapse/collapsed-through-child-simple.html
 fast/block/positioning/003.html
 fast/block/positioning/004.html
@@ -174,11 +187,16 @@ fast/block/positioning/043.html
 fast/block/positioning/044.html
 fast/block/positioning/045.html
 fast/block/positioning/046.html
+fast/block/positioning/048.html
 fast/block/positioning/049.html
+fast/block/positioning/050.html
 fast/block/positioning/052.html
 fast/block/positioning/054.html
 fast/block/positioning/060.html
+fast/block/positioning/abs-inside-inline-rel.html
+fast/block/positioning/absolute-length-of-neg-666666.html
 fast/block/positioning/absolute-positioning-no-scrollbar.html
+fast/block/positioning/border-change-relayout-test.html
 fast/block/positioning/change-containing-block-for-absolute-positioned.html
 fast/block/positioning/change-containing-block-for-fixed-positioned.html
 fast/block/positioning/complex-positioned-movement.html
@@ -192,6 +210,7 @@ fast/block/positioning/negative-rel-position.html
 fast/block/positioning/abs-inside-inline-rel.html
 fast/block/positioning/pref-width-change.html
 fast/block/positioning/relative-overflow-replaced-float.html
+fast/block/positioning/relative-overflow-replaced-float.html
 fast/block/positioning/static-inline-position-dynamic.html
 fast/block/inside-inlines/basic-float-intrusion.html
 fast/block/inside-inlines/crash-on-first-line-change.html