Skip to content

Commit

Permalink
[css-flex] Flex layout should not invalidate preferred width bits of …
Browse files Browse the repository at this point in the history 
…flex items at the end of layout.

https://bugs.webkit.org/show_bug.cgi?id=264303
rdar://117181858

Reviewed by Alan Baradlay.

In certain situations flex layout will invalidate the preferred widths
bits of flex items towards the end of layout (in layoutAndPlaceChildren),
with a call to updateBlockChildDirtyBitsBeforeLayout. This is incorrect
since by the time we reach layoutAndPlaceChildren flex layout will not
compute the preferred widths of these items again. This leaves the
render tree in a bad state where we may compute the preferred widths
of a flex item at some point during layout, dirty this bit later, and
exit layout with this bit dirtied.

In this state we may not be able to correctly invalidate the ancestor
chain of a flex item in certain situations like in the added test case.
When the image is loaded in the test case we will be unable to
invalidate the preferred widths bits of its ancestor chain since the
item had already been dirtied so we assume the rest of the chain has
already been handled.

Instead, we should be performing this type of invalidation in one of
the following scenarios:
1.) The content of a flex item changes
2.) Certain style changes on the flex item that would impact the
preferred widths computations of its items. One example of this is a
flex item with an aspect ratio since the aspect ratio could impact its
"content based minimum size," if the flexbox's cross size size changes
in a specific way: https://drafts.csswg.org/css-flexbox-1/#content-based-minimum-size

This patch focuses on correcting flex layout to more closely follow
the principles in 2. updateFlexItemDirtyBitsBeforeLayout was added to
replace the call to updateBlockChildDirtyBitsBeforeLayout so that we
can continue to dirty flex items for layout without dirtying their
preferred widths bits as well.

Instead, in RenderFlexibleBox::styleDidChange we can call
needsPreferredWidthsRecalculation on each flex item to determine if we
should dirty the preferred widths bit of it.

* LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/nested-flex-image-loading-invalidates-intrinsic-sizes-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/nested-flex-image-loading-invalidates-intrinsic-sizes.html: Added.
* Source/WebCore/rendering/RenderFlexibleBox.cpp:
(WebCore::updateFlexItemDirtyBitsBeforeLayout):
(WebCore::RenderFlexibleBox::styleDidChange):
(WebCore::RenderFlexibleBox::layoutAndPlaceChildren):

Canonical link: https://commits.webkit.org/271995@main
  • Loading branch information
@sammygill
sammygill committed Dec 13, 2023
1 parent e64e4c4 commit efb0cb8
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 6 deletions.
7 changes: 7 additions & 0 deletions ...tests/css/css-flexbox/nested-flex-image-loading-invalidates-intrinsic-sizes-expected.html
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<!DOCTYPE html>
<html>
<body>
<p>Test passes if there is a filled green square.</p>
<div style="width:100px; height: 100px; background-color: green;"></div>
</body>
</html>
31 changes: 31 additions & 0 deletions ...platform-tests/css/css-flexbox/nested-flex-image-loading-invalidates-intrinsic-sizes.html
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<link rel="author" title="Sammy Gill" href="mailto:sammy.gill@apple.com">
<meta name="assert" content="Loaded image correctly invalidates intrinsic widths of its ancestor chain so that it can be recomputed in flex layout">
<link rel="match" href="/css/reference/ref-filled-green-100px-square-only.html">
<style>
.flexbox {
display: flex;
}
.green-border {
border: 20px solid green;
}
.img {
max-width: 100%;
max-height: 100%;
}
</style>
</head>
<body>
<p>Test passes if there is a filled green square.</p>
<div class="flexbox">
<div>
<div class="flexbox green-border">
<img src="/css/support/60x60-green.png" class="img">
</div>
</div>
</div>
</body>
</html>
27 changes: 21 additions & 6 deletions Source/WebCore/rendering/RenderFlexibleBox.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,17 @@ class OverridingSizesScope {
std::optional<LayoutUnit> m_overridingHeight;
};

static void updateFlexItemDirtyBitsBeforeLayout(bool relayoutFlexItem, RenderBox& flexItem)
{
if (flexItem.isOutOfFlowPositioned())
return;

// FIXME: Technically percentage height objects only need a relayout if their percentage isn't going to be turned into
// an auto value. Add a method to determine this, so that we can avoid the relayout.
if (relayoutFlexItem || flexItem.hasRelativeLogicalHeight())
flexItem.setChildNeedsLayout(MarkOnlyThis);
}

void RenderFlexibleBox::computeChildIntrinsicLogicalWidths(RenderObject& childObject, LayoutUnit& minPreferredLogicalWidth, LayoutUnit& maxPreferredLogicalWidth) const
{
ASSERT(childObject.isRenderBox());
Expand Down Expand Up @@ -360,15 +371,19 @@ void RenderFlexibleBox::styleDidChange(StyleDifference diff, const RenderStyle*
if (!oldStyle || diff != StyleDifference::Layout)
return;

if (oldStyle->resolvedAlignItems(selfAlignmentNormalBehavior()).position() == ItemPosition::Stretch) {
auto oldStyleAlignItemsIsStrecth = oldStyle->resolvedAlignItems(selfAlignmentNormalBehavior()).position() == ItemPosition::Stretch;
for (auto& flexItem : childrenOfType<RenderBox>(*this)) {
if (flexItem.needsPreferredWidthsRecalculation())
flexItem.setPreferredLogicalWidthsDirty(true, MarkingBehavior::MarkOnlyThis);

// Flex items that were previously stretching need to be relayed out so we
// can compute new available cross axis space. This is only necessary for
// stretching since other alignment values don't change the size of the
// box.
for (auto& child : childrenOfType<RenderBox>(*this)) {
ItemPosition previousAlignment = child.style().resolvedAlignSelf(oldStyle, selfAlignmentNormalBehavior()).position();
if (previousAlignment == ItemPosition::Stretch && previousAlignment != child.style().resolvedAlignSelf(&style(), selfAlignmentNormalBehavior()).position())
child.setChildNeedsLayout(MarkOnlyThis);
if (oldStyleAlignItemsIsStrecth) {
ItemPosition previousAlignment = flexItem.style().resolvedAlignSelf(oldStyle, selfAlignmentNormalBehavior()).position();
if (previousAlignment == ItemPosition::Stretch && previousAlignment != flexItem.style().resolvedAlignSelf(&style(), selfAlignmentNormalBehavior()).position())
flexItem.setChildNeedsLayout(MarkOnlyThis);
}
}
}
Expand Down Expand Up @@ -2207,7 +2222,7 @@ void RenderFlexibleBox::layoutAndPlaceChildren(LayoutUnit& crossAxisOffset, Flex
// So, redo it here.
forceChildRelayout = true;
}
updateBlockChildDirtyBitsBeforeLayout(forceChildRelayout, child);
updateFlexItemDirtyBitsBeforeLayout(forceChildRelayout, child);
if (!child.needsLayout())
child.markForPaginationRelayoutIfNeeded();
if (child.needsLayout())
Expand Down

0 comments on commit efb0cb8

Please sign in to comment.