Changeset 240436 in webkit

Timestamp:

Jan 24, 2019 9:15:46 AM (5 years ago)

Author:

Alan Bujtas

Message:

[LFC][BFC][MarginCollapsing] MarginCollapse::collapsedVerticalValues should not return computed non-collapsed values.
​https://bugs.webkit.org/show_bug.cgi?id=193768

Reviewed by Antti Koivisto.

When it comes to the actual used values it does not really matter, only from correctness point of view.
(This patch also moves some checks to their correct place.)

• layout/blockformatting/BlockMarginCollapse.cpp:

(WebCore::Layout::BlockFormattingContext::MarginCollapse::marginBeforeCollapsesWithPreviousSiblingMarginAfter):
(WebCore::Layout::BlockFormattingContext::MarginCollapse::marginBeforeCollapsesWithFirstInFlowChildMarginBefore):
(WebCore::Layout::BlockFormattingContext::MarginCollapse::marginAfterCollapsesWithLastInFlowChildMarginAfter):
(WebCore::Layout::BlockFormattingContext::MarginCollapse::positiveNegativeMarginBefore):
(WebCore::Layout::BlockFormattingContext::MarginCollapse::positiveNegativeMarginAfter):
(WebCore::Layout::BlockFormattingContext::MarginCollapse::collapsedVerticalValues):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• layout/blockformatting/BlockMarginCollapse.cpp (modified) (6 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-01-24  Zalan Bujtas  <zalan@apple.com>
+
+        [LFC][BFC][MarginCollapsing] MarginCollapse::collapsedVerticalValues should not return computed non-collapsed values.
+        https://bugs.webkit.org/show_bug.cgi?id=193768
+
+        Reviewed by Antti Koivisto.
+
+        When it comes to the actual used values it does not really matter, only from correctness point of view.
+        (This patch also moves some checks to their correct place.)
+
+        * layout/blockformatting/BlockMarginCollapse.cpp:
+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::marginBeforeCollapsesWithPreviousSiblingMarginAfter):
+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::marginBeforeCollapsesWithFirstInFlowChildMarginBefore):
+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::marginAfterCollapsesWithLastInFlowChildMarginAfter):
+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::positiveNegativeMarginBefore):
+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::positiveNegativeMarginAfter):
+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::collapsedVerticalValues):
+
 2019-01-23  Simon Fraser  <simon.fraser@apple.com>
 

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

@@ -157 +157 @@
     auto& previousInFlowSibling = *layoutBox.previousInFlowSibling();
 
+    if (Quirks::shouldIgnoreMarginAfter(layoutState, previousInFlowSibling))
+        return false;
+
     // Margins between a floated box and any other box do not collapse.
     if (layoutBox.isFloatingPositioned() || previousInFlowSibling.isFloatingPositioned())
@@ -201 +204 @@
 
     auto& firstInFlowChild = *downcast<Container>(layoutBox).firstInFlowChild();
+    if (Quirks::shouldIgnoreMarginBefore(layoutState, firstInFlowChild))
+        return false;
+
     if (!firstInFlowChild.isBlockLevelBox())
         return false;
@@ -321 +327 @@
 
     auto& lastInFlowChild = *downcast<Container>(layoutBox).lastInFlowChild();
+    if (Quirks::shouldIgnoreMarginAfter(layoutState, lastInFlowChild))
+        return false;
+
     if (!lastInFlowChild.isBlockLevelBox())
         return false;
@@ -536 +545 @@
 {
     auto firstChildCollapsedMarginBefore = [&]() -> PositiveAndNegativeVerticalMargin::Values {
-        if (!is<Container>(layoutBox))
-            return { };
-        auto* firstInFlowChild = downcast<Container>(layoutBox).firstInFlowChild();
-        if (!firstInFlowChild)
-            return { };
         if (!marginBeforeCollapsesWithFirstInFlowChildMarginBefore(layoutState, layoutBox))
             return { };
-        if (Quirks::shouldIgnoreMarginBefore(layoutState, *firstInFlowChild))
-            return { };
-        return positiveNegativeValues(layoutState, *firstInFlowChild, MarginType::Before);
+        return positiveNegativeValues(layoutState, *downcast<Container>(layoutBox).firstInFlowChild(), MarginType::Before);
     };
 
     auto previouSiblingCollapsedMarginAfter = [&]() -> PositiveAndNegativeVerticalMargin::Values {
-        auto* previousInFlowSibling = layoutBox.previousInFlowSibling();
-        if (!previousInFlowSibling)
-            return { };
         if (!marginBeforeCollapsesWithPreviousSiblingMarginAfter(layoutState, layoutBox))
             return { };
-        if (Quirks::shouldIgnoreMarginAfter(layoutState, *previousInFlowSibling))
-            return { };
-        return positiveNegativeValues(layoutState, *previousInFlowSibling, MarginType::After);
+        return positiveNegativeValues(layoutState, *downcast<Container>(layoutBox).previousInFlowSibling(), MarginType::After);
     };
 
@@ -569 +566 @@
 {
     auto lastChildCollapsedMarginAfter = [&]() -> PositiveAndNegativeVerticalMargin::Values {
-        if (!is<Container>(layoutBox))
-            return { };
-        auto* lastInFlowChild = downcast<Container>(layoutBox).lastInFlowChild();
-        if (!lastInFlowChild)
-            return { };
         if (!marginAfterCollapsesWithLastInFlowChildMarginAfter(layoutState, layoutBox))
             return { };
-        if (Quirks::shouldIgnoreMarginAfter(layoutState, *lastInFlowChild))
-            return { };
-        return positiveNegativeValues(layoutState, *lastInFlowChild, MarginType::After);
+        return positiveNegativeValues(layoutState, *downcast<Container>(layoutBox).lastInFlowChild(), MarginType::After);
     };
 
@@ -625 +615 @@
     // 2. Get min/max margin top values from the previous in-flow sibling, if we are collapsing margin top with it.
     // 3. Get this layout box's computed margin top value.
-    // 4. Update the min/max value and compute the final margin. 
+    // 4. Update the min/max value and compute the final margin.
     auto positiveNegativeMarginBefore = MarginCollapse::positiveNegativeMarginBefore(layoutState, layoutBox, nonCollapsedValues);
     auto positiveNegativeMarginAfter = MarginCollapse::positiveNegativeMarginAfter(layoutState, layoutBox, nonCollapsedValues);
 
-    auto marginsCollapseThrough = MarginCollapse::marginsCollapseThrough(layoutState, layoutBox); 
+    auto marginsCollapseThrough = MarginCollapse::marginsCollapseThrough(layoutState, layoutBox);
     if (marginsCollapseThrough) {
         positiveNegativeMarginBefore = computedPositiveAndNegativeMargin(positiveNegativeMarginBefore, positiveNegativeMarginAfter);
-        positiveNegativeMarginAfter = positiveNegativeMarginBefore;         
-    }
-
+        positiveNegativeMarginAfter = positiveNegativeMarginBefore;
+    }
+
+    // FIXME: Move state saving out of this function.
     auto& blockFormattingState = downcast<BlockFormattingState>(layoutState.formattingStateForBox(layoutBox));
     blockFormattingState.setPositiveAndNegativeVerticalMargin(layoutBox, { positiveNegativeMarginBefore, positiveNegativeMarginAfter });
 
-    return { marginValue(positiveNegativeMarginBefore), marginValue(positiveNegativeMarginAfter), marginsCollapseThrough }; 
+    auto beforeMarginIsCollapsedValue = marginBeforeCollapsesWithFirstInFlowChildMarginBefore(layoutState, layoutBox) || marginBeforeCollapsesWithPreviousSiblingMarginAfter(layoutState, layoutBox);
+    auto afterMarginIsCollapsedValue = marginAfterCollapsesWithLastInFlowChildMarginAfter(layoutState, layoutBox);
+
+    if ((beforeMarginIsCollapsedValue && afterMarginIsCollapsedValue) || marginsCollapseThrough)
+        return { marginValue(positiveNegativeMarginBefore), marginValue(positiveNegativeMarginAfter), marginsCollapseThrough };
+    if (beforeMarginIsCollapsedValue)
+        return { marginValue(positiveNegativeMarginBefore), { }, false };
+    return { { }, marginValue(positiveNegativeMarginAfter), false };
 }