V1: allow lightbox to add its own intersection observer to the schedu…

…ler (#33281)

* V1: allow lightbox to add its own intersection observer to the scheduler

* Added tests

extensions/amp-lightbox/0.1/amp-lightbox.js

@@ -41,6 +41,7 @@ import {htmlFor} from '../../../src/static-template';
 import {isInFie} from '../../../src/iframe-helper';
 import {toArray} from '../../../src/types';
 import {tryFocus} from '../../../src/dom';
+import {unmountAll} from '../../../src/utils/resource-container-helper';
 
 /** @const {string} */
 const TAG = 'amp-lightbox';
@@ -391,6 +392,8 @@ class AmpLightbox extends AMP.BaseElement {
     element.addEventListener('transitionend', onAnimationEnd);
     element.addEventListener('animationend', onAnimationEnd);
 
+    this.setAsContainer();
+
     // TODO: instead of laying out children all at once, layout children based
     // on visibility.
     const owners = Services.ownersForDoc(this.element);
@@ -616,6 +619,12 @@ class AmpLightbox extends AMP.BaseElement {
 
     this.untieCloseButton_();
 
+    this.removeAsContainer();
+
+    // Unmount all children when the lightbox is closed. They will automatically
+    // remount when the lightbox is opened again.
+    unmountAll(this.element);
+
     Services.ownersForDoc(this.element).schedulePause(
       this.element,
       dev().assertElement(this.container_)

extensions/amp-lightbox/0.1/test/test-amp-lightbox.js

@@ -322,5 +322,73 @@ describes.realWin(
         expect(tryFocusSpy).to.be.calledWith(impl.closeButtonHeader_);
       });
     });
+
+    it('should set itself as a container when fully opened', async () => {
+      const lightbox = createLightbox();
+      const impl = await lightbox.getImpl(false);
+      env.sandbox.stub(lightbox, 'setAsContainerInternal');
+
+      const finalizeSpy = env.sandbox.spy(impl, 'finalizeOpen_');
+      impl.getHistory_ = () => {
+        return {
+          pop: () => {},
+          push: () => Promise.resolve(11),
+        };
+      };
+
+      const args = {};
+      const openInvocation = {
+        method: 'open',
+        args,
+        satisfiesTrust: () => true,
+      };
+      impl.executeAction(openInvocation);
+
+      expect(lightbox.setAsContainerInternal).to.not.be.called;
+
+      await whenCalled(finalizeSpy);
+
+      expect(lightbox.setAsContainerInternal).to.be.calledOnce;
+    });
+
+    it('should set and remove itself as a container and unmount children', async () => {
+      const lightbox = createLightbox();
+
+      // Lightbox has a child.
+      const child = dom.createElementWithAttributes(doc, 'amp-img', {
+        layout: 'nodisplay',
+      });
+      lightbox.appendChild(child);
+      env.sandbox.stub(child, 'unmount');
+
+      const openButton = createOpeningButton('openingButton');
+      const closeButton = createCloseButton();
+      lightbox.appendChild(closeButton);
+
+      const impl = await lightbox.getImpl(false);
+      env.sandbox.stub(lightbox, 'setAsContainerInternal');
+      env.sandbox.stub(lightbox, 'removeAsContainerInternal');
+
+      const openSpy = env.sandbox.spy(impl, 'finalizeOpen_');
+      const closeSpy = env.sandbox.spy(impl, 'finalizeClose_');
+      impl.getHistory_ = () => {
+        return {
+          pop: () => {},
+          push: () => Promise.resolve(11),
+        };
+      };
+
+      openButton.click();
+      expect(lightbox.setAsContainerInternal).to.not.be.called;
+      await whenCalled(openSpy);
+      expect(lightbox.setAsContainerInternal).to.be.calledOnce;
+
+      closeButton.click();
+      expect(lightbox.removeAsContainerInternal).to.not.be.called;
+      expect(child.unmount).to.not.be.called;
+      await whenCalled(closeSpy);
+      expect(lightbox.removeAsContainerInternal).to.be.calledOnce;
+      expect(child.unmount).to.be.calledOnce;
+    });
   }
 );

src/base-element.js

@@ -460,6 +460,28 @@ export class BaseElement {
     // Subclasses may override.
   }
 
+  /**
+   * Set itself as a container element that can be monitored by the scheduler
+   * for auto-mounting. Scheduler is used for V1 elements. A container is
+   * usually a top-level scrollable overlay such as a lightbox or a sidebar.
+   * The main scheduler (`IntersectionObserver`) cannot properly handle elements
+   * inside a non-document scroller and this method instructs the scheduler
+   * to also use the `IntersectionObserver` corresponding to the container.
+   *
+   * @param {!Element=} opt_scroller A child of the container that should be
+   * monitored. Typically a scrollable element.
+   */
+  setAsContainer(opt_scroller) {
+    this.element.setAsContainerInternal(opt_scroller);
+  }
+
+  /**
+   * Removes itself as a container. See `setAsContainer`.
+   */
+  removeAsContainer() {
+    this.element.removeAsContainerInternal();
+  }
+
   /**
    * Subclasses can override this method to indicate that it is has
    * render-blocking service.

src/custom-element.js

@@ -809,6 +809,29 @@ function createBaseCustomElementClass(win, elementConnectedCallback) {
       });
     }
 
+    /**
+     * See `BaseElement.setAsContainer`.
+     *
+     * @param {!Element=} opt_scroller A child of the container that should be
+     * monitored. Typically a scrollable element.
+     * @restricted
+     * @final
+     */
+    setAsContainerInternal(opt_scroller) {
+      const builder = getSchedulerForDoc(this.getAmpDoc());
+      builder.setContainer(this, opt_scroller);
+    }
+
+    /**
+     * See `BaseElement.removeAsContainer`.
+     * @restricted
+     * @final
+     */
+    removeAsContainerInternal() {
+      const builder = getSchedulerForDoc(this.getAmpDoc());
+      builder.removeContainer(this);
+    }
+
     /**
      * Update the internal ready state.
      *

src/dom.js

@@ -980,3 +980,14 @@ export function dispatchCustomEvent(node, name, opt_data, opt_options) {
   event.initEvent(name, bubbles, cancelable);
   node.dispatchEvent(event);
 }
+
+/**
+ * Ensures the child is contained by the parent, but not the parent itself.
+ *
+ * @param {!Node} parent
+ * @param {!Node} child
+ * @return {boolean}
+ */
+export function containsNotSelf(parent, child) {
+  return child !== parent && parent.contains(child);
+}

src/service/scheduler.js

@@ -17,12 +17,15 @@
 import {LayoutPriority} from '../layout';
 import {READY_SCAN_SIGNAL} from './resources-interface';
 import {VisibilityState} from '../visibility-state';
+import {containsNotSelf, hasNextNodeInDocumentOrder, isIframed} from '../dom';
+import {devAssert} from '../log';
 import {getServiceForDoc, registerServiceBuilderForDoc} from '../service';
-import {hasNextNodeInDocumentOrder, isIframed} from '../dom';
 import {removeItem} from '../utils/array';
 
 const ID = 'scheduler';
 
+const ROOT_MARGIN = '250% 31.25%';
+
 /** @implements {../service.Disposable} */
 export class Scheduler {
   /** @param {!./ampdoc-impl.AmpDoc} ampdoc  */
@@ -37,10 +40,12 @@ export class Scheduler {
       // Root bounds are not important, so we can use the `root:null` for a
       // top-level window.
       root: isIframed(win) ? win.document : null,
-      rootMargin: '250% 31.25%',
-      threshold: 0.001,
+      rootMargin: ROOT_MARGIN,
     });
 
+    /** @private @const {!Map<!Element, !IntersectionObserver>} */
+    this.containerMap_ = new Map();
+
     /** @private @const {!Map<!AmpElement, {asap: boolean, isIntersecting: boolean}>} */
     this.targets_ = new Map();
 
@@ -85,6 +90,13 @@ export class Scheduler {
     if (target.deferredBuild()) {
       this.targets_.set(target, {asap: false, isIntersecting: false});
       this.observer_.observe(target);
+      if (this.containerMap_.size > 0) {
+        this.containerMap_.forEach((observer, container) => {
+          if (containsNotSelf(container, target)) {
+            observer.observe(target);
+          }
+        });
+      }
     } else {
       this.targets_.set(target, {asap: false, isIntersecting: true});
     }
@@ -103,13 +115,65 @@ export class Scheduler {
     this.targets_.delete(target);
 
     this.observer_.unobserve(target);
+    if (this.containerMap_.size > 0) {
+      this.containerMap_.forEach((observer) => {
+        observer.unobserve(target);
+      });
+    }
 
     if (this.parsingTargets_) {
       removeItem(this.parsingTargets_, target);
       this.checkParsing_();
     }
   }
 
+  /**
+   * Adds the observer for the specified container. The first observer to
+   * find an intersection will trigger the element's mount.
+   *
+   * @param {!Element} container
+   * @param {!Element=} opt_scroller
+   */
+  setContainer(container, opt_scroller) {
+    devAssert(!opt_scroller || container.contains(opt_scroller));
+    if (this.containerMap_.has(container)) {
+      return;
+    }
+
+    // Create observer.
+    const {win} = this.ampdoc_;
+    const observer = new win.IntersectionObserver((e) => this.observed_(e), {
+      root: opt_scroller || container,
+      rootMargin: ROOT_MARGIN,
+    });
+    this.containerMap_.set(container, observer);
+
+    // Subscribe all pending children. Ignore `asap` targets since they
+    // will be scheduled immediately and do not need an intersection
+    // observer input.
+    this.targets_.forEach(({asap}, target) => {
+      if (!asap && containsNotSelf(container, target)) {
+        observer.observe(target);
+      }
+    });
+  }
+
+  /**
+   * Removes the container and its observer that were set by the `setContainer`.
+   *
+   * @param {!Element} container
+   */
+  removeContainer(container) {
+    const observer = this.containerMap_.get(container);
+    if (!observer) {
+      return;
+    }
+
+    // Disconnect. All children will be unobserved automatically.
+    observer.disconnect();
+    this.containerMap_.delete(container);
+  }
+
   /** @private*/
   signalScanReady_() {
     if (this.ampdoc_.isReady() && !this.scheduledReady_) {
@@ -180,14 +244,17 @@ export class Scheduler {
    */
   observed_(entries) {
     for (let i = 0; i < entries.length; i++) {
-      const {target, isIntersecting} = entries[i];
+      const {target, isIntersecting: isThisIntersecting} = entries[i];
 
       const current = this.targets_.get(target);
       if (!current) {
         continue;
       }
 
-      this.targets_.set(target, {asap: current.asap, isIntersecting});
+      const isIntersecting = isThisIntersecting || current.isIntersecting;
+      if (isIntersecting !== current.isIntersecting) {
+        this.targets_.set(target, {asap: current.asap, isIntersecting});
+      }
       if (isIntersecting) {
         this.maybeBuild_(target);
       }

src/utils/display-observer.js

@@ -15,6 +15,7 @@
  */
 
 import {VisibilityState} from '../visibility-state';
+import {containsNotSelf} from '../dom';
 import {getServiceForDoc, registerServiceBuilderForDoc} from '../service';
 import {pushIfNotExist, removeItem} from './array';
 import {rethrowAsync} from '../log';
@@ -493,15 +494,6 @@ function findObserverByContainer(observers, container) {
   return -1;
 }
 
-/**
- * @param {!Element} container
- * @param {!Element} child
- * @return {boolean}
- */
-function containsNotSelf(container, child) {
-  return child !== container && container.contains(child);
-}
-
 /**
  * @param {!Array<function(boolean)>} callbacks
  * @param {!Element} target

test/unit/test-custom-element-v1.js

@@ -934,6 +934,42 @@ describes.realWin('CustomElement V1', {amp: true}, (env) => {
     });
   });
 
+  describe('setAsContainerInternal', () => {
+    let element, scroller, impl;
+
+    beforeEach(async () => {
+      builderMock.expects('schedule').atLeast(0);
+
+      element = new ElementClass();
+      doc.body.appendChild(element);
+      impl = await element.getImpl();
+
+      scroller = doc.createElement('div');
+      element.appendChild(scroller);
+    });
+
+    it('should propagate setAsContainerInternal without scroller', () => {
+      builderMock
+        .expects('setContainer')
+        .withExactArgs(element, undefined)
+        .once();
+      impl.setAsContainer();
+    });
+
+    it('should propagate setAsContainerInternal with scroller', () => {
+      builderMock
+        .expects('setContainer')
+        .withExactArgs(element, scroller)
+        .once();
+      impl.setAsContainer(scroller);
+    });
+
+    it('should propagate removeAsContainerInternal', () => {
+      builderMock.expects('removeContainer').withExactArgs(element).once();
+      impl.removeAsContainer();
+    });
+  });
+
   describe('setReadyStateInternal', () => {
     let element;