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

Changeset 284359 in webkit


Ignore:
Timestamp:
Oct 18, 2021, 3:07:52 AM (5 years ago)
Author:
svillar@igalia.com
Message:

[css-flexbox] Improve & simplify the flex-basis computation
​https://bugs.webkit.org/show_bug.cgi?id=230755

Reviewed by Manuel Rego Casasnovas.

Source/WebCore:

The flex-basis computation code was a bit convoluted. It had some pre-computations for items
with intrinsic main size that were causing several issues due to reentrancy. Actually those
computations where not needed at all for the flex basis computation but for a later stage, the
computation of the hyphotetical main size in which we need to compute 'min-{width|height}: auto'.

That's why the code that was executed before the flex-basis computation is now part of
computeFlexItemMinMaxSizes(). As we are no longer doing a layout before computing the flex-basis,
a layout has to be added to those cases in which the main size is the block size of the child. Apart
from that the flex-basis computation uses a newly defined RAII class to set the main size of the item
to the value specified by flex-basis which is what the specs mandate.

Last but not least, the computeInnerFlexBaseSizeForChild() method was renamed to computeFlexBaseSizeForChild()
which fits better with the terminology used in the specs.

Flex basis computation is already covered by the WPT test suite, there is no need for extra tests. This patch
fixes the only flex-basis-* test case that was not passing.

  • rendering/RenderFlexibleBox.cpp:

(WebCore::RenderFlexibleBox::computeMainAxisExtentForChild): Removed obsolete comment. Added
a layoutIfNeeded() as we cannot be sure that the item was already laid out any more.
(WebCore::ScopedFlexBasisAsMainSize::ScopedFlexBasisAsChildMainSize): New RAII class.
(WebCore::ScopedFlexBasisAsMainSize::~ScopedFlexBasisAsChildMainSize):
(WebCore::RenderFlexibleBox::computeFlexBaseSizeForChild): Renamed from computeInnerFlexBaseSizeForChild.
(WebCore::RenderFlexibleBox::computeFlexItemMinMaxSizes): Added relayoutChildren parameter.
(WebCore::RenderFlexibleBox::constructFlexItem): Moved code to computeFlexItemMinMaxSizes().
(WebCore::RenderFlexibleBox::layoutAndPlaceChildren):
(WebCore::RenderFlexibleBox::computeInnerFlexBaseSizeForChild): Deleted.

  • rendering/RenderFlexibleBox.h:

LayoutTests:

Location:
trunk
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r284347 r284359  
     12021-09-24  Sergio Villar Senin  <svillar@igalia.com>
     2
     3        [css-flexbox] Improve & simplify the flex-basis computation
     4        https://bugs.webkit.org/show_bug.cgi?id=230755
     5
     6        Reviewed by Manuel Rego Casasnovas.
     7
     8        * TestExpectations: Unskipped flex-basis-011.html which is now working fine.
     9
    1102021-10-17  Dean Jackson  <dino@apple.com>
    211

  • trunk/LayoutTests/TestExpectations

    r284343 r284359  
    42204220webkit.org/b/221479 imported/w3c/web-platform-tests/css/css-flexbox/flexbox-flex-basis-content-004a.html [ ImageOnlyFailure ]
    42214221
    4222 # flex-basis with %.
    4223 webkit.org/b/ imported/w3c/web-platform-tests/css/css-flexbox/flex-basis-011.html [ ImageOnlyFailure ]
    4224 
    42254222# Flex item's min|max content contributions
    42264223webkit.org/b/230747 imported/w3c/web-platform-tests/css/css-flexbox/flex-container-max-content-001.html [ ImageOnlyFailure ]

  • trunk/Source/WebCore/ChangeLog

    r284358 r284359  
     12021-09-24  Sergio Villar Senin  <svillar@igalia.com>
     2
     3        [css-flexbox] Improve & simplify the flex-basis computation
     4        https://bugs.webkit.org/show_bug.cgi?id=230755
     5
     6        Reviewed by Manuel Rego Casasnovas.
     7
     8        The flex-basis computation code was a bit convoluted. It had some pre-computations for items
     9        with intrinsic main size that were causing several issues due to reentrancy. Actually those
     10        computations where not needed at all for the flex basis computation but for a later stage, the
     11        computation of the hyphotetical main size in which we need to compute 'min-{width|height}: auto'.
     12
     13        That's why the code that was executed before the flex-basis computation is now part of
     14        computeFlexItemMinMaxSizes(). As we are no longer doing a layout before computing the flex-basis,
     15        a layout has to be added to those cases in which the main size is the block size of the child. Apart
     16        from that the flex-basis computation uses a newly defined RAII class to set the main size of the item
     17        to the value specified by flex-basis which is what the specs mandate.
     18
     19        Last but not least, the computeInnerFlexBaseSizeForChild() method was renamed to computeFlexBaseSizeForChild()
     20        which fits better with the terminology used in the specs.
     21
     22        Flex basis computation is already covered by the WPT test suite, there is no need for extra tests. This patch
     23        fixes the only flex-basis-* test case that was not passing.
     24
     25        * rendering/RenderFlexibleBox.cpp:
     26        (WebCore::RenderFlexibleBox::computeMainAxisExtentForChild): Removed obsolete comment. Added
     27        a layoutIfNeeded() as we cannot be sure that the item was already laid out any more.
     28        (WebCore::ScopedFlexBasisAsMainSize::ScopedFlexBasisAsChildMainSize): New RAII class.
     29        (WebCore::ScopedFlexBasisAsMainSize::~ScopedFlexBasisAsChildMainSize):
     30        (WebCore::RenderFlexibleBox::computeFlexBaseSizeForChild): Renamed from computeInnerFlexBaseSizeForChild.
     31        (WebCore::RenderFlexibleBox::computeFlexItemMinMaxSizes): Added relayoutChildren parameter.
     32        (WebCore::RenderFlexibleBox::constructFlexItem): Moved code to computeFlexItemMinMaxSizes().
     33        (WebCore::RenderFlexibleBox::layoutAndPlaceChildren):
     34        (WebCore::RenderFlexibleBox::computeInnerFlexBaseSizeForChild): Deleted.
     35        * rendering/RenderFlexibleBox.h:
     36
    1372021-10-18  Philippe Normand  <pnormand@igalia.com>
    238

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

    r282463 r284359  
    3535#include "HitTestResult.h"
    3636#include "LayoutRepainter.h"
     37#include "RenderBox.h"
    3738#include "RenderChildIterator.h"
    3839#include "RenderLayer.h"
    3940#include "RenderLayoutState.h"
     41#include "RenderObjectEnums.h"
    4042#include "RenderStyleConstants.h"
    4143#include "RenderView.h"
    … …  
    629631    // writing mode. Otherwise we need the logical height.
    630632    if (!mainAxisIsChildInlineAxis(child)) {
    631         // We don't have to check for "auto" here - computeContentLogicalHeight
    632         // will just return a null Optional for that case anyway. It's safe to access
    633         // scrollbarLogicalHeight here because ComputeNextFlexLine will have
    634         // already forced layout on the child. We previously did a layout out the child
    635         // if necessary (see ComputeNextFlexLine and the call to
    636         // childHasIntrinsicMainAxisSize) so we can be sure that the two height
    637         // calls here will return up-to-date data.
     633        child.layoutIfNeeded();
    638634        std::optional<LayoutUnit> height = child.computeContentLogicalHeight(sizeType, size, cachedChildIntrinsicContentLogicalHeight(child));
    639635        if (!height)
    … …  
    1001997}
    1002998
    1003 LayoutUnit RenderFlexibleBox::computeInnerFlexBaseSizeForChild(RenderBox& child, LayoutUnit mainAxisBorderAndPadding)
     999// This is a RAII class that is used to temporarily set the flex basis as the child size in the main axis.
     1000class ScopedFlexBasisAsChildMainSize {
     1001public:
     1002    ScopedFlexBasisAsChildMainSize(RenderBox& child, Length flexBasis, bool mainAxisIsInlineAxis)
     1003        : m_child(child)
     1004        , m_mainAxisIsInlineAxis(mainAxisIsInlineAxis)
     1005    {
     1006        if (m_mainAxisIsInlineAxis) {
     1007            m_originalLength = m_child.style().logicalWidth();
     1008            m_child.mutableStyle().setLogicalWidth(Length(flexBasis));
     1009            return;
     1010        }
     1011        m_originalLength = m_child.style().logicalHeight();
     1012        m_child.mutableStyle().setLogicalHeight(Length(flexBasis));
     1013    }
     1014    ~ScopedFlexBasisAsChildMainSize()
     1015    {
     1016        if (m_mainAxisIsInlineAxis)
     1017            m_child.mutableStyle().setLogicalWidth(Length(m_originalLength));
     1018        else
     1019            m_child.mutableStyle().setLogicalHeight(Length(m_originalLength));
     1020    }
     1021private:
     1022    RenderBox& m_child;
     1023    bool m_mainAxisIsInlineAxis;
     1024    Length m_originalLength;
     1025};
     1026
     1027// https://drafts.csswg.org/css-flexbox/#algo-main-item
     1028LayoutUnit RenderFlexibleBox::computeFlexBaseSizeForChild(RenderBox& child, LayoutUnit mainAxisBorderAndPadding)
    10041029{
    10051030    Length flexBasis = flexBasisForChild(child);
     1031    // 9.3.2 A.
    10061032    if (childMainSizeIsDefinite(child, flexBasis))
    10071033        return std::max(0_lu, computeMainAxisExtentForChild(child, MainOrPreferredSize, flexBasis).value());
    10081034
     1035    // 9.3.2 B.
    10091036    if (childHasComputableAspectRatioAndCrossSizeIsConsideredDefinite(child)) {
    10101037        const Length& crossSizeLength = crossSizeLengthForChild(MainOrPreferredSize, child);
    … …  
    10121039    }
    10131040
    1014     // The flex basis is indefinite (=auto), so we need to compute the actual width of the child.
    1015     LayoutUnit mainAxisExtent;
    1016     if (!mainAxisIsChildInlineAxis(child)) {
    1017         ASSERT(!child.needsLayout());
    1018         ASSERT(m_intrinsicSizeAlongMainAxis.contains(&child));
    1019         mainAxisExtent = m_intrinsicSizeAlongMainAxis.get(&child);
    1020     } else {
    1021         // We don't need to add scrollbarLogicalWidth here because the preferred
    1022         // width includes the scrollbar, even for overflow: auto.
    1023         mainAxisExtent = child.maxPreferredLogicalWidth();
    1024     }
    1025     return mainAxisExtent - mainAxisBorderAndPadding;
     1041    // FIXME: implement 9.3.2 C.
     1042    // FIXME: implement 9.3.2 D.
     1043
     1044    // 9.3.2 E. Otherwise, size the item into the available space using its used flex basis in place of its main size.
     1045    {
     1046        ScopedFlexBasisAsChildMainSize flexBasisScope(child, flexBasis, mainAxisIsChildInlineAxis(child));
     1047        if (mainAxisIsChildInlineAxis(child))
     1048            return child.maxPreferredLogicalWidth() - mainAxisBorderAndPadding;
     1049
     1050        if (childHasIntrinsicMainAxisSize(child))
     1051            child.setNeedsLayout(MarkOnlyThis);
     1052        child.layoutIfNeeded();
     1053        return child.logicalHeight() - mainAxisBorderAndPadding;
     1054    }
    10261055}
    10271056
    … …  
    12611290}
    12621291
    1263 std::pair<LayoutUnit, LayoutUnit> RenderFlexibleBox::computeFlexItemMinMaxSizes(RenderBox& child)
     1292std::pair<LayoutUnit, LayoutUnit> RenderFlexibleBox::computeFlexItemMinMaxSizes(RenderBox& child, bool relayoutChildren)
    12641293{
    12651294    Length max = mainSizeLengthForChild(MaxSize, child);
    … …  
    12801309        if (child.isRenderReplaced() && childHasComputableAspectRatio(child) && childCrossSizeIsDefinite(child, childCrossSizeLength))
    12811310            contentSize = computeMainSizeFromAspectRatioUsing(child, childCrossSizeLength);
    1282         else
     1311        else {
     1312            if (childHasIntrinsicMainAxisSize(child)) {
     1313                // If this condition is true, then computeMainAxisExtentForChild will call
     1314                // child.intrinsicContentLogicalHeight() and child.scrollbarLogicalHeight(),
     1315                // so if the child has intrinsic min/max/preferred size, run layout on it now to make sure
     1316                // its logical height and scroll bars are up to date.
     1317                updateBlockChildDirtyBitsBeforeLayout(relayoutChildren, child);
     1318                // Don't resolve percentages in children. This is especially important for the min-height calculation,
     1319                // where we want percentages to be treated as auto.
     1320                if (child.needsLayout() || !m_intrinsicSizeAlongMainAxis.contains(&child)) {
     1321                    if (isHorizontalWritingMode() == child.isHorizontalWritingMode())
     1322                        child.setOverridingContainingBlockContentLogicalHeight(std::nullopt);
     1323                    else
     1324                        child.setOverridingContainingBlockContentLogicalWidth(std::nullopt);
     1325
     1326                    child.clearOverridingContentSize();
     1327                    child.setChildNeedsLayout(MarkOnlyThis);
     1328                    child.layoutIfNeeded();
     1329                    cacheChildMainSize(child);
     1330                    child.clearOverridingContainingBlockContentSize();
     1331                }
     1332            }
    12831333            contentSize = computeMainAxisExtentForChild(child, MinSize, Length(LengthType::MinContent)).value_or(0);
     1334        }
    12841335        if (child.hasIntrinsicAspectRatio() && child.intrinsicSize().height())
    12851336            contentSize = adjustChildSizeForAspectRatioCrossAxisMinAndMax(child, contentSize);
    … …  
    13681419    auto childHadLayout = child.everHadLayout();
    13691420    child.clearOverridingContentSize();
    1370     if (childHasIntrinsicMainAxisSize(child)) {
    1371         // If this condition is true, then computeMainAxisExtentForChild will call
    1372         // child.intrinsicContentLogicalHeight() and child.scrollbarLogicalHeight(),
    1373         // so if the child has intrinsic min/max/preferred size, run layout on it now to make sure
    1374         // its logical height and scroll bars are up to date.
    1375         updateBlockChildDirtyBitsBeforeLayout(relayoutChildren, child);
    1376         // Don't resolve percentages in children. This is especially important for the min-height calculation,
    1377         // where we want percentages to be treated as auto. For flex-basis itself, this is not a problem because
    1378         // by definition we have an indefinite flex basis here and thus percentages should not resolve.
    1379         if (child.needsLayout() || !m_intrinsicSizeAlongMainAxis.contains(&child)) {
    1380             if (isHorizontalWritingMode() == child.isHorizontalWritingMode())
    1381                 child.setOverridingContainingBlockContentLogicalHeight(std::nullopt);
    1382             else
    1383                 child.setOverridingContainingBlockContentLogicalWidth(std::nullopt);
    1384             child.clearOverridingContentSize();
    1385             child.setChildNeedsLayout(MarkOnlyThis);
    1386             child.layoutIfNeeded();
    1387             cacheChildMainSize(child);
    1388             child.clearOverridingContainingBlockContentSize();
    1389         }
    1390     }
    1391    
    13921421    LayoutUnit borderAndPadding = isHorizontalFlow() ? child.horizontalBorderAndPaddingExtent() : child.verticalBorderAndPaddingExtent();
    1393     LayoutUnit childInnerFlexBaseSize = computeInnerFlexBaseSizeForChild(child, borderAndPadding);
     1422    LayoutUnit childFlexBaseSize = computeFlexBaseSizeForChild(child, borderAndPadding);
    13941423    LayoutUnit margin = isHorizontalFlow() ? child.horizontalMarginExtent() : child.verticalMarginExtent();
    1395     return FlexItem(child, childInnerFlexBaseSize, borderAndPadding, margin, computeFlexItemMinMaxSizes(child), childHadLayout);
     1424    return FlexItem(child, childFlexBaseSize, borderAndPadding, margin, computeFlexItemMinMaxSizes(child, relayoutChildren), childHadLayout);
    13961425}
    13971426   
    … …  
    18891918        }
    18901919        // We may have already forced relayout for orthogonal flowing children in
    1891         // computeInnerFlexBaseSizeForChild.
     1920        // computeFlexBaseSizeForChild.
    18921921        bool forceChildRelayout = relayoutChildren && !m_relaidOutChildren.contains(&child);
    18931922        if (!forceChildRelayout && childHasPercentHeightDescendants(child)) {
    … …  
    21442173        LayoutUnit childWidth = std::max(0_lu, lineCrossAxisExtent - crossAxisMarginExtentForChild(child));
    21452174        childWidth = child.constrainLogicalWidthInFragmentByMinMax(childWidth, crossAxisContentExtent(), *this, nullptr);
    2146        
     2175
    21472176        if (childWidth != child.logicalWidth()) {
    21482177            child.setOverridingLogicalWidth(childWidth);

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

    r282463 r284359  
    151151    LayoutUnit computeMainSizeFromAspectRatioUsing(const RenderBox& child, Length crossSizeLength) const;
    152152    void setFlowAwareLocationForChild(RenderBox& child, const LayoutPoint&);
    153     LayoutUnit computeInnerFlexBaseSizeForChild(RenderBox& child, LayoutUnit mainAxisBorderAndPadding);
     153    LayoutUnit computeFlexBaseSizeForChild(RenderBox& child, LayoutUnit mainAxisBorderAndPadding);
    154154    void adjustAlignmentForChild(RenderBox& child, LayoutUnit);
    155155    ItemPosition alignmentForChild(const RenderBox& child) const;
    … …  
    177177    LayoutUnit computeChildMarginValue(Length margin);
    178178    void prepareOrderIteratorAndMargins();
    179     std::pair<LayoutUnit, LayoutUnit> computeFlexItemMinMaxSizes(RenderBox& child);
     179    std::pair<LayoutUnit, LayoutUnit> computeFlexItemMinMaxSizes(RenderBox& child, bool relayoutChildren);
    180180    LayoutUnit adjustChildSizeForAspectRatioCrossAxisMinAndMax(const RenderBox& child, LayoutUnit childSize);
    181181    FlexItem constructFlexItem(RenderBox&, bool relayoutChildren);
Note: See TracChangeset for help on using the changeset viewer.