Attachment 356127 Details for Bug 192214 – Patch

[patch] Patch

bug-192214-20181129212740.patch (text/plain), 11.94 KB, created by zalan on 2018-11-29 21:27:43 PST

(hide)

Description:

Filename:

MIME Type:

Creator: zalan

Created: 2018-11-29 21:27:43 PST

Size: 11.94 KB

patch

obsolete

>Subversion Revision: 238701
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 365a507eb8f099ce5fdbcbb6b1c5cd9bfd3304f4..f8d5b221ccdec9a54bca1af9efe84bec445b848d 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,36 @@
>+2018-11-29  Zalan Bujtas  <zalan@apple.com>
>+
>+        [LFC][BFC][MarginCollapsing] Do not use computed display box values for border and padding
>+        https://bugs.webkit.org/show_bug.cgi?id=192214
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Border and padding values are not necessarily computed yet when we try to estimate the margin top value. Estimating margin top is required
>+        to be able to place floats (vertically) sooner than we would compute the final vertical position for a regular block box.
>+
>+        <body><div style="float: left"></div><div><div style="margin: 10px;"></div></div>
>+
>+        In the above example, to estimate a final vertical position of the floating box, we need to know whether the nested div's margin is collapsed
>+        all the way up to the body. However in order to find it out we need to check for borders and paddings (they stop margin collapsing).
>+        At the time when the floating box is being laied out, those <div> block boxes are still far down in the layout queue.
>+
>+        * layout/blockformatting/BlockFormattingContext.h:
>+        * layout/blockformatting/BlockFormattingContextGeometry.cpp:
>+        (WebCore::Layout::BlockFormattingContext::Geometry::inFlowNonReplacedHeightAndMargin):
>+        * layout/blockformatting/BlockMarginCollapse.cpp:
>+        (WebCore::Layout::hasBorder):
>+        (WebCore::Layout::hasPadding):
>+        (WebCore::Layout::hasBorderBefore):
>+        (WebCore::Layout::hasBorderAfter):
>+        (WebCore::Layout::hasPaddingBefore):
>+        (WebCore::Layout::hasPaddingAfter):
>+        (WebCore::Layout::BlockFormattingContext::Geometry::MarginCollapse::isMarginTopCollapsedWithParent):
>+        (WebCore::Layout::isMarginBottomCollapsedThrough):
>+        (WebCore::Layout::BlockFormattingContext::Geometry::MarginCollapse::marginTop):
>+        (WebCore::Layout::BlockFormattingContext::Geometry::MarginCollapse::marginBottom):
>+        (WebCore::Layout::BlockFormattingContext::Geometry::MarginCollapse::isMarginBottomCollapsedWithParent):
>+        (WebCore::Layout::BlockFormattingContext::Geometry::MarginCollapse::collapsedMarginBottomFromLastChild):
>+
> 2018-11-29  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC][BFC][Quirk] Body and html height stretching.
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContext.h b/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>index 38bdc400a0ce9c832b159ecc4e48ce6966ca0a76..e677e0174f0ed58c162fb6b713de096b11f0fd30 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>@@ -89,7 +89,7 @@ private:
>             static LayoutUnit marginTop(const LayoutState&, const Box&);
>             static LayoutUnit marginBottom(const LayoutState&, const Box&);
> 
>-            static bool isMarginBottomCollapsedWithParent(const LayoutState&, const Box&);
>+            static bool isMarginBottomCollapsedWithParent(const Box&);
>             static bool isMarginTopCollapsedWithParentMarginBottom(const Box&);
> 
>         private:
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContextGeometry.cpp b/Source/WebCore/layout/blockformatting/BlockFormattingContextGeometry.cpp
>index 86695dcf5651de92f72de477aa9edda35fa554cb..9e694ca60cad5d3baab228f0a24c134888cfd77c 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContextGeometry.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContextGeometry.cpp
>@@ -83,7 +83,7 @@ HeightAndMargin BlockFormattingContext::Geometry::inFlowNonReplacedHeightAndMarg
>         // 2. the bottom edge of the bottom (possibly collapsed) margin of its last in-flow child, if the child's bottom margin...
>         auto* lastInFlowChild = downcast<Container>(layoutBox).lastInFlowChild();
>         ASSERT(lastInFlowChild);
>-        if (!MarginCollapse::isMarginBottomCollapsedWithParent(layoutState, *lastInFlowChild)) {
>+        if (!MarginCollapse::isMarginBottomCollapsedWithParent(*lastInFlowChild)) {
>             auto& lastInFlowDisplayBox = layoutState.displayBoxForLayoutBox(*lastInFlowChild);
>             return { lastInFlowDisplayBox.bottom() + lastInFlowDisplayBox.marginBottom() - borderAndPaddingTop, nonCollapsedMargin, collapsedMargin };
>         }
>diff --git a/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp b/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
>index 98099b82223d487c41c57b2fe21d2aa412bb6a3d..eb793b3cd4f06dbee597da8b445b67903d407c21 100644
>--- a/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
>@@ -36,6 +36,39 @@
> namespace WebCore {
> namespace Layout {
> 
>+static bool hasBorder(const BorderValue& borderValue)
>+{
>+    if (borderValue.style() == BorderStyle::None || borderValue.style() == BorderStyle::Hidden)
>+        return false;
>+    return !!borderValue.width();
>+}
>+
>+static bool hasPadding(const Length& paddingValue)
>+{
>+    // FIXME: Check if percent value needs to be resolved.
>+    return !paddingValue.isZero();
>+}
>+
>+static bool hasBorderBefore(const Box& layoutBox)
>+{
>+    return hasBorder(layoutBox.style().borderBefore());
>+}
>+
>+static bool hasBorderAfter(const Box& layoutBox)
>+{
>+    return hasBorder(layoutBox.style().borderAfter());
>+}
>+
>+static bool hasPaddingBefore(const Box& layoutBox)
>+{
>+    return hasPadding(layoutBox.style().paddingBefore());
>+}
>+
>+static bool hasPaddingAfter(const Box& layoutBox)
>+{
>+    return hasPadding(layoutBox.style().paddingAfter());
>+}
>+
> static LayoutUnit marginValue(LayoutUnit currentMarginValue, LayoutUnit candidateMarginValue)
> {
>     if (!candidateMarginValue)
>@@ -94,13 +127,13 @@ bool BlockFormattingContext::Geometry::MarginCollapse::isMarginTopCollapsedWithP
>     if (layoutBox.isFloatingOrOutOfFlowPositioned())
>         return false;
> 
>-    // We never margin collapse the initial containing block.
>-    ASSERT(layoutBox.parent());
>-    auto& parent = *layoutBox.parent();
>     // Only the first inlflow child collapses with parent.
>     if (layoutBox.previousInFlowSibling())
>         return false;
> 
>+    // We never margin collapse the initial containing block.
>+    ASSERT(layoutBox.parent());
>+    auto& parent = *layoutBox.parent();
>     if (parent.establishesBlockFormattingContext())
>         return false;
> 
>@@ -108,11 +141,10 @@ bool BlockFormattingContext::Geometry::MarginCollapse::isMarginTopCollapsedWithP
>     if (parent.isDocumentBox() || parent.isInitialContainingBlock())
>         return false;
> 
>-    auto& parentDisplayBox = layoutState.displayBoxForLayoutBox(parent);
>-    if (parentDisplayBox.borderTop())
>+    if (hasBorderBefore(parent))
>         return false;
> 
>-    if (parentDisplayBox.paddingTop().value_or(0))
>+    if (hasPaddingBefore(parent))
>         return false;
> 
>     if (BlockFormattingContext::Quirks::shouldIgnoreMarginTop(layoutState, layoutBox))
>@@ -121,17 +153,15 @@ bool BlockFormattingContext::Geometry::MarginCollapse::isMarginTopCollapsedWithP
>     return true;
> }
> 
>-static bool isMarginBottomCollapsedThrough(const LayoutState& layoutState, const Box& layoutBox)
>+static bool isMarginBottomCollapsedThrough(const Box& layoutBox)
> {
>     ASSERT(layoutBox.isBlockLevelBox());
> 
>     // If the top and bottom margins of a box are adjoining, then it is possible for margins to collapse through it.
>-    auto& displayBox = layoutState.displayBoxForLayoutBox(layoutBox);
>-
>-    if (displayBox.borderTop() || displayBox.borderBottom())
>+    if (hasBorderBefore(layoutBox) || hasBorderAfter(layoutBox))
>         return false;
> 
>-    if (displayBox.paddingTop().value_or(0) || displayBox.paddingBottom().value_or(0))
>+    if (hasPaddingBefore(layoutBox) || hasPaddingAfter(layoutBox))
>         return false;
> 
>     if (!layoutBox.style().height().isAuto() || !layoutBox.style().minHeight().isAuto())
>@@ -219,7 +249,7 @@ LayoutUnit BlockFormattingContext::Geometry::MarginCollapse::marginTop(const Lay
>         return 0;
> 
>     if (!isMarginTopCollapsedWithSibling(layoutBox)) {
>-        if (!isMarginBottomCollapsedThrough(layoutState, layoutBox))
>+        if (!isMarginBottomCollapsedThrough(layoutBox))
>             return nonCollapsedMarginTop(layoutState, layoutBox);
>         // Compute the collapsed through value.
>         auto marginTop = nonCollapsedMarginTop(layoutState, layoutBox);
>@@ -246,10 +276,10 @@ LayoutUnit BlockFormattingContext::Geometry::MarginCollapse::marginBottom(const
>     ASSERT(layoutBox.isBlockLevelBox());
> 
>     // TODO: take _hasAdjoiningMarginTopAndBottom() into account.
>-    if (isMarginBottomCollapsedWithParent(layoutState, layoutBox))
>+    if (isMarginBottomCollapsedWithParent(layoutBox))
>         return 0;
> 
>-    if (isMarginBottomCollapsedThrough(layoutState, layoutBox))
>+    if (isMarginBottomCollapsedThrough(layoutBox))
>         return 0;
> 
>     // Floats and out of flow positioned boxes do not collapse their margins.
>@@ -263,7 +293,7 @@ LayoutUnit BlockFormattingContext::Geometry::MarginCollapse::marginBottom(const
>     return nonCollapsedMarginBottom(layoutState, layoutBox);
> }
> 
>-bool BlockFormattingContext::Geometry::MarginCollapse::isMarginBottomCollapsedWithParent(const LayoutState& layoutState, const Box& layoutBox)
>+bool BlockFormattingContext::Geometry::MarginCollapse::isMarginBottomCollapsedWithParent(const Box& layoutBox)
> {
>     // last inflow box to parent.
>     // https://www.w3.org/TR/CSS21/box.html#collapsing-margins
>@@ -275,7 +305,7 @@ bool BlockFormattingContext::Geometry::MarginCollapse::isMarginBottomCollapsedWi
>     if (layoutBox.isFloatingOrOutOfFlowPositioned())
>         return false;
> 
>-    if (isMarginBottomCollapsedThrough(layoutState, layoutBox))
>+    if (isMarginBottomCollapsedThrough(layoutBox))
>         return false;
> 
>     // We never margin collapse the initial containing block.
>@@ -292,11 +322,10 @@ bool BlockFormattingContext::Geometry::MarginCollapse::isMarginBottomCollapsedWi
>     if (parent.isDocumentBox() || parent.isInitialContainingBlock())
>         return false;
> 
>-    auto& parentDisplayBox = layoutState.displayBoxForLayoutBox(parent);
>-    if (parentDisplayBox.borderTop())
>+    if (hasBorderBefore(parent))
>         return false;
> 
>-    if (parentDisplayBox.paddingTop().value_or(0))
>+    if (hasPaddingBefore(parent))
>         return false;
> 
>     if (!parent.style().height().isAuto())
>@@ -324,7 +353,7 @@ LayoutUnit BlockFormattingContext::Geometry::MarginCollapse::collapsedMarginBott
> 
>     // FIXME: Check for collapsed through margin.
>     auto& lastInFlowChild = *downcast<Container>(layoutBox).lastInFlowChild();
>-    if (!isMarginBottomCollapsedWithParent(layoutState, lastInFlowChild))
>+    if (!isMarginBottomCollapsedWithParent(lastInFlowChild))
>         return 0;
> 
>     // Collect collapsed margin bottom recursively.
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index acf88883d51c0ce3c20b6a1b2d5f13cb1cfbce62..e3907a7d0d4297e1c4b6bcfb2bc24f5d2f393360 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,12 @@
>+2018-11-29  Zalan Bujtas  <zalan@apple.com>
>+
>+        [LFC][BFC][MarginCollapsing] Do not use computed display box values for border and padding
>+        https://bugs.webkit.org/show_bug.cgi?id=192214
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * LayoutReloaded/misc/LFC-passing-tests.txt:
>+
> 2018-11-29  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC][BFC][Quirk] Body and html height stretching.
>diff --git a/Tools/LayoutReloaded/misc/LFC-passing-tests.txt b/Tools/LayoutReloaded/misc/LFC-passing-tests.txt
>index a4e272aa1e5fa87ce43dc1ca86c5c0e489f686c5..c7216771f99a137499ddf0bfadd983d3e28c4d6d 100644
>--- a/Tools/LayoutReloaded/misc/LFC-passing-tests.txt
>+++ b/Tools/LayoutReloaded/misc/LFC-passing-tests.txt
>@@ -140,3 +140,4 @@ fast/block/positioning/046.html
> fast/block/positioning/049.html
> fast/block/positioning/052.html
> fast/block/positioning/054.html
>+fast/block/inside-inlines/basic-float-intrusion.html

Flags:

koivisto: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 192214: 356127