Changeset 277222 in webkit

Timestamp:

May 7, 2021 6:48:22 PM (15 months ago)

Author:

Alan Bujtas

Message:

[css-flexbox] Flex item construction may affect sibling flex item height computation
​https://bugs.webkit.org/show_bug.cgi?id=225489

Reviewed by Sergio Villar Senin.

Source/WebCore:

Flex item construction triggers layout both on the flex item and on its descendants (see constructFlexItem).
During this layout a percent height descendant may indirectly set an incorrect value on the flex container's
m_hasDefiniteHeight (this is due to the odd way we choose to resolve percent height values on the ancestor chain,
see setOverridingContainingBlockContentLogicalHeight(WTF::nullopt)).
Now this incorrect m_hasDefiniteHeight value (Indefinite) causes the next sibling's (also) percent height
resolve fail as it tells the flex item that the flex container can't help with resolving the percent height value.
As the result we end up with an incorrect, 0px height value for this sibling.
e.g.
<div style="height: 100px; display: flex; flex-direction: column;">

<div id=flexItemA style="height: 50px;"><div style="height: 100%;"></div></div>
<div id=flexItemB style="height: 50%;"></div>

</div>
By the time we get to the construction of flexItemB, the RenderFlexibleBox's (flex container) m_hasDefiniteHeight
is already (and incorrectly) set to Indefinite as the result of us trying to resolve flexItemA's descendant height percentage.
This Indefinite value makes the flexItemB's height resolution fail as we believe that the flex container has indefinite height
e.g "height: auto", while it is clearly resolvable (50% of 100px).
Now if we flip the order and flexItemB comes first, the test case passes fine.
It is very unfortunate that some descendant height resolving process can trigger a state change on the ancestor RenderFlexibleBox, but
fixing it would certainly require some substantial architectural change (e.g. pushing down the constraints to the geometry functions instead of
walking the containing block chain).
In this patch, we just scope m_hasDefiniteHeight so that the RenderFlexibleBox's state is not effected by the flex item construction.

Test: fast/flexbox/flex-column-with-percent-height-descendants.html

• rendering/RenderFlexibleBox.cpp:

(WebCore::RenderFlexibleBox::layoutFlexItems):

LayoutTests:

• fast/flexbox/flex-column-with-percent-height-descendants-expected.html: Added.
• fast/flexbox/flex-column-with-percent-height-descendants.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants-expected.html (added)
• LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderFlexibleBox.cpp (modified) (5 diffs)
• Source/WebCore/rendering/RenderFlexibleBox.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-05-07  Zalan Bujtas  <zalan@apple.com>
+
+        [css-flexbox] Flex item construction may affect sibling flex item height computation
+        https://bugs.webkit.org/show_bug.cgi?id=225489
+
+        Reviewed by Sergio Villar Senin.
+
+        * fast/flexbox/flex-column-with-percent-height-descendants-expected.html: Added.
+        * fast/flexbox/flex-column-with-percent-height-descendants.html: Added.
+
 2021-05-07  Amir Mark Jr  <amir_mark@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-05-07  Zalan Bujtas  <zalan@apple.com>
+
+        [css-flexbox] Flex item construction may affect sibling flex item height computation
+        https://bugs.webkit.org/show_bug.cgi?id=225489
+
+        Reviewed by Sergio Villar Senin.
+
+        Flex item construction triggers layout both on the flex item and on its descendants (see constructFlexItem).
+        During this layout a percent height descendant may indirectly set an incorrect value on the flex container's 
+        m_hasDefiniteHeight (this is due to the odd way we choose to resolve percent height values on the ancestor chain, 
+        see setOverridingContainingBlockContentLogicalHeight(WTF::nullopt)).
+        Now this incorrect m_hasDefiniteHeight value (Indefinite) causes the next sibling's (also) percent height
+        resolve fail as it tells the flex item that the flex container can't help with resolving the percent height value.
+        As the result we end up with an incorrect, 0px height value for this sibling.
+        e.g.
+        <div style="height: 100px; display: flex; flex-direction: column;">
+          <div id=flexItemA style="height: 50px;"><div style="height: 100%;"></div></div>
+          <div id=flexItemB style="height: 50%;"></div>
+        </div>
+        By the time we get to the construction of flexItemB, the RenderFlexibleBox's (flex container) m_hasDefiniteHeight
+        is already (and incorrectly) set to Indefinite as the result of us trying to resolve flexItemA's descendant height percentage.
+        This Indefinite value makes the flexItemB's height resolution fail as we believe that the flex container has indefinite height
+        e.g "height: auto", while it is clearly resolvable (50% of 100px).
+        Now if we flip the order and flexItemB comes first, the test case passes fine.
+        It is very unfortunate that some descendant height resolving process can trigger a state change on the ancestor RenderFlexibleBox, but
+        fixing it would certainly require some substantial architectural change (e.g. pushing down the constraints to the geometry functions instead of
+        walking the containing block chain).
+        In this patch, we just scope m_hasDefiniteHeight so that the RenderFlexibleBox's state is not effected by the flex item construction. 
+
+        Test: fast/flexbox/flex-column-with-percent-height-descendants.html
+
+        * rendering/RenderFlexibleBox.cpp:
+        (WebCore::RenderFlexibleBox::layoutFlexItems):
+
 2021-05-07  Devin Rousso  <drousso@apple.com>
 

trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp

@@ -269 +269 @@
     m_relaidOutChildren.clear();
     
-    bool oldInLayout = m_inLayout;
-    m_inLayout = true;
-    
     if (recomputeLogicalWidth())
         relayoutChildren = true;
@@ -322 +319 @@
 
     clearNeedsLayout();
-    
-    m_inLayout = oldInLayout;
 }
 
@@ -846 +841 @@
         return false;
     bool definite = child.computePercentageLogicalHeight(flexBasis, updateDescendants).hasValue();
-    if (m_inLayout && (isHorizontalWritingMode() == child.isHorizontalWritingMode())) {
+    if (!m_inFlexItemConstruction && (isHorizontalWritingMode() == child.isHorizontalWritingMode())) {
         // We can reach this code even while we're not laying ourselves out, such
         // as from mainSizeForPercentageResolution.
@@ -966 +961 @@
     // should work off this list of a subset.
     // TODO(cbiesinger): That second part is not yet true.
-    Vector<FlexItem> allItems;
-    m_orderIterator.first();
-    for (RenderBox* child = m_orderIterator.currentChild(); child; child = m_orderIterator.next()) {
-        if (m_orderIterator.shouldSkipChild(*child)) {
-            // Out-of-flow children are not flex items, so we skip them here.
-            if (child->isOutOfFlowPositioned())
-                prepareChildForPositionedLayout(*child);
-            continue;
-        }
-        allItems.append(constructFlexItem(*child, relayoutChildren));
-    }
-
-    // constructFlexItem() might set the override containing block height so any value cached for definiteness might be incorrect.
-    m_hasDefiniteHeight = SizeDefiniteness::Unknown;
-    
+    auto allItems = constructFlexItems(relayoutChildren);
     const LayoutUnit lineBreakLength = mainAxisContentExtent(LayoutUnit::max());
     LayoutUnit gapBetweenItems = computeGap(GapType::BetweenItems);
@@ -1295 +1276 @@
 }
 
-FlexItem RenderFlexibleBox::constructFlexItem(RenderBox& child, bool relayoutChildren)
-{
-    child.clearOverridingContentSize();
-    if (childHasIntrinsicMainAxisSize(child)) {
-        // If this condition is true, then computeMainAxisExtentForChild will call
-        // child.intrinsicContentLogicalHeight() and child.scrollbarLogicalHeight(),
-        // so if the child has intrinsic min/max/preferred size, run layout on it now to make sure
-        // its logical height and scroll bars are up to date.
-        updateBlockChildDirtyBitsBeforeLayout(relayoutChildren, child);
-        // Don't resolve percentages in children. This is especially important for the min-height calculation,
-        // where we want percentages to be treated as auto. For flex-basis itself, this is not a problem because
-        // by definition we have an indefinite flex basis here and thus percentages should not resolve.
-        if (child.needsLayout() || !m_intrinsicSizeAlongMainAxis.contains(&child)) {
-            if (isHorizontalWritingMode() == child.isHorizontalWritingMode())
-                child.setOverridingContainingBlockContentLogicalHeight(WTF::nullopt);
-            else
-                child.setOverridingContainingBlockContentLogicalWidth(WTF::nullopt);
-            child.clearOverridingContentSize();
-            child.setChildNeedsLayout(MarkOnlyThis);
-            child.layoutIfNeeded();
-            cacheChildMainSize(child);
-            child.clearOverridingContainingBlockContentSize();
-        }
-    }
-    
-    LayoutUnit borderAndPadding = isHorizontalFlow() ? child.horizontalBorderAndPaddingExtent() : child.verticalBorderAndPaddingExtent();
-    LayoutUnit childInnerFlexBaseSize = computeInnerFlexBaseSizeForChild(child, borderAndPadding);
-    LayoutUnit childMinMaxAppliedMainAxisExtent = adjustChildSizeForMinAndMax(child, childInnerFlexBaseSize);
-    LayoutUnit margin = isHorizontalFlow() ? child.horizontalMarginExtent() : child.verticalMarginExtent();
-    return FlexItem(child, childInnerFlexBaseSize, childMinMaxAppliedMainAxisExtent, borderAndPadding, margin);
+Vector<FlexItem> RenderFlexibleBox::constructFlexItems(bool relayoutChildren)
+{
+    SetForScope<bool> inFlexItemConstruction(m_inFlexItemConstruction, true);
+
+    Vector<FlexItem> flexItems;
+    for (auto* child = m_orderIterator.first(); child; child = m_orderIterator.next()) {
+        if (m_orderIterator.shouldSkipChild(*child)) {
+            // Out-of-flow children are not flex items, so we skip them here.
+            if (child->isOutOfFlowPositioned())
+                prepareChildForPositionedLayout(*child);
+            continue;
+        }
+
+        auto constructFlexItemForChildBox = [&](auto& childBox) {
+            childBox.clearOverridingContentSize();
+            if (childHasIntrinsicMainAxisSize(childBox)) {
+                // If this condition is true, then computeMainAxisExtentForChild will call
+                // child.intrinsicContentLogicalHeight() and child.scrollbarLogicalHeight(),
+                // so if the child has intrinsic min/max/preferred size, run layout on it now to make sure
+                // its logical height and scroll bars are up to date.
+                updateBlockChildDirtyBitsBeforeLayout(relayoutChildren, childBox);
+                // Don't resolve percentages in children. This is especially important for the min-height calculation,
+                // where we want percentages to be treated as auto. For flex-basis itself, this is not a problem because
+                // by definition we have an indefinite flex basis here and thus percentages should not resolve.
+                if (childBox.needsLayout() || !m_intrinsicSizeAlongMainAxis.contains(&childBox)) {
+                    if (isHorizontalWritingMode() == childBox.isHorizontalWritingMode())
+                        childBox.setOverridingContainingBlockContentLogicalHeight(WTF::nullopt);
+                    else
+                        childBox.setOverridingContainingBlockContentLogicalWidth(WTF::nullopt);
+                    childBox.clearOverridingContentSize();
+                    childBox.setChildNeedsLayout(MarkOnlyThis);
+                    childBox.layoutIfNeeded();
+                    cacheChildMainSize(childBox);
+                    childBox.clearOverridingContainingBlockContentSize();
+                }
+            }
+
+            auto borderAndPadding = isHorizontalFlow() ? childBox.horizontalBorderAndPaddingExtent() : childBox.verticalBorderAndPaddingExtent();
+            auto childInnerFlexBaseSize = computeInnerFlexBaseSizeForChild(childBox, borderAndPadding);
+            auto childMinMaxAppliedMainAxisExtent = adjustChildSizeForMinAndMax(childBox, childInnerFlexBaseSize);
+            auto margin = isHorizontalFlow() ? childBox.horizontalMarginExtent() : childBox.verticalMarginExtent();
+            return FlexItem(childBox, childInnerFlexBaseSize, childMinMaxAppliedMainAxisExtent, borderAndPadding, margin);
+        };
+        flexItems.append(constructFlexItemForChildBox(*child));
+    }
+    return flexItems;
 }
     

trunk/Source/WebCore/rendering/RenderFlexibleBox.h

@@ -176 +176 @@
     LayoutUnit adjustChildSizeForMinAndMax(RenderBox& child, LayoutUnit childSize);
     LayoutUnit adjustChildSizeForAspectRatioCrossAxisMinAndMax(const RenderBox& child, LayoutUnit childSize);
-    FlexItem constructFlexItem(RenderBox&, bool relayoutChildren);
+    Vector<FlexItem> constructFlexItems(bool relayoutChildren);
     
     void freezeInflexibleItems(FlexSign, Vector<FlexItem>& children, LayoutUnit& remainingFreeSpace, double& totalFlexGrow, double& totalFlexShrink, double& totalWeightedFlexShrink);
@@ -221 +221 @@
     // This is SizeIsUnknown outside of layoutBlock()
     SizeDefiniteness m_hasDefiniteHeight { SizeDefiniteness::Unknown };
-    bool m_inLayout { false };
+    bool m_inFlexItemConstruction { false };
     bool m_shouldResetChildLogicalHeightBeforeLayout { false };
 };