ScrollAnchor: Only allow scroll anchor to be "just off screen"

During scroll anchoring, before layout, we determine an on-screen
anchor and then process the layout and ensure that one of the top
corners (left or right depending on writing mode) is positioned
in the same spot as before. However, this causes problems if the
size of the anchor also changes (with visibility) to be far off
screen. Instead, only allow the anchor to move "just off screen",
with one of its edges touching the viewport area.

This is only applied to content-visibility: auto elements that
have a pending layout.

The effect is similar: the object is moved off screen, but it prevents
unstable behavior with content-visibility: auto.

Note that the intent of scroll anchoring is to keep the visible
contents from shifting. However, in this case the contents are
shifting anyway (since the anchor is off screen), so manipulating
exactly how far off-screen should be ok. It is also limited to
only content-visibility auto elements.

R=chrishtr@chromium.org

Bug: 1138801
Change-Id: I1fbc5f8b634563c770db29bf30ef6d409d810614
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2481348Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: vmpstr <vmpstr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818681}

third_party/blink/renderer/core/display_lock/display_lock_context.cc

@@ -365,6 +365,7 @@ bool DisplayLockContext::ShouldLayoutChildren() const {
 void DisplayLockContext::DidLayoutChildren() {
   // Since we did layout on children already, we'll clear this.
   child_layout_was_blocked_ = false;
+  had_lifecycle_update_since_last_unlock_ = true;
 }
 
 bool DisplayLockContext::ShouldPrePaintChildren() const {
@@ -510,6 +511,7 @@ void DisplayLockContext::NotifyForcedUpdateScopeEnded() {
 void DisplayLockContext::Unlock() {
   DCHECK(IsLocked());
   is_locked_ = false;
+  had_lifecycle_update_since_last_unlock_ = false;
   UpdateDocumentBookkeeping(true, !activatable_mask_, false,
                             !activatable_mask_);
 

third_party/blink/renderer/core/display_lock/display_lock_context.h

@@ -210,6 +210,11 @@ class CORE_EXPORT DisplayLockContext final
   // Debugging functions.
   String RenderAffectingStateToString() const;
 
+  bool IsAuto() const { return state_ == EContentVisibility::kAuto; }
+  bool HadLifecycleUpdateSinceLastUnlock() const {
+    return had_lifecycle_update_since_last_unlock_;
+  }
+
  private:
   // Give access to |NotifyForcedUpdateScopeStarted()| and
   // |NotifyForcedUpdateScopeEnded()|.
@@ -396,6 +401,8 @@ class CORE_EXPORT DisplayLockContext final
   bool render_affecting_state_[static_cast<int>(
       RenderAffectingState::kNumRenderAffectingStates)] = {false};
   int keep_unlocked_count_ = 0;
+
+  bool had_lifecycle_update_since_last_unlock_ = false;
 };
 
 }  // namespace blink

third_party/blink/renderer/core/display_lock/display_lock_context_test.cc

@@ -3094,23 +3094,153 @@ TEST_F(DisplayLockContextRenderingTest,
   EXPECT_TRUE(element->GetDisplayLockContext()->IsLocked());
   EXPECT_EQ(GetDocument().scrollingElement()->scrollTop(), 29000.);
 
-  // It again gets unlocked and shrink.
+  // It again gets unlocked and shrink. This time scroll anchoring puts it right
+  // off the edge of the screen.
   UpdateAllLifecyclePhasesForTest();
   EXPECT_FALSE(element->GetDisplayLockContext()->IsLocked());
-  EXPECT_EQ(GetDocument().scrollingElement()->scrollTop(), 29000.);
+  EXPECT_EQ(GetDocument().scrollingElement()->scrollTop(), 23008.);
+
+  // On the next update we select the following element as an anchor and the
+  // scroll offset doesn't change.
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_FALSE(element->GetDisplayLockContext()->IsLocked());
+  EXPECT_EQ(GetDocument().scrollingElement()->scrollTop(), 23008.);
+
+  // Subsequent updates are in a stable state.
+  for (int i = 0; i < 5; ++i) {
+    UpdateAllLifecyclePhasesForTest();
+    EXPECT_FALSE(element->GetDisplayLockContext()->IsLocked());
+    EXPECT_EQ(GetDocument().scrollingElement()->scrollTop(), 23008.);
+  }
+}
+
+TEST_F(DisplayLockContextRenderingTest,
+       AutoReachesStableStateOnContentSmallerThanLockedSizeInLtr) {
+  SetHtmlInnerHTML(R"HTML(
+    <style>
+      body { writing-mode: vertical-lr }
+      .spacer { block-size: 20000px; }
+      .auto {
+        content-visibility: auto;
+        contain-intrinsic-size: 20000px 1px;
+      }
+      .auto > div {
+        block-size: 3000px;
+      }
+    </style>
+
+    <div class=spacer></div>
+    <div id=e1 class=auto><div>content</div></div>
+    <div id=e2 class=auto><div>content</div></div>
+    <div class=spacer></div>
+  )HTML");
+
+  UpdateAllLifecyclePhasesForTest();
+
+  GetDocument().scrollingElement()->setScrollLeft(29000);
+
+  Element* element = GetDocument().getElementById("e1");
+
+  // Note that this test also unlock/relocks #e2 but we only care about #e1
+  // settling into a steady state.
+
+  // Initially we start with locked in the viewport.
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_TRUE(element->GetDisplayLockContext()->IsLocked());
+  EXPECT_EQ(GetDocument().scrollingElement()->scrollLeft(), 29000.);
+
+  // It gets unlocked because it's in the viewport.
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_FALSE(element->GetDisplayLockContext()->IsLocked());
+  EXPECT_EQ(GetDocument().scrollingElement()->scrollLeft(), 29000.);
 
-  // On the next relock we select the following element as an anchor and thus
-  // the scroll top changes to be higher.
+  // By unlocking it, it shrinks so next time it gets relocked.
   UpdateAllLifecyclePhasesForTest();
   EXPECT_TRUE(element->GetDisplayLockContext()->IsLocked());
-  EXPECT_GT(GetDocument().scrollingElement()->scrollTop(), 29000.);
+  EXPECT_EQ(GetDocument().scrollingElement()->scrollLeft(), 29000.);
+
+  // It again gets unlocked and shrink. This time scroll anchoring puts it right
+  // off the edge of the screen.
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_FALSE(element->GetDisplayLockContext()->IsLocked());
+  EXPECT_EQ(GetDocument().scrollingElement()->scrollLeft(), 23008.);
+
+  // On the next update we select the following element as an anchor and the
+  // scroll offset doesn't change.
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_FALSE(element->GetDisplayLockContext()->IsLocked());
+  EXPECT_EQ(GetDocument().scrollingElement()->scrollLeft(), 23008.);
+
+  // Subsequent updates are in a stable state.
+  for (int i = 0; i < 5; ++i) {
+    UpdateAllLifecyclePhasesForTest();
+    EXPECT_FALSE(element->GetDisplayLockContext()->IsLocked());
+    EXPECT_EQ(GetDocument().scrollingElement()->scrollLeft(), 23008.);
+  }
+}
+
+TEST_F(DisplayLockContextRenderingTest,
+       AutoReachesStableStateOnContentSmallerThanLockedSizeInRtl) {
+  SetHtmlInnerHTML(R"HTML(
+    <style>
+      body { writing-mode: vertical-rl }
+      .spacer { block-size: 20000px; }
+      .auto {
+        content-visibility: auto;
+        contain-intrinsic-size: 20000px 1px;
+      }
+      .auto > div {
+        block-size: 3000px;
+      }
+    </style>
+
+    <div class=spacer></div>
+    <div id=e1 class=auto><div>content</div></div>
+    <div id=e2 class=auto><div>content</div></div>
+    <div class=spacer></div>
+  )HTML");
+
+  UpdateAllLifecyclePhasesForTest();
+
+  GetDocument().scrollingElement()->setScrollLeft(-29000);
+
+  Element* element = GetDocument().getElementById("e1");
+
+  // Note that this test also unlock/relocks #e2 but we only care about #e1
+  // settling into a steady state.
+
+  // Initially we start with locked in the viewport.
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_TRUE(element->GetDisplayLockContext()->IsLocked());
+  EXPECT_EQ(GetDocument().scrollingElement()->scrollLeft(), -29000.);
+
+  // It gets unlocked because it's in the viewport.
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_FALSE(element->GetDisplayLockContext()->IsLocked());
+  EXPECT_EQ(GetDocument().scrollingElement()->scrollLeft(), -29000.);
+
+  // By unlocking it, it shrinks so next time it gets relocked.
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_TRUE(element->GetDisplayLockContext()->IsLocked());
+  EXPECT_EQ(GetDocument().scrollingElement()->scrollLeft(), -29000.);
+
+  // It again gets unlocked and shrink. This time scroll anchoring puts it right
+  // off the edge of the screen.
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_FALSE(element->GetDisplayLockContext()->IsLocked());
+  EXPECT_EQ(GetDocument().scrollingElement()->scrollLeft(), -23008.);
+
+  // On the next update we select the following element as an anchor and the
+  // scroll offset doesn't change.
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_FALSE(element->GetDisplayLockContext()->IsLocked());
+  EXPECT_EQ(GetDocument().scrollingElement()->scrollLeft(), -23008.);
 
-  // Subsequent updates no longer unlock the element because even if its locked
-  // state it is far enough off-screen.
+  // Subsequent updates are in a stable state.
   for (int i = 0; i < 5; ++i) {
     UpdateAllLifecyclePhasesForTest();
-    EXPECT_TRUE(element->GetDisplayLockContext()->IsLocked());
-    EXPECT_GT(GetDocument().scrollingElement()->scrollTop(), 29000.);
+    EXPECT_FALSE(element->GetDisplayLockContext()->IsLocked());
+    EXPECT_EQ(GetDocument().scrollingElement()->scrollLeft(), -23008.);
   }
 }
 

third_party/blink/renderer/core/display_lock/display_lock_utilities.cc

@@ -611,4 +611,12 @@ bool DisplayLockUtilities::PrePaintBlockedInParentFrame(LayoutView* view) {
   return false;
 }
 
+bool DisplayLockUtilities::IsAutoWithoutLayout(const LayoutObject& object) {
+  auto* context = object.GetDisplayLockContext();
+  if (!context)
+    return false;
+  return !context->IsLocked() && context->IsAuto() &&
+         !context->HadLifecycleUpdateSinceLastUnlock();
+}
+
 }  // namespace blink

third_party/blink/renderer/core/display_lock/display_lock_utilities.h

@@ -174,6 +174,8 @@ class CORE_EXPORT DisplayLockUtilities {
 
   static bool PrePaintBlockedInParentFrame(LayoutView* layout_view);
 
+  static bool IsAutoWithoutLayout(const LayoutObject& object);
+
  private:
   static bool UpdateStyleAndLayoutForRangeIfNeeded(
       const EphemeralRangeInFlatTree& range,

third_party/blink/renderer/core/layout/scroll_anchor.cc

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "third_party/blink/renderer/core/css/css_markup.h"
+#include "third_party/blink/renderer/core/display_lock/display_lock_utilities.h"
 #include "third_party/blink/renderer/core/dom/element_traversal.h"
 #include "third_party/blink/renderer/core/dom/nth_index_cache.h"
 #include "third_party/blink/renderer/core/dom/static_node_list.h"
@@ -273,6 +274,21 @@ static const String ComputeUniqueSelector(Node* anchor_node) {
   return builder.ToString();
 }
 
+static LayoutRect GetVisibleRect(ScrollableArea* scroller) {
+  auto visible_rect =
+      ScrollerLayoutBox(scroller)->OverflowClipRect(LayoutPoint());
+
+  const ComputedStyle* style = ScrollerLayoutBox(scroller)->Style();
+  LayoutRectOutsets scroll_padding(
+      MinimumValueForLength(style->ScrollPaddingTop(), visible_rect.Height()),
+      MinimumValueForLength(style->ScrollPaddingRight(), visible_rect.Width()),
+      MinimumValueForLength(style->ScrollPaddingBottom(),
+                            visible_rect.Height()),
+      MinimumValueForLength(style->ScrollPaddingLeft(), visible_rect.Width()));
+  visible_rect.Contract(scroll_padding);
+  return visible_rect;
+}
+
 ScrollAnchor::ExamineResult ScrollAnchor::Examine(
     const LayoutObject* candidate) const {
   if (candidate == ScrollerLayoutBox(scroller_))
@@ -296,17 +312,7 @@ ScrollAnchor::ExamineResult ScrollAnchor::Examine(
     return ExamineResult(kSkip);
 
   LayoutRect candidate_rect = RelativeBounds(candidate, scroller_);
-  LayoutRect visible_rect =
-      ScrollerLayoutBox(scroller_)->OverflowClipRect(LayoutPoint());
-
-  const ComputedStyle* style = ScrollerLayoutBox(scroller_)->Style();
-  LayoutRectOutsets scroll_padding(
-      MinimumValueForLength(style->ScrollPaddingTop(), visible_rect.Height()),
-      MinimumValueForLength(style->ScrollPaddingRight(), visible_rect.Width()),
-      MinimumValueForLength(style->ScrollPaddingBottom(),
-                            visible_rect.Height()),
-      MinimumValueForLength(style->ScrollPaddingLeft(), visible_rect.Width()));
-  visible_rect.Contract(scroll_padding);
+  LayoutRect visible_rect = GetVisibleRect(scroller_);
 
   bool occupies_space =
       candidate_rect.Width() > 0 && candidate_rect.Height() > 0;
@@ -331,6 +337,8 @@ void ScrollAnchor::FindAnchor() {
     anchor_object_->SetIsScrollAnchorObject();
     saved_relative_offset_ =
         ComputeRelativeOffset(anchor_object_, scroller_, corner_);
+    anchor_is_cv_auto_without_layout_ =
+        DisplayLockUtilities::IsAutoWithoutLayout(*anchor_object_);
   }
 }
 
@@ -506,16 +514,43 @@ IntSize ScrollAnchor::ComputeAdjustment() const {
                                                        scroller_, corner_)) -
                   RoundedIntSize(saved_relative_offset_);
 
+  LayoutRect anchor_rect = RelativeBounds(anchor_object_, scroller_);
+
   // Only adjust on the block layout axis.
   const LayoutBox* scroller_box = ScrollerLayoutBox(scroller_);
-  if (scroller_box->IsHorizontalWritingMode()) {
+  if (scroller_box->IsHorizontalWritingMode())
     delta.SetWidth(0);
-  } else {
-    // If block direction is flipped, delta is a logical value, so flip it to
-    // make it physical.
-    if (scroller_box->HasFlippedBlocksWritingMode())
-      delta.SetWidth(-delta.Width());
+  else
     delta.SetHeight(0);
+
+  if (anchor_is_cv_auto_without_layout_) {
+    // See the effect delta would have on the anchor rect.
+    // If the anchor is now off-screen (in block direction) then make sure it's
+    // just at the edge.
+    anchor_rect.Move(-delta);
+    if (scroller_box->IsHorizontalWritingMode()) {
+      if (anchor_rect.MaxY() < 0)
+        delta.SetHeight(delta.Height() + anchor_rect.MaxY().ToInt());
+    } else {
+      // For the flipped blocks writing mode, we need to adjust the offset to
+      // align the opposite edge of the block (MaxX edge instead of X edge).
+      if (scroller_box->HasFlippedBlocksWritingMode()) {
+        auto visible_rect = GetVisibleRect(scroller_);
+        if (anchor_rect.X() > visible_rect.MaxX()) {
+          delta.SetWidth(delta.Width() - (anchor_rect.X().ToInt() -
+                                          visible_rect.MaxX().ToInt()));
+        }
+      } else if (anchor_rect.MaxX() < 0) {
+        delta.SetWidth(delta.Width() + anchor_rect.MaxX().ToInt());
+      }
+    }
+  }
+
+  // If block direction is flipped, delta is a logical value, so flip it to
+  // make it physical.
+  if (!scroller_box->IsHorizontalWritingMode() &&
+      scroller_box->HasFlippedBlocksWritingMode()) {
+    delta.SetWidth(-delta.Width());
   }
   return delta;
 }
@@ -528,6 +563,13 @@ void ScrollAnchor::Adjust() {
   if (!anchor_object_)
     return;
   IntSize adjustment = ComputeAdjustment();
+
+  // We should pick a new anchor if we had an unlaid-out content-visibility
+  // auto. It should have been laid out, so if it is still the best candidate,
+  // we will select it without this boolean set.
+  if (anchor_is_cv_auto_without_layout_)
+    ClearSelf();
+
   if (adjustment.IsZero())
     return;
 

third_party/blink/renderer/core/layout/scroll_anchor.h

@@ -177,6 +177,11 @@ class CORE_EXPORT ScrollAnchor final {
   // True iff an adjustment check has been queued with the FrameView but not yet
   // performed.
   bool queued_;
+
+  // This is set to true if the last anchor we have selected is a
+  // 'content-visibility: auto' element that did not yet have a layout after
+  // becoming visible.
+  bool anchor_is_cv_auto_without_layout_ = false;
 };
 
 }  // namespace blink