Fix when entire scroller is off-screen and then comes on-screen.

The fix is to always observe the entire container of elements so that
if it comes onscreen, we sync.

Also remove ResizeObversers on the elements as the cases it was
defending against (suddenly expanding onto the screen) are covered by
observing the container.



Bug: 998480
Change-Id: I0c253453e0b2f642d686c7213a11d152c1f8330a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1774008Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Fergal Daly <fergal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691581}

third_party/blink/renderer/core/script/resources/layered_api/elements/virtual-scroller/index.mjs

@@ -44,11 +44,7 @@ export class VirtualScrollerElement extends HTMLElement {
     shadowRoot.adoptedStyleSheets = [generateStyleSheet()];
     shadowRoot.appendChild(document.createElement('slot'));
 
-    const visibilityManager = new VisibilityManager(this.children);
-
-    new ResizeObserver(() => {
-      visibilityManager.scheduleSync();
-    }).observe(this);
+    const visibilityManager = new VisibilityManager(this);
 
     new MutationObserver(records => {
       visibilityManager.applyMutationObserverRecords(records);

third_party/blink/renderer/core/script/resources/layered_api/elements/virtual-scroller/visibility-manager.mjs

@@ -125,34 +125,28 @@ class SizeManager {
  * elements. This list of elements is assumed to be in vertical
  * display order (e.g. from lowest to highest offset).
  *
- * It uses resize and intersection observers on all of the visible
- * elements to ensure that changes that impact visibility cause us to
- * recalulate things (e.g. scrolling, restyling).
+ * It uses intersection observers to ensure that changes that impact
+ * visibility cause us to recalulate things (e.g. scrolling,
+ * restyling).
  */
 export class VisibilityManager {
   #sizeManager = new SizeManager();
   #elements;
   #syncRAFToken;
 
-  #elementIntersectionObserver;
-  #elementResizeObserver;
+  #intersectionObserver;
 
   #revealed = new Set();
 
-  constructor(elements) {
-    this.#elements = elements;
+  constructor(container) {
+    this.#elements = container.children;
 
     // We want to sync if any element's size changes or if it becomes
     // more/less visible.
-    this.#elementIntersectionObserver = new IntersectionObserver(() => {
-      this.scheduleSync();
-    });
-    // TODO(fergal): Remove this? I'm not sure that we need the resize
-    // observer. Any resize that is important to us seems like it will
-    // also involve an intersection change.
-    this.#elementResizeObserver = new ResizeObserver(() => {
+    this.#intersectionObserver = new IntersectionObserver(() => {
       this.scheduleSync();
     });
+    this.#intersectionObserver.observe(container);
 
     for (const element of this.#elements) {
       this.#didAdd(element);
@@ -268,8 +262,7 @@ export class VisibilityManager {
   #reveal =
       element => {
         this.#revealed.add(element);
-        this.#elementIntersectionObserver.observe(element);
-        this.#elementResizeObserver.observe(element);
+        this.#intersectionObserver.observe(element);
         this.#unlock(element);
       }
 
@@ -305,8 +298,7 @@ export class VisibilityManager {
   #hide =
       element => {
         this.#revealed.delete(element);
-        this.#elementIntersectionObserver.unobserve(element);
-        this.#elementResizeObserver.unobserve(element);
+        this.#intersectionObserver.unobserve(element);
         element.displayLock
             .acquire({
               timeout: Infinity,
@@ -357,8 +349,7 @@ export class VisibilityManager {
           this.#unlock(element);
         }
         this.#revealed.delete(element);
-        this.#elementIntersectionObserver.unobserve(element);
-        this.#elementResizeObserver.unobserve(element);
+        this.#intersectionObserver.unobserve(element);
         this.#sizeManager.remove(element);
       }
 

third_party/blink/web_tests/wpt_internal/virtual-scroller/resources/helpers.mjs

@@ -19,6 +19,17 @@ export function div(id) {
   return d;
 }
 
+/**
+ * Creates a 5000px DIV with a solid border with id and textContent
+ * set to |id|.
+ */
+export function largeDiv(id) {
+  const large = div(id);
+  large.style.height = "5000px";
+  large.style.border = "solid";
+  return large;
+}
+
 /**
  * Creates a container DIV with |n| child DIVs with their margin=|margin|.
  */
@@ -68,11 +79,23 @@ export function words(n) {
 }
 
 /**
- * Allow the current frame to end and then call |callback| asap in the next
- * frame.
+ * Allow the next |n| frames to end and then call |callback| ASAP in
+ * the following frame.
 */
-export function nextFrame(callback) {
-  window.requestAnimationFrame(() => {
+export function inNFrames(n, callback) {
+  if (n == 0) {
     window.setTimeout(callback, 0);
-  });
+  } else {
+    window.requestAnimationFrame(() => {
+      inNFrames(n - 1, callback);
+    });
+  }
+}
+
+/**
+ * Allow the current frame to end and then call |callback| asap in the
+ * next frame.
+*/
+export function nextFrame(callback) {
+  inNFrames(1, callback);
 }

third_party/blink/web_tests/wpt_internal/virtual-scroller/resources/ref-tests.mjs

@@ -64,6 +64,28 @@ export function testResize(target) {
   });
 }
 
+export function testScrollFromOffScreen(target) {
+  // This scrollTo and nextFrame are not necessary for the ref-test
+  // however it helps when trying to debug this test in a
+  // browser. Without it, the scroll offset may be preserved across
+  // page reloads.
+  document.body.scrollTo(0, 0);
+  helpers.nextFrame(() => {
+    // The page is a large element (much bigger than the page)
+    // followed by the scroller. We then scroll down to the scroller.
+    const largeSibling = helpers.largeDiv("large");
+    target.before(largeSibling);
+    const child = helpers.div("child");
+    target.appendChild(child);
+
+    // Give the scroller time to settle.
+    helpers.inNFrames(10, () => {
+      window.scrollBy(0, largeSibling.getBoundingClientRect().height);
+      helpers.stopWaiting();
+    });
+  });
+}
+
 /**
  * Runs |test| with a <virtual-scroller>, waiting until the custom element is
  * defined.

third_party/blink/web_tests/wpt_internal/virtual-scroller/scroll-from-off-screen-ref.html

+<!DOCTYPE html>
+<html class="reftest-wait">
+  <style>body { overflow:hidden; }</style>
+  <script type="module">
+    import * as refTests from './resources/ref-tests.mjs';
+    refTests.withDiv(refTests.testScrollFromOffScreen);
+  </script>
+</html>

third_party/blink/web_tests/wpt_internal/virtual-scroller/scroll-from-off-screen.html

+<!DOCTYPE html>
+<html class="reftest-wait">
+  <meta charset="utf8">
+  <title>Scroller is off-screen and gets scrolled on-screen.</title>
+  <style>body { overflow:hidden; }</style>
+  <link rel="author" title="Fergal Daly" href="mailto:fergal@chromium.org">
+  <link rel="match" href="scroll-from-off-screen-ref.html">
+  <script type="module">
+    import 'std:elements/virtual-scroller';
+    import * as refTests from './resources/ref-tests.mjs';
+    refTests.withVirtualScroller(refTests.testScrollFromOffScreen);
+  </script>
+</html>