Skip to content

Commit

Permalink
Simplify scroll tracking and greatly increase frequency in which we
Browse files Browse the repository at this point in the history
emit viewport change events on scroll.

- The scroll event now always fires in vsync and always measures
scrollTop.
- The viewport change event now happens at the latest 3 frames after
scrolling ended.
- Increased speed at which we send change event. Needed due to resolution at slow speed, but I also feel the value is better.

Changes ad intersection to fire "on scroll". Fixes ampproject#873

Fixes a bug where only the first viewability provider in an ad would get an initial viewability change record. Fixes ampproject#1126
  • Loading branch information
cramforce committed Dec 18, 2015
1 parent 4dfa7cf commit 3543226
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 68 deletions.
13 changes: 10 additions & 3 deletions builtins/amp-ad.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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_();
});
}
Expand All @@ -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;
Expand All @@ -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_() {
Expand Down
69 changes: 36 additions & 33 deletions src/viewport.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';


Expand Down Expand Up @@ -74,6 +75,9 @@ export class Viewport {
/** @private {?number} */
this./*OK*/scrollTop_ = null;

/** @private {?number} */
this.lastMeasureScrollTop_ = null;

/** @private {?number} */
this./*OK*/scrollLeft_ = null;

Expand All @@ -83,6 +87,9 @@ export class Viewport {
/** @private {number} */
this.scrollMeasureTime_ = 0;

/** @private {Vsync} */
this.vsync_ = vsyncFor(win);

/** @private {boolean} */
this.scrollTracking_ = false;

Expand All @@ -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();
Expand Down Expand Up @@ -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_;
Expand Down Expand Up @@ -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);
}
}

Expand Down
6 changes: 3 additions & 3 deletions test/functional/test-amp-ad.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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);
});
});
Expand Down
45 changes: 16 additions & 29 deletions test/functional/test-viewport.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -363,7 +349,8 @@ describe('Viewport META', () => {
}
return undefined;
}
}
},
location: {}
};

binding = new ViewportBindingVirtual_(windowApi, viewer);
Expand Down

0 comments on commit 3543226

Please sign in to comment.