Changeset 279271 in webkit

Timestamp:

Jun 25, 2021 1:59:13 AM (13 months ago)

Author:

svillar@igalia.com

Message:

[css-flexbox] Do not clamp flex base size with {min|max}-{height|width}
​https://bugs.webkit.org/show_bug.cgi?id=225590

Reviewed by Alan Bujtas.

Source/WebCore:

When computing flex base size we should not clamp it with neither min-{height|width}
nor max-{height|width}. That would be done later and will be called the hypothetical
main size.

For most of the cases we were already doing it, however there are some particular cases
in which we have to call the generic layout code which does clamp the sizes using min
and max sizes. In order to make those code paths work, we reset the min|max values to
their initial ones (so they effectively become inactive) and then we set the original
values back after computing the base size.

• rendering/RenderFlexibleBox.cpp:

(WebCore::ScopedUnboundedBox::ScopedUnboundedBox): New scoped class that sets min|max sizes
to their initial values on instantiation and restores the original values on destruction.
(WebCore::ScopedUnboundedBox::~ScopedUnboundedBox):
(WebCore::RenderFlexibleBox::constructFlexItems): Wrap the base size computation with a
ScopedUnboundedBox.

LayoutTests:

The patch allows us to pass 3 new tests. We're adding percentage-max-height-003.html to
the list of expected failures because these changes make it fail. This is not really a
regression however because although the size of the deepest flex item was correct (and thus
allowed us to pass the test) the other sizes were completely wrong. So it's more an issue
of the test not being complete enough and passing artificially than anything else.

• TestExpectations: Unskipped 3 tests & skipped 1.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/TestExpectations (modified) (3 diffs)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderFlexibleBox.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-06-14  Sergio Villar Senin  <svillar@igalia.com>
+
+        [css-flexbox] Do not clamp flex base size with {min|max}-{height|width}
+        https://bugs.webkit.org/show_bug.cgi?id=225590
+
+        Reviewed by Alan Bujtas.
+
+        The patch allows us to pass 3 new tests. We're adding percentage-max-height-003.html to
+        the list of expected failures because these changes make it fail. This is not really a
+        regression however because although the size of the deepest flex item was correct (and thus
+        allowed us to pass the test) the other sizes were completely wrong. So it's more an issue
+        of the test not being complete enough and passing artificially than anything else.
+
+        * TestExpectations: Unskipped 3 tests & skipped 1.
+
 2021-06-25  Arcady Goldmints-Orlov  <agoldmints@igalia.com>
 

trunk/LayoutTests/TestExpectations

@@ -1218 +1218 @@
 webkit.org/b/136754 imported/w3c/web-platform-tests/css/css-flexbox/flexbox-safe-overflow-position-001.html [ ImageOnlyFailure ]
 webkit.org/b/136754 imported/w3c/web-platform-tests/css/css-flexbox/percentage-max-height-002.html [ ImageOnlyFailure ]
+webkit.org/b/136754 imported/w3c/web-platform-tests/css/css-flexbox/percentage-max-height-003.html [ ImageOnlyFailure ]
 webkit.org/b/210243 imported/w3c/web-platform-tests/css/css-flexbox/percentage-size-quirks-002.html [ Failure ]
 webkit.org/b/136754 imported/w3c/web-platform-tests/css/css-flexbox/scrollbars-no-margin.html [ ImageOnlyFailure ]
@@ -3950 +3951 @@
 webkit.org/b/209080 imported/w3c/web-platform-tests/css/css-writing-modes/vertical-alignment-srl-040.xht [ ImageOnlyFailure ]
 
-webkit.org/b/219343 imported/w3c/web-platform-tests/css/css-flexbox/flex-aspect-ratio-img-column-012.html [ ImageOnlyFailure ]
 webkit.org/b/219343 imported/w3c/web-platform-tests/css/css-flexbox/flex-aspect-ratio-img-column-015.html [ ImageOnlyFailure ]
-webkit.org/b/219343 imported/w3c/web-platform-tests/css/css-flexbox/flex-aspect-ratio-img-row-007.html [ ImageOnlyFailure ]
 webkit.org/b/219343 imported/w3c/web-platform-tests/css/css-flexbox/flex-aspect-ratio-img-row-010.html [ ImageOnlyFailure ]
 webkit.org/b/219343 imported/w3c/web-platform-tests/css/css-flexbox/aspect-ratio-intrinsic-size-001.html [ ImageOnlyFailure ]
@@ -4024 +4023 @@
 webkit.org/b/221473 imported/w3c/web-platform-tests/css/css-flexbox/table-as-item-inflexible-in-column-2.html [ ImageOnlyFailure ]
 webkit.org/b/221473 imported/w3c/web-platform-tests/css/css-flexbox/table-as-item-inflexible-in-row-2.html [ ImageOnlyFailure ]
-webkit.org/b/221473 imported/w3c/web-platform-tests/css/css-flexbox/table-as-item-min-height-1.html [ ImageOnlyFailure ]
 
 # SVGs as flex items.

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-06-14  Sergio Villar Senin  <svillar@igalia.com>
+
+        [css-flexbox] Do not clamp flex base size with {min|max}-{height|width}
+        https://bugs.webkit.org/show_bug.cgi?id=225590
+
+        Reviewed by Alan Bujtas.
+
+        When computing flex base size we should not clamp it with neither min-{height|width}
+        nor max-{height|width}. That would be done later and will be called the hypothetical
+        main size.
+
+        For most of the cases we were already doing it, however there are some particular cases
+        in which we have to call the generic layout code which does clamp the sizes using min
+        and max sizes. In order to make those code paths work, we reset the min|max values to
+        their initial ones (so they effectively become inactive) and then we set the original
+        values back after computing the base size.
+
+        * rendering/RenderFlexibleBox.cpp:
+        (WebCore::ScopedUnboundedBox::ScopedUnboundedBox): New scoped class that sets min|max sizes
+        to their initial values on instantiation and restores the original values on destruction.
+        (WebCore::ScopedUnboundedBox::~ScopedUnboundedBox):
+        (WebCore::RenderFlexibleBox::constructFlexItems): Wrap the base size computation with a
+        ScopedUnboundedBox.
+
 2021-06-09  Sergio Villar Senin  <svillar@igalia.com>
 

trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp

@@ -1371 +1371 @@
 }
 
+// A scoped class that resets either min-width & max-width or min-height & max-height (depending on the
+// flex container main size) to their initial values on instantiation. It later resets them back to their
+// original values on destruction. It's used when computing the flex base sizes of flex items as "when determining 
+// the flex base size, the item’s min and max main sizes are ignored (no clamping occurs)". See
+// https://drafts.csswg.org/css-flexbox/#line-sizing
+class ScopedUnboundedBox {
+WTF_MAKE_NONCOPYABLE(ScopedUnboundedBox);
+public:
+    ScopedUnboundedBox(RenderBox& box, bool mainAxisIsChildInlineAxis)
+        : m_box(box)
+        , m_isHorizontalWritingModeInParallelFlow(mainAxisIsChildInlineAxis == box.isHorizontalWritingMode())
+    {
+        m_originalMin = m_isHorizontalWritingModeInParallelFlow ? m_box.style().minWidth() : m_box.style().minHeight();
+        m_originalMax = m_isHorizontalWritingModeInParallelFlow ? m_box.style().maxWidth() : m_box.style().maxHeight();
+        if (m_isHorizontalWritingModeInParallelFlow) {
+            m_box.mutableStyle().setMinWidth(RenderStyle::initialMinSize());
+            m_box.mutableStyle().setMaxWidth(RenderStyle::initialMaxSize());
+            return;
+        }
+        m_box.mutableStyle().setMinHeight(RenderStyle::initialMinSize());
+        m_box.mutableStyle().setMaxHeight(RenderStyle::initialMaxSize());
+    }
+
+
+    ~ScopedUnboundedBox()
+    {
+        if (m_isHorizontalWritingModeInParallelFlow) {
+            m_box.mutableStyle().setMinWidth(WTFMove(m_originalMin));
+            m_box.mutableStyle().setMaxWidth(WTFMove(m_originalMax));
+            return;
+        }
+        m_box.mutableStyle().setMinHeight(WTFMove(m_originalMin));
+        m_box.mutableStyle().setMaxHeight(WTFMove(m_originalMax));
+    }
+
+private:
+    RenderBox& m_box;
+    bool m_isHorizontalWritingModeInParallelFlow;
+    Length m_originalMin;
+    Length m_originalMax;
+};
+
 FlexItem RenderFlexibleBox::constructFlexItem(RenderBox& child, bool relayoutChildren)
 {
     auto childHadLayout = child.everHadLayout();
-    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(std::nullopt);
-            else
-                child.setOverridingContainingBlockContentLogicalWidth(std::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 childInnerFlexBaseSize;
+    {
+        ScopedUnboundedBox unbounded(child, mainAxisIsChildInlineAxis(child));
+        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(std::nullopt);
+                else
+                    child.setOverridingContainingBlockContentLogicalWidth(std::nullopt);
+                child.clearOverridingContentSize();
+                child.setChildNeedsLayout(MarkOnlyThis);
+                child.layoutIfNeeded();
+                cacheChildMainSize(child);
+                child.clearOverridingContainingBlockContentSize();
+            }
+        }
+        childInnerFlexBaseSize = computeInnerFlexBaseSizeForChild(child, borderAndPadding);
+    }
+    
     LayoutUnit margin = isHorizontalFlow() ? child.horizontalMarginExtent() : child.verticalMarginExtent();
     return FlexItem(child, childInnerFlexBaseSize, borderAndPadding, margin, computeFlexItemMinMaxSizes(child), childHadLayout);