[CLS] Don't let content-visibility:auto affect CLS

A previous CL (*) already did this up to the first observation.
Now we enforce it for all times content skips and unskips.

The rationale for this change is that content-visibility is a
UA-controlled best practice, and it is mostly up to the UA to avoid
layout shifts when using this feature.

Note that there will still be layout shifts reported for elements
adjacent to (but not descendants of) content-visibility:auto elements
if the content-visibility:auto element resizes and causes a shift.
The change in this CL applies only to the content-visibility:auto
element itself.

Bug: 1151526

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

docs/speed/metrics_changelog/2020_11_cls.md

@@ -16,23 +16,15 @@ position elements into account. This has been fixed in Chrome 88.
 
 ### Bug fix for content-visibility: auto
 
-When the `content-visibility: auto` feature was shipped in Chrome 85, a CLS-impacting
-flaw was present: the first time that content is rendered near the screen, a double render
-occurs, once with sizing determined by `contain-intrinsic-size` and the second time
-by the actual size of the content.
-
-If the content was actually on-screen during the double render, it would affect
-CLS scores; therefore the flaw mostly affects above-the-fold
-`content-visibility:auto` content. For such content, there was an unavoidable
-impact on CLS due to the double-render. In Chrome 85, this was a somewhat
-accurate result, because there was a user-visible flicker for
-such above-the-fold content. In Chrome 86, this was
-[fixed](https://chromium-review.googlesource.com/c/chromium/src/+/2348415)
-to avoid a flicker, but the problem with CLS remained.
-
-In Chrome 88, the CLS issue was [fixed](https://chromium-review.googlesource.com/c/chromium/src/+/2556211).
-Going forward, there should be no first-render CLS penalty for
-`content-visibility:auto` content.
+When the `content-visibility: auto` feature was shipped in Chrome 85, a
+CLS-impacting flaw was present: changes between the skipped and not-skipped
+state of a `content-visibility: auto` subtree caused an observed layout shift
+in the `content;visibility: auto` element as it resized.
+
+In Chrome 88, the CLS issue was [fixed](https://crbug.com/1151526).
+Going forward, there should be no CLS penalty for such elements. (Note that
+there still may be a layout shift for onscreen elements adjacent to (but not
+descendants of) the `content-visibility: auto` element.
 
 ## How does this affect a site's metrics?
 

docs/speed/metrics_changelog/cls.md

@@ -5,7 +5,7 @@ This is a list of changes to [Cumulative Layout Shift](https://web.dev/cls).
 * 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)
-  * Metric definition improvement: [No penalty for first unskipped render of content-visibility:auto content](2020_11_cls.md)
+  * Metric definition improvement: [no penalty for content-visibility: auto content](2020_11_cls.md)
 * Chrome 87
   * Metric definition improvement: [Fix problem in Cumulative Layout shift calculation of impact region](2020_10_cls_2.md)
   * Metric definition improvement: [Cumulative Layout Shift properly handles clipping of elements styled contain:paint](2020_10_cls_2.md)

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

@@ -1914,6 +1914,7 @@ class CORE_EXPORT LayoutBox : public LayoutBoxModelObject {
     void SavePreviousSize() {
       GetLayoutBox().previous_size_ = GetLayoutBox().Size();
     }
+    void ClearPreviousSize() { GetLayoutBox().previous_size_ = LayoutSize(); }
     void SavePreviousOverflowData();
     void ClearPreviousOverflowData() {
       DCHECK(!GetLayoutBox().HasVisualOverflow());

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

@@ -394,13 +394,13 @@ TEST_F(LayoutShiftTrackerTest, CompositedOverflowExpansion) {
 TEST_F(LayoutShiftTrackerTest, ContentVisibilityAutoFirstPaint) {
   SetBodyInnerHTML(R"HTML(
     <style>
-      #target {
+      .auto {
         content-visibility: auto;
         contain-intrinsic-size: 1px;
         width: 100px;
       }
     </style>
-    <div id=target>
+    <div id=target class=auto>
       <div style="width: 100px; height: 100px"></div>
     </div>
   )HTML");
@@ -417,24 +417,23 @@ TEST_F(LayoutShiftTrackerTest,
        ContentVisibilityAutoOffscreenAfterScrollFirstPaint) {
   SetBodyInnerHTML(R"HTML(
     <style>
-      #target {
+      .auto {
         content-visibility: auto;
         contain-intrinsic-size: 1px;
         width: 100px;
       }
     </style>
-    <div id=target style="position: relative; top: 100000px">
+    <div id=target class=auto style="position: relative; top: 100000px">
       <div style="width: 100px; height: 100px"></div>
     </div>
   )HTML");
   auto* target = To<LayoutBox>(GetLayoutObjectByElementId("target"));
-
   // #target starts offsceen, which doesn't count for CLS.
   EXPECT_FLOAT_EQ(0, GetLayoutShiftTracker().Score());
   EXPECT_EQ(LayoutSize(100, 1), target->Size());
 
   // In the next frame, we scroll it onto the screen, but it still doesn't
-  // count for CLS, and its subtree is not yet unskipped because the
+  // count for CLS, and its subtree is not yet unskipped, because the
   // intersection observation takes effect on the subsequent frame.
   GetDocument().domWindow()->scrollTo(0, 100000);
   UpdateAllLifecyclePhasesForTest();
@@ -453,13 +452,13 @@ TEST_F(LayoutShiftTrackerTest,
 TEST_F(LayoutShiftTrackerTest, ContentVisibilityHiddenFirstPaint) {
   SetBodyInnerHTML(R"HTML(
     <style>
-      #target {
+      .auto {
         content-visibility: hidden;
         contain-intrinsic-size: 1px;
         width: 100px;
       }
     </style>
-    <div id=target>
+    <div id=target class=auto>
       <div style="width: 100px; height: 100px"></div>
     </div>
   )HTML");
@@ -470,4 +469,91 @@ TEST_F(LayoutShiftTrackerTest, ContentVisibilityHiddenFirstPaint) {
   EXPECT_EQ(LayoutSize(100, 1), target->Size());
 }
 
+TEST_F(LayoutShiftTrackerTest, ContentVisibilityAutoResize) {
+  SetBodyInnerHTML(R"HTML(
+    <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 id=target><div class=contained></div></div>
+  )HTML");
+
+  // Skipped subtrees don't cause CLS impact.
+  UpdateAllLifecyclePhasesForTest();
+  auto* target = To<LayoutBox>(GetLayoutObjectByElementId("target"));
+  EXPECT_FLOAT_EQ(0, GetLayoutShiftTracker().Score());
+  EXPECT_EQ(LayoutSize(100, 100), target->Size());
+}
+
+TEST_F(LayoutShiftTrackerTest,
+       ContentVisibilityAutoOnscreenAndOffscreenAfterScrollFirstPaint) {
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      .auto {
+        content-visibility: auto;
+        contain-intrinsic-size: 1px;
+        width: 100px;
+      }
+    </style>
+    <div id=onscreen class=auto>
+      <div style="width: 100px; height: 100px"></div>
+    </div>
+    <div id=offscreen class=auto style="position: relative; top: 100000px">
+      <div style="width: 100px; height: 100px"></div>
+    </div>
+  )HTML");
+  auto* offscreen = To<LayoutBox>(GetLayoutObjectByElementId("offscreen"));
+  auto* onscreen = To<LayoutBox>(GetLayoutObjectByElementId("onscreen"));
+
+  // #offscreen starts offsceen, which doesn't count for CLS.
+  EXPECT_FLOAT_EQ(0, GetLayoutShiftTracker().Score());
+  EXPECT_EQ(LayoutSize(100, 1), offscreen->Size());
+  EXPECT_EQ(LayoutSize(100, 100), onscreen->Size());
+
+  // In the next frame, we scroll it onto the screen, but it still doesn't
+  // count for CLS, and its subtree is not yet unskipped, because the
+  // intersection observation takes effect on the subsequent frame.
+  GetDocument().domWindow()->scrollTo(0, 100000 + 100);
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_FLOAT_EQ(0, GetLayoutShiftTracker().Score());
+  EXPECT_EQ(LayoutSize(100, 1), offscreen->Size());
+  EXPECT_EQ(LayoutSize(100, 100), onscreen->Size());
+
+  // Now the subtree is unskipped, and #offscreen renders at size 100x100.
+  // Nevertheless, there is no impact on CLS.
+  UpdateAllLifecyclePhasesForTest();
+  offscreen = To<LayoutBox>(GetLayoutObjectByElementId("offscreen"));
+  onscreen = To<LayoutBox>(GetLayoutObjectByElementId("onscreen"));
+
+  // Target's LayoutObject gets re-attached.
+  offscreen = To<LayoutBox>(GetLayoutObjectByElementId("offscreen"));
+  EXPECT_FLOAT_EQ(0, GetLayoutShiftTracker().Score());
+  EXPECT_EQ(LayoutSize(100, 100), offscreen->Size());
+  EXPECT_EQ(LayoutSize(100, 1), onscreen->Size());
+
+  // Now scroll the element back off-screen.
+  GetDocument().domWindow()->scrollTo(0, 0);
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_FLOAT_EQ(0, GetLayoutShiftTracker().Score());
+  EXPECT_EQ(LayoutSize(100, 100), offscreen->Size());
+  EXPECT_EQ(LayoutSize(100, 1), onscreen->Size());
+
+  // In the subsequent frame, #offscreen becomes locked and changes its
+  // layout size (and vice-versa for #onscreen).
+  UpdateAllLifecyclePhasesForTest();
+  offscreen = To<LayoutBox>(GetLayoutObjectByElementId("offscreen"));
+  onscreen = To<LayoutBox>(GetLayoutObjectByElementId("onscreen"));
+
+  EXPECT_FLOAT_EQ(0, GetLayoutShiftTracker().Score());
+  EXPECT_EQ(LayoutSize(100, 1), offscreen->Size());
+  EXPECT_EQ(LayoutSize(100, 100), onscreen->Size());
+}
+
 }  // namespace blink

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

@@ -392,22 +392,15 @@ bool BoxPaintInvalidator::NeedsToSavePreviousContentBoxRect() {
   return false;
 }
 
-static bool IsContentVisibilityAutoWithoutFirstIntersection(
-    const LayoutBox& box) {
-  if (!box.GetNode() || !box.GetNode()->IsElementNode())
-    return false;
-
-  auto* display_lock_context =
-      To<Element>(box.GetNode())->GetDisplayLockContext();
+static bool IsContentVisibilityAutoAndHidden(const LayoutBox& box) {
+  auto* display_lock_context = box.GetDisplayLockContext();
   if (!display_lock_context)
     return false;
 
-  if (!display_lock_context->IsLocked() ||
-      display_lock_context->HadAnyViewportIntersectionNotifications() ||
-      !display_lock_context->IsAuto())
+  if (!display_lock_context->IsAuto())
     return false;
 
-  return true;
+  return display_lock_context->IsLocked();
 }
 
 bool BoxPaintInvalidator::NeedsToSavePreviousOverflowData() {
@@ -430,20 +423,19 @@ bool BoxPaintInvalidator::NeedsToSavePreviousOverflowData() {
 
 void BoxPaintInvalidator::SavePreviousBoxGeometriesIfNeeded() {
   auto mutable_box = box_.GetMutableForPainting();
-  // If a box has content-visibility:auto and has not yet been deemed to be near
-  // the viewport, then don't save its previous size. This prevents the
-  // LayoutShiftTracker (which computes CLS) from recording a layout shift when
-  // the element's subtree become unskipped for the first time. This is almost
-  // exactly equivalent to the current behavior of CLS computation for offscreen
-  // elements - i.e. those elements are not counted for CLS. The only difference
-  // is the one-frame leeway granted after box_ appears near the screen.
+  // 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 (!IsContentVisibilityAutoWithoutFirstIntersection(box_))
+  if (IsContentVisibilityAutoAndHidden(box_))
+    mutable_box.ClearPreviousSize();
+  else
     mutable_box.SavePreviousSize();
 
   if (NeedsToSavePreviousOverflowData())

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

@@ -2,13 +2,16 @@
 <title>Layout Instability: off-screen content-visibility:auto content</title>
 <link rel="help" href="https://wicg.github.io/layout-instability/" />
 <style>
-  #target {
+  #auto {
     content-visibility: auto;
     contain-intrinsic-size: 1px;
     width: 100px;
   }
 </style>
-<div id=target style="position: relative; top: 100000px">
+<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>
@@ -21,7 +24,12 @@ promise_test(async () => {
   // Wait for the initial render to complete.
   await waitForAnimationFrames(2);
 
-  window.scrollTo(0, 100000);
+  window.scrollTo(0, 100000 + 100);
+  await waitForAnimationFrames(2);
+
+  assert_equals(watcher.score, 0);
+
+  window.scrollTo(0, 0);
   await waitForAnimationFrames(2);
 
   assert_equals(watcher.score, 0);

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>
+promise_test(async () => {
+  const watcher = new ScoreWatcher;
+
+  // Wait for the initial render to complete.
+  await waitForAnimationFrames(2);
+
+  assert_equals(watcher.score, 0);
+}, 'off-screen content-visibility:auto');
+
+</script>