- Revision
- 277222
- Author
- za...@apple.com
- Date
- 2021-05-07 18:48:22 -0700 (Fri, 07 May 2021)
Log Message
[css-flexbox] Flex item construction may affect sibling flex item height computation
https://bugs.webkit.org/show_bug.cgi?id=225489
Reviewed by Sergio Villar Senin.
Source/WebCore:
Flex item construction triggers layout both on the flex item and on its descendants (see constructFlexItem).
During this layout a percent height descendant may indirectly set an incorrect value on the flex container's
m_hasDefiniteHeight (this is due to the odd way we choose to resolve percent height values on the ancestor chain,
see setOverridingContainingBlockContentLogicalHeight(WTF::nullopt)).
Now this incorrect m_hasDefiniteHeight value (Indefinite) causes the next sibling's (also) percent height
resolve fail as it tells the flex item that the flex container can't help with resolving the percent height value.
As the result we end up with an incorrect, 0px height value for this sibling.
e.g.
<div style="height: 100px; display: flex; flex-direction: column;">
<div id=flexItemA style="height: 50px;"><div style="height: 100%;"></div></div>
<div id=flexItemB style="height: 50%;"></div>
</div>
By the time we get to the construction of flexItemB, the RenderFlexibleBox's (flex container) m_hasDefiniteHeight
is already (and incorrectly) set to Indefinite as the result of us trying to resolve flexItemA's descendant height percentage.
This Indefinite value makes the flexItemB's height resolution fail as we believe that the flex container has indefinite height
e.g "height: auto", while it is clearly resolvable (50% of 100px).
Now if we flip the order and flexItemB comes first, the test case passes fine.
It is very unfortunate that some descendant height resolving process can trigger a state change on the ancestor RenderFlexibleBox, but
fixing it would certainly require some substantial architectural change (e.g. pushing down the constraints to the geometry functions instead of
walking the containing block chain).
In this patch, we just scope m_hasDefiniteHeight so that the RenderFlexibleBox's state is not effected by the flex item construction.
Test: fast/flexbox/flex-column-with-percent-height-descendants.html
* rendering/RenderFlexibleBox.cpp:
(WebCore::RenderFlexibleBox::layoutFlexItems):
LayoutTests:
* fast/flexbox/flex-column-with-percent-height-descendants-expected.html: Added.
* fast/flexbox/flex-column-with-percent-height-descendants.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (277221 => 277222)
--- trunk/LayoutTests/ChangeLog 2021-05-08 01:26:31 UTC (rev 277221)
+++ trunk/LayoutTests/ChangeLog 2021-05-08 01:48:22 UTC (rev 277222)
@@ -1,3 +1,13 @@
+2021-05-07 Zalan Bujtas <za...@apple.com>
+
+ [css-flexbox] Flex item construction may affect sibling flex item height computation
+ https://bugs.webkit.org/show_bug.cgi?id=225489
+
+ Reviewed by Sergio Villar Senin.
+
+ * fast/flexbox/flex-column-with-percent-height-descendants-expected.html: Added.
+ * fast/flexbox/flex-column-with-percent-height-descendants.html: Added.
+
2021-05-07 Amir Mark Jr <amir_m...@apple.com>
[BigSur Release Wk2 Arm64] imported/w3c/web-platform-tests/navigation-timing/test_performance_attributes.sub.html is flaky failing
Added: trunk/LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants-expected.html (0 => 277222)
--- trunk/LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants-expected.html (rev 0)
+++ trunk/LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants-expected.html 2021-05-08 01:48:22 UTC (rev 277222)
@@ -0,0 +1,8 @@
+<!DOCTYPE html>
+<style>
+div {
+ height: 100px;
+ background-color: green;
+}
+</style>
+<div></div>
Added: trunk/LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants.html (0 => 277222)
--- trunk/LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants.html (rev 0)
+++ trunk/LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants.html 2021-05-08 01:48:22 UTC (rev 277222)
@@ -0,0 +1,25 @@
+<!DOCTYPE html>
+<style>
+.flexContainer {
+ display: flex;
+ flex-direction: column;
+ position: relative;
+ height: 100px;
+}
+.firstChild {
+ background-color: yellow;
+ height: 50px;
+}
+.secondChild {
+ height: 50%;
+ background-color: green;
+}
+.percentHeightDescendant {
+ height: 100%;
+ background-color: green
+}
+</style>
+<div class=flexContainer>
+ <div class=firstChild><div class=percentHeightDescendant></div></div>
+ <div class=secondChild></div>
+</div>
Modified: trunk/Source/WebCore/ChangeLog (277221 => 277222)
--- trunk/Source/WebCore/ChangeLog 2021-05-08 01:26:31 UTC (rev 277221)
+++ trunk/Source/WebCore/ChangeLog 2021-05-08 01:48:22 UTC (rev 277222)
@@ -1,3 +1,37 @@
+2021-05-07 Zalan Bujtas <za...@apple.com>
+
+ [css-flexbox] Flex item construction may affect sibling flex item height computation
+ https://bugs.webkit.org/show_bug.cgi?id=225489
+
+ Reviewed by Sergio Villar Senin.
+
+ Flex item construction triggers layout both on the flex item and on its descendants (see constructFlexItem).
+ During this layout a percent height descendant may indirectly set an incorrect value on the flex container's
+ m_hasDefiniteHeight (this is due to the odd way we choose to resolve percent height values on the ancestor chain,
+ see setOverridingContainingBlockContentLogicalHeight(WTF::nullopt)).
+ Now this incorrect m_hasDefiniteHeight value (Indefinite) causes the next sibling's (also) percent height
+ resolve fail as it tells the flex item that the flex container can't help with resolving the percent height value.
+ As the result we end up with an incorrect, 0px height value for this sibling.
+ e.g.
+ <div style="height: 100px; display: flex; flex-direction: column;">
+ <div id=flexItemA style="height: 50px;"><div style="height: 100%;"></div></div>
+ <div id=flexItemB style="height: 50%;"></div>
+ </div>
+ By the time we get to the construction of flexItemB, the RenderFlexibleBox's (flex container) m_hasDefiniteHeight
+ is already (and incorrectly) set to Indefinite as the result of us trying to resolve flexItemA's descendant height percentage.
+ This Indefinite value makes the flexItemB's height resolution fail as we believe that the flex container has indefinite height
+ e.g "height: auto", while it is clearly resolvable (50% of 100px).
+ Now if we flip the order and flexItemB comes first, the test case passes fine.
+ It is very unfortunate that some descendant height resolving process can trigger a state change on the ancestor RenderFlexibleBox, but
+ fixing it would certainly require some substantial architectural change (e.g. pushing down the constraints to the geometry functions instead of
+ walking the containing block chain).
+ In this patch, we just scope m_hasDefiniteHeight so that the RenderFlexibleBox's state is not effected by the flex item construction.
+
+ Test: fast/flexbox/flex-column-with-percent-height-descendants.html
+
+ * rendering/RenderFlexibleBox.cpp:
+ (WebCore::RenderFlexibleBox::layoutFlexItems):
+
2021-05-07 Devin Rousso <drou...@apple.com>
[iOS] fix inconsistency around the meaning of `rate` and `defaultPlaybackRate` between WebKit and AVKit
Modified: trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp (277221 => 277222)
--- trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp 2021-05-08 01:26:31 UTC (rev 277221)
+++ trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp 2021-05-08 01:48:22 UTC (rev 277222)
@@ -268,9 +268,6 @@
resetLogicalHeightBeforeLayoutIfNeeded();
m_relaidOutChildren.clear();
- bool oldInLayout = m_inLayout;
- m_inLayout = true;
-
if (recomputeLogicalWidth())
relayoutChildren = true;
@@ -321,8 +318,6 @@
repainter.repaintAfterLayout();
clearNeedsLayout();
-
- m_inLayout = oldInLayout;
}
void RenderFlexibleBox::appendChildFrameRects(ChildFrameRects& childFrameRects)
@@ -845,7 +840,7 @@
if (m_hasDefiniteHeight == SizeDefiniteness::Indefinite)
return false;
bool definite = child.computePercentageLogicalHeight(flexBasis, updateDescendants).hasValue();
- if (m_inLayout && (isHorizontalWritingMode() == child.isHorizontalWritingMode())) {
+ if (!m_inFlexItemConstruction && (isHorizontalWritingMode() == child.isHorizontalWritingMode())) {
// We can reach this code even while we're not laying ourselves out, such
// as from mainSizeForPercentageResolution.
m_hasDefiniteHeight = definite ? SizeDefiniteness::Definite : SizeDefiniteness::Indefinite;
@@ -965,21 +960,7 @@
// Set up our master list of flex items. All of the rest of the algorithm
// should work off this list of a subset.
// TODO(cbiesinger): That second part is not yet true.
- Vector<FlexItem> allItems;
- m_orderIterator.first();
- for (RenderBox* child = m_orderIterator.currentChild(); child; child = m_orderIterator.next()) {
- if (m_orderIterator.shouldSkipChild(*child)) {
- // Out-of-flow children are not flex items, so we skip them here.
- if (child->isOutOfFlowPositioned())
- prepareChildForPositionedLayout(*child);
- continue;
- }
- allItems.append(constructFlexItem(*child, relayoutChildren));
- }
-
- // constructFlexItem() might set the override containing block height so any value cached for definiteness might be incorrect.
- m_hasDefiniteHeight = SizeDefiniteness::Unknown;
-
+ auto allItems = constructFlexItems(relayoutChildren);
const LayoutUnit lineBreakLength = mainAxisContentExtent(LayoutUnit::max());
LayoutUnit gapBetweenItems = computeGap(GapType::BetweenItems);
LayoutUnit gapBetweenLines = computeGap(GapType::BetweenLines);
@@ -1294,36 +1275,52 @@
return childSize;
}
-FlexItem RenderFlexibleBox::constructFlexItem(RenderBox& child, bool relayoutChildren)
+Vector<FlexItem> RenderFlexibleBox::constructFlexItems(bool relayoutChildren)
{
- 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(WTF::nullopt);
- else
- child.setOverridingContainingBlockContentLogicalWidth(WTF::nullopt);
- child.clearOverridingContentSize();
- child.setChildNeedsLayout(MarkOnlyThis);
- child.layoutIfNeeded();
- cacheChildMainSize(child);
- child.clearOverridingContainingBlockContentSize();
+ SetForScope<bool> inFlexItemConstruction(m_inFlexItemConstruction, true);
+
+ Vector<FlexItem> flexItems;
+ for (auto* child = m_orderIterator.first(); child; child = m_orderIterator.next()) {
+ if (m_orderIterator.shouldSkipChild(*child)) {
+ // Out-of-flow children are not flex items, so we skip them here.
+ if (child->isOutOfFlowPositioned())
+ prepareChildForPositionedLayout(*child);
+ continue;
}
+
+ auto constructFlexItemForChildBox = [&](auto& childBox) {
+ childBox.clearOverridingContentSize();
+ if (childHasIntrinsicMainAxisSize(childBox)) {
+ // 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, childBox);
+ // 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 (childBox.needsLayout() || !m_intrinsicSizeAlongMainAxis.contains(&childBox)) {
+ if (isHorizontalWritingMode() == childBox.isHorizontalWritingMode())
+ childBox.setOverridingContainingBlockContentLogicalHeight(WTF::nullopt);
+ else
+ childBox.setOverridingContainingBlockContentLogicalWidth(WTF::nullopt);
+ childBox.clearOverridingContentSize();
+ childBox.setChildNeedsLayout(MarkOnlyThis);
+ childBox.layoutIfNeeded();
+ cacheChildMainSize(childBox);
+ childBox.clearOverridingContainingBlockContentSize();
+ }
+ }
+
+ auto borderAndPadding = isHorizontalFlow() ? childBox.horizontalBorderAndPaddingExtent() : childBox.verticalBorderAndPaddingExtent();
+ auto childInnerFlexBaseSize = computeInnerFlexBaseSizeForChild(childBox, borderAndPadding);
+ auto childMinMaxAppliedMainAxisExtent = adjustChildSizeForMinAndMax(childBox, childInnerFlexBaseSize);
+ auto margin = isHorizontalFlow() ? childBox.horizontalMarginExtent() : childBox.verticalMarginExtent();
+ return FlexItem(childBox, childInnerFlexBaseSize, childMinMaxAppliedMainAxisExtent, borderAndPadding, margin);
+ };
+ flexItems.append(constructFlexItemForChildBox(*child));
}
-
- LayoutUnit borderAndPadding = isHorizontalFlow() ? child.horizontalBorderAndPaddingExtent() : child.verticalBorderAndPaddingExtent();
- LayoutUnit childInnerFlexBaseSize = computeInnerFlexBaseSizeForChild(child, borderAndPadding);
- LayoutUnit childMinMaxAppliedMainAxisExtent = adjustChildSizeForMinAndMax(child, childInnerFlexBaseSize);
- LayoutUnit margin = isHorizontalFlow() ? child.horizontalMarginExtent() : child.verticalMarginExtent();
- return FlexItem(child, childInnerFlexBaseSize, childMinMaxAppliedMainAxisExtent, borderAndPadding, margin);
+ return flexItems;
}
void RenderFlexibleBox::freezeViolations(Vector<FlexItem*>& violations, LayoutUnit& availableFreeSpace, double& totalFlexGrow, double& totalFlexShrink, double& totalWeightedFlexShrink)
Modified: trunk/Source/WebCore/rendering/RenderFlexibleBox.h (277221 => 277222)
--- trunk/Source/WebCore/rendering/RenderFlexibleBox.h 2021-05-08 01:26:31 UTC (rev 277221)
+++ trunk/Source/WebCore/rendering/RenderFlexibleBox.h 2021-05-08 01:48:22 UTC (rev 277222)
@@ -175,7 +175,7 @@
void prepareOrderIteratorAndMargins();
LayoutUnit adjustChildSizeForMinAndMax(RenderBox& child, LayoutUnit childSize);
LayoutUnit adjustChildSizeForAspectRatioCrossAxisMinAndMax(const RenderBox& child, LayoutUnit childSize);
- FlexItem constructFlexItem(RenderBox&, bool relayoutChildren);
+ Vector<FlexItem> constructFlexItems(bool relayoutChildren);
void freezeInflexibleItems(FlexSign, Vector<FlexItem>& children, LayoutUnit& remainingFreeSpace, double& totalFlexGrow, double& totalFlexShrink, double& totalWeightedFlexShrink);
bool resolveFlexibleLengths(FlexSign, Vector<FlexItem>&, LayoutUnit initialFreeSpace, LayoutUnit& remainingFreeSpace, double& totalFlexGrow, double& totalFlexShrink, double& totalWeightedFlexShrink);
@@ -220,7 +220,7 @@
// This is SizeIsUnknown outside of layoutBlock()
SizeDefiniteness m_hasDefiniteHeight { SizeDefiniteness::Unknown };
- bool m_inLayout { false };
+ bool m_inFlexItemConstruction { false };
bool m_shouldResetChildLogicalHeightBeforeLayout { false };
};
