Move fixed bounds for overlap to absolute space

When computing the bounding box for fixed-pos layers, the resulting
bounding box was stored on AncestorDependentCompositingInputs in
viewport space (absolute space adjusted by scroll offset). The scroll
offset that was used is the current scroll offset during compositing
inputs update time. Call this ScrollOffsetA. Later during compositing
layer assignment, when computing overlap, the fixed layer bounding box
is converted from viewport space to absolute space by adjusting it by
the current scroll offset. The expectation is that this 2nd scroll
offset is also ScrollOfffsetA. However, in some cases, a compositing
update will be requested that will skip compositing inputs update and
only do overlap and assignment. When the scroll offset changes between
computing the fixed layer's viewport space bounds in the previous
compositing inputs update and the later compositing assignment update,
the wrong scroll offset will be used and the fixed layer bounds will
fail to be converted to true absolute space, leading to overlap testing
errors.

This change addresses this issue by storing fixed layer bounds in
absolute space instead. Note that this issue does not happen when the
CompositingOptimizations feature is enabled as it doesn't use the cached
bounds from compositing inputs update.

Bug: 1124753
Change-Id: I85928386fd43ca0b5a86b34877ef2a4afdabd73a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2411693
Commit-Queue: Sam Fortiner <samfort@microsoft.com>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807955}

third_party/blink/renderer/core/paint/compositing/compositing_inputs_updater.cc

@@ -451,20 +451,16 @@ void CompositingInputsUpdater::UpdateAncestorDependentCompositingInputs(
   if (!RuntimeEnabledFeatures::CompositingOptimizationsEnabled()) {
     // The final value for |unclipped_absolute_bounding_box| needs to be
     // in absolute, unscrolled space, without any scroll applied.
-
     properties.unclipped_absolute_bounding_box =
         EnclosingIntRect(geometry_map_->AbsoluteRect(
             layer->BoundingBoxForCompositingOverlapTest()));
 
-    bool affected_by_scroll = root_layer_->GetScrollableArea() &&
-                              layer->IsAffectedByScrollOf(root_layer_);
-
-    // At ths point, |unclipped_absolute_bounding_box| is in viewport space.
-    // To convert to absolute space, add scroll offset for non-fixed layers.
-    if (affected_by_scroll) {
+    // At this point, |unclipped_absolute_bounding_box| is in viewport space.
+    // To convert to absolute space, add scroll offset. Note that even fixed
+    // layers are in viewport space due to expanding their bounding box to
+    // include the extent they could cover from scrolling to min/max offsets.
     properties.unclipped_absolute_bounding_box.Move(
         RoundedIntSize(root_layer_->GetScrollableArea()->GetScrollOffset()));
-    }
 
     // For sticky-positioned elements, the scroll offset is sometimes included
     // and sometimes not, depending on whether the sticky element is affixed or
@@ -483,15 +479,8 @@ void CompositingInputsUpdater::UpdateAncestorDependentCompositingInputs(
                              cache_slot, kIgnoreOverlayScrollbarSize,
                              kIgnoreOverflowClipAndScroll),
             clip_rect);
+    // |snapped_clip_rect| is in absolute space
     IntRect snapped_clip_rect = PixelSnappedIntRect(clip_rect.Rect());
-    // |snapped_clip_rect| is in absolute space space, but with scroll applied.
-    // To convert to absolute, unscrolled space, subtract scroll offsets for
-    // fixed layers.
-    if (root_layer_->GetScrollableArea() && !affected_by_scroll) {
-      snapped_clip_rect.Move(
-          RoundedIntSize(-root_layer_->GetScrollableArea()->GetScrollOffset()));
-    }
-
     properties.clipped_absolute_bounding_box =
         properties.unclipped_absolute_bounding_box;
     properties.clipped_absolute_bounding_box.Intersect(snapped_clip_rect);

third_party/blink/renderer/core/paint/compositing/compositing_requirements_updater.cc

@@ -353,20 +353,6 @@ void CompositingRequirementsUpdater::UpdateRecursive(
                            ? layer->ClippedAbsoluteBoundingBox()
                            : layer->UnclippedAbsoluteBoundingBox();
 
-  if (!RuntimeEnabledFeatures::CompositingOptimizationsEnabled()) {
-    PaintLayer* root_layer = layout_view_.Layer();
-    // |abs_bounds| does not include root scroller offset. For the purposes
-    // of overlap, this only matters for fixed-position objects, and their
-    // relative position to other elements. Therefore, it's still correct to,
-    // instead of adding scroll to all non-fixed elements, add a reverse scroll
-    // to ones that are fixed.
-    if (root_layer->GetScrollableArea() &&
-        !layer->IsAffectedByScrollOf(root_layer)) {
-      abs_bounds.Move(
-          RoundedIntSize(root_layer->GetScrollableArea()->GetScrollOffset()));
-    }
-  }
-
   absolute_descendant_bounding_box = abs_bounds;
   if (layer_can_be_composited && current_recursion_data.testing_overlap_ &&
       !RequiresCompositingOrSquashing(direct_reasons)) {

third_party/blink/renderer/core/paint/compositing/compositing_requirements_updater_test.cc

@@ -5,6 +5,7 @@
 #include "third_party/blink/renderer/core/paint/compositing/compositing_requirements_updater.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/renderer/core/paint/compositing/paint_layer_compositor.h"
 #include "third_party/blink/renderer/core/paint/paint_layer.h"
 #include "third_party/blink/renderer/core/paint/paint_layer_scrollable_area.h"
 #include "third_party/blink/renderer/core/testing/core_unit_test_helper.h"
@@ -234,4 +235,136 @@ TEST_F(CompositingRequirementsUpdaterTest,
                 ->GetCompositingReasons());
 }
 
+TEST_F(CompositingRequirementsUpdaterTest,
+       FixedCausesOverlapWithOnlyCompositingAssignmentUpdateNoOpt) {
+  // TODO(chrishtr): Remove this test when the CompositingOptimizations feature
+  // ships.
+  ScopedCompositingOptimizationsForTest feature_scope(false);
+  LoadAhem();
+
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      body {
+        height: 2000px;
+      }
+      #fixed {
+        position: fixed;
+        width: 50%;
+        height: 100px;
+      }
+      #target {
+        position: relative;
+        width: 25%;
+        height: 50px;
+      }
+      #changeme {
+        position: absolute;
+        top: 400px;
+      }
+    </style>
+    <div id=fixed></div>
+    <div id=target></div>
+    <div id=changeme>A</div>
+  )HTML");
+
+  // Scroll down so that relative isn't overlapping fixed unless fixed expanded
+  // bounds is considered.
+  GetDocument().View()->LayoutViewport()->SetScrollOffset(
+      ScrollOffset(0, 200), mojom::blink::ScrollType::kProgrammatic);
+  UpdateAllLifecyclePhasesForTest();
+
+  EXPECT_EQ(CompositingReason::kScrollDependentPosition,
+            ToLayoutBoxModelObject(GetLayoutObjectByElementId("fixed"))
+                ->Layer()
+                ->GetCompositingReasons());
+  EXPECT_EQ(CompositingReason::kOverlap,
+            ToLayoutBoxModelObject(GetLayoutObjectByElementId("target"))
+                    ->Layer()
+                    ->GetCompositingReasons() &
+                CompositingReason::kOverlap);
+
+  // Request a compositing update (assignment) without causing a compositing
+  // inputs update. This could be caused in a real webpage by a layout size
+  // change.
+  GetDocument()
+      .View()
+      ->GetLayoutView()
+      ->Compositor()
+      ->SetNeedsCompositingUpdate(
+          kCompositingUpdateAfterCompositingInputChange);
+  UpdateAllLifecyclePhasesForTest();
+
+  // 'target' should still be composited by overlapping with fixed.
+  EXPECT_EQ(CompositingReason::kOverlap,
+            ToLayoutBoxModelObject(GetLayoutObjectByElementId("target"))
+                    ->Layer()
+                    ->GetCompositingReasons() &
+                CompositingReason::kOverlap);
+}
+
+TEST_F(CompositingRequirementsUpdaterTest,
+       FixedCausesOverlapWithOnlyCompositingAssignmentUpdate) {
+  ScopedCompositingOptimizationsForTest feature_scope(true);
+  LoadAhem();
+
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      body {
+        height: 2000px;
+      }
+      #fixed {
+        position: fixed;
+        width: 50%;
+        height: 100px;
+      }
+      #target {
+        position: relative;
+        width: 25%;
+        height: 50px;
+      }
+      #changeme {
+        position: absolute;
+        top: 400px;
+      }
+    </style>
+    <div id=fixed></div>
+    <div id=target></div>
+    <div id=changeme>A</div>
+  )HTML");
+
+  // Scroll down so that relative isn't overlapping fixed unless fixed expanded
+  // bounds is considered.
+  GetDocument().View()->LayoutViewport()->SetScrollOffset(
+      ScrollOffset(0, 200), mojom::blink::ScrollType::kProgrammatic);
+  UpdateAllLifecyclePhasesForTest();
+
+  EXPECT_EQ(CompositingReason::kScrollDependentPosition,
+            ToLayoutBoxModelObject(GetLayoutObjectByElementId("fixed"))
+                ->Layer()
+                ->GetCompositingReasons());
+  EXPECT_EQ(CompositingReason::kOverlap,
+            ToLayoutBoxModelObject(GetLayoutObjectByElementId("target"))
+                    ->Layer()
+                    ->GetCompositingReasons() &
+                CompositingReason::kOverlap);
+
+  // Request a compositing update (assignment) without causing a compositing
+  // inputs update. This could be caused in a real webpage by a layout size
+  // change.
+  GetDocument()
+      .View()
+      ->GetLayoutView()
+      ->Compositor()
+      ->SetNeedsCompositingUpdate(
+          kCompositingUpdateAfterCompositingInputChange);
+  UpdateAllLifecyclePhasesForTest();
+
+  // 'target' should still be composited by overlapping with fixed.
+  EXPECT_EQ(CompositingReason::kOverlap,
+            ToLayoutBoxModelObject(GetLayoutObjectByElementId("target"))
+                    ->Layer()
+                    ->GetCompositingReasons() &
+                CompositingReason::kOverlap);
+}
+
 }  // namespace blink