⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 266695 in webkit


Ignore:
Timestamp:
Sep 7, 2020, 2:20:46 AM (6 years ago)
Author:
svillar@igalia.com
Message:

[css-flexbox] min-height: auto not applied to nested flexboxes.
​https://bugs.webkit.org/show_bug.cgi?id=210089

Reviewed by Daniel Bates.

Source/WebCore:

Nested flexboxes with column direction were not computing correctly min-size:auto because
we were explicitly preventing them from doing so in the code. Implemented the required bits to
make it work correctly and thus removed the retriction. The idea is to set an indefinite override
containing block size so that percentages would be resolved to auto as spec'ed. The code which
decides whether to apply min-size:auto was refactored in the shouldApplyMinSizeAutoForChild() method.

In order not to cause regressions some other two additional changes were also implemented. First we
had to adjust childHasIntrinsicMainAxisSize() so that it also takes into account the cases where
shouldApplyMinSizeAutoForChild() is true and return true. Secondly we had to add an additional case
to mainAxisLengthIsDefinite() so that it returns false for column flows where the flexBasis is intrinsic.

Inspired by Blink's crrev.com/c/1641510, crrev.com/c/1269995 & crrev.com/c/1786297 by <​cbiesinger@chromium.org>

  • rendering/RenderFlexibleBox.cpp:

(WebCore::RenderFlexibleBox::shouldApplyMinSizeAutoForChild const): Refactored from
RenderFlexibleBox::adjustChildSizeForMinAndMax.
(WebCore::RenderFlexibleBox::mainAxisLengthIsDefinite const): Additional case for column flows.
(WebCore::RenderFlexibleBox::layoutFlexItems): Reset m_hasDefiniteHeight to Unknown after calling
constructFlexItem() because the latter might set now an override containing block height which basically
potentially makes any cached size value incorrect.
(WebCore::RenderFlexibleBox::adjustChildSizeForMinAndMax): Directly call shouldApplyMinSizeAutoForChild().
(WebCore::RenderFlexibleBox::constructFlexItem): Set an indefinite override containing block size for
children with percentage sizes so that they're resolved as auto.
(WebCore::RenderFlexibleBox::childHasIntrinsicMainAxisSize const): Return true for those cases where the
main axis length is indefinite and also when shouldApplyMinSizeAutoForChild().

  • rendering/RenderFlexibleBox.h:

LayoutTests:

Apart from enabling some tests we're removing a test which is now invalid as it was added
under the condition that we were not matching the specs wrt percentage height computation in
column flexboxes.

  • TestExpectations: Removed two test cases that are passing now.
  • fast/flexbox/nested-column-intrinsic-min-disabled-expected.html: Removed.
  • fast/flexbox/nested-column-intrinsic-min-disabled.html: Removed.
  • platform/ios/fast/forms/auto-fill-button/input-strong-password-auto-fill-button-expected.txt: Updated.
  • platform/mac/fast/forms/auto-fill-button/input-strong-password-auto-fill-button-expected.txt: Ditto.
Location:
trunk
Files:
2 deleted
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r266684 r266695  
     12020-09-02  Sergio Villar Senin  <svillar@igalia.com>
     2
     3        [css-flexbox] min-height: auto not applied to nested flexboxes.
     4        https://bugs.webkit.org/show_bug.cgi?id=210089
     5
     6        Reviewed by Daniel Bates.
     7
     8        Apart from enabling some tests we're removing a test which is now invalid as it was added
     9        under the condition that we were not matching the specs wrt percentage height computation in
     10        column flexboxes.
     11
     12        * TestExpectations: Removed two test cases that are passing now.
     13        * fast/flexbox/nested-column-intrinsic-min-disabled-expected.html: Removed.
     14        * fast/flexbox/nested-column-intrinsic-min-disabled.html: Removed.
     15        * platform/ios/fast/forms/auto-fill-button/input-strong-password-auto-fill-button-expected.txt: Updated.
     16        * platform/mac/fast/forms/auto-fill-button/input-strong-password-auto-fill-button-expected.txt: Ditto.
     17
    1182020-09-06  Chris Dumez  <cdumez@apple.com>
    219

  • trunk/LayoutTests/TestExpectations

    r266684 r266695  
    39363936webkit.org/b/209649 imported/w3c/web-platform-tests/css/css-flexbox/padding-overflow-crash.html [ ImageOnlyFailure ]
    39373937webkit.org/b/210077 imported/w3c/web-platform-tests/css/css-flexbox/flex-basis-010.html [ ImageOnlyFailure ]
    3938 webkit.org/b/210089 imported/w3c/web-platform-tests/css/css-flexbox/flex-minimum-height-flex-items-011.xht [ ImageOnlyFailure ]
    3939 webkit.org/b/210089 imported/w3c/web-platform-tests/css/css-flexbox/flex-minimum-height-flex-items-013.html [ ImageOnlyFailure ]
    39403938webkit.org/b/210093 imported/w3c/web-platform-tests/css/css-flexbox/select-element-zero-height-001.html [ ImageOnlyFailure ]
    39413939webkit.org/b/210093 imported/w3c/web-platform-tests/css/css-flexbox/select-element-zero-height-002.html [ ImageOnlyFailure ]

  • trunk/LayoutTests/platform/ios/fast/forms/auto-fill-button/input-strong-password-auto-fill-button-expected.txt

    r262716 r266695  
    4949layer at (654,57) size 20x12 scrollWidth 94 scrollHeight 26
    5050  RenderFlexibleBox {DIV} at (6,3) size 21x13
    51     RenderBlock {DIV} at (0,12) size 20x14 [color=#000000CC]
     51    RenderBlock {DIV} at (0,12) size 95x14 [color=#000000CC]
    5252      RenderText {#text} at (6,0) size 89x14
    5353        text run at (6,0) width 89: "Strong Password"
    … …  
    6363layer at (176,134) size 20x12 scrollWidth 94 scrollHeight 26
    6464  RenderFlexibleBox {DIV} at (6,47) size 21x13
    65     RenderBlock {DIV} at (0,12) size 20x14 [color=#000000CC]
     65    RenderBlock {DIV} at (0,12) size 95x14 [color=#000000CC]
    6666      RenderText {#text} at (6,0) size 89x14
    6767        text run at (6,0) width 89: "Strong Password"

  • trunk/LayoutTests/platform/mac/fast/forms/auto-fill-button/input-strong-password-auto-fill-button-expected.txt

    r232480 r266695  
    4949layer at (653,55) size 20x13 scrollWidth 94 scrollHeight 26
    5050  RenderFlexibleBox {DIV} at (3,3) size 20x13
    51     RenderBlock {DIV} at (0,13) size 20x13 [color=#000000CC]
     51    RenderBlock {DIV} at (0,13) size 95x13 [color=#000000CC]
    5252      RenderText {#text} at (6,0) size 89x13
    5353        text run at (6,0) width 89: "Strong Password"
    … …  
    6363layer at (177,133) size 20x13 scrollWidth 94 scrollHeight 26
    6464  RenderFlexibleBox {DIV} at (3,46) size 20x14
    65     RenderBlock {DIV} at (0,13) size 20x13 [color=#000000CC]
     65    RenderBlock {DIV} at (0,13) size 95x13 [color=#000000CC]
    6666      RenderText {#text} at (6,0) size 89x13
    6767        text run at (6,0) width 89: "Strong Password"

  • trunk/Source/WebCore/ChangeLog

    r266693 r266695  
     12020-09-02  Sergio Villar Senin  <svillar@igalia.com>
     2
     3        [css-flexbox] min-height: auto not applied to nested flexboxes.
     4        https://bugs.webkit.org/show_bug.cgi?id=210089
     5
     6        Reviewed by Daniel Bates.
     7
     8        Nested flexboxes with column direction were not computing correctly min-size:auto because
     9        we were explicitly preventing them from doing so in the code. Implemented the required bits to
     10        make it work correctly and thus removed the retriction. The idea is to set an indefinite override
     11        containing block size so that percentages would be resolved to auto as spec'ed. The code which
     12        decides whether to apply min-size:auto was refactored in the shouldApplyMinSizeAutoForChild() method.
     13
     14        In order not to cause regressions some other two additional changes were also implemented. First we
     15        had to adjust childHasIntrinsicMainAxisSize() so that it also takes into account the cases where
     16        shouldApplyMinSizeAutoForChild() is true and return true. Secondly we had to add an additional case
     17        to mainAxisLengthIsDefinite() so that it returns false for column flows where the flexBasis is intrinsic.
     18
     19        Inspired by Blink's crrev.com/c/1641510, crrev.com/c/1269995 & crrev.com/c/1786297 by <cbiesinger@chromium.org>
     20
     21        * rendering/RenderFlexibleBox.cpp:
     22        (WebCore::RenderFlexibleBox::shouldApplyMinSizeAutoForChild const): Refactored from
     23        RenderFlexibleBox::adjustChildSizeForMinAndMax.
     24        (WebCore::RenderFlexibleBox::mainAxisLengthIsDefinite const): Additional case for column flows.
     25        (WebCore::RenderFlexibleBox::layoutFlexItems): Reset m_hasDefiniteHeight to Unknown after calling
     26        constructFlexItem() because the latter might set now an override containing block height which basically
     27        potentially makes any cached size value incorrect.
     28        (WebCore::RenderFlexibleBox::adjustChildSizeForMinAndMax): Directly call shouldApplyMinSizeAutoForChild().
     29        (WebCore::RenderFlexibleBox::constructFlexItem): Set an indefinite override containing block size for
     30        children with percentage sizes so that they're resolved as auto.
     31        (WebCore::RenderFlexibleBox::childHasIntrinsicMainAxisSize const): Return true for those cases where the
     32        main axis length is indefinite and also when shouldApplyMinSizeAutoForChild().
     33        * rendering/RenderFlexibleBox.h:
     34
    1352020-09-06  Myles C. Maxfield  <mmaxfield@apple.com>
    236

  • trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp

    r266691 r266695  
    439439}
    440440
     441bool RenderFlexibleBox::shouldApplyMinSizeAutoForChild(const RenderBox& child) const
     442{
     443    // css-flexbox section 4.5
     444    auto minSize = isHorizontalFlow() ? child.style().minWidth() : child.style().minHeight();
     445    return minSize.isAuto() && mainAxisOverflowForChild(child) == Overflow::Visible;
     446}
     447
    441448Length RenderFlexibleBox::flexBasisForChild(const RenderBox& child) const
    442449{
    … …  
    797804    if (flexBasis.isAuto())
    798805        return false;
     806    if (isColumnFlow() && flexBasis.isIntrinsic())
     807        return false;
    799808    if (flexBasis.isPercentOrCalculated()) {
    800809        if (!isColumnFlow() || m_hasDefiniteHeight == SizeDefiniteness::Definite)
    … …  
    915924    }
    916925    m_reversedOrderIteratorForHitTesting.reverse();
     926
     927    // constructFlexItem() might set the override containing block height so any value cached for definiteness might be incorrect.
     928    m_hasDefiniteHeight = SizeDefiniteness::Unknown;
    917929   
    918930    const LayoutUnit lineBreakLength = mainAxisContentExtent(LayoutUnit::max());
    … …  
    11291141        return std::max(childSize, std::max(0_lu, computeMainAxisExtentForChild(child, MinSize, min).valueOr(childSize)));
    11301142   
    1131     if (!isFlexibleBoxImpl() && min.isAuto() && mainAxisOverflowForChild(child) == Overflow::Visible && !(isColumnFlow() && is<RenderFlexibleBox>(child))) {
    1132         // FIXME: For now, we do not handle min-height: auto for nested
    1133         // column flexboxes. We need to implement
    1134         // https://drafts.csswg.org/css-flexbox/#intrinsic-sizes before that
    1135         // produces reasonable results. Tracking bug: https://crbug.com/581553
    1136         // css-flexbox section 4.5
     1143    if (shouldApplyMinSizeAutoForChild(child)) {
    11371144        // FIXME: If the min value is expected to be valid here, we need to come up with a non optional version of computeMainAxisExtentForChild and
    11381145        // ensure it's valid through the virtual calls of computeIntrinsicLogicalContentHeightUsing.
    … …  
    12331240FlexItem RenderFlexibleBox::constructFlexItem(RenderBox& child, bool relayoutChildren)
    12341241{
    1235     // If this condition is true, then computeMainAxisExtentForChild will call
    1236     // child.intrinsicContentLogicalHeight() and
    1237     // child.scrollbarLogicalHeight(), so if the child has intrinsic
    1238     // min/max/preferred size, run layout on it now to make sure its logical
    1239     // height and scroll bars are up to date.
    1240     if (childHasIntrinsicMainAxisSize(child) && child.needsLayout()) {
    1241         child.clearOverrideContentSize();
    1242         child.setChildNeedsLayout(MarkOnlyThis);
    1243         child.layoutIfNeeded();
    1244         cacheChildMainSize(child);
    1245         relayoutChildren = false;
     1242    if (childHasIntrinsicMainAxisSize(child)) {
     1243        // If this condition is true, then computeMainAxisExtentForChild will call
     1244        // child.intrinsicContentLogicalHeight() and child.scrollbarLogicalHeight(),
     1245        // so if the child has intrinsic min/max/preferred size, run layout on it now to make sure
     1246        // its logical height and scroll bars are up to date.
     1247        updateBlockChildDirtyBitsBeforeLayout(relayoutChildren, child);
     1248        // Don't resolve percentages in children. This is especially important for the min-height calculation,
     1249        // where we want percentages to be treated as auto. For flex-basis itself, this is not a problem because
     1250        // by definition we have an indefinite flex basis here and thus percentages should not resolve.
     1251        if (child.needsLayout() || !m_intrinsicSizeAlongMainAxis.contains(&child)) {
     1252            if (isHorizontalWritingMode() == child.isHorizontalWritingMode())
     1253                child.setOverrideContainingBlockContentLogicalHeight(WTF::nullopt);
     1254            else
     1255                child.setOverrideContainingBlockContentLogicalWidth(WTF::nullopt);
     1256            child.clearOverrideContentSize();
     1257            child.setChildNeedsLayout(MarkOnlyThis);
     1258            child.layoutIfNeeded();
     1259            cacheChildMainSize(child);
     1260            relayoutChildren = false;
     1261            child.clearOverrideContainingBlockContentSize();
     1262        }
    12461263    }
    12471264   
    … …  
    15571574bool RenderFlexibleBox::childHasIntrinsicMainAxisSize(const RenderBox& child) const
    15581575{
    1559     bool result = false;
    1560     if (isHorizontalFlow() != child.style().isHorizontalWritingMode()) {
    1561         Length childFlexBasis = flexBasisForChild(child);
    1562         Length childMinSize = isHorizontalFlow() ? child.style().minWidth() : child.style().minHeight();
    1563         Length childMaxSize = isHorizontalFlow() ? child.style().maxWidth() : child.style().maxHeight();
    1564         if (childFlexBasis.isIntrinsic() || childMinSize.isIntrinsicOrAuto() || childMaxSize.isIntrinsic())
    1565             result = true;
    1566     }
    1567     return result;
     1576    if (isHorizontalFlow() == child.style().isHorizontalWritingMode())
     1577        return false;
     1578
     1579    Length childFlexBasis = flexBasisForChild(child);
     1580    Length childMinSize = isHorizontalFlow() ? child.style().minWidth() : child.style().minHeight();
     1581    Length childMaxSize = isHorizontalFlow() ? child.style().maxWidth() : child.style().maxHeight();
     1582    // FIXME: we must run mainAxisLengthIsDefinite() because it might end up calling computePercentageLogicalHeight()
     1583    // which has some side effects like calling addPercentHeightDescendant() for example so it is not possible to skip
     1584    // the call for example by moving it to the end of the conditional expression. This is error-prone and we should
     1585    // refactor computePercentageLogicalHeight() at some point so that it only computes stuff without those side effects.
     1586    if (!mainAxisLengthIsDefinite(child, childFlexBasis) || childMinSize.isIntrinsic() || childMaxSize.isIntrinsic())
     1587        return true;
     1588
     1589    if (shouldApplyMinSizeAutoForChild(child))
     1590        return true;
     1591
     1592    return false;
    15681593}
    15691594

  • trunk/Source/WebCore/rendering/RenderFlexibleBox.h

    r263792 r266695  
    114114    bool isMultiline() const;
    115115    Length flexBasisForChild(const RenderBox& child) const;
     116    bool shouldApplyMinSizeAutoForChild(const RenderBox&) const;
    116117    LayoutUnit crossAxisExtentForChild(const RenderBox& child) const;
    117118    LayoutUnit crossAxisIntrinsicExtentForChild(const RenderBox& child) const;
Note: See TracChangeset for help on using the changeset viewer.