Merge pull request #1163 from cramforce/viewability-frequency

Simplify scroll tracking and greatly increase frequency of viewability events

builtins/amp-ad.js

@@ -20,7 +20,7 @@ import {getIntersectionChangeEntry} from '../src/intersection-observer';
 import {isLayoutSizeDefined} from '../src/layout';
 import {loadPromise} from '../src/event-helper';
 import {registerElement} from '../src/custom-element';
-import {getIframe, listenOnce, postMessage, prefetchBootstrap} from
+import {getIframe, listen, listenOnce, postMessage, prefetchBootstrap} from
     '../src/3p-frame';
 import {adPrefetch, adPreconnect} from '../ads/_prefetch';
 import {timer} from '../src/timer';
@@ -212,7 +212,12 @@ export function installAd(win) {
           this.deferMutate(this.noContentHandler_.bind(this));
         });
         // Triggered by context.observeIntersection(…) inside the ad iframe.
-        listenOnce(this.iframe_, 'send-intersections', () => {
+        // We use listen instead of listenOnce, because a single ad might
+        // have multiple parties wanting to receive viewability data.
+        // The second time this is called, it doesn't do much but it
+        // guarantees that the receiver gets an initial intersection change
+        // record.
+        listen(this.iframe_, 'send-intersections', () => {
           this.startSendingIntersectionChanges_();
         });
       }
@@ -227,7 +232,7 @@ export function installAd(win) {
       // it is visible.
       if (inViewport) {
         this.unlistenViewportChanges_ =
-            this.getViewport().onChanged(this.sendAdIntersection_.bind(this));
+            this.getViewport().onScroll(this.sendAdIntersection_.bind(this));
       } else if (this.unlistenViewportChanges_) {
         this.unlistenViewportChanges_();
         this.unlistenViewportChanges_ = null;
@@ -239,6 +244,8 @@ export function installAd(win) {
      * observing its position in the viewport.
      * Sets a flag, measures the iframe position if necessary and sends
      * one change record to the iframe.
+     * Note that this method may be called more than once if a single ad
+     * has multiple parties interested in viewability data.
      * @private
      */
     startSendingIntersectionChanges_() {

src/viewport.js

@@ -23,6 +23,7 @@ import {onDocumentReady} from './document-state';
 import {platform} from './platform';
 import {px, setStyle, setStyles} from './style';
 import {timer} from './timer';
+import {vsyncFor} from './vsync';
 import {viewerFor} from './viewer';
 
 
@@ -74,6 +75,9 @@ export class Viewport {
     /** @private {?number} */
     this./*OK*/scrollTop_ = null;
 
+    /** @private {?number} */
+    this.lastMeasureScrollTop_ = null;
+
     /** @private {?number} */
     this./*OK*/scrollLeft_ = null;
 
@@ -83,6 +87,9 @@ export class Viewport {
     /** @private {number} */
     this.scrollMeasureTime_ = 0;
 
+    /** @private {Vsync} */
+    this.vsync_ = vsyncFor(win);
+
     /** @private {boolean} */
     this.scrollTracking_ = false;
 
@@ -101,6 +108,9 @@ export class Viewport {
     /** @private {string|undefined} */
     this.originalViewportMetaString_ = undefined;
 
+    /** @private @const (function()) */
+    this.boundThrottledScroll_ = this.throttledScroll_.bind(this);
+
     this.viewer_.onViewportEvent(() => {
       this.binding_.updateViewerViewport(this.viewer_);
       const paddingTop = this.viewer_.getPaddingTop();
@@ -155,7 +165,7 @@ export class Viewport {
    * @return {number}
    */
   getScrollLeft() {
-    if (this./*OK*/scrollleft_ == null) {
+    if (this./*OK*/scrollLeft_ == null) {
       this./*OK*/scrollLeft_ = this.binding_.getScrollLeft();
     }
     return this./*OK*/scrollLeft_;
@@ -379,59 +389,52 @@ export class Viewport {
   /** @private */
   scroll_() {
     this.scrollCount_++;
-    this.scrollObservable_.fire();
-
     this.scrollLeft_ = this.binding_.getScrollLeft();
-
-    if (this.scrollTracking_) {
-      return;
-    }
-
     const newScrollTop = this.binding_.getScrollTop();
     if (newScrollTop < 0) {
       // iOS and some other browsers use negative values of scrollTop for
       // overscroll. Overscroll does not affect the viewport and thus should
       // be ignored here.
       return;
     }
-
-    this.scrollTracking_ = true;
     this.scrollTop_ = newScrollTop;
-    this.scrollMeasureTime_ = timer.now();
-    timer.delay(() => this.scrollDeferred_(), 500);
+    if (!this.scrollTracking_) {
+      this.scrollTracking_ = true;
+      const now = timer.now();
+      // Wait 2 frames and then request an animation frame.
+      timer.delay(() => this.vsync_.measure(
+          this.throttledScroll_.bind(this, now, newScrollTop)), 36);
+    }
+    this.scrollObservable_.fire();
   }
 
-  /** @private */
-  scrollDeferred_() {
+  /**
+   * This method is called about every 3 frames (assuming 60hz) and it
+   * is called in a vsync measure task.
+   * @param {number} referenceTime Time when the scroll measurement, that
+   *     triggered this call made, was made.
+   * @param {number} referenceTop Scrolltop at that time.
+   * @private
+   */
+  throttledScroll_(referenceTime, referenceTop) {
     this.scrollTracking_ = false;
-    const newScrollTop = this.binding_.getScrollTop();
-    if (this.scrollTop_ === null) {
-      // If the scrollTop was reset while waiting for the next scroll event
-      // we have to assume that velocity is 0 - there's no other way we can
-      // calculate it.
-      this.scrollTop_ = newScrollTop;
-    }
+    const newScrollTop = this.scrollTop_ = this.binding_.getScrollTop();
     const now = timer.now();
     let velocity = 0;
-    if (now != this.scrollMeasureTime_) {
-      velocity = (newScrollTop - this.scrollTop_) /
-          (now - this.scrollMeasureTime_);
+    if (now != referenceTime) {
+      velocity = (newScrollTop - referenceTop) /
+          (now - referenceTime);
     }
     log.fine(TAG_, 'scroll: ' +
         'scrollTop=' + newScrollTop + '; ' +
         'velocity=' + velocity);
-    this.scrollTop_ = newScrollTop;
-    this.scrollMeasureTime_ = now;
     // TODO(dvoytenko): confirm the desired value and document it well.
-    // Currently, this is 20px/second -> 0.02px/millis
-    if (Math.abs(velocity) < 0.02) {
+    // Currently, this is 30px/second -> 0.03px/millis
+    if (Math.abs(velocity) < 0.03) {
       this.changed_(/* relayoutAll */ false, velocity);
     } else {
-      timer.delay(() => {
-        if (!this.scrollTracking_) {
-          this.scrollDeferred_();
-        }
-      }, 250);
+      timer.delay(() => this.vsync_.measure(
+          this.throttledScroll_.bind(this, now, newScrollTop)), 20);
     }
   }
 

test/functional/test-amp-ad.js

@@ -19,7 +19,7 @@ import {installAd, AD_LOAD_TIME_MS} from '../../builtins/amp-ad';
 import {viewportFor} from
     '../../src/viewport';
 import {timer} from '../../src/timer';
-import {vsync} from '../../src/vsync';
+import {vsyncFor} from '../../src/vsync';
 import * as sinon from 'sinon';
 
 describe('amp-ad', () => {
@@ -230,12 +230,12 @@ describe('amp-ad', () => {
       expect(posts).to.have.length(1);
       ampAd.viewportCallback(true);
       expect(posts).to.have.length(2);
-      viewport.changed_(false, 0);
+      viewport.scroll_();
       expect(posts).to.have.length(3);
       ampAd.viewportCallback(false);
       expect(posts).to.have.length(4);
       // No longer listening.
-      viewport.changed_(false, 0);
+      viewport.scroll_();
       expect(posts).to.have.length(4);
     });
   });

test/functional/test-viewport.js

@@ -48,7 +48,10 @@ describe('Viewport', () => {
     windowApi = {
       document: {
         documentElement: {style: {}}
-      }
+      },
+      location: {},
+      setTimeout: window.setTimeout,
+      requestAnimationFrame: fn => window.setTimeout(fn, 16)
     };
     binding = new ViewportBindingVirtual_(windowApi, viewer);
     viewport = new Viewport(windowApi, binding, viewer);
@@ -132,44 +135,27 @@ describe('Viewport', () => {
     viewport.onChanged(event => {
       changeEvent = event;
     });
-    viewer.getScrollTop = () => {return 34;};
+    viewer.getScrollTop = () => 34;
     viewerViewportHandler();
     expect(changeEvent).to.equal(null);
 
     // Not enough time past.
-    clock.tick(100);
-    viewer.getScrollTop = () => {return 35;};
-    viewerViewportHandler();
+    clock.tick(8);
     expect(changeEvent).to.equal(null);
-
-    // A bit more time.
-    clock.tick(750);
-    expect(changeEvent).to.not.equal(null);
-    expect(changeEvent.relayoutAll).to.equal(false);
-    expect(changeEvent.velocity).to.be.closeTo(0.002, 1e-4);
-  });
-
-  it('should defer scroll events and react to reset of scroll pos', () => {
-    let changeEvent = null;
-    viewport.onChanged(event => {
-      changeEvent = event;
-    });
-    viewer.getScrollTop = () => {return 34;};
-    viewerViewportHandler();
+    clock.tick(8);
     expect(changeEvent).to.equal(null);
-
-    // Not enough time past.
-    clock.tick(100);
-    viewer.getScrollTop = () => {return 35;};
+    viewer.getScrollTop = () => 35;
     viewerViewportHandler();
+    clock.tick(16);
     expect(changeEvent).to.equal(null);
 
-    // Reset and wait a bit more time.
-    viewport./*OK*/scrollTop_ = null;
-    clock.tick(750);
+    // A bit more time.
+    clock.tick(16);
+    expect(changeEvent).to.equal(null);
+    clock.tick(4);
     expect(changeEvent).to.not.equal(null);
     expect(changeEvent.relayoutAll).to.equal(false);
-    expect(changeEvent.velocity).to.equal(0);
+    expect(changeEvent.velocity).to.be.closeTo(0.019230, 1e-4);
   });
 
   it('should update scroll pos and reset cache', () => {
@@ -363,7 +349,8 @@ describe('Viewport META', () => {
             }
             return undefined;
           }
-        }
+        },
+        location: {}
       };
 
       binding = new ViewportBindingVirtual_(windowApi, viewer);