diff --git a/builtins/amp-ad.js b/builtins/amp-ad.js index 3ecb95780a3d..75c745ee537e 100644 --- a/builtins/amp-ad.js +++ b/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_() { diff --git a/src/viewport.js b/src/viewport.js index 17fa4cd6847f..8399bafc6f60 100644 --- a/src/viewport.js +++ b/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,14 +389,7 @@ 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 @@ -394,44 +397,44 @@ export class Viewport { // 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); } } diff --git a/test/functional/test-amp-ad.js b/test/functional/test-amp-ad.js index 48ad2a649fda..8ab93eeecbc2 100644 --- a/test/functional/test-amp-ad.js +++ b/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); }); }); diff --git a/test/functional/test-viewport.js b/test/functional/test-viewport.js index 509001b49ef5..bd9988c56c1d 100644 --- a/test/functional/test-viewport.js +++ b/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);