Title: [277435] trunk
- Revision
- 277435
- Author
- [email protected]
- Date
- 2021-05-13 08:00:12 -0700 (Thu, 13 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.
In this patch, we just reset the m_hasDefiniteHeight flag inside the loop to ensure that each flex item
starts with a fresh height percent resolve state.
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 (277434 => 277435)
--- trunk/LayoutTests/ChangeLog 2021-05-13 14:30:23 UTC (rev 277434)
+++ trunk/LayoutTests/ChangeLog 2021-05-13 15:00:12 UTC (rev 277435)
@@ -1,3 +1,13 @@
+2021-05-13 Zalan Bujtas <[email protected]>
+
+ [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-13 Chris Fleizach <[email protected]>
AX: Crash at WebCore::Document::updateLayout
Added: trunk/LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants-crash-expected.txt (0 => 277435)
--- trunk/LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants-crash-expected.txt 2021-05-13 15:00:12 UTC (rev 277435)
@@ -0,0 +1,2 @@
+PASS if no crash or assert.
+
Added: trunk/LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants-crash.html (0 => 277435)
--- trunk/LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants-crash.html (rev 0)
+++ trunk/LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants-crash.html 2021-05-13 15:00:12 UTC (rev 277435)
@@ -0,0 +1,30 @@
+<style>
+.flex-container {
+ display: flex;
+ flex-direction: column;
+}
+
+.fixed-flex-item {
+ position: fixed;
+ height: 200px;
+}
+.heigh-percentage-descendant {
+ height: 100%;
+}
+
+.sibling {
+ height: 100%;
+}
+
+</style>
+PASS if no crash or assert.
+<div class=flex-container>
+ <div class=fixed-flex-item>
+ <div class=heigh-percentage-descendant></div>
+ </div>
+ <div class=sibling></div>
+</div>
+<script>
+if (window.testRunner)
+ testRunner.dumpAsText();
+</script>
\ No newline at end of file
Added: trunk/LayoutTests/fast/flexbox/flex-column-with-percent-height-descendants-expected.html (0 => 277435)
--- 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-13 15:00:12 UTC (rev 277435)
@@ -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 => 277435)
--- 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-13 15:00:12 UTC (rev 277435)
@@ -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 (277434 => 277435)
--- trunk/Source/WebCore/ChangeLog 2021-05-13 14:30:23 UTC (rev 277434)
+++ trunk/Source/WebCore/ChangeLog 2021-05-13 15:00:12 UTC (rev 277435)
@@ -1,3 +1,37 @@
+2021-05-13 Zalan Bujtas <[email protected]>
+
+ [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.
+ In this patch, we just reset the m_hasDefiniteHeight flag inside the loop to ensure that each flex item
+ starts with a fresh height percent resolve state.
+
+ Test: fast/flexbox/flex-column-with-percent-height-descendants.html
+
+ * rendering/RenderFlexibleBox.cpp:
+ (WebCore::RenderFlexibleBox::layoutFlexItems):
+
2021-05-13 Chris Fleizach <[email protected]>
AX: Crash at WebCore::Document::updateLayout
Modified: trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp (277434 => 277435)
--- trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp 2021-05-13 14:30:23 UTC (rev 277434)
+++ trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp 2021-05-13 15:00:12 UTC (rev 277435)
@@ -993,10 +993,9 @@
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;
}
-
- // constructFlexItem() might set the override containing block height so any value cached for definiteness might be incorrect.
- m_hasDefiniteHeight = SizeDefiniteness::Unknown;
const LayoutUnit lineBreakLength = mainAxisContentExtent(LayoutUnit::max());
LayoutUnit gapBetweenItems = computeGap(GapType::BetweenItems);
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
