Revert "Move fixed bounds for overlap to absolute space"

This reverts commit 8c72e30b.

Reason for revert: Breaks overlap detection between non-fixed-pos siblings that are children of fixed-pos layers. See https://crbug.com/1137974

Original change's description:
> 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}

TBR=pdr@chromium.org,samfort@microsoft.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1124753
Change-Id: I60fedcd48097a1e71f6ceea14707a01df7c29e28
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2504578Reviewed-by: Sam Fortiner <samfort@microsoft.com>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Sam Fortiner <samfort@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#821600}

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

@@ -455,16 +455,20 @@ 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()));
 
-    // 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()));
+    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) {
+      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,8 +487,15 @@ 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

@@ -356,6 +356,20 @@ 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,7 +5,6 @@
 #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"
@@ -235,136 +234,4 @@ 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