Incorrectly positioned out-of-flow box when layout boundary is present

alanbaradlay · alanbaradlay · commit 0effd07b7ef3 · 2023-04-01T06:51:34.000-07:00
https://bugs.webkit.org/show_bug.cgi?id=254666 Reviewed by Antti Koivisto. Consider the following case: <containing-block> <layout-boundary> Inline-content <out-of-flow-box> </layout-boundary> </containing-block> e.g. <div style="overflow: hidden"> some text <div style="position: absolute"></div> </div> 1. "inline content" gets mutated and the associated renderers are marked dirty. During #1 we climb the ancestor chain and mark containing blocks dirty to ensure the subsequent layout has a correct entry point. We either (most of the time) stop at the ICB (RenderView) during this walk or at a layout-boundary. 2. Subsequent layout is initiated starting at layout-boundary. The geometry of the freshly laid out inline-content may affect the out-of-flow-box's static position. In inline layout code (both legacy and IFC) at this point we only set the "static" position assuming that layout eventually reaches the out-of-flow-box's containing block which would set the final top/left coords. However since this layout is bound to layout-boundary's subtree, we never get to the containing-block. While this is an invalidation bug where we fail to mark the containing-block dirty (by not stopping at layout-boundary), it's expensive to figure out if there's a descendent of the layout-boundary with an ancestor containing block (outside of layout-boundary's subtree). Instead set the out-of-flow-box's coordinates here at inline layout and let the containing block update it as part of the normal layout flow (when we actually get to the containing block). This is technically correct since this renderer's position is its static position until the containing block updates it as applicable (and the special "static position" handling could be considered as a render tree artifact). * LayoutTests/fast/block/positioning/static_out_of_flow_inside_layout_boundary-expected.html: Added. * LayoutTests/fast/block/positioning/static_out_of_flow_inside_layout_boundary.html: Added. * Source/WebCore/layout/integration/inline/LayoutIntegrationLineLayout.cpp: (WebCore::LayoutIntegration::LineLayout::updateRenderTreePositions): Canonical link: https://commits.webkit.org/262470@main
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
@@ -3671,6 +3671,7 @@ webkit.org/b/203451 imported/w3c/web-platform-tests/css/css-position/static-posi
 webkit.org/b/203451 imported/w3c/web-platform-tests/css/css-position/static-position/vrl-rtl-rtl.html [ ImageOnlyFailure ]
 
 webkit.org/b/254666 [ Debug ] imported/w3c/web-platform-tests/css/css-position/position-absolute-crash-chrome-005.html [ Skip ]
+webkit.org/b/254666 [ Debug ] fast/block/positioning/static_out_of_flow_inside_layout_boundary.html [ Skip ]
 
 webkit.org/b/203606 imported/w3c/web-platform-tests/css/css-display/display-change-object-iframe.html [ ImageOnlyFailure ]
 webkit.org/b/203606 imported/w3c/web-platform-tests/css/css-display/display-flow-root-list-item-001.html [ ImageOnlyFailure ]
diff --git a/LayoutTests/fast/block/positioning/static_out_of_flow_inside_layout_boundary-expected.html b/LayoutTests/fast/block/positioning/static_out_of_flow_inside_layout_boundary-expected.html
@@ -0,0 +1,8 @@
+<style>
+div {
+  width: 100px;
+  height: 50px;
+  background-color: green;
+}
+</style>
+<div></div>
diff --git a/LayoutTests/fast/block/positioning/static_out_of_flow_inside_layout_boundary.html b/LayoutTests/fast/block/positioning/static_out_of_flow_inside_layout_boundary.html
@@ -0,0 +1,31 @@
+<style>
+.container {
+  width: 100px;
+  height: 50px;
+  background-color: red;
+  overflow: hidden;
+}
+
+.out_of_flow {
+  display: inline;
+  width: 100px;
+  height: 50px;
+  background-color: green;
+  position: absolute;
+}
+
+#remove_this {
+  display: inline-block;
+  width: 100px;
+  height: 10px;
+  background-color: blue;
+}
+</style>
+<div class=container>
+  <div id=remove_this></div>
+  <div class=out_of_flow></div>
+</div>
+<script>
+document.body.offsetHeight;
+remove_this.style.display = "none";
+</script>
diff --git a/Source/WebCore/layout/integration/inline/LayoutIntegrationLineLayout.cpp b/Source/WebCore/layout/integration/inline/LayoutIntegrationLineLayout.cpp
@@ -666,19 +666,27 @@ void LineLayout::updateRenderTreePositions(const Vector<LineAdjustment>& lineAdj
 
             renderer.setLocation(Layout::BoxGeometry::borderBoxRect(visualGeometry).topLeft());
             renderer.repaint();
+            continue;
         }
 
         if (layoutBox.isOutOfFlowPositioned()) {
             ASSERT(renderer.layer());
             auto& layer = *renderer.layer();
             auto logicalBorderBoxRect = LayoutRect { Layout::BoxGeometry::borderBoxRect(logicalGeometry) };
 
-            if (layoutBox.style().isOriginalDisplayInlineType())
+            if (layoutBox.style().isOriginalDisplayInlineType()) {
+                auto previousStaticPosition = LayoutPoint { layer.staticInlinePosition(), layer.staticBlockPosition() };
                 blockFlow.setStaticInlinePositionForChild(renderer, logicalBorderBoxRect.y(), logicalBorderBoxRect.x());
 
+                auto delta = logicalBorderBoxRect.location() - previousStaticPosition;
+                // A non-statically positioned out-of-flow box's layout will override the top/left values we are setting here.
+                renderer.move(delta.width(), delta.height());
+            }
+
             layer.setStaticBlockPosition(logicalBorderBoxRect.y());
             layer.setStaticInlinePosition(logicalBorderBoxRect.x());
 
+            // FIXME: Figure out if this is really needed (see webkit.org/b/254666).
             if (layoutBox.style().hasStaticInlinePosition(renderer.isHorizontalWritingMode()))
                 renderer.setChildNeedsLayout(MarkOnlyThis);
             continue;
