Skip to content
Permalink
Browse files
[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:

* TestExpectations: Unskipped flex-basis-011.html which is now working fine.

Canonical link: https://commits.webkit.org/243144@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@284359 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
svillar committed Oct 18, 2021
1 parent 5109fec commit 5afc07c51c8f0679901552bad8b177e70632fe9a
Showing 5 changed files with 123 additions and 52 deletions.
@@ -1,3 +1,12 @@
2021-09-24 Sergio Villar Senin <svillar@igalia.com>

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

Reviewed by Manuel Rego Casasnovas.

* TestExpectations: Unskipped flex-basis-011.html which is now working fine.

2021-10-17 Dean Jackson <dino@apple.com>

[WebXR] Stubs for WebXR Hand Input Module
@@ -4219,9 +4219,6 @@ webkit.org/b/221479 imported/w3c/web-platform-tests/css/css-flexbox/flexbox-flex
webkit.org/b/221479 imported/w3c/web-platform-tests/css/css-flexbox/flexbox-flex-basis-content-003a.html [ ImageOnlyFailure ]
webkit.org/b/221479 imported/w3c/web-platform-tests/css/css-flexbox/flexbox-flex-basis-content-004a.html [ ImageOnlyFailure ]

# flex-basis with %.
webkit.org/b/ imported/w3c/web-platform-tests/css/css-flexbox/flex-basis-011.html [ ImageOnlyFailure ]

# Flex item's min|max content contributions
webkit.org/b/230747 imported/w3c/web-platform-tests/css/css-flexbox/flex-container-max-content-001.html [ ImageOnlyFailure ]
webkit.org/b/230747 imported/w3c/web-platform-tests/css/css-flexbox/flex-container-min-content-001.html [ ImageOnlyFailure ]
@@ -1,3 +1,39 @@
2021-09-24 Sergio Villar Senin <svillar@igalia.com>

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

Reviewed by Manuel Rego Casasnovas.

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:

2021-10-18 Philippe Normand <pnormand@igalia.com>

[MediaStream] Mock video source background not fully filled for custom video resolution
@@ -34,9 +34,11 @@
#include "FlexibleBoxAlgorithm.h"
#include "HitTestResult.h"
#include "LayoutRepainter.h"
#include "RenderBox.h"
#include "RenderChildIterator.h"
#include "RenderLayer.h"
#include "RenderLayoutState.h"
#include "RenderObjectEnums.h"
#include "RenderStyleConstants.h"
#include "RenderView.h"
#include "WritingMode.h"
@@ -628,13 +630,7 @@ std::optional<LayoutUnit> RenderFlexibleBox::computeMainAxisExtentForChild(Rende
// horizontal flow and horizontal writing mode, or vertical flow and vertical
// writing mode. Otherwise we need the logical height.
if (!mainAxisIsChildInlineAxis(child)) {
// We don't have to check for "auto" here - computeContentLogicalHeight
// will just return a null Optional for that case anyway. It's safe to access
// scrollbarLogicalHeight here because ComputeNextFlexLine will have
// already forced layout on the child. We previously did a layout out the child
// if necessary (see ComputeNextFlexLine and the call to
// childHasIntrinsicMainAxisSize) so we can be sure that the two height
// calls here will return up-to-date data.
child.layoutIfNeeded();
std::optional<LayoutUnit> height = child.computeContentLogicalHeight(sizeType, size, cachedChildIntrinsicContentLogicalHeight(child));
if (!height)
return height;
@@ -1000,29 +996,62 @@ void RenderFlexibleBox::clearCachedMainSizeForChild(const RenderBox& child)
m_intrinsicSizeAlongMainAxis.remove(&child);
}

LayoutUnit RenderFlexibleBox::computeInnerFlexBaseSizeForChild(RenderBox& child, LayoutUnit mainAxisBorderAndPadding)
// This is a RAII class that is used to temporarily set the flex basis as the child size in the main axis.
class ScopedFlexBasisAsChildMainSize {
public:
ScopedFlexBasisAsChildMainSize(RenderBox& child, Length flexBasis, bool mainAxisIsInlineAxis)
: m_child(child)
, m_mainAxisIsInlineAxis(mainAxisIsInlineAxis)
{
if (m_mainAxisIsInlineAxis) {
m_originalLength = m_child.style().logicalWidth();
m_child.mutableStyle().setLogicalWidth(Length(flexBasis));
return;
}
m_originalLength = m_child.style().logicalHeight();
m_child.mutableStyle().setLogicalHeight(Length(flexBasis));
}
~ScopedFlexBasisAsChildMainSize()
{
if (m_mainAxisIsInlineAxis)
m_child.mutableStyle().setLogicalWidth(Length(m_originalLength));
else
m_child.mutableStyle().setLogicalHeight(Length(m_originalLength));
}
private:
RenderBox& m_child;
bool m_mainAxisIsInlineAxis;
Length m_originalLength;
};

// https://drafts.csswg.org/css-flexbox/#algo-main-item
LayoutUnit RenderFlexibleBox::computeFlexBaseSizeForChild(RenderBox& child, LayoutUnit mainAxisBorderAndPadding)
{
Length flexBasis = flexBasisForChild(child);
// 9.3.2 A.
if (childMainSizeIsDefinite(child, flexBasis))
return std::max(0_lu, computeMainAxisExtentForChild(child, MainOrPreferredSize, flexBasis).value());

// 9.3.2 B.
if (childHasComputableAspectRatioAndCrossSizeIsConsideredDefinite(child)) {
const Length& crossSizeLength = crossSizeLengthForChild(MainOrPreferredSize, child);
return adjustChildSizeForAspectRatioCrossAxisMinAndMax(child, computeMainSizeFromAspectRatioUsing(child, crossSizeLength));
}

// The flex basis is indefinite (=auto), so we need to compute the actual width of the child.
LayoutUnit mainAxisExtent;
if (!mainAxisIsChildInlineAxis(child)) {
ASSERT(!child.needsLayout());
ASSERT(m_intrinsicSizeAlongMainAxis.contains(&child));
mainAxisExtent = m_intrinsicSizeAlongMainAxis.get(&child);
} else {
// We don't need to add scrollbarLogicalWidth here because the preferred
// width includes the scrollbar, even for overflow: auto.
mainAxisExtent = child.maxPreferredLogicalWidth();
// FIXME: implement 9.3.2 C.
// FIXME: implement 9.3.2 D.

// 9.3.2 E. Otherwise, size the item into the available space using its used flex basis in place of its main size.
{
ScopedFlexBasisAsChildMainSize flexBasisScope(child, flexBasis, mainAxisIsChildInlineAxis(child));
if (mainAxisIsChildInlineAxis(child))
return child.maxPreferredLogicalWidth() - mainAxisBorderAndPadding;

if (childHasIntrinsicMainAxisSize(child))
child.setNeedsLayout(MarkOnlyThis);
child.layoutIfNeeded();
return child.logicalHeight() - mainAxisBorderAndPadding;
}
return mainAxisExtent - mainAxisBorderAndPadding;
}

void RenderFlexibleBox::layoutFlexItems(bool relayoutChildren)
@@ -1260,7 +1289,7 @@ void RenderFlexibleBox::prepareOrderIteratorAndMargins()
}
}

std::pair<LayoutUnit, LayoutUnit> RenderFlexibleBox::computeFlexItemMinMaxSizes(RenderBox& child)
std::pair<LayoutUnit, LayoutUnit> RenderFlexibleBox::computeFlexItemMinMaxSizes(RenderBox& child, bool relayoutChildren)
{
Length max = mainSizeLengthForChild(MaxSize, child);
std::optional<LayoutUnit> maxExtent = std::nullopt;
@@ -1279,8 +1308,30 @@ std::pair<LayoutUnit, LayoutUnit> RenderFlexibleBox::computeFlexItemMinMaxSizes(
Length childCrossSizeLength = crossSizeLengthForChild(MainOrPreferredSize, child);
if (child.isRenderReplaced() && childHasComputableAspectRatio(child) && childCrossSizeIsDefinite(child, childCrossSizeLength))
contentSize = computeMainSizeFromAspectRatioUsing(child, childCrossSizeLength);
else
else {
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.
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();
}
}
contentSize = computeMainAxisExtentForChild(child, MinSize, Length(LengthType::MinContent)).value_or(0);
}
if (child.hasIntrinsicAspectRatio() && child.intrinsicSize().height())
contentSize = adjustChildSizeForAspectRatioCrossAxisMinAndMax(child, contentSize);
ASSERT(contentSize >= 0);
@@ -1367,32 +1418,10 @@ FlexItem RenderFlexibleBox::constructFlexItem(RenderBox& child, bool relayoutChi
{
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 childFlexBaseSize = computeFlexBaseSizeForChild(child, borderAndPadding);
LayoutUnit margin = isHorizontalFlow() ? child.horizontalMarginExtent() : child.verticalMarginExtent();
return FlexItem(child, childInnerFlexBaseSize, borderAndPadding, margin, computeFlexItemMinMaxSizes(child), childHadLayout);
return FlexItem(child, childFlexBaseSize, borderAndPadding, margin, computeFlexItemMinMaxSizes(child, relayoutChildren), childHadLayout);
}

void RenderFlexibleBox::freezeViolations(Vector<FlexItem*>& violations, LayoutUnit& availableFreeSpace, double& totalFlexGrow, double& totalFlexShrink, double& totalWeightedFlexShrink)
@@ -1888,7 +1917,7 @@ void RenderFlexibleBox::layoutAndPlaceChildren(LayoutUnit& crossAxisOffset, Vect
resetAutoMarginsAndLogicalTopInCrossAxis(child);
}
// We may have already forced relayout for orthogonal flowing children in
// computeInnerFlexBaseSizeForChild.
// computeFlexBaseSizeForChild.
bool forceChildRelayout = relayoutChildren && !m_relaidOutChildren.contains(&child);
if (!forceChildRelayout && childHasPercentHeightDescendants(child)) {
// Have to force another relayout even though the child is sized
@@ -2143,7 +2172,7 @@ void RenderFlexibleBox::applyStretchAlignmentToChild(RenderBox& child, LayoutUni
} else if (!mainAxisIsChildInlineAxis(child) && child.style().logicalWidth().isAuto()) {
LayoutUnit childWidth = std::max(0_lu, lineCrossAxisExtent - crossAxisMarginExtentForChild(child));
childWidth = child.constrainLogicalWidthInFragmentByMinMax(childWidth, crossAxisContentExtent(), *this, nullptr);

if (childWidth != child.logicalWidth()) {
child.setOverridingLogicalWidth(childWidth);
child.setChildNeedsLayout(MarkOnlyThis);
@@ -150,7 +150,7 @@ class RenderFlexibleBox : public RenderBlock {
void computeChildIntrinsicLogicalWidths(RenderObject&, LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const override;
LayoutUnit computeMainSizeFromAspectRatioUsing(const RenderBox& child, Length crossSizeLength) const;
void setFlowAwareLocationForChild(RenderBox& child, const LayoutPoint&);
LayoutUnit computeInnerFlexBaseSizeForChild(RenderBox& child, LayoutUnit mainAxisBorderAndPadding);
LayoutUnit computeFlexBaseSizeForChild(RenderBox& child, LayoutUnit mainAxisBorderAndPadding);
void adjustAlignmentForChild(RenderBox& child, LayoutUnit);
ItemPosition alignmentForChild(const RenderBox& child) const;
bool canComputePercentageFlexBasis(const RenderBox& child, const Length& flexBasis, UpdatePercentageHeightDescendants);
@@ -176,7 +176,7 @@ class RenderFlexibleBox : public RenderBlock {

LayoutUnit computeChildMarginValue(Length margin);
void prepareOrderIteratorAndMargins();
std::pair<LayoutUnit, LayoutUnit> computeFlexItemMinMaxSizes(RenderBox& child);
std::pair<LayoutUnit, LayoutUnit> computeFlexItemMinMaxSizes(RenderBox& child, bool relayoutChildren);
LayoutUnit adjustChildSizeForAspectRatioCrossAxisMinAndMax(const RenderBox& child, LayoutUnit childSize);
FlexItem constructFlexItem(RenderBox&, bool relayoutChildren);

0 comments on commit 5afc07c

Please sign in to comment.