From 14d60c820432ba50a774f7be672eb115e8693183 Mon Sep 17 00:00:00 2001
From: Dima Voytenko <dvoytenko@google.com>
Date: Tue, 23 Mar 2021 17:06:03 -0700
Subject: [PATCH] review fixes

---
 src/base-element.js      | 6 +++++-
 src/custom-element.js    | 2 +-
 src/dom.js               | 4 ++--
 src/service/scheduler.js | 9 +++++++--
 4 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/base-element.js b/src/base-element.js
index f2768107e73f7..f1d96789cf114 100644
--- a/src/base-element.js
+++ b/src/base-element.js
@@ -462,7 +462,11 @@ export class BaseElement {
 
   /**
    * Set itself as a container element that can be monitored by the scheduler
-   * for auto-mounting.
+   * 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.
diff --git a/src/custom-element.js b/src/custom-element.js
index 6e6bb897bc4f0..b4c7a55bce7f2 100644
--- a/src/custom-element.js
+++ b/src/custom-element.js
@@ -824,7 +824,7 @@ function createBaseCustomElementClass(win, elementConnectedCallback) {
     }
 
     /**
-     * See `BaseElement.setAsContainer`.
+     * See `BaseElement.removeAsContainer`.
      * @restricted
      * @final
      */
diff --git a/src/dom.js b/src/dom.js
index 53266f1bfc8ff..5474e155f0e9e 100644
--- a/src/dom.js
+++ b/src/dom.js
@@ -984,8 +984,8 @@ export function dispatchCustomEvent(node, name, opt_data, opt_options) {
 /**
  * Ensures the child is contained by the parent, but not the parent itself.
  *
- * @param {!Element} parent
- * @param {!Element} child
+ * @param {!Node} parent
+ * @param {!Node} child
  * @return {boolean}
  */
 export function containsNotSelf(parent, child) {
diff --git a/src/service/scheduler.js b/src/service/scheduler.js
index 1c7dd3478ca1e..f287a9d2c2270 100644
--- a/src/service/scheduler.js
+++ b/src/service/scheduler.js
@@ -146,7 +146,9 @@ export class Scheduler {
     });
     this.containerMap_.set(container, observer);
 
-    // Subscribe all pending children.
+    // 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);
@@ -167,6 +169,7 @@ export class Scheduler {
 
     // Disconnect. All children will be unobserved automatically.
     observer.disconnect();
+    this.containerMap_.delete(container);
   }
 
   /** @private*/
@@ -247,7 +250,9 @@ export class Scheduler {
       }
 
       const isIntersecting = isThisIntersecting || current.isIntersecting;
-      this.targets_.set(target, {asap: current.asap, isIntersecting});
+      if (isIntersecting !== current.isIntersecting) {
+        this.targets_.set(target, {asap: current.asap, isIntersecting});
+      }
       if (isIntersecting) {
         this.maybeBuild_(target);
       }