🐛Intersection Observer Polyfill Mutation Observer - Fixed Race Condit…

…ion (ampproject#19168)

* Got a working half-broken page that we can test with

* COnfirmed the bug is that the pass isn't long enough

* Going to go home. But I learned that it doesn't fire until the child image element is created.

* I was doing something

* Fixed the polyfill completely

* Fixed linting

* Fixed all precommit checks

* Removed the one-off fandango example

* Made PR Changes

* Created an onNextPass for resources

* Fixed tests for ampdoc

* Removed the original getLayoutRect

* Removed a dangling opt_calledFromMutationObserver

* Made PR Changes

* Fixed refresh tests for intersection observer

* Removed returns

* Fixed some linting errors

* Made PR Changes

* Fixed linting errors

extensions/amp-a4a/0.1/test/test-refresh.js

@@ -191,6 +191,7 @@ describe('refresh', () => {
         return {
           getRootNode: () => {return window.document;},
           win: window,
+          isSingleDoc: () => {return true;},
         };
       });
 

src/intersection-observer-polyfill.js

@@ -242,10 +242,7 @@ export class IntersectionObserverPolyfill {
      */
     this.mutationObserver_ = null;
 
-    /** @private {?./service/viewport/viewport-impl.Viewport} */
-    this.viewport_ = null;
-
-    /** @private {?Pass} */
+    /** @private {Pass} */
     this.mutationPass_ = null;
   }
 
@@ -294,12 +291,10 @@ export class IntersectionObserverPolyfill {
     // from multiple documents.
     const ampdoc = Services.ampdoc(element);
     if (ampdoc.win.MutationObserver && !this.mutationObserver_) {
-      this.viewport_ = Services.viewportForDoc(element);
-      this.mutationPass_ = new Pass(ampdoc.win, () => {
-        if (this.viewport_) {
-          this.tick(this.viewport_.getRect());
-        }
-      });
+      this.mutationPass_ = new Pass(
+          ampdoc.win,
+          this.handleMutationObserverPass_.bind(this, element)
+      );
       this.mutationObserver_ = new ampdoc.win.MutationObserver(
           this.handleMutationObserverNotification_.bind(this)
       );
@@ -356,7 +351,10 @@ export class IntersectionObserverPolyfill {
 
     for (let i = 0; i < this.observeEntries_.length; i++) {
       const change = this.getValidIntersectionChangeEntry_(
-          this.observeEntries_[i], hostViewport, opt_iframe);
+          this.observeEntries_[i],
+          hostViewport,
+          opt_iframe
+      );
       if (change) {
         changes.push(change);
       }
@@ -376,22 +374,18 @@ export class IntersectionObserverPolyfill {
    * @param {!ElementIntersectionStateDef} state
    * @param {!./layout-rect.LayoutRectDef} hostViewport hostViewport's rect
    * @param {./layout-rect.LayoutRectDef=} opt_iframe iframe container rect
+   *    If opt_iframe is provided, all LayoutRect has position relative to
+   *    the iframe. If opt_iframe is not provided,
+   *    all LayoutRect has position relative to the host document.
    * @return {?IntersectionObserverEntry} A valid change entry or null if ratio
    * @private
    */
   getValidIntersectionChangeEntry_(state, hostViewport, opt_iframe) {
     const {element} = state;
 
-    // Normalize container LayoutRect to be relative to page
-    let ownerRect = null;
-
-    // If opt_iframe is provided, all LayoutRect has position relative to
-    // the iframe.
-    // If opt_iframe is not provided, all LayoutRect has position relative to
-    // the host document.
     const elementRect = element.getLayoutBox();
     const owner = element.getOwner();
-    ownerRect = owner && owner.getLayoutBox();
+    const ownerRect = owner && owner.getLayoutBox();
 
     // calculate intersectionRect. that the element intersects with hostViewport
     // and intersects with owner element and container iframe if exists.
@@ -426,7 +420,21 @@ export class IntersectionObserverPolyfill {
 
     // Wait one animation frame so that other mutations may arrive.
     this.mutationPass_.schedule(16);
-    return;
+  }
+
+  /**
+   * Handle Mutation Observer Pass
+   * This performas the tick, and is wrapped in a paas
+   * To handle throttling of the observer
+   * @param {!Element} element
+   * @private
+   */
+  handleMutationObserverPass_(element) {
+    const viewport = Services.viewportForDoc(element);
+    const resources = Services.resourcesForDoc(element);
+    resources.onNextPass(() => {
+      this.tick(viewport.getRect());
+    });
   }
 
   /**
@@ -438,7 +446,6 @@ export class IntersectionObserverPolyfill {
       this.mutationObserver_.disconnect();
     }
     this.mutationObserver_ = null;
-    this.viewport_ = null;
     if (this.mutationPass_) {
       this.mutationPass_.cancel();
     }

src/service/resources-impl.js

@@ -211,6 +211,9 @@ export class Resources {
     /** @private {boolean} */
     this.maybeChangeHeight_ = false;
 
+    /** @const @private {!Array<function()>} */
+    this.passCallbacks_ = [];
+
     /** @private @const {!FiniteStateMachine<!VisibilityState>} */
     this.visibilityStateMachine_ = new FiniteStateMachine(
         this.viewer_.getVisibilityState()
@@ -1140,6 +1143,14 @@ export class Resources {
     this.schedulePass();
   }
 
+  /**
+   * Registers a callback to be called when the next pass happens.
+   * @param {function()} callback
+   */
+  onNextPass(callback) {
+    this.passCallbacks_.push(callback);
+  }
+
   /**
    * Runs a pass immediately.
    */
@@ -1169,7 +1180,7 @@ export class Resources {
       this.contentHeight_ = this.viewport_.getContentHeight();
       this.viewer_.sendMessage('documentHeight',
           dict({'height': this.contentHeight_}), /* cancelUnsent */true);
-      dev().fine(TAG_, 'document height on load: ' + this.contentHeight_);
+      dev().fine(TAG_, 'document height on load: %s', this.contentHeight_);
     }
 
     const viewportSize = this.viewport_.getSize();
@@ -1201,11 +1212,17 @@ export class Resources {
           this.viewer_.sendMessage('documentHeight',
               dict({'height': measuredContentHeight}), /* cancelUnsent */true);
           this.contentHeight_ = measuredContentHeight;
-          dev().fine(TAG_, 'document height changed: ' + this.contentHeight_);
+          dev().fine(TAG_, 'document height changed: %s', this.contentHeight_);
           this.viewport_.contentHeightChanged();
         }
       });
     }
+
+    for (let i = 0; i < this.passCallbacks_.length; i++) {
+      const fn = this.passCallbacks_[i];
+      fn();
+    }
+    this.passCallbacks_.length = 0;
   }
 
   /**

test/functional/test-intersection-observer-polyfill.js

@@ -25,6 +25,12 @@ import {
 import {Services} from '../../src/services';
 import {layoutRectLtwh} from '../../src/layout-rect';
 
+const fakeAmpDoc = {
+  getRootNode: () => {return window.document;},
+  win: window,
+  isSingleDoc: () => {return true;},
+};
+
 describe('IntersectionObserverApi', () => {
   let sandbox;
   let onScrollSpy;
@@ -70,12 +76,7 @@ describe('IntersectionObserverApi', () => {
     sandbox.stub(Services, 'viewportForDoc').callsFake(() => {
       return mockViewport;
     });
-    sandbox.stub(Services, 'ampdoc').callsFake(() => {
-      return {
-        getRootNode: () => {return window.document;},
-        win: window,
-      };
-    });
+    sandbox.stub(Services, 'ampdoc').callsFake(() => fakeAmpDoc);
     testEle = {
       isBuilt: () => {return true;},
       getOwner: () => {return null;},
@@ -207,6 +208,7 @@ describe('IntersectionObserverPolyfill', () => {
   beforeEach(() => {
     sandbox = sinon.sandbox;
     sandbox.stub(performance, 'now').callsFake(() => 100);
+    sandbox.stub(Services, 'ampdoc').callsFake(() => fakeAmpDoc);
   });
 
   afterEach(() => {
@@ -304,13 +306,15 @@ describe('IntersectionObserverPolyfill', () => {
           },
         };
       });
-      sandbox.stub(Services, 'ampdoc').callsFake(() => {
+      sandbox.stub(Services, 'resourcesForDoc').callsFake(() => {
         return {
-          getRootNode: () => {return window.document;},
-          win: window,
+          onNextPass: callback => {
+            callback();
+          },
         };
       });
 
+
       element = {
         isBuilt: () => {return true;},
         getOwner: () => {return null;},

test/functional/test-resources.js

@@ -1634,6 +1634,24 @@ describe('Resources discoverWork', () => {
       });
     });
   });
+
+  describe('onNextPass', () => {
+
+    it('should only run callbacks once.', () => {
+      resources.isRuntimeOn_ = true;
+      resources.documentReady_ = true;
+      resources.firstPassAfterDocumentReady_ = true;
+
+      const passCallback = sandbox.spy();
+      resources.onNextPass(passCallback);
+
+      resources.doPass();
+      expect(passCallback).to.be.calledOnce;
+
+      resources.doPass();
+      expect(passCallback).to.be.calledOnce;
+    });
+  });
 });
 
 describes.realWin('Resources contentHeight', {