[LFC][Floats] Decouple clearance computation and margin collapsing reset.
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 22 Jan 2019 17:25:59 +0000 (17:25 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 22 Jan 2019 17:25:59 +0000 (17:25 +0000)
https://bugs.webkit.org/show_bug.cgi?id=193670

Reviewed by Antti Koivisto.

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:

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

Source/WebCore/ChangeLog
Source/WebCore/layout/FormattingContext.cpp
Source/WebCore/layout/FormattingContext.h
Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
Source/WebCore/layout/floats/FloatingContext.cpp
Source/WebCore/layout/floats/FloatingContext.h

index 84752c5..2eae79f 100644 (file)
@@ -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 Antti Koivisto.
+
+        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-22  Frederic Wang  <fwang@igalia.com>
 
         Minor refactoring of the scrolling code
index 2bb4adb..1b0da82 100644 (file)
@@ -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)
index 34aae97..ccb2790 100644 (file)
@@ -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:
index 18777d3..5317d75 100644 (file)
@@ -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
index 086a5d1..57564d6 100644 (file)
@@ -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();
index ea8ede0..2f460e0 100644 (file)
@@ -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(); }