Ignore layout shift when visibility:hidden becomes visible

We should ignore layout shift if the object was ineligible for layout
shift tracking during the previous paint invalidation. Add
LayoutObject::ShouldSkipNextLayoutShiftTracking() and set it during
paint invalidation if the object is ineligible for layout shift
tracking. In the next paint invalidation, if the flag is true, skip
layout shift tracking and reset the flag.

This also applies to content-visibility:auto for which we cleared
LayoutBox's previous size to prevent layout shift tracking for the next
cycle. The new flag is basically equivalent to the old method, but also
covers more cases, e.g. the old method would fail if the object had
visual overflow because PreviousPhysicalVisualOverflowRect() was not
based on PreviousSize() and empty.

Bug: 1152869
Change-Id: I000489dd8093dab8d2ab3605ad4ce3bd39fd11b0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2591367Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837627}

docs/speed/metrics_changelog/2020_12_cls.md

+# Cumulative Layout Shift Changes in Chrome 89
+
+## Changes in Chrome 89
+
+### Ignore layout shift when visibility:hidden becomes visible
+
+We have been always ignoring layout shifts when the element has
+visibility:hidden. However before Chrome 89, if the element having
+visibility:hidden became visible and shift at the same time, we reported layout
+shift. Now we also consider the previous visibility and ignore layout shift in
+the case.
+[Source code for this change](https://chromium-review.googlesource.com/c/chromium/src/+/2591367)
+
+## How does this affect a site's metrics?
+
+All of these changes only affect sites with specific types of content. Here are
+the specifics for each change:
+
+### Ignore layout shift when visibility:hidden becomes visible
+
+Sites using visibility:hidden to hide layout changes may see a decrease in
+their Cumulative Layout Shift scores.
+
+## When were users affected?
+
+Chrome 89 is currently scheduled to be released the week of March 2, 2021.

docs/speed/metrics_changelog/cls.md

@@ -2,6 +2,8 @@
 
 This is a list of changes to [Cumulative Layout Shift](https://web.dev/cls).
 
+* Chrome 89
+  * Metric definition improvement: [Ignore layout shift when visibility:hidden becomes visible](2020_12_cls.md)
 * Chrome 88
   * Metric definition improvement: [Cumulative layout shift properly detects shifts of fixed position elements](2020_11_cls.md)
   * Metric definition improvement: [Cumulative layout shift properly detects shifts of descendents of a sticky element](2020_11_cls.md)

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

@@ -3128,6 +3128,10 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
     }
 #endif
 
+    void SetShouldSkipNextLayoutShiftTracking(bool b) {
+      layout_object_.SetShouldSkipNextLayoutShiftTracking(b);
+    }
+
     FragmentData& FirstFragment() { return layout_object_.fragment_; }
 
     void EnsureId() { layout_object_.fragment_.EnsureId(); }
@@ -3340,6 +3344,13 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
     return bitfields_.TransformAffectsVectorEffect();
   }
 
+  bool ShouldSkipNextLayoutShiftTracking() const {
+    return bitfields_.ShouldSkipNextLayoutShiftTracking();
+  }
+  void SetShouldSkipNextLayoutShiftTracking(bool b) {
+    bitfields_.SetShouldSkipNextLayoutShiftTracking(b);
+  }
+
  protected:
   // Identifiers for each of LayoutObject subclasses.
   // The identifier name for blink::LayoutFoo should be kLayoutObjectFoo.
@@ -3770,6 +3781,7 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
           is_layout_ng_object_for_list_marker_image_(false),
           is_table_column_constraints_dirty_(false),
           transform_affects_vector_effect_(false),
+          should_skip_next_layout_shift_tracking_(true),
           positioned_state_(kIsStaticallyPositioned),
           selection_state_(static_cast<unsigned>(SelectionState::kNone)),
           subtree_paint_property_update_reasons_(
@@ -4084,6 +4096,11 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
     ADD_BOOLEAN_BITFIELD(transform_affects_vector_effect_,
                          TransformAffectsVectorEffect);
 
+    // Whether to skip layout shift tracking in the next paint invalidation.
+    // See PaintInvalidator::UpdateLayoutShiftTracking().
+    ADD_BOOLEAN_BITFIELD(should_skip_next_layout_shift_tracking_,
+                         ShouldSkipNextLayoutShiftTracking);
+
    private:
     // This is the cached 'position' value of this object
     // (see ComputedStyle::position).

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

@@ -170,6 +170,11 @@ bool LayoutShiftTracker::NeedsToTrack(const LayoutObject& object) const {
   if (!object.IsBox())
     return false;
 
+  if (auto* display_lock_context = object.GetDisplayLockContext()) {
+    if (display_lock_context->IsAuto() && display_lock_context->IsLocked())
+      return false;
+  }
+
   // Don't report shift of anonymous objects. Will report the children because
   // we want report real DOM nodes.
   if (object.IsAnonymous())
@@ -178,7 +183,7 @@ bool LayoutShiftTracker::NeedsToTrack(const LayoutObject& object) const {
   if (object.StyleRef().Visibility() != EVisibility::kVisible)
     return false;
 
-  // Ignore sticky-positioend objects that move on scroll.
+  // Ignore sticky-positioned objects that move on scroll.
   // TODO(skobes): Find a way to detect when these objects shift.
   if (object.IsStickyPositioned())
     return false;
@@ -675,7 +680,8 @@ void ReattachHookScope::NotifyDetach(const Node& node) {
   if (!top_)
     return;
   auto* layout_object = node.GetLayoutObject();
-  if (!layout_object || !layout_object->IsBox())
+  if (!layout_object || layout_object->ShouldSkipNextLayoutShiftTracking() ||
+      !layout_object->IsBox())
     return;
 
   auto& map = top_->geometries_before_detach_;
@@ -708,6 +714,7 @@ void ReattachHookScope::NotifyAttach(const Node& node) {
       .SetPreviousGeometryForLayoutShiftTracking(
           iter->value.paint_offset, iter->value.size,
           iter->value.visual_overflow_rect);
+  layout_object->SetShouldSkipNextLayoutShiftTracking(false);
 }
 
 }  // namespace blink

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

@@ -538,10 +538,19 @@ TEST_F(LayoutShiftTrackerTest,
   EXPECT_EQ(LayoutSize(100, 100), offscreen->Size());
   EXPECT_EQ(LayoutSize(100, 1), onscreen->Size());
 
+  // Move |offscreen| (which is visible and unlocked now), for which we should
+  // report layout shift.
+  To<Element>(offscreen->GetNode())
+      ->setAttribute(html_names::kStyleAttr,
+                     "position: relative; top: 100100px");
+  UpdateAllLifecyclePhasesForTest();
+  auto score = GetLayoutShiftTracker().Score();
+  EXPECT_GT(score, 0);
+
   // Now scroll the element back off-screen.
   GetDocument().domWindow()->scrollTo(0, 0);
   UpdateAllLifecyclePhasesForTest();
-  EXPECT_FLOAT_EQ(0, GetLayoutShiftTracker().Score());
+  EXPECT_FLOAT_EQ(score, GetLayoutShiftTracker().Score());
   EXPECT_EQ(LayoutSize(100, 100), offscreen->Size());
   EXPECT_EQ(LayoutSize(100, 1), onscreen->Size());
 
@@ -551,7 +560,7 @@ TEST_F(LayoutShiftTrackerTest,
   offscreen = To<LayoutBox>(GetLayoutObjectByElementId("offscreen"));
   onscreen = To<LayoutBox>(GetLayoutObjectByElementId("onscreen"));
 
-  EXPECT_FLOAT_EQ(0, GetLayoutShiftTracker().Score());
+  EXPECT_FLOAT_EQ(score, GetLayoutShiftTracker().Score());
   EXPECT_EQ(LayoutSize(100, 1), offscreen->Size());
   EXPECT_EQ(LayoutSize(100, 100), onscreen->Size());
 }

third_party/blink/renderer/core/paint/box_paint_invalidator.cc

@@ -397,17 +397,6 @@ bool BoxPaintInvalidator::NeedsToSavePreviousContentBoxRect() {
   return false;
 }
 
-static bool IsContentVisibilityAutoAndHidden(const LayoutBox& box) {
-  auto* display_lock_context = box.GetDisplayLockContext();
-  if (!display_lock_context)
-    return false;
-
-  if (!display_lock_context->IsAuto())
-    return false;
-
-  return display_lock_context->IsLocked();
-}
-
 bool BoxPaintInvalidator::NeedsToSavePreviousOverflowData() {
   if (box_.HasVisualOverflow() || box_.HasLayoutOverflow())
     return true;
@@ -428,20 +417,7 @@ bool BoxPaintInvalidator::NeedsToSavePreviousOverflowData() {
 
 void BoxPaintInvalidator::SavePreviousBoxGeometriesIfNeeded() {
   auto mutable_box = box_.GetMutableForPainting();
-  // Don't save previous sizes for hidden content-visibility: auto elements.
-  // This ensures content-visibility: auto never by itself causes a layout
-  // shift, because the LayoutShiftTracker will ignore elements whose previous
-  // or current size are empty.
-  //
-  // Note: The downside of this approach is that any change in size of |box_|
-  // up to the time unskipping occurs will cause a full paint invalidation
-  // of |box_|. Since this has no effect on its subtree (which is skipped),
-  // and |box_| is not on-screen, this should not have a significant performance
-  // impact.
-  if (IsContentVisibilityAutoAndHidden(box_))
-    mutable_box.ClearPreviousSize();
-  else
-    mutable_box.SavePreviousSize();
+  mutable_box.SavePreviousSize();
 
   if (NeedsToSavePreviousOverflowData())
     mutable_box.SavePreviousOverflowData();

third_party/blink/renderer/core/paint/paint_invalidator.cc

@@ -177,8 +177,10 @@ void PaintInvalidator::UpdateLayoutShiftTracking(
     return;
 
   auto& layout_shift_tracker = object.GetFrameView()->GetLayoutShiftTracker();
-  if (!layout_shift_tracker.NeedsToTrack(object))
+  if (!layout_shift_tracker.NeedsToTrack(object)) {
+    object.GetMutableForPainting().SetShouldSkipNextLayoutShiftTracking(true);
     return;
+  }
 
   PropertyTreeStateOrAlias property_tree_state(
       *tree_builder_context.current.transform,
@@ -216,6 +218,10 @@ void PaintInvalidator::UpdateLayoutShiftTracking(
   PhysicalRect new_rect = box.PhysicalVisualOverflowRect();
   PhysicalRect old_rect = box.PreviousPhysicalVisualOverflowRect();
   bool should_report_layout_shift = [&]() -> bool {
+    if (box.ShouldSkipNextLayoutShiftTracking()) {
+      box.GetMutableForPainting().SetShouldSkipNextLayoutShiftTracking(false);
+      return false;
+    }
     // If the layout shift root has changed, LayoutShiftTracker can't use the
     // current paint property tree to map the old rect.
     if (tree_builder_context.current.layout_shift_root_changed)

third_party/blink/web_tests/external/wpt/layout-instability/content-visibility-auto-offscreen.html

 <!DOCTYPE html>
 <title>Layout Instability: off-screen content-visibility:auto content</title>
 <link rel="help" href="https://wicg.github.io/layout-instability/" />
-<style>
-  #auto {
-    content-visibility: auto;
-    contain-intrinsic-size: 1px;
-    width: 100px;
-  }
-</style>
-<div class=auto>
-  <div style="width: 100px; height: 100px"></div>
-</div>
-<div class=auto style="position: relative; top: 100000px">
-  <div style="width: 100px; height: 100px"></div>
-</div>
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <script src="resources/util.js"></script>
 <script>
+// These scripts need to be before the contents because we need to ensure no
+// layout shifts during page load.
 promise_test(async () => {
   const watcher = new ScoreWatcher;
 
@@ -29,10 +18,30 @@ promise_test(async () => {
 
   assert_equals(watcher.score, 0);
 
+  // This should report a layout shift as target is now visible.
+  target.style.top = '100100px';
+
+  await watcher.promise;
+  const expectedScore = computeExpectedScore(100 * 100, 100);
+  assert_equals(watcher.score, expectedScore);
+
+  // No new layout shift should be reported when target is scrolled out of screeen.
   window.scrollTo(0, 0);
   await waitForAnimationFrames(2);
 
-  assert_equals(watcher.score, 0);
+  assert_equals(watcher.score, expectedScore);
 }, 'off-screen content-visibility:auto');
-
 </script>
+<style>
+  .auto {
+    content-visibility: auto;
+    contain-intrinsic-size: 1px;
+    width: 100px;
+  }
+</style>
+<div class=auto>
+  <div style="width: 100px; height: 100px"></div>
+</div>
+<div id="target" class=auto style="position: relative; top: 100000px">
+  <div style="width: 100px; height: 100px"></div>
+</div>
\ No newline at end of file

third_party/blink/web_tests/external/wpt/layout-instability/content-visibility-auto-onscreen.html

 <!DOCTYPE html>
 <title>Layout Instability: on-screen content-visibility:auto content</title>
 <link rel="help" href="https://wicg.github.io/layout-instability/" />
-<style>
-  #target {
-    content-visibility: auto;
-    contain-intrinsic-size: 1px;
-    width: 100px;
-  }
-</style>
-<div id=target>
-  <div style="width: 100px; height: 100px"></div>
-</div>
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <script src="resources/util.js"></script>
 <script>
+// These scripts need to be before the contents because we need to ensure no
+// layout shifts during page load.
 promise_test(async () => {
   const watcher = new ScoreWatcher;
 
@@ -22,5 +14,14 @@ promise_test(async () => {
   await waitForAnimationFrames(2);
   assert_equals(watcher.score, 0);
 }, 'on-screen content-visibility:auto');
-
 </script>
+<style>
+  #target {
+    content-visibility: auto;
+    contain-intrinsic-size: 1px;
+    width: 100px;
+  }
+</style>
+<div id=target>
+  <div style="width: 100px; height: 100px"></div>
+</div>

third_party/blink/web_tests/external/wpt/layout-instability/content-visibility-auto-resize.html

 <!DOCTYPE html>
 <title>Layout Instability: resizing content-visibility:auto content</title>
 <link rel="help" href="https://wicg.github.io/layout-instability/" />
-<style>
-      .auto {
-        content-visibility: auto;
-        contain-intrinsic-size: 10px 3000px;
-        width: 100px;
-      }
-      .contained {
-        height: 100px;
-      }
-    </style>
-    <div class=a><div class=contained></div></div>
-    <div class=a ><div class=contained></div></div>
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <script src="resources/util.js"></script>
 <script>
+// These scripts need to be before the contents because we need to ensure no
+// layout shifts during page load.
 promise_test(async () => {
   const watcher = new ScoreWatcher;
 
@@ -25,5 +15,16 @@ promise_test(async () => {
 
   assert_equals(watcher.score, 0);
 }, 'off-screen content-visibility:auto');
-
 </script>
+<style>
+  .auto {
+    content-visibility: auto;
+    contain-intrinsic-size: 10px 3000px;
+    width: 100px;
+  }
+  .contained {
+    height: 100px;
+  }
+</style>
+<div class=auto><div class=contained></div></div>
+<div class=auto><div class=contained></div></div>

third_party/blink/web_tests/external/wpt/layout-instability/content-visibility-hidden.html

 <!DOCTYPE html>
 <title>Layout Instability: content-visibility:hidden content</title>
 <link rel="help" href="https://wicg.github.io/layout-instability/" />
-<style>
-  #target {
-    content-visibility: hidden;
-    contain-intrinsic-size: 1px;
-    width: 100px;
-  }
-</style>
-<div id=target>
-  <div style="width: 100px; height: 100px"></div>
-</div>
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <script src="resources/util.js"></script>
 <script>
+// These scripts need to be before the contents because we need to ensure no
+// layout shifts during page load.
 promise_test(async () => {
   const watcher = new ScoreWatcher;
 
@@ -22,5 +14,14 @@ promise_test(async () => {
   await waitForAnimationFrames(2);
   assert_equals(watcher.score, 0);
 }, 'on-screen content-visibility:auto');
-
 </script>
+<style>
+  #target {
+    content-visibility: hidden;
+    contain-intrinsic-size: 1px;
+    width: 100px;
+  }
+</style>
+<div id=target>
+  <div style="width: 100px; height: 100px"></div>
+</div>

third_party/blink/web_tests/external/wpt/layout-instability/visibility-hidden-layout-and-visible.html

+<!DOCTYPE html>
+<title>Layout Instability: visibility:hidden change with layout</title>
+<link rel="help" href="https://wicg.github.io/layout-instability/" />
+<div id="target" style="position: absolute; top: 0; width: 200px; height: 200px; visibility: hidden"></div>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="resources/util.js"></script>
+<script>
+
+promise_test(async () => {
+  const watcher = new ScoreWatcher;
+
+  // Wait for the initial render to complete.
+  await waitForAnimationFrames(2);
+
+  // Shift target, for which no shift should be reported because it's hidden.
+  target.style.top = '200px';
+  target.style.visibility = 'visible';
+
+  await waitForAnimationFrames(2);
+  // No shift should be reported.
+  assert_equals(watcher.score, 0);
+
+  // Shift again, for which shift should be reported.
+  target.style.top = '300px';
+
+  await watcher.promise;
+  const expectedScore = computeExpectedScore(200 * (200 + 100), 100);
+  assert_equals(watcher.score, expectedScore);
+
+}, 'visibility:hidden change with layout');
+
+</script>