Changeset 240253 in webkit

Timestamp:

Jan 22, 2019 9:25:59 AM (5 years ago)

Author:

Alan Bujtas

Message:

[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:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• layout/FormattingContext.cpp (modified) (1 diff)
• layout/FormattingContext.h (modified) (1 diff)
• layout/blockformatting/BlockFormattingContext.cpp (modified) (2 diffs)
• layout/floats/FloatingContext.cpp (modified) (4 diffs)
• layout/floats/FloatingContext.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/layout/FormattingContext.cpp

@@ -185 +185 @@
 }
 
-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;
 }
 

trunk/Source/WebCore/layout/FormattingContext.h

@@ -60 +60 @@
 
     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);
 

trunk/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp

@@ -292 +292 @@
         return;
 
-    auto& layoutState = this->layoutState();
     // For formatting roots, we already precomputed final position.
     if (!layoutBox.establishesFormattingContext())
@@ -298 +297 @@
     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);
 }
 

trunk/Source/WebCore/layout/floats/FloatingContext.cpp

@@ -192 +192 @@
 }
 
-Optional<Position> FloatingContext::verticalPositionWithClearance(const Box& layoutBox) const
+FloatingContext::ClearancePosition FloatingContext::verticalPositionWithClearance(const Box& layoutBox) const
 {
     ASSERT(layoutBox.hasFloatClear());
@@ -201 +201 @@
         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)
@@ -211 +211 @@
         // 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;
@@ -252 +238 @@
         // 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 };
-
-        auto containingBlockRootRelativeTop = FormattingContext::mapTopLeftToAncestor(layoutState, *layoutBox.containingBlock(), downcast<Container>(m_floatingState.root())).y;
-        return Position { rootRelativeTop - containingBlockRootRelativeTop };
+            return { Position { rootRelativeTop }, clearance };
+
+        auto containingBlockRootRelativeTop = FormattingContext::mapTopToAncestor(layoutState, *layoutBox.containingBlock(), downcast<Container>(m_floatingState.root()));
+        return { Position { rootRelativeTop - containingBlockRootRelativeTop }, clearance };
     };
 

trunk/Source/WebCore/layout/floats/FloatingContext.h

@@ -53 +53 @@
     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: