Attachment 359738 Details for Bug 193670 – Patch

[patch] Patch

bug-193670-20190122083911.patch (text/plain), 12.48 KB, created by zalan on 2019-01-22 08:39:18 PST

(hide)

Description:

Filename:

MIME Type:

Creator: zalan

Created: 2019-01-22 08:39:18 PST

Size: 12.48 KB

patch

obsolete

>Subversion Revision: 240240
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index c5e820f9805f37df0b64bcd6a3a6a58cf1b58653..64c42cad3b830b6c6065cd0631a88bd3d402ae76 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,23 @@
>+2019-01-22  Zalan Bujtas  <zalan@apple.com>
>+
>+        [LFC][Floats] Decouple clearance computation and margin collapsing reset.
>+        https://bugs.webkit.org/show_bug.cgi?id=193670
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Move margin collapsing reset logic from FloatingContext to BlockFormattingContext. It's the BlockFormattingContext's job to do.
>+        This is also in preparation for adding clear to static position.
>+
>+        * layout/FormattingContext.cpp:
>+        (WebCore::Layout::FormattingContext::mapTopToAncestor):
>+        (WebCore::Layout::FormattingContext::mapTopLeftToAncestor): Deleted.
>+        * layout/FormattingContext.h:
>+        * layout/blockformatting/BlockFormattingContext.cpp:
>+        (WebCore::Layout::BlockFormattingContext::computeVerticalPositionForFloatClear const):
>+        * layout/floats/FloatingContext.cpp:
>+        (WebCore::Layout::FloatingContext::verticalPositionWithClearance const):
>+        * layout/floats/FloatingContext.h:
>+
> 2019-01-21  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC][Floats] Take float top position into account when computing containing block height.
>diff --git a/Source/WebCore/layout/FormattingContext.cpp b/Source/WebCore/layout/FormattingContext.cpp
>index 2bb4adb89a11679b3654407ecc55e2ee02c8902b..1b0da821d05b4539c6e0f2fdad82358128f09c82 100644
>--- a/Source/WebCore/layout/FormattingContext.cpp
>+++ b/Source/WebCore/layout/FormattingContext.cpp
>@@ -184,10 +184,14 @@ Display::Box FormattingContext::mapBoxToAncestor(const LayoutState& layoutState,
>     return mappedDisplayBox;
> }
> 
>-Point FormattingContext::mapTopLeftToAncestor(const LayoutState& layoutState, const Box& layoutBox, const Container& ancestor)
>+LayoutUnit FormattingContext::mapTopToAncestor(const LayoutState& layoutState, const Box& layoutBox, const Container& ancestor)
> {
>     ASSERT(layoutBox.isDescendantOf(ancestor));
>-    return mapCoordinateToAncestor(layoutState, layoutState.displayBoxForLayoutBox(layoutBox).topLeft(), *layoutBox.containingBlock(), ancestor);
>+    auto top = layoutState.displayBoxForLayoutBox(layoutBox).top();
>+    auto* container = layoutBox.containingBlock();
>+    for (; container && container != &ancestor; container = container->containingBlock())
>+        top += layoutState.displayBoxForLayoutBox(*container).top();
>+    return top;
> }
> 
> Point FormattingContext::mapCoordinateToAncestor(const LayoutState& layoutState, Point position, const Container& containingBlock, const Container& ancestor)
>diff --git a/Source/WebCore/layout/FormattingContext.h b/Source/WebCore/layout/FormattingContext.h
>index 34aae97887bcc42b3225bed64db30169d67cc28b..ccb2790c2a5555c0d36b140c16ee483ab4950308 100644
>--- a/Source/WebCore/layout/FormattingContext.h
>+++ b/Source/WebCore/layout/FormattingContext.h
>@@ -59,7 +59,7 @@ public:
>     virtual InstrinsicWidthConstraints instrinsicWidthConstraints() const = 0;
> 
>     static Display::Box mapBoxToAncestor(const LayoutState&, const Box&, const Container& ancestor);
>-    static Point mapTopLeftToAncestor(const LayoutState&, const Box&, const Container& ancestor);
>+    static LayoutUnit mapTopToAncestor(const LayoutState&, const Box&, const Container& ancestor);
>     static Point mapCoordinateToAncestor(const LayoutState&, Point, const Container& containingBlock, const Container& ancestor);
> 
> protected:
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>index 18777d39acef7a1e17c7f9c3b71045d2814dfeec..5317d75c690b28ad00b41bd3acf1111f829ef136 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>@@ -291,14 +291,46 @@ void BlockFormattingContext::computeVerticalPositionForFloatClear(const Floating
>     if (floatingContext.floatingState().isEmpty())
>         return;
> 
>-    auto& layoutState = this->layoutState();
>     // For formatting roots, we already precomputed final position.
>     if (!layoutBox.establishesFormattingContext())
>         computeEstimatedMarginBeforeForAncestors(layoutBox);
>     ASSERT(hasPrecomputedMarginBefore(layoutBox));
> 
>-    if (auto verticalPositionWithClearance = floatingContext.verticalPositionWithClearance(layoutBox))
>-        layoutState.displayBoxForLayoutBox(layoutBox).setTop(*verticalPositionWithClearance);
>+    // 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
>diff --git a/Source/WebCore/layout/floats/FloatingContext.cpp b/Source/WebCore/layout/floats/FloatingContext.cpp
>index 086a5d1d94f08871fd43c2493b6b0cd2df318975..57564d63b9a062cca967012629114c8634ea670c 100644
>--- a/Source/WebCore/layout/floats/FloatingContext.cpp
>+++ b/Source/WebCore/layout/floats/FloatingContext.cpp
>@@ -191,7 +191,7 @@ Optional<Point> FloatingContext::positionForFloatAvoiding(const Box& layoutBox)
>     return { floatAvoider.rectInContainingBlock().topLeft() };
> }
> 
>-Optional<Position> FloatingContext::verticalPositionWithClearance(const Box& layoutBox) const
>+FloatingContext::ClearancePosition FloatingContext::verticalPositionWithClearance(const Box& layoutBox) const
> {
>     ASSERT(layoutBox.hasFloatClear());
>     ASSERT(layoutBox.isBlockLevelBox());
>@@ -200,7 +200,7 @@ Optional<Position> FloatingContext::verticalPositionWithClearance(const Box& lay
>     if (m_floatingState.isEmpty())
>         return { };
> 
>-    auto bottom = [&](Optional<PositionInContextRoot> floatBottom) -> Optional<Position> {
>+    auto bottom = [&](Optional<PositionInContextRoot> floatBottom) -> ClearancePosition {
>         // 'bottom' is in the formatting root's coordinate system.
>         if (!floatBottom)
>             return { };
>@@ -210,37 +210,23 @@ Optional<Position> FloatingContext::verticalPositionWithClearance(const Box& lay
>         //
>         // 1. The amount necessary to place the border edge of the block even with the bottom outer edge of the lowest float that is to be cleared.
>         // 2. The amount necessary to place the top border edge of the block at its hypothetical position.
>-
>         auto& layoutState = this->layoutState();
>-        auto& displayBox = layoutState.displayBoxForLayoutBox(layoutBox);
>-        auto rootRelativeTop = FormattingContext::mapTopLeftToAncestor(layoutState, layoutBox, downcast<Container>(m_floatingState.root())).y;
>+        auto rootRelativeTop = FormattingContext::mapTopToAncestor(layoutState, layoutBox, downcast<Container>(m_floatingState.root()));
>         auto clearance = *floatBottom - rootRelativeTop;
>         if (clearance <= 0)
>             return { };
> 
>-        displayBox.setHasClearance();
>-        // Clearance inhibits margin collapsing. Let's reset the relevant adjoining margins.
>+        // Clearance inhibits margin collapsing.
>         if (auto* previousInFlowSibling = layoutBox.previousInFlowSibling()) {
>-            auto& previousInFlowDisplayBox = layoutState.displayBoxForLayoutBox(*previousInFlowSibling);
>             // Does this box with clearance actually collapse its margin before with the previous inflow box's margin after? 
>-            auto verticalMargin = displayBox.verticalMargin();
>+            auto verticalMargin = layoutState.displayBoxForLayoutBox(layoutBox).verticalMargin();
>             if (verticalMargin.hasCollapsedValues() && verticalMargin.collapsedValues().before) {
>-                // Reset previous bottom after and current margin before to non-collapsing.
>-                auto previousVerticalMargin = previousInFlowDisplayBox.verticalMargin();
>-                ASSERT(previousVerticalMargin.hasCollapsedValues() && previousVerticalMargin.collapsedValues().after);
>-
>+                auto previousVerticalMargin = layoutState.displayBoxForLayoutBox(*previousInFlowSibling).verticalMargin();
>                 auto collapsedMargin = *verticalMargin.collapsedValues().before;
>-                previousVerticalMargin.setCollapsedValues({ previousVerticalMargin.collapsedValues().before, { } });
>-                previousInFlowDisplayBox.setVerticalMargin(previousVerticalMargin);
>-
>-                verticalMargin.setCollapsedValues({ { }, verticalMargin.collapsedValues().after });
>-                displayBox.setVerticalMargin(verticalMargin);
>-
>                 auto nonCollapsedMargin = previousVerticalMargin.after() + verticalMargin.before();
>                 auto marginDifference = nonCollapsedMargin - collapsedMargin;
>                 // Move the box to the position where it would be with non-collapsed margins.
>                 rootRelativeTop += marginDifference;
>-
>                 // Having negative clearance is also normal. It just means that the box with the non-collapsed margins is now lower than it needs to be.
>                 clearance -= marginDifference;
>             }
>@@ -251,10 +237,10 @@ Optional<Position> FloatingContext::verticalPositionWithClearance(const Box& lay
> 
>         // The return vertical position is in the containing block's coordinate system. Convert it to the formatting root's coordinate system if needed.
>         if (layoutBox.containingBlock() == &m_floatingState.root())
>-            return Position { rootRelativeTop };
>+            return { Position { rootRelativeTop }, clearance };
> 
>-        auto containingBlockRootRelativeTop = FormattingContext::mapTopLeftToAncestor(layoutState, *layoutBox.containingBlock(), downcast<Container>(m_floatingState.root())).y;
>-        return Position { rootRelativeTop - containingBlockRootRelativeTop };
>+        auto containingBlockRootRelativeTop = FormattingContext::mapTopToAncestor(layoutState, *layoutBox.containingBlock(), downcast<Container>(m_floatingState.root()));
>+        return { Position { rootRelativeTop - containingBlockRootRelativeTop }, clearance };
>     };
> 
>     auto clear = layoutBox.style().clear();
>diff --git a/Source/WebCore/layout/floats/FloatingContext.h b/Source/WebCore/layout/floats/FloatingContext.h
>index ea8ede04e2af5f13e067dc4c692b3eb8a31c5b79..2f460e0ad354591d873129e948442be3e2bc7572 100644
>--- a/Source/WebCore/layout/floats/FloatingContext.h
>+++ b/Source/WebCore/layout/floats/FloatingContext.h
>@@ -52,7 +52,12 @@ public:
> 
>     Point positionForFloat(const Box&) const;
>     Optional<Point> positionForFloatAvoiding(const Box&) const;
>-    Optional<Position> verticalPositionWithClearance(const Box&) const;
>+
>+    struct ClearancePosition {
>+        Optional<Position> position;
>+        Optional<LayoutUnit> clearance;
>+    };
>+    ClearancePosition verticalPositionWithClearance(const Box&) const;
> 
> private:
>     LayoutState& layoutState() const { return m_floatingState.layoutState(); }

Actions: View | Formatted Diff | Diff

Attachments on bug 193670: 359738