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: Canonical link: https://commits.webkit.org/249626@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@292855 268f45cc-cd09-0410-ab3c-d52691b4dbfc
haiku · Apr 14, 2022 · c59aec5 · c59aec5
1 parent a7330d6
commit c59aec5
Show file tree

Hide file tree

Showing 7 changed files with 131 additions and 10 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,15 @@
+2022-04-13  Alan Bujtas  <zalan@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  Truitt Savell  <tsavell@apple.com>
 
         [ Monterey WK2 ] media/media-source/media-source-webm-vorbis-partial.html is a constant failure

diff --git a/LayoutTests/fast/block/fixed-inside-absolute-positioned-expected.html b/LayoutTests/fast/block/fixed-inside-absolute-positioned-expected.html
@@ -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>
diff --git a/LayoutTests/fast/block/fixed-inside-absolute-positioned.html b/LayoutTests/fast/block/fixed-inside-absolute-positioned.html
@@ -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>
diff --git a/LayoutTests/platform/mac-wk1/TestExpectations b/LayoutTests/platform/mac-wk1/TestExpectations
@@ -1046,8 +1046,6 @@ imported/w3c/web-platform-tests/credential-management/ [ Skip ]
 
 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 ]

diff --git a/LayoutTests/platform/mac/TestExpectations b/LayoutTests/platform/mac/TestExpectations
@@ -2294,5 +2294,3 @@ http/tests/webgpu [ Pass ]
 
 # 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 ]
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,63 @@
+2022-04-13  Alan Bujtas  <zalan@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  Chris Dumez  <cdumez@apple.com>
 
         Use [AtomString] where appropriate in IDL files for performance

diff --git a/Source/WebCore/rendering/RenderBlock.cpp b/Source/WebCore/rendering/RenderBlock.cpp
@@ -28,6 +28,7 @@
 #include "DocumentInlines.h"
 #include "Editor.h"
 #include "Element.h"
+#include "ElementInlines.h"
 #include "EventRegion.h"
 #include "FloatQuad.h"
 #include "Frame.h"
@@ -153,8 +154,7 @@ static void removeFromTrackedRendererMaps(RenderBox& descendant)
 
 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 +167,28 @@ class PositionedDescendantsMap {
             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() || isInTopLayerOrBackdrop(positionedDescendant.style(), positionedDescendant.element()))
+            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 +1811,7 @@ void RenderBlock::insertPositionedObject(RenderBox& positioned)
         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)
Original file line number	Diff line number	Diff line change
Expand Up		@@ -2294,5 +2294,3 @@ http/tests/webgpu [ Pass ]

		# 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 ]