[LFC] The static position for an out-of-flow box should include the previous sibling...
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Jun 2018 07:55:53 +0000 (07:55 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Jun 2018 07:55:53 +0000 (07:55 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187169

Reviewed by Antti Koivisto.

When computing the static position of an absolutely positioned box, we need to look at the previous sibling's bottom margin.
If the previous sibling happens to collapse its bottom margin with the parent's bottom margin, we still need to account for it
and compute the static vertical position as if the bottom margin was not collapsed.

* layout/FormattingContext.cpp:
(WebCore::Layout::FormattingContext::computeFloatingHeightAndMargin const):
(WebCore::Layout::FormattingContext::computeOutOfFlowVerticalGeometry const):
* layout/FormattingContextGeometry.cpp:
(WebCore::Layout::staticVerticalPositionForOutOfFlowPositioned):
* layout/LayoutContext.cpp:
(WebCore::Layout::LayoutContext::initializeRoot):
* layout/Verification.cpp:
(WebCore::Layout::outputMismatchingBoxInformationIfNeeded):
* layout/blockformatting/BlockFormattingContext.cpp:
(WebCore::Layout::BlockFormattingContext::computeInFlowHeightAndMargin const):
* layout/displaytree/DisplayBox.cpp:
(WebCore::Display::Box::nonCollapsedMarginBox const):
* layout/displaytree/DisplayBox.h:
(WebCore::Display::Box::setHasValidVerticalNonCollapsedMargin):
(WebCore::Display::Box::setVerticalMargin):
(WebCore::Display::Box::setVerticalNonCollapsedMargin):
(WebCore::Display::Box::nonCollapsedMarginTop const):
(WebCore::Display::Box::nonCollapsedMarginBottom const):

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

Source/WebCore/ChangeLog
Source/WebCore/layout/FormattingContext.cpp
Source/WebCore/layout/FormattingContextGeometry.cpp
Source/WebCore/layout/LayoutContext.cpp
Source/WebCore/layout/Verification.cpp
Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
Source/WebCore/layout/displaytree/DisplayBox.cpp
Source/WebCore/layout/displaytree/DisplayBox.h

index aeee5a8..972ad9b 100644 (file)
@@ -1,3 +1,34 @@
+2018-06-29  Zalan Bujtas  <zalan@apple.com>
+
+        [LFC] The static position for an out-of-flow box should include the previous sibling's collapsed margin
+        https://bugs.webkit.org/show_bug.cgi?id=187169
+
+        Reviewed by Antti Koivisto.
+
+        When computing the static position of an absolutely positioned box, we need to look at the previous sibling's bottom margin.
+        If the previous sibling happens to collapse its bottom margin with the parent's bottom margin, we still need to account for it
+        and compute the static vertical position as if the bottom margin was not collapsed.
+
+        * layout/FormattingContext.cpp:
+        (WebCore::Layout::FormattingContext::computeFloatingHeightAndMargin const):
+        (WebCore::Layout::FormattingContext::computeOutOfFlowVerticalGeometry const):
+        * layout/FormattingContextGeometry.cpp:
+        (WebCore::Layout::staticVerticalPositionForOutOfFlowPositioned):
+        * layout/LayoutContext.cpp:
+        (WebCore::Layout::LayoutContext::initializeRoot):
+        * layout/Verification.cpp:
+        (WebCore::Layout::outputMismatchingBoxInformationIfNeeded):
+        * layout/blockformatting/BlockFormattingContext.cpp:
+        (WebCore::Layout::BlockFormattingContext::computeInFlowHeightAndMargin const):
+        * layout/displaytree/DisplayBox.cpp:
+        (WebCore::Display::Box::nonCollapsedMarginBox const):
+        * layout/displaytree/DisplayBox.h:
+        (WebCore::Display::Box::setHasValidVerticalNonCollapsedMargin):
+        (WebCore::Display::Box::setVerticalMargin):
+        (WebCore::Display::Box::setVerticalNonCollapsedMargin):
+        (WebCore::Display::Box::nonCollapsedMarginTop const):
+        (WebCore::Display::Box::nonCollapsedMarginBottom const):
+
 2018-06-27  Antoine Quint  <graouts@apple.com>
 
         [Web Animations] Using a Web Animation leaks the Document
index f284d1e..c420a84 100644 (file)
@@ -58,6 +58,7 @@ void FormattingContext::computeFloatingHeightAndMargin(LayoutContext& layoutCont
     displayBox.moveVertically(heightAndMargin.margin.top);
     ASSERT(!heightAndMargin.collapsedMargin);
     displayBox.setVerticalMargin(heightAndMargin.margin);
+    displayBox.setVerticalNonCollapsedMargin(heightAndMargin.margin);
 }
 
 void FormattingContext::computeFloatingWidthAndMargin(LayoutContext& layoutContext, const Box& layoutBox, Display::Box& displayBox) const
@@ -83,6 +84,7 @@ void FormattingContext::computeOutOfFlowVerticalGeometry(LayoutContext& layoutCo
     displayBox.setContentBoxHeight(verticalGeometry.heightAndMargin.height);
     ASSERT(!verticalGeometry.heightAndMargin.collapsedMargin);
     displayBox.setVerticalMargin(verticalGeometry.heightAndMargin.margin);
+    displayBox.setVerticalNonCollapsedMargin(verticalGeometry.heightAndMargin.margin);
 }
 
 void FormattingContext::computeBorderAndPadding(LayoutContext& layoutContext, const Box& layoutBox, Display::Box& displayBox) const
index c62b233..5df2664 100644 (file)
@@ -80,6 +80,11 @@ static LayoutUnit staticVerticalPositionForOutOfFlowPositioned(const LayoutConte
     ASSERT(layoutBox.isOutOfFlowPositioned());
 
     LayoutUnit top;
+    // Add sibling offset
+    if (auto* previousInFlowSibling = layoutBox.previousInFlowSibling()) {
+        auto& previousInFlowDisplayBox = *layoutContext.displayBoxForLayoutBox(*previousInFlowSibling);
+        top += previousInFlowDisplayBox.bottom() + previousInFlowDisplayBox.nonCollapsedMarginBottom();
+    }
     // Resolve top all the way up to the containing block.
     auto* containingBlock = layoutBox.containingBlock();
     for (auto* parent = layoutBox.parent(); parent; parent = parent->parent()) {
@@ -88,11 +93,6 @@ static LayoutUnit staticVerticalPositionForOutOfFlowPositioned(const LayoutConte
         if (parent == containingBlock)
             break;
     }
-    // Add sibling offset
-    if (auto* previousInFlowSibling = layoutBox.previousInFlowSibling()) {
-        auto& previousInFlowDisplayBox = *layoutContext.displayBoxForLayoutBox(*previousInFlowSibling);
-        top += previousInFlowDisplayBox.bottom() + previousInFlowDisplayBox.marginBottom();
-    }
     // FIXME: floatings need to be taken into account.
     return top;
 }
index 9999617..751eb62 100644 (file)
@@ -59,6 +59,7 @@ void LayoutContext::initializeRoot(const Container& root, const LayoutSize& cont
     // FIXME: m_root could very well be a formatting context root with ancestors and resolvable border and padding (as opposed to the topmost root)
     displayBox.setHorizontalMargin({ });
     displayBox.setVerticalMargin({ });
+    displayBox.setVerticalNonCollapsedMargin({ });
     displayBox.setBorder({ });
     displayBox.setPadding({ });
     displayBox.setContentBoxHeight(containerSize.height());
index 6697ea8..48164a0 100644 (file)
@@ -67,19 +67,10 @@ static bool outputMismatchingBoxInformationIfNeeded(TextStream& stream, const La
         return true;
     }
 
-#ifndef NDEBUG
     if (renderer.marginBoxRect() != displayBox->nonCollapsedMarginBox()) {
         outputRect("marginBox", renderer.marginBoxRect(), displayBox->nonCollapsedMarginBox());
         return true;
     }
-#else
-    // For now in non-debug builds, verify the horizontal margin only
-    if (renderer.marginBoxRect().left() != displayBox->marginBox().left()
-        || renderer.marginBoxRect().right() != displayBox->marginBox().right() ) {
-        outputRect("marginBox", renderer.marginBoxRect(), displayBox->marginBox());
-        return true;
-    }
-#endif
 
     if (renderer.borderBoxRect() != displayBox->borderBox()) {
         outputRect("borderBox", renderer.borderBoxRect(), displayBox->borderBox());
index c9c8275..2a057c6 100644 (file)
@@ -194,9 +194,7 @@ void BlockFormattingContext::computeInFlowHeightAndMargin(LayoutContext& layoutC
     displayBox.setContentBoxHeight(heightAndMargin.height);
     displayBox.moveVertically(heightAndMargin.collapsedMargin.value_or(heightAndMargin.margin).top);
     displayBox.setVerticalMargin(heightAndMargin.collapsedMargin.value_or(heightAndMargin.margin));
-#ifndef NDEBUG
     displayBox.setVerticalNonCollapsedMargin(heightAndMargin.margin);
-#endif
 }
 
 void BlockFormattingContext::computeInFlowWidthAndMargin(LayoutContext& layoutContext, const Box& layoutBox, Display::Box& displayBox) const
index dc1f31b..b020c1e 100644 (file)
@@ -62,19 +62,17 @@ Box::Rect Box::marginBox() const
     return marginBox;
 }
 
-#ifndef NDEBUG
 Box::Rect Box::nonCollapsedMarginBox() const
 {
     auto borderBox = this->borderBox();
 
     Rect marginBox;
-    marginBox.setTop(borderBox.top() - m_nonCollapsedVertivalMargin.top);
+    marginBox.setTop(borderBox.top() - nonCollapsedMarginTop());
     marginBox.setLeft(borderBox.left() - marginLeft());
-    marginBox.setHeight(borderBox.height() + m_nonCollapsedVertivalMargin.top + m_nonCollapsedVertivalMargin.bottom);
+    marginBox.setHeight(borderBox.height() + nonCollapsedMarginTop() + nonCollapsedMarginBottom());
     marginBox.setWidth(borderBox.width() + marginLeft() + marginRight());
     return marginBox;
 }
-#endif
 
 Box::Rect Box::borderBox() const
 {
index 52bd9aa..758023c 100644 (file)
@@ -131,6 +131,9 @@ public:
     LayoutUnit marginBottom() const;
     LayoutUnit marginRight() const;
 
+    LayoutUnit nonCollapsedMarginTop() const;
+    LayoutUnit nonCollapsedMarginBottom() const;
+
     LayoutUnit borderTop() const;
     LayoutUnit borderLeft() const;
     LayoutUnit borderBottom() const;
@@ -147,9 +150,8 @@ public:
     LayoutUnit contentBoxWidth() const;
 
     Rect marginBox() const;
-#ifndef NDEBUG
     Rect nonCollapsedMarginBox() const;
-#endif
+
     Rect borderBox() const;
     Rect paddingBox() const;
     Rect contentBox() const;
@@ -189,9 +191,8 @@ private:
 
     void setHorizontalMargin(HorizontalEdges);
     void setVerticalMargin(VerticalEdges);
-#ifndef NDEBUG
-    void setVerticalNonCollapsedMargin(VerticalEdges margin) {  m_nonCollapsedVertivalMargin = margin; }
-#endif
+    void setVerticalNonCollapsedMargin(VerticalEdges);
+
     void setBorder(Edges);
     void setPadding(Edges);
 
@@ -201,6 +202,7 @@ private:
     void invalidatePadding() { m_hasValidPadding = false; }
 
     void setHasValidVerticalMargin() { m_hasValidVerticalMargin = true; }
+    void setHasValidVerticalNonCollapsedMargin() { m_hasValidVerticalNonCollapsedMargin = true; }
     void setHasValidHorizontalMargin() { m_hasValidHorizontalMargin = true; }
 
     void setHasValidBorder() { m_hasValidBorder = true; }
@@ -217,15 +219,15 @@ private:
     LayoutUnit m_contentHeight;
 
     Edges m_margin;
-#ifndef NDEBUG
-    VerticalEdges m_nonCollapsedVertivalMargin;
-#endif
+    VerticalEdges m_verticalNonCollapsedMargin;
+
     Edges m_border;
     Edges m_padding;
 
 #if !ASSERT_DISABLED
     bool m_hasValidHorizontalMargin { false };
     bool m_hasValidVerticalMargin { false };
+    bool m_hasValidVerticalNonCollapsedMargin { false };
     bool m_hasValidBorder { false };
     bool m_hasValidPadding { false };
     bool m_hasValidContentHeight { false };
@@ -466,6 +468,14 @@ inline void Box::setVerticalMargin(VerticalEdges margin)
     m_margin.vertical = margin;
 }
 
+inline void Box::setVerticalNonCollapsedMargin(VerticalEdges margin)
+{
+#if !ASSERT_DISABLED
+    setHasValidVerticalNonCollapsedMargin();
+#endif
+    m_verticalNonCollapsedMargin = margin;
+}
+
 inline void Box::setBorder(Edges border)
 {
 #if !ASSERT_DISABLED
@@ -506,6 +516,18 @@ inline LayoutUnit Box::marginRight() const
     return m_margin.horizontal.right;
 }
 
+inline LayoutUnit Box::nonCollapsedMarginTop() const
+{
+    ASSERT(m_hasValidVerticalNonCollapsedMargin);
+    return m_verticalNonCollapsedMargin.top;
+}
+
+inline LayoutUnit Box::nonCollapsedMarginBottom() const
+{
+    ASSERT(m_hasValidVerticalNonCollapsedMargin);
+    return m_verticalNonCollapsedMargin.bottom;
+}
+
 inline LayoutUnit Box::paddingTop() const
 {
     ASSERT(m_hasValidPadding);