Attachment 360342 Details for Bug 193896 – Patch

[patch] Patch

bug-193896-20190128080944.patch (text/plain), 15.82 KB, created by zalan on 2019-01-28 08:09:55 PST

(hide)

Description:

Filename:

MIME Type:

Creator: zalan

Created: 2019-01-28 08:09:55 PST

Size: 15.82 KB

patch

obsolete

>Subversion Revision: 240582
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index a07bfdf571ecb965440eb62e5b2faf65b9b5123a..d650ed7859cbfc56c4f7a4abd95be95bba53ca3f 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,54 @@
>+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 NOBODY (OOPS!).
>+
>+        This patch implements quirk margin value collpasing. 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 collpased 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
>diff --git a/Source/WebCore/layout/MarginTypes.h b/Source/WebCore/layout/MarginTypes.h
>index 5254f44f44fb80f9ab06f364473526b309c8e5ac..e42492bac94adf4f041ef0905393c3326ec9c98f 100644
>--- a/Source/WebCore/layout/MarginTypes.h
>+++ b/Source/WebCore/layout/MarginTypes.h
>@@ -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;
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContext.h b/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>index 572ca265d9e1016eb646554a0535b019a8808742..3e99c42b98bc212e2b26c782dcd63dc91666a207 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>@@ -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&);
>     };
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContextQuirks.cpp b/Source/WebCore/layout/blockformatting/BlockFormattingContextQuirks.cpp
>index a125b6482b0664e74ee5bffc6af452664483c623..c29f4913f54202e44aded3e9055e03638d1f8c3e 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContextQuirks.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContextQuirks.cpp
>@@ -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);
> }
> 
> }
>diff --git a/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp b/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
>index 4424fb298eaaeaa26ef8a3271082e865d62757aa..e18cb292031515c4915c2dbb7401b8512416752f 100644
>--- a/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
>@@ -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)
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 7f994b58c3f8462ae6237f1f0b977cdab2a01608..2c0c725866e41a3aeb890e3a880a89d08ece238c 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        * 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
>diff --git a/Tools/LayoutReloaded/misc/LFC-passing-tests.txt b/Tools/LayoutReloaded/misc/LFC-passing-tests.txt
>index ebf5693ce18dc747d72972c271a01a9c19e661e3..135934ee8235b6c1a278dd0e176a80a932373954 100644
>--- a/Tools/LayoutReloaded/misc/LFC-passing-tests.txt
>+++ b/Tools/LayoutReloaded/misc/LFC-passing-tests.txt
>@@ -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

Flags:

koivisto: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193896: 360342