Title: [292817] trunk
- Revision
- 292817
- Author
- za...@apple.com
- Date
- 2022-04-13 11:50:18 -0700 (Wed, 13 Apr 2022)
Log Message
REGRESSION (r292043): [ Mac ] fast/block/positioning/fixed-container-with-relative-parent.html is a flaky image failure
https://bugs.webkit.org/show_bug.cgi?id=239101
<rdar://problem/91603539>
Reviewed by Antti Koivisto.
Source/WebCore:
1. Out of flow boxes are laid out independently from each other as the last step of their containing block layout.
2. However their static positions are computed during regular in-flow layout (as if their positions were static).
In order to do #1, we maintain a ListHashSet for the out-of-flow boxes and insert them at #2 (and we also have
a corresponding HashMap<ContainingBlock, ListHasSet>).
Normally this is a very simple list of descendant positioned boxes and since out-of-flow boxes don't interact with each
other, their position in the list is not important.
e.g.
<div id=A style="position: relative">
<div>
<div id=B style="position: absolute"></div>
<div id=C style="position: absolute"></div>
</div>
</div>
At in-flow layout (#2), we insert B and C to "ListHashSet of A" as we come across them in DOM order and compute their static positions.
Later in the layout flow when we get to the "let's layout the out-of-flow boxes" phase (#1) we simply walk
the ListHashSet and lay out B and C (but "C and B" order would also work just fine).
However the ICB (RenderView) is a special containing block as it can hold different types of out-of-flow boxes (absolute and fixed)
and those out-of-flow boxes may have layout dependencies.
e.g.
<body><div id=A class=absolute><div id=B class=fixed></div></div></body>
ICB's ListHasSet has both A and B, but in this case there's (static)layout dependency between these boxes.
In order to figure out the static position of B, we have to have A laid out first. In order to lay out A before B,
B has to be preceded by A in ICB's ListHasSet.
Now full layout always guarantees the correct order.
However in case of partial layout since we don't run a full #2, the ListHasSet may end up having an unexpected order.
e.g.
<body><div id=A class=absolute><div id=B><div id=C class=fixed></div></div></div></body>
1. The initial (full) layout produces the following (correct) order for the ICB's ListHasSet -> AC.
2. A subsequent partial layout (e.g. triggered by A's position change) runs an in-flow layout on the <body> which
(re-)appends A to the ListHasSet (CA <- incorrect order). Now at this point we assume that the in-flow layout picks up B
which eventually (re-)appends C to the ListHashSet (AC <- correct order). However since B does not need layout, we just
stop at <body> which leaves us with an unexpected ListHashSet.
3. As part of the ICB's out-of-flow layout, we pick C as the first box to lay out followed by A. However since C's static
position depends on A's position, we end up using stale geometry when computing C's static position.
This patch fixes this issue by ensuring the absolute positioned boxes always come first in the ICB's ListHasSet (note
that their order is not really important -see above. What's important is that a potential (as-if-static) containing block always
comes before the fixed boxes).
Test: fast/block/fixed-inside-absolute-positioned.html
* rendering/RenderBlock.cpp:
(WebCore::PositionedDescendantsMap::addDescendant):
(WebCore::RenderBlock::insertPositionedObject):
LayoutTests:
* fast/block/fixed-inside-absolute-positioned-expected.html: Added.
* fast/block/fixed-inside-absolute-positioned.html: Added.
* platform/mac-wk1/TestExpectations:
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (292816 => 292817)
--- trunk/LayoutTests/ChangeLog 2022-04-13 18:48:19 UTC (rev 292816)
+++ trunk/LayoutTests/ChangeLog 2022-04-13 18:50:18 UTC (rev 292817)
@@ -1,3 +1,15 @@
+2022-04-13 Alan Bujtas <za...@apple.com>
+
+ REGRESSION (r292043): [ Mac ] fast/block/positioning/fixed-container-with-relative-parent.html is a flaky image failure
+ https://bugs.webkit.org/show_bug.cgi?id=239101
+ <rdar://problem/91603539>
+
+ Reviewed by Antti Koivisto.
+
+ * fast/block/fixed-inside-absolute-positioned-expected.html: Added.
+ * fast/block/fixed-inside-absolute-positioned.html: Added.
+ * platform/mac-wk1/TestExpectations:
+
2022-04-13 Simon Fraser <simon.fra...@apple.com>
[css-scroll-snap] scrollIntoView fails with scroll-snap-type on :root
Added: trunk/LayoutTests/fast/block/fixed-inside-absolute-positioned-expected.html (0 => 292817)
--- trunk/LayoutTests/fast/block/fixed-inside-absolute-positioned-expected.html (rev 0)
+++ trunk/LayoutTests/fast/block/fixed-inside-absolute-positioned-expected.html 2022-04-13 18:50:18 UTC (rev 292817)
@@ -0,0 +1,13 @@
+<style>
+body {
+ margin: 0px;
+}
+div {
+ width: 100px;
+ height: 100px;
+ background-color: green;
+ margin-top: 8px;
+ margin-left: 100px;
+}
+</style>
+<div></div>
\ No newline at end of file
Added: trunk/LayoutTests/fast/block/fixed-inside-absolute-positioned.html (0 => 292817)
--- trunk/LayoutTests/fast/block/fixed-inside-absolute-positioned.html (rev 0)
+++ trunk/LayoutTests/fast/block/fixed-inside-absolute-positioned.html 2022-04-13 18:50:18 UTC (rev 292817)
@@ -0,0 +1,21 @@
+<style>
+.outer {
+ position: absolute;
+ width: 100px;
+ height: 100px;
+ background-color: red;
+}
+
+.inner {
+ position: fixed;
+ width: 100px;
+ height: 100px;
+ background-color: green;
+}
+</style>
+<!-- Pass if no red -->
+<div id=moveThis class=outer><div><div class=inner></div></div></div>
+<script>
+document.body.offsetHeight;
+moveThis.style.left = "100px";
+</script>
\ No newline at end of file
Modified: trunk/LayoutTests/platform/mac/TestExpectations (292816 => 292817)
--- trunk/LayoutTests/platform/mac/TestExpectations 2022-04-13 18:48:19 UTC (rev 292816)
+++ trunk/LayoutTests/platform/mac/TestExpectations 2022-04-13 18:50:18 UTC (rev 292817)
@@ -2294,5 +2294,3 @@
# fractional pixel diff between modern and legacy line layout.
imported/blink/fast/multicol/vertical-lr/float-content-break.html [ ImageOnlyFailure ]
-
-webkit.org/b/239101 fast/block/positioning/fixed-container-with-relative-parent.html [ Pass ImageOnlyFailure ]
\ No newline at end of file
Modified: trunk/LayoutTests/platform/mac-wk1/TestExpectations (292816 => 292817)
--- trunk/LayoutTests/platform/mac-wk1/TestExpectations 2022-04-13 18:48:19 UTC (rev 292816)
+++ trunk/LayoutTests/platform/mac-wk1/TestExpectations 2022-04-13 18:50:18 UTC (rev 292817)
@@ -1046,8 +1046,6 @@
webkit.org/b/182554 transitions/transition-display-property.html [ Pass ImageOnlyFailure ]
-webkit.org/b/181834 [ Debug ] fast/block/positioning/fixed-container-with-relative-parent.html [ Pass ImageOnlyFailure ]
-
imported/w3c/web-platform-tests/css/selectors/selector-placeholder-shown-type-change-002.html [ ImageOnlyFailure ]
webkit.org/b/184456 imported/w3c/web-platform-tests/html/semantics/embedded-content/the-embed-element/embed-in-object-fallback.html [ Pass Failure ]
Modified: trunk/Source/WebCore/ChangeLog (292816 => 292817)
--- trunk/Source/WebCore/ChangeLog 2022-04-13 18:48:19 UTC (rev 292816)
+++ trunk/Source/WebCore/ChangeLog 2022-04-13 18:50:18 UTC (rev 292817)
@@ -1,3 +1,63 @@
+2022-04-13 Alan Bujtas <za...@apple.com>
+
+ REGRESSION (r292043): [ Mac ] fast/block/positioning/fixed-container-with-relative-parent.html is a flaky image failure
+ https://bugs.webkit.org/show_bug.cgi?id=239101
+ <rdar://problem/91603539>
+
+ Reviewed by Antti Koivisto.
+
+ 1. Out of flow boxes are laid out independently from each other as the last step of their containing block layout.
+ 2. However their static positions are computed during regular in-flow layout (as if their positions were static).
+
+ In order to do #1, we maintain a ListHashSet for the out-of-flow boxes and insert them at #2 (and we also have
+ a corresponding HashMap<ContainingBlock, ListHasSet>).
+
+ Normally this is a very simple list of descendant positioned boxes and since out-of-flow boxes don't interact with each
+ other, their position in the list is not important.
+ e.g.
+ <div id=A style="position: relative">
+ <div>
+ <div id=B style="position: absolute"></div>
+ <div id=C style="position: absolute"></div>
+ </div>
+ </div>
+
+ At in-flow layout (#2), we insert B and C to "ListHashSet of A" as we come across them in DOM order and compute their static positions.
+ Later in the layout flow when we get to the "let's layout the out-of-flow boxes" phase (#1) we simply walk
+ the ListHashSet and lay out B and C (but "C and B" order would also work just fine).
+
+ However the ICB (RenderView) is a special containing block as it can hold different types of out-of-flow boxes (absolute and fixed)
+ and those out-of-flow boxes may have layout dependencies.
+ e.g.
+ <body><div id=A class=absolute><div id=B class=fixed></div></div></body>
+
+ ICB's ListHasSet has both A and B, but in this case there's (static)layout dependency between these boxes.
+ In order to figure out the static position of B, we have to have A laid out first. In order to lay out A before B,
+ B has to be preceded by A in ICB's ListHasSet.
+
+ Now full layout always guarantees the correct order.
+ However in case of partial layout since we don't run a full #2, the ListHasSet may end up having an unexpected order.
+ e.g.
+ <body><div id=A class=absolute><div id=B><div id=C class=fixed></div></div></div></body>
+
+ 1. The initial (full) layout produces the following (correct) order for the ICB's ListHasSet -> AC.
+ 2. A subsequent partial layout (e.g. triggered by A's position change) runs an in-flow layout on the <body> which
+ (re-)appends A to the ListHasSet (CA <- incorrect order). Now at this point we assume that the in-flow layout picks up B
+ which eventually (re-)appends C to the ListHashSet (AC <- correct order). However since B does not need layout, we just
+ stop at <body> which leaves us with an unexpected ListHashSet.
+ 3. As part of the ICB's out-of-flow layout, we pick C as the first box to lay out followed by A. However since C's static
+ position depends on A's position, we end up using stale geometry when computing C's static position.
+
+ This patch fixes this issue by ensuring the absolute positioned boxes always come first in the ICB's ListHasSet (note
+ that their order is not really important -see above. What's important is that a potential (as-if-static) containing block always
+ comes before the fixed boxes).
+
+ Test: fast/block/fixed-inside-absolute-positioned.html
+
+ * rendering/RenderBlock.cpp:
+ (WebCore::PositionedDescendantsMap::addDescendant):
+ (WebCore::RenderBlock::insertPositionedObject):
+
2022-04-13 Antti Koivisto <an...@apple.com>
[CSS Container Queries] Limit query range syntax
Modified: trunk/Source/WebCore/rendering/RenderBlock.cpp (292816 => 292817)
--- trunk/Source/WebCore/rendering/RenderBlock.cpp 2022-04-13 18:48:19 UTC (rev 292816)
+++ trunk/Source/WebCore/rendering/RenderBlock.cpp 2022-04-13 18:50:18 UTC (rev 292817)
@@ -153,8 +153,7 @@
class PositionedDescendantsMap {
public:
- enum class MoveDescendantToEnd { No, Yes };
- void addDescendant(const RenderBlock& containingBlock, RenderBox& positionedDescendant, MoveDescendantToEnd moveDescendantToEnd)
+ void addDescendant(const RenderBlock& containingBlock, RenderBox& positionedDescendant)
{
// Protect against double insert where a descendant would end up with multiple containing blocks.
auto* previousContainingBlock = m_containerMap.get(&positionedDescendant);
@@ -167,8 +166,28 @@
return makeUnique<TrackedRendererListHashSet>();
}).iterator->value;
- bool isNewEntry = moveDescendantToEnd == MoveDescendantToEnd::Yes ? descendants->appendOrMoveToLast(&positionedDescendant).isNewEntry
- : descendants->add(&positionedDescendant).isNewEntry;
+ auto isNewEntry = false;
+ if (!is<RenderView>(containingBlock) || descendants->isEmpty())
+ isNewEntry = descendants->add(&positionedDescendant).isNewEntry;
+ else if (positionedDescendant.isFixedPositioned())
+ isNewEntry = descendants->appendOrMoveToLast(&positionedDescendant).isNewEntry;
+ else {
+ auto ensureLayoutDepentBoxPosition = [&] {
+ // RenderView is a special containing block as it may hold both absolute and fixed positioned containing blocks.
+ // When a fixed positioned box is also a descendant of an absolute positioned box anchored to the RenderView,
+ // we have to make sure that the absolute positioned box is inserted before the fixed box to follow
+ // block layout dependency.
+ for (auto it = descendants->begin(); it != descendants->end(); ++it) {
+ if ((*it)->isFixedPositioned()) {
+ isNewEntry = descendants->insertBefore(it, &positionedDescendant).isNewEntry;
+ return;
+ }
+ }
+ isNewEntry = descendants->appendOrMoveToLast(&positionedDescendant).isNewEntry;
+ };
+ ensureLayoutDepentBoxPosition();
+ }
+
if (!isNewEntry) {
ASSERT(m_containerMap.contains(&positionedDescendant));
return;
@@ -1791,8 +1810,7 @@
ASSERT(posChildNeedsLayout() || view().frameView().layoutContext().isInLayout());
setPosChildNeedsLayoutBit(true);
}
- positionedDescendantsMap().addDescendant(*this, positioned, isRenderView() ? PositionedDescendantsMap::MoveDescendantToEnd::Yes
- : PositionedDescendantsMap::MoveDescendantToEnd::No);
+ positionedDescendantsMap().addDescendant(*this, positioned);
}
void RenderBlock::removePositionedObject(const RenderBox& rendererToRemove)
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
