Attachment 360116 Details for Bug 193824 – Patch

[patch] Patch

Patch.txt (text/plain), 17.36 KB, created by zalan on 2019-01-25 08:07:33 PST

(hide)

Description:

Filename:

MIME Type:

Creator: zalan

Created: 2019-01-25 08:07:33 PST

Size: 17.36 KB

patch

obsolete

>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index d158b70c90a..b8a9a0deb70 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,36 @@
>+2019-01-25  Zalan Bujtas  <zalan@apple.com>
>+
>+        [LFC][BFC][MarginCollapsing] Add "clear" to static position computation.
>+        https://bugs.webkit.org/show_bug.cgi?id=193824
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When clear property is set and floats are present, we have to estimate and set the box's vertical position during
>+        static positioning to be able to properly layout its subtree.
>+        
>+        <div style="float: left; width: 100px; height: 100px;"></div>
>+        <div style="clear: left;">
>+          <div style="float: left; width: 100px; height: 100px;"></div>
>+        </div>
>+
>+        In the above example since the second float's parent clears the first float, the second float is positioned below
>+        the first float. If we didn't push down (clear) the box, the float child would get placed next to the first float.
>+
>+        * layout/blockformatting/BlockFormattingContext.cpp:
>+        (WebCore::Layout::BlockFormattingContext::layout const):
>+        (WebCore::Layout::BlockFormattingContext::layoutFormattingContextRoot const):
>+        (WebCore::Layout::BlockFormattingContext::computeStaticPosition const):
>+        (WebCore::Layout::BlockFormattingContext::computeEstimatedVerticalPosition const):
>+        (WebCore::Layout::BlockFormattingContext::computeEstimatedVerticalPositionForFloatClear const):
>+        (WebCore::Layout::BlockFormattingContext::computeHeightAndMargin const):
>+        (WebCore::Layout::BlockFormattingContext::verticalPositionWithMargin const):
>+        (WebCore::Layout::BlockFormattingContext::computeVerticalPositionForFloatClear const): Deleted.
>+        (WebCore::Layout::BlockFormattingContext::adjustedVerticalPositionAfterMarginCollapsing const): Deleted.
>+        * layout/blockformatting/BlockFormattingContext.h:
>+        * layout/blockformatting/BlockMarginCollapse.cpp:
>+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::estimatedMarginBefore):
>+        * layout/displaytree/DisplayBox.h:
>+
> 2019-01-24  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC][BFC][MarginCollapsing] Move positive/negative margin value updating to a dedicated function
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>index 7fc4ebc358d..0f9c1b746ad 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>@@ -90,7 +90,7 @@ void BlockFormattingContext::layout() const
>             LOG_WITH_STREAM(FormattingContextLayout, stream << "[Compute] -> [Position][Border][Padding][Width][Margin] -> for layoutBox(" << &layoutBox << ")");
>             computeBorderAndPadding(layoutBox);
>             computeWidthAndMargin(layoutBox);
>-            computeStaticPosition(layoutBox);
>+            computeStaticPosition(floatingContext, layoutBox);
>             if (!is<Container>(layoutBox) || !downcast<Container>(layoutBox).hasInFlowOrFloatingChild())
>                 break;
>             layoutQueue.append(downcast<Container>(layoutBox).firstInFlowOrFloatingChild());
>@@ -105,9 +105,6 @@ void BlockFormattingContext::layout() const
>             // Formatting root boxes are special-cased and they don't come here.
>             ASSERT(!layoutBox.establishesFormattingContext());
>             computeHeightAndMargin(layoutBox);
>-            // Finalize position with clearance.
>-            if (layoutBox.hasFloatClear())
>-                computeVerticalPositionForFloatClear(floatingContext, layoutBox);
>             if (!is<Container>(layoutBox))
>                 continue;
>             auto& container = downcast<Container>(layoutBox);
>@@ -130,7 +127,7 @@ void BlockFormattingContext::layoutFormattingContextRoot(FloatingContext& floati
>     LOG_WITH_STREAM(FormattingContextLayout, stream << "[Compute] -> [Position][Border][Padding][Width][Margin] -> for layoutBox(" << &layoutBox << ")");
>     computeBorderAndPadding(layoutBox);
>     computeWidthAndMargin(layoutBox);
>-    computeStaticPosition(layoutBox);
>+    computeStaticPosition(floatingContext, layoutBox);
>     // Swich over to the new formatting context (the one that the root creates).
>     auto formattingContext = layoutState().createFormattingContext(layoutBox);
>     formattingContext->layout();
>@@ -143,9 +140,7 @@ void BlockFormattingContext::layoutFormattingContextRoot(FloatingContext& floati
>     if (layoutBox.isFloatingPositioned()) {
>         computeFloatingPosition(floatingContext, layoutBox);
>         floatingContext.floatingState().append(layoutBox);
>-    } else if (layoutBox.hasFloatClear())
>-        computeVerticalPositionForFloatClear(floatingContext, layoutBox);
>-    else if (layoutBox.establishesBlockFormattingContext())
>+    } else if (layoutBox.establishesBlockFormattingContext())
>         computePositionToAvoidFloats(floatingContext, layoutBox);
> 
>     // Now that we computed the root's height, we can go back and layout the out-of-flow descedants (if any).
>@@ -176,12 +171,12 @@ void BlockFormattingContext::placeInFlowPositionedChildren(const Container& cont
>     LOG_WITH_STREAM(FormattingContextLayout, stream << "End: move in-flow positioned children -> parent: " << &container);
> }
> 
>-void BlockFormattingContext::computeStaticPosition(const Box& layoutBox) const
>+void BlockFormattingContext::computeStaticPosition(const FloatingContext& floatingContext, const Box& layoutBox) const
> {
>     auto& layoutState = this->layoutState();
>     layoutState.displayBoxForLayoutBox(layoutBox).setTopLeft(Geometry::staticPosition(layoutState, layoutBox));
>     if (layoutBox.hasFloatClear())
>-        computeEstimatedVerticalPositionForFloatClear(layoutBox);
>+        computeEstimatedVerticalPositionForFloatClear(floatingContext, layoutBox);
>     else if (layoutBox.establishesFormattingContext())
>         computeEstimatedVerticalPositionForFormattingRoot(layoutBox);
> }
>@@ -197,7 +192,7 @@ void BlockFormattingContext::computeEstimatedVerticalPosition(const Box& layoutB
>     auto collapsedValues = UsedVerticalMargin::CollapsedValues { estimatedMarginBefore.collapsedValue, { }, estimatedMarginBefore.isCollapsedThrough };
>     auto verticalMargin = UsedVerticalMargin { nonCollapsedValues, collapsedValues };
>     displayBox.setVerticalMargin(verticalMargin);
>-    displayBox.setTop(adjustedVerticalPositionAfterMarginCollapsing(layoutBox, verticalMargin));
>+    displayBox.setTop(verticalPositionWithMargin(layoutBox, verticalMargin));
> #if !ASSERT_DISABLED
>     displayBox.setHasEstimatedMarginBefore();
> #endif
>@@ -243,8 +238,25 @@ void BlockFormattingContext::computeEstimatedVerticalPositionForFormattingRoot(c
>     }
> }
> 
>-void BlockFormattingContext::computeEstimatedVerticalPositionForFloatClear(const Box&) const
>+void BlockFormattingContext::computeEstimatedVerticalPositionForFloatClear(const FloatingContext& floatingContext, const Box& layoutBox) const
> {
>+    ASSERT(layoutBox.hasFloatClear());
>+    if (floatingContext.floatingState().isEmpty())
>+        return;
>+    // The static position with clear requires margin esitmation to see if clearance is needed.
>+    computeEstimatedVerticalPosition(layoutBox);
>+    computeEstimatedVerticalPositionForAncestors(layoutBox);
>+    auto verticalPositionAndClearance = floatingContext.verticalPositionWithClearance(layoutBox);
>+    if (!verticalPositionAndClearance.position) {
>+        ASSERT(!verticalPositionAndClearance.clearance);
>+        return;
>+    }
>+
>+    auto& displayBox = layoutState().displayBoxForLayoutBox(layoutBox);
>+    ASSERT(*verticalPositionAndClearance.position >= displayBox.top());
>+    displayBox.setTop(*verticalPositionAndClearance.position);
>+    if (verticalPositionAndClearance.clearance)
>+        displayBox.setHasClearance();
> }
> 
> #ifndef NDEBUG
>@@ -292,54 +304,6 @@ void BlockFormattingContext::computePositionToAvoidFloats(const FloatingContext&
>         layoutState.displayBoxForLayoutBox(layoutBox).setTopLeft(*adjustedPosition);
> }
> 
>-void BlockFormattingContext::computeVerticalPositionForFloatClear(const FloatingContext& floatingContext, const Box& layoutBox) const
>-{
>-    ASSERT(layoutBox.hasFloatClear());
>-    if (floatingContext.floatingState().isEmpty())
>-        return;
>-
>-    // For formatting roots, we already precomputed final position.
>-    if (!layoutBox.establishesFormattingContext())
>-        computeEstimatedVerticalPositionForAncestors(layoutBox);
>-    ASSERT(hasPrecomputedMarginBefore(layoutBox));
>-
>-    // 1. Compute and adjust the vertical position with clearance.
>-    // 2. Reset margin collapsing with previous in-flow sibling if clerance is present.
>-    auto verticalPositionAndClearance = floatingContext.verticalPositionWithClearance(layoutBox);
>-    if (!verticalPositionAndClearance.position)
>-        return;
>-
>-    // Adjust vertical position first.
>-    auto& layoutState = this->layoutState();
>-    auto& displayBox = layoutState.displayBoxForLayoutBox(layoutBox);
>-    displayBox.setTop(*verticalPositionAndClearance.position);
>-    if (!verticalPositionAndClearance.clearance)
>-        return;
>-
>-    displayBox.setHasClearance();
>-    // Adjust margin collapsing with clearance.
>-    auto* previousInFlowSibling = layoutBox.previousInFlowSibling();
>-    if (!previousInFlowSibling)
>-        return;
>-
>-    // Clearance inhibits margin collapsing. Let's reset the relevant adjoining margins.
>-    // Does this box with clearance actually collapse its margin before with the previous inflow box's margin after? 
>-    auto verticalMargin = displayBox.verticalMargin();
>-    if (!verticalMargin.hasCollapsedValues() || !verticalMargin.collapsedValues().before)
>-        return;
>-
>-    // Reset previous after and current before margin values to non-collapsing.
>-    auto& previousInFlowDisplayBox = layoutState.displayBoxForLayoutBox(*previousInFlowSibling); 
>-    auto previousVerticalMargin = previousInFlowDisplayBox.verticalMargin();
>-    ASSERT(previousVerticalMargin.hasCollapsedValues() && previousVerticalMargin.collapsedValues().after);
>-
>-    previousVerticalMargin.setCollapsedValues({ previousVerticalMargin.collapsedValues().before, { } });
>-    verticalMargin.setCollapsedValues({ { }, verticalMargin.collapsedValues().after });
>-
>-    previousInFlowDisplayBox.setVerticalMargin(previousVerticalMargin);
>-    displayBox.setVerticalMargin(verticalMargin);
>-}
>-
> void BlockFormattingContext::computeWidthAndMargin(const Box& layoutBox) const
> {
>     auto& layoutState = this->layoutState();
>@@ -428,7 +392,7 @@ void BlockFormattingContext::computeHeightAndMargin(const Box& layoutBox) const
> 
>     ASSERT(!hasEstimatedMarginBefore(layoutBox) || estimatedMarginBefore(layoutBox).usedValue() == verticalMargin.before());
>     removeEstimatedMarginBefore(layoutBox);
>-    displayBox.setTop(adjustedVerticalPositionAfterMarginCollapsing(layoutBox, verticalMargin));
>+    displayBox.setTop(verticalPositionWithMargin(layoutBox, verticalMargin));
>     displayBox.setContentBoxHeight(heightAndMargin.height);
>     displayBox.setVerticalMargin(verticalMargin);
> 
>@@ -501,14 +465,19 @@ FormattingContext::InstrinsicWidthConstraints BlockFormattingContext::instrinsic
>     return instrinsicWidthConstraints;
> }
> 
>-LayoutUnit BlockFormattingContext::adjustedVerticalPositionAfterMarginCollapsing(const Box& layoutBox, const UsedVerticalMargin& verticalMargin) const
>+LayoutUnit BlockFormattingContext::verticalPositionWithMargin(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 including 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)
>-    // 3. Go to previous box and start from step #1 until we hit the parent box.
>+    // Now that we've computed the final margin before, let's shift the box's vertical position if needed.
>+    // 1. Check if the box has clearance. If so, we've already precomputed/finalized the top value and vertical margin does not impact it anymore.
>+    // 2. Check if the margin before collapses with the previous box's margin after. if not -> return previous box's bottom including margin after + marginBefore
>+    // 3. 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)
>+    // 4. Go to previous box and start from step #1 until we hit the parent box.
>     auto& layoutState = this->layoutState();
>+    auto& displayBox = layoutState.displayBoxForLayoutBox(layoutBox);
>+    if (displayBox.hasClearance())
>+        return displayBox.top();
>+
>     auto* currentLayoutBox = &layoutBox;
>     while (currentLayoutBox) {
>         if (!currentLayoutBox->previousInFlowSibling())
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContext.h b/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>index 382b2a2e2ea..572ca265d9e 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>@@ -58,18 +58,17 @@ private:
>     void computeWidthAndMargin(const Box&) const;
>     void computeHeightAndMargin(const Box&) const;
> 
>-    void computeStaticPosition(const Box&) const;
>+    void computeStaticPosition(const FloatingContext&, const Box&) const;
>     void computeFloatingPosition(const FloatingContext&, const Box&) const;
>     void computePositionToAvoidFloats(const FloatingContext&, const Box&) const;
>-    void computeVerticalPositionForFloatClear(const FloatingContext&, const Box&) const;
> 
>     void computeEstimatedVerticalPosition(const Box&) const;
>     void computeEstimatedVerticalPositionForAncestors(const Box&) const;
>     void computeEstimatedVerticalPositionForFormattingRoot(const Box&) const;
>-    void computeEstimatedVerticalPositionForFloatClear(const Box&) const;
>+    void computeEstimatedVerticalPositionForFloatClear(const FloatingContext&, const Box&) const;
> 
>     InstrinsicWidthConstraints instrinsicWidthConstraints() const override;
>-    LayoutUnit adjustedVerticalPositionAfterMarginCollapsing(const Box&, const UsedVerticalMargin&) const;
>+    LayoutUnit verticalPositionWithMargin(const Box&, const UsedVerticalMargin&) const;
> 
>     // This class implements positioning and sizing for boxes participating in a block formatting context.
>     class Geometry : public FormattingContext::Geometry {
>diff --git a/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp b/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
>index d41405c34dc..02f7f00114a 100644
>--- a/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
>@@ -582,11 +582,9 @@ EstimatedMarginBefore BlockFormattingContext::MarginCollapse::estimatedMarginBef
>         return { };
> 
>     ASSERT(layoutBox.isBlockLevelBox());
>-    // Can't estimate vertical margins for out of flow boxes (and we shouldn't need to do it for float boxes).
>-    ASSERT(layoutBox.isInFlow());
>+    // Don't estimate vertical margins for out of flow boxes.
>+    ASSERT(layoutBox.isInFlow() || layoutBox.isFloatingPositioned());
>     ASSERT(!layoutBox.replaced());
>-    // Can't cross block formatting context boundary.
>-    ASSERT(!layoutBox.establishesBlockFormattingContext());
>     
>     auto computedVerticalMargin = Geometry::computedVerticalMargin(layoutState, layoutBox);
>     auto nonCollapsedMargin = UsedVerticalMargin::NonCollapsedValues { computedVerticalMargin.before.valueOr(0), computedVerticalMargin.after.valueOr(0) };
>diff --git a/Source/WebCore/layout/displaytree/DisplayBox.h b/Source/WebCore/layout/displaytree/DisplayBox.h
>index b7549f1fc40..fd31dfe7d0e 100644
>--- a/Source/WebCore/layout/displaytree/DisplayBox.h
>+++ b/Source/WebCore/layout/displaytree/DisplayBox.h
>@@ -242,7 +242,7 @@ private:
>     Layout::UsedHorizontalMargin m_horizontalMargin;
>     Layout::UsedVerticalMargin m_verticalMargin;
>     Layout::ComputedHorizontalMargin m_horizontalComputedMargin;
>-    bool m_hasClearance;
>+    bool m_hasClearance { false };
> 
>     Layout::Edges m_border;
>     Optional<Layout::Edges> m_padding;
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 5282ee4b060..0a493f84fa5 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,12 @@
>+2019-01-25  Zalan Bujtas  <zalan@apple.com>
>+
>+        [LFC][BFC][MarginCollapsing] Add "clear" to static position computation.
>+        https://bugs.webkit.org/show_bug.cgi?id=193824
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * LayoutReloaded/misc/LFC-passing-tests.txt:
>+
> 2019-01-24  Fujii Hironori  <Hironori.Fujii@sony.com>
> 
>         [Win][WebKitTestRunner] Implement EventSenderProxy
>diff --git a/Tools/LayoutReloaded/misc/LFC-passing-tests.txt b/Tools/LayoutReloaded/misc/LFC-passing-tests.txt
>index e8592bd0eb4..393c713e16e 100644
>--- a/Tools/LayoutReloaded/misc/LFC-passing-tests.txt
>+++ b/Tools/LayoutReloaded/misc/LFC-passing-tests.txt
>@@ -133,6 +133,7 @@ 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/044.html
> fast/block/positioning/003.html
> fast/block/positioning/004.html
> fast/block/positioning/005.html

Flags:

koivisto: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193824: 360116