[lFC][Floating] Add estimated margin top computation.
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 16 Aug 2018 15:43:53 +0000 (15:43 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 16 Aug 2018 15:43:53 +0000 (15:43 +0000)
https://bugs.webkit.org/show_bug.cgi?id=188619

Reviewed by Antti Koivisto.

In order to figure out whether a box should avoid a float, we need to know the final positions of both (ignore relative positioning for now).
In block formatting context the final position for a normal flow box includes
1. the static position and
2. the corresponding (non)collapsed margins.
Now the vertical margins are computed when all the descendants are finalized, because the margin values might be depending on the height of the box
(and the height might be based on the content).
So when we get to the point where we intersect the box with the float to decide if the box needs to move, we don't yet have the final vertical position.

The idea here is that as long as we don't cross the block formatting context boundary, we should be able to pre-compute the final top margin.
(if this holds true for all the cases, the estimated prefix could be removed and just use marginTop() instead.)

Covered by existing block-only tests.

* layout/blockformatting/BlockFormattingContext.cpp:
(WebCore::Layout::BlockFormattingContext::layout const):
(WebCore::Layout::BlockFormattingContext::computeEstimatedMarginTop const):
(WebCore::Layout::BlockFormattingContext::computeEstimatedMarginTopForAncestors const):
(WebCore::Layout::BlockFormattingContext::computeFloatingPosition const):
(WebCore::Layout::BlockFormattingContext::computeVerticalPositionForFloatClear const):
(WebCore::Layout::BlockFormattingContext::computeInFlowHeightAndMargin const):
* layout/blockformatting/BlockFormattingContext.h:
* layout/blockformatting/BlockMarginCollapse.cpp:
(WebCore::Layout::BlockFormattingContext::MarginCollapse::estimatedMarginTop):
* layout/displaytree/DisplayBox.cpp:
(WebCore::Display::Box::Box):
* layout/displaytree/DisplayBox.h:
(WebCore::Display::Box::setHasValidTop):
(WebCore::Display::Box::setHasValidLeft):
(WebCore::Display::Box::top const):
(WebCore::Display::Box::left const):
(WebCore::Display::Box::topLeft const):
(WebCore::Display::Box::setTopLeft):
(WebCore::Display::Box::setTop):
(WebCore::Display::Box::setLeft):
(WebCore::Display::Box::setVerticalMargin):
(WebCore::Display::Box::setEstimatedMarginTop):
(WebCore::Display::Box::estimatedMarginTop const):

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

Source/WebCore/ChangeLog
Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
Source/WebCore/layout/blockformatting/BlockFormattingContext.h
Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
Source/WebCore/layout/displaytree/DisplayBox.cpp
Source/WebCore/layout/displaytree/DisplayBox.h

index 32c996f..124348f 100644 (file)
@@ -1,5 +1,50 @@
 2018-08-16  Zalan Bujtas  <zalan@apple.com>
 
+        [lFC][Floating] Add estimated margin top computation.
+        https://bugs.webkit.org/show_bug.cgi?id=188619
+
+        Reviewed by Antti Koivisto.
+
+        In order to figure out whether a box should avoid a float, we need to know the final positions of both (ignore relative positioning for now).
+        In block formatting context the final position for a normal flow box includes
+        1. the static position and
+        2. the corresponding (non)collapsed margins.
+        Now the vertical margins are computed when all the descendants are finalized, because the margin values might be depending on the height of the box
+        (and the height might be based on the content).
+        So when we get to the point where we intersect the box with the float to decide if the box needs to move, we don't yet have the final vertical position.
+
+        The idea here is that as long as we don't cross the block formatting context boundary, we should be able to pre-compute the final top margin.
+        (if this holds true for all the cases, the estimated prefix could be removed and just use marginTop() instead.)
+
+        Covered by existing block-only tests.
+
+        * layout/blockformatting/BlockFormattingContext.cpp:
+        (WebCore::Layout::BlockFormattingContext::layout const):
+        (WebCore::Layout::BlockFormattingContext::computeEstimatedMarginTop const):
+        (WebCore::Layout::BlockFormattingContext::computeEstimatedMarginTopForAncestors const):
+        (WebCore::Layout::BlockFormattingContext::computeFloatingPosition const):
+        (WebCore::Layout::BlockFormattingContext::computeVerticalPositionForFloatClear const):
+        (WebCore::Layout::BlockFormattingContext::computeInFlowHeightAndMargin const):
+        * layout/blockformatting/BlockFormattingContext.h:
+        * layout/blockformatting/BlockMarginCollapse.cpp:
+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::estimatedMarginTop):
+        * layout/displaytree/DisplayBox.cpp:
+        (WebCore::Display::Box::Box):
+        * layout/displaytree/DisplayBox.h:
+        (WebCore::Display::Box::setHasValidTop):
+        (WebCore::Display::Box::setHasValidLeft):
+        (WebCore::Display::Box::top const):
+        (WebCore::Display::Box::left const):
+        (WebCore::Display::Box::topLeft const):
+        (WebCore::Display::Box::setTopLeft):
+        (WebCore::Display::Box::setTop):
+        (WebCore::Display::Box::setLeft):
+        (WebCore::Display::Box::setVerticalMargin):
+        (WebCore::Display::Box::setEstimatedMarginTop):
+        (WebCore::Display::Box::estimatedMarginTop const):
+
+2018-08-16  Zalan Bujtas  <zalan@apple.com>
+
         [LFC][BFC] Display::Box interface should reflect that padding is optional.
         https://bugs.webkit.org/show_bug.cgi?id=188630
 
index c61e554..6edbd9e 100644 (file)
@@ -113,7 +113,7 @@ void BlockFormattingContext::layout(LayoutContext& layoutContext, FormattingStat
             computeHeightAndMargin(layoutContext, layoutBox, displayBox);
             // Finalize position with clearance.
             if (layoutBox.hasFloatClear())
-                computeVerticalPositionForFloatClear(floatingContext, layoutBox, displayBox);
+                computeVerticalPositionForFloatClear(layoutContext, floatingContext, layoutBox, displayBox);
             if (!is<Container>(layoutBox))
                 continue;
             auto& container = downcast<Container>(layoutBox);
@@ -156,6 +156,39 @@ void BlockFormattingContext::computeStaticPosition(LayoutContext& layoutContext,
     displayBox.setTopLeft(Geometry::staticPosition(layoutContext, layoutBox));
 }
 
+void BlockFormattingContext::computeEstimatedMarginTop(LayoutContext& layoutContext, const Box& layoutBox, Display::Box& displayBox) const
+{
+    auto estimatedMarginTop = MarginCollapse::estimatedMarginTop(layoutContext, layoutBox);
+    displayBox.setEstimatedMarginTop(estimatedMarginTop);
+    displayBox.moveVertically(estimatedMarginTop);
+}
+
+void BlockFormattingContext::computeEstimatedMarginTopForAncestors(LayoutContext& layoutContext, const Box& layoutBox) const
+{
+    // We only need to estimate margin top for float related layout.
+    ASSERT(layoutBox.isFloatingPositioned() || layoutBox.hasFloatClear());
+
+    // In order to figure out whether a box should avoid a float, we need to know the final positions of both (ignore relative positioning for now).
+    // In block formatting context the final position for a normal flow box includes
+    // 1. the static position and
+    // 2. the corresponding (non)collapsed margins.
+    // Now the vertical margins are computed when all the descendants are finalized, because the margin values might be depending on the height of the box
+    // (and the height might be based on the content).
+    // So when we get to the point where we intersect the box with the float to decide if the box needs to move, we don't yet have the final vertical position.
+    //
+    // The idea here is that as long as we don't cross the block formatting context boundary, we should be able to pre-compute the final top margin.
+
+    for (auto* ancestor = layoutBox.containingBlock(); ancestor && !ancestor->establishesBlockFormattingContext(); ancestor = ancestor->containingBlock()) {
+        auto* displayBox = layoutContext.displayBoxForLayoutBox(*ancestor);
+        ASSERT(displayBox);
+        // FIXME: with incremental layout, we might actually have a valid (non-estimated) margin top as well.
+        if (displayBox->estimatedMarginTop())
+            return;
+
+        computeEstimatedMarginTop(layoutContext, *ancestor, *displayBox);
+    }
+}
+
 void BlockFormattingContext::computeFloatingPosition(LayoutContext& layoutContext, FloatingContext& floatingContext, const Box& layoutBox, Display::Box& displayBox) const
 {
     ASSERT(layoutBox.isFloatingPositioned());
@@ -166,13 +199,18 @@ void BlockFormattingContext::computeFloatingPosition(LayoutContext& layoutContex
         auto& previousDisplayBox = *layoutContext.displayBoxForLayoutBox(*previousInFlowBox);
         displayBox.moveVertically(previousDisplayBox.nonCollapsedMarginBottom() - previousDisplayBox.marginBottom());
     }
+    computeEstimatedMarginTopForAncestors(layoutContext, layoutBox);
     displayBox.setTopLeft(floatingContext.positionForFloat(layoutBox));
     floatingContext.floatingState().append(layoutBox);
 }
 
-void BlockFormattingContext::computeVerticalPositionForFloatClear(const FloatingContext& floatingContext, const Box& layoutBox, Display::Box& displayBox) const
+void BlockFormattingContext::computeVerticalPositionForFloatClear(LayoutContext& layoutContext, const FloatingContext& floatingContext, const Box& layoutBox, Display::Box& displayBox) const
 {
     ASSERT(layoutBox.hasFloatClear());
+    if (floatingContext.floatingState().isEmpty())
+        return;
+
+    computeEstimatedMarginTopForAncestors(layoutContext, layoutBox);
     if (auto verticalPositionWithClearance = floatingContext.verticalPositionWithClearance(layoutBox))
         displayBox.setTop(*verticalPositionWithClearance);
 }
@@ -208,9 +246,13 @@ void BlockFormattingContext::computeInFlowHeightAndMargin(LayoutContext& layoutC
 {
     auto heightAndMargin = Geometry::inFlowHeightAndMargin(layoutContext, layoutBox);
     displayBox.setContentBoxHeight(heightAndMargin.height);
-    displayBox.moveVertically(heightAndMargin.collapsedMargin.value_or(heightAndMargin.margin).top);
-    displayBox.setVerticalMargin(heightAndMargin.collapsedMargin.value_or(heightAndMargin.margin));
+    auto marginValue = heightAndMargin.collapsedMargin.value_or(heightAndMargin.margin);
+    displayBox.setVerticalMargin(marginValue);
     displayBox.setVerticalNonCollapsedMargin(heightAndMargin.margin);
+
+    // This box has already been moved by the estimated vertical margin. No need to move it again.
+    if (!displayBox.estimatedMarginTop())
+        displayBox.moveVertically(marginValue.top);
 }
 
 void BlockFormattingContext::computeInFlowWidthAndMargin(LayoutContext& layoutContext, const Box& layoutBox, Display::Box& displayBox) const
index 3247ed5..e8687a3 100644 (file)
@@ -57,10 +57,12 @@ private:
 
     void computeStaticPosition(LayoutContext&, const Box&, Display::Box&) const override;
     void computeFloatingPosition(LayoutContext&, FloatingContext&, const Box&, Display::Box&) const;
-    void computeVerticalPositionForFloatClear(const FloatingContext&, const Box&, Display::Box&) const;
+    void computeVerticalPositionForFloatClear(LayoutContext&, const FloatingContext&, const Box&, Display::Box&) const;
     void computeInFlowPositionedPosition(LayoutContext&, const Box&, Display::Box&) const override;
     void computeInFlowWidthAndMargin(LayoutContext&, const Box&, Display::Box&) const;
     void computeInFlowHeightAndMargin(LayoutContext&, const Box&, Display::Box&) const;
+    void computeEstimatedMarginTop(LayoutContext&, const Box&, Display::Box&) const;
+    void computeEstimatedMarginTopForAncestors(LayoutContext&, const Box&) const;
 
     FormattingContext::InstrinsicWidthConstraints instrinsicWidthConstraints(LayoutContext&, const Box&) const override;
 
@@ -87,6 +89,7 @@ private:
     class MarginCollapse {
     public:
         static LayoutUnit marginTop(const LayoutContext&, const Box&);
+        static LayoutUnit estimatedMarginTop(const LayoutContext&, const Box&);
         static LayoutUnit marginBottom(const LayoutContext&, const Box&);
 
         static bool isMarginBottomCollapsedWithParent(const LayoutContext&, const Box&);
index 8610f1b..0db8fb4 100644 (file)
@@ -332,6 +332,18 @@ LayoutUnit BlockFormattingContext::MarginCollapse::nonCollapsedMarginBottom(cons
     return marginValue(computedNonCollapsedMarginBottom(layoutContext, layoutBox), collapsedMarginBottomFromLastChild(layoutContext, layoutBox));
 }
 
+LayoutUnit BlockFormattingContext::MarginCollapse::estimatedMarginTop(const LayoutContext& layoutContext, const Box& layoutBox)
+{
+    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());
+    // Can't cross block formatting context boundary.
+    ASSERT(!layoutBox.establishesBlockFormattingContext());
+
+    // Let's just use the normal path for now.
+    return marginTop(layoutContext, layoutBox);
+}
+
 }
 }
 #endif
index 98afcc5..718b16e 100644 (file)
@@ -59,9 +59,12 @@ Box::Box(const Box& other)
     , m_contentHeight(other.m_contentHeight)
     , m_margin(other.m_margin)
     , m_verticalNonCollapsedMargin(other.m_verticalNonCollapsedMargin)
+    , m_estimatedMarginTop(other.m_estimatedMarginTop)
     , m_border(other.m_border)
     , m_padding(other.m_padding)
 #if !ASSERT_DISABLED
+    , m_hasValidTop(other.m_hasValidTop)
+    , m_hasValidLeft(other.m_hasValidLeft)
     , m_hasValidHorizontalMargin(other.m_hasValidHorizontalMargin)
     , m_hasValidVerticalMargin(other.m_hasValidVerticalMargin)
     , m_hasValidVerticalNonCollapsedMargin(other.m_hasValidVerticalNonCollapsedMargin)
index 3e624f0..f722a18 100644 (file)
@@ -117,12 +117,12 @@ public:
 
     ~Box();
 
-    LayoutUnit top() const { return m_topLeft.y(); }
-    LayoutUnit left() const { return m_topLeft.x(); }
+    LayoutUnit top() const;
+    LayoutUnit left() const;
     LayoutUnit bottom() const { return top() + height(); }
     LayoutUnit right() const { return left() + width(); }
 
-    LayoutPoint topLeft() const { return m_topLeft; }
+    LayoutPoint topLeft() const;
     LayoutPoint bottomRight() const { return { right(), bottom() }; }
 
     LayoutSize size() const { return { width(), height() }; }
@@ -138,6 +138,7 @@ public:
 
     LayoutUnit nonCollapsedMarginTop() const;
     LayoutUnit nonCollapsedMarginBottom() const;
+    std::optional<LayoutUnit> estimatedMarginTop() const { return m_estimatedMarginTop; }
 
     LayoutUnit borderTop() const;
     LayoutUnit borderLeft() const;
@@ -172,9 +173,9 @@ private:
         BoxSizing boxSizing { BoxSizing::ContentBox };
     };
 
-    void setTopLeft(const LayoutPoint& topLeft) { m_topLeft = topLeft; }
-    void setTop(LayoutUnit top) { m_topLeft.setY(top); }
-    void setLeft(LayoutUnit left) { m_topLeft.setX(left); }
+    void setTopLeft(const LayoutPoint&);
+    void setTop(LayoutUnit);
+    void setLeft(LayoutUnit);
     void moveHorizontally(LayoutUnit offset) { m_topLeft.move(offset, { }); }
     void moveVertically(LayoutUnit offset) { m_topLeft.move({ }, offset); }
 
@@ -184,6 +185,7 @@ private:
     void setHorizontalMargin(Layout::HorizontalEdges);
     void setVerticalMargin(Layout::VerticalEdges);
     void setVerticalNonCollapsedMargin(Layout::VerticalEdges);
+    void setEstimatedMarginTop(LayoutUnit marginTop) { m_estimatedMarginTop = marginTop; }
 
     void setBorder(Layout::Edges);
     void setPadding(std::optional<Layout::Edges>);
@@ -193,6 +195,8 @@ private:
     void invalidateBorder() { m_hasValidBorder = false; }
     void invalidatePadding() { m_hasValidPadding = false; }
 
+    void setHasValidTop() { m_hasValidTop = true; }
+    void setHasValidLeft() { m_hasValidLeft = true; }
     void setHasValidVerticalMargin() { m_hasValidVerticalMargin = true; }
     void setHasValidVerticalNonCollapsedMargin() { m_hasValidVerticalNonCollapsedMargin = true; }
     void setHasValidHorizontalMargin() { m_hasValidHorizontalMargin = true; }
@@ -212,11 +216,14 @@ private:
 
     Layout::Edges m_margin;
     Layout::VerticalEdges m_verticalNonCollapsedMargin;
+    std::optional<LayoutUnit> m_estimatedMarginTop;
 
     Layout::Edges m_border;
     std::optional<Layout::Edges> m_padding;
 
 #if !ASSERT_DISABLED
+    bool m_hasValidTop { false };
+    bool m_hasValidLeft { false };
     bool m_hasValidHorizontalMargin { false };
     bool m_hasValidVerticalMargin { false };
     bool m_hasValidVerticalNonCollapsedMargin { false };
@@ -416,6 +423,50 @@ inline Box::Rect::operator LayoutRect() const
     return m_rect;
 }
 
+inline LayoutUnit Box::top() const
+{
+    ASSERT(m_hasValidTop && (m_estimatedMarginTop || m_hasValidVerticalMargin));
+    return m_topLeft.y();
+}
+
+inline LayoutUnit Box::left() const
+{
+    ASSERT(m_hasValidLeft && m_hasValidHorizontalMargin);
+    return m_topLeft.x();
+}
+
+inline LayoutPoint Box::topLeft() const
+{
+    ASSERT(m_hasValidTop && (m_estimatedMarginTop || m_hasValidVerticalMargin));
+    ASSERT(m_hasValidLeft && m_hasValidHorizontalMargin);
+    return m_topLeft;
+}
+
+inline void Box::setTopLeft(const LayoutPoint& topLeft)
+{
+#if !ASSERT_DISABLED
+    setHasValidTop();
+    setHasValidLeft();
+#endif
+    m_topLeft = topLeft;
+}
+
+inline void Box::setTop(LayoutUnit top)
+{
+#if !ASSERT_DISABLED
+    setHasValidTop();
+#endif
+    m_topLeft.setY(top);
+}
+
+inline void Box::setLeft(LayoutUnit left)
+{
+#if !ASSERT_DISABLED
+    setHasValidLeft();
+#endif
+    m_topLeft.setX(left);
+}
+
 inline void Box::setContentBoxHeight(LayoutUnit height)
 { 
 #if !ASSERT_DISABLED
@@ -457,6 +508,7 @@ inline void Box::setVerticalMargin(Layout::VerticalEdges margin)
 #if !ASSERT_DISABLED
     setHasValidVerticalMargin();
 #endif
+    ASSERT(!m_estimatedMarginTop || *m_estimatedMarginTop == margin.top);
     m_margin.vertical = margin;
 }