Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support 'hidden' trigger for visibility-v3 #8805

Merged
merged 3 commits into from Apr 19, 2017
Merged

Support 'hidden' trigger for visibility-v3 #8805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b99c00d
add createReadyReportPromise func to visibility-model
zhouyx Apr 17, 2017
e12899d
address comment
zhouyx Apr 19, 2017
33a0d3c
fix
zhouyx Apr 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions examples/analytics.amp.html
View file
Open in desktop
Expand Up @@ -88,6 +88,7 @@
"visibilitySpec": {
"selector": "#anim-id",
"visiblePercentageMin": 20,
"visiblePercentageMin": 1000,
"continuousTimeMax": 5000
},
"vars": {
Expand Down
13 changes: 12 additions & 1 deletion extensions/amp-analytics/0.1/analytics-root.js
View file
Open in desktop
Expand Up @@ -27,7 +27,10 @@ import {dev, user} from '../../../src/log';
import {getMode} from '../../../src/mode';
import {layoutRectLtwh} from '../../../src/layout-rect';
import {map} from '../../../src/utils/object';
import {viewportForDoc} from '../../../src/services';
import {
viewerForDoc,
viewportForDoc,
} from '../../../src/services';
import {whenContentIniLoad} from '../../../src/friendly-iframe-embed';

const TAG = 'amp-analytics';
Expand Down Expand Up @@ -88,6 +91,14 @@ export class AnalyticsRoot {
*/
getRoot() {}

/**
* The viewer of analytics root
* @return {!../../../src/service/viewer-impl.Viewer}
*/
getViewer() {
return viewerForDoc(this.ampdoc);
}

/**
* The root element within the analytics root.
*
Expand Down
27 changes: 27 additions & 0 deletions extensions/amp-analytics/0.1/events.js
View file
Open in desktop
Expand Up @@ -377,6 +377,11 @@ export class VisibilityTracker extends EventTracker {
const selector = config['selector'] || visibilitySpec['selector'];
const waitForSpec = visibilitySpec['waitFor'];
const visibilityManager = this.root.getVisibilityManager();
// special polyfill for eventType: 'hidden'
let createReadyReportPromiseFunc = null;
if (eventType == 'hidden-v3') {
createReadyReportPromiseFunc = this.createReportReadyPromise_.bind(this);
}

// Root selectors are delegated to analytics roots.
if (!selector || selector == ':root' || selector == ':host') {
Expand All @@ -385,6 +390,7 @@ export class VisibilityTracker extends EventTracker {
return visibilityManager.listenRoot(
visibilitySpec,
this.getReadyPromise(waitForSpec, selector),
createReadyReportPromiseFunc,
this.onEvent_.bind(
this, eventType, listener, this.root.getRootElement()));
}
Expand All @@ -404,6 +410,7 @@ export class VisibilityTracker extends EventTracker {
element,
visibilitySpec,
this.getReadyPromise(waitForSpec, selector, element),
createReadyReportPromiseFunc,
this.onEvent_.bind(this, eventType, listener, element));
});
return function() {
Expand All @@ -413,6 +420,26 @@ export class VisibilityTracker extends EventTracker {
};
}

/**
* @return {!Promise}
* @visibleForTesting
*/
createReportReadyPromise_() {
const viewer = this.root.getViewer();

if (!viewer.isVisible()) {
return Promise.resolve();
}

return new Promise(resolve => {
viewer.onVisibilityChanged(() => {
if (!viewer.isVisible()) {
resolve();
}
});
});
}

/**
* @param {string|undefined} waitForSpec
* @param {string|undefined} selector
Expand Down
10 changes: 8 additions & 2 deletions extensions/amp-analytics/0.1/instrumentation.js
View file
Open in desktop
Expand Up @@ -104,6 +104,11 @@ const EVENT_TRACKERS = {
allowedFor: ALLOWED_FOR_ALL,
klass: VisibilityTracker,
},
'hidden-v3': {
name: 'visible-v3', // Reuse tracker with visibility
allowedFor: ALLOWED_FOR_ALL,
klass: VisibilityTracker,
},
};

/** @const {string} */
Expand Down Expand Up @@ -566,8 +571,9 @@ export class AnalyticsGroup {
addTrigger(config, handler) {
let eventType = dev().assertString(config['on']);
// TODO(dvoytenko, #8121): Cleanup visibility-v3 experiment.
if (eventType == 'visible' && this.visibilityV3_) {
eventType = 'visible-v3';
if ((eventType == 'visible' || eventType == 'hidden')
&& this.visibilityV3_) {
eventType += '-v3';
}
let trackerProfile = EVENT_TRACKERS[eventType];
if (!trackerProfile && !isEnumValue(AnalyticsEventType, eventType)) {
Expand Down
53 changes: 53 additions & 0 deletions extensions/amp-analytics/0.1/test/test-events.js
View file
Open in desktop
Expand Up @@ -411,6 +411,7 @@ describes.realWin('Events', {amp: 1}, env => {
let eventResolver, eventPromise;
let saveCallback;
let matchEmptySpec;
let matchFunc;

beforeEach(() => {
tracker = new VisibilityTracker(root);
Expand All @@ -429,6 +430,15 @@ describes.realWin('Events', {amp: 1}, env => {
matchEmptySpec = sinon.match(arg => {
return Object.keys(arg).length == 0;
});
matchFunc = sinon.match(arg => {
if (typeof arg == 'function') {
const promise = arg();
if (typeof promise.then == 'function') {
return true;
}
}
return false;
});
saveCallback = sinon.match(arg => {
if (typeof arg == 'function') {
saveCallback.callback = arg;
Expand All @@ -455,6 +465,7 @@ describes.realWin('Events', {amp: 1}, env => {
.withExactArgs(
matchEmptySpec,
/* readyPromise */ null,
/* createReadyReportPromiseFunc */ null,
saveCallback)
.returns(unlisten)
.once();
Expand Down Expand Up @@ -482,6 +493,7 @@ describes.realWin('Events', {amp: 1}, env => {
.withExactArgs(
matchEmptySpec,
readyPromise,
null,
saveCallback)
.returns(unlisten)
.once();
Expand Down Expand Up @@ -510,6 +522,7 @@ describes.realWin('Events', {amp: 1}, env => {
.withExactArgs(
config.visibilitySpec,
readyPromise,
/* createReadyReportPromiseFunc */ null,
saveCallback)
.returns(unlisten)
.once();
Expand Down Expand Up @@ -540,6 +553,7 @@ describes.realWin('Events', {amp: 1}, env => {
target,
config.visibilitySpec,
readyPromise,
/* createReadyReportPromiseFunc */ null,
saveCallback)
.returns(unlisten)
.once();
Expand Down Expand Up @@ -580,6 +594,7 @@ describes.realWin('Events', {amp: 1}, env => {
target,
matchEmptySpec,
readyPromise,
/* createReadyReportPromiseFunc */ null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need tests to make sure we add calls with non-null createReadyReportPromiseFunc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a test, to check if createReadyReportPromiseFunc is a function that return a promise when eventType is hidden-v3.

saveCallback)
.returns(unlisten)
.once();
Expand All @@ -593,6 +608,30 @@ describes.realWin('Events', {amp: 1}, env => {
});
});

it('should pass func to get reportReady with "hidden" trigger', () => {
const config = {visibilitySpec: {selector: '.target', waitFor: 'none'}};
visibilityManagerMock
.expects('listenElement')
.withExactArgs(
target,
config.visibilitySpec,
/* readyPromise */ null,
/* createReadyReportPromiseFunc */ matchFunc,
saveCallback)
.returns(null)
.once();
tracker.add(analyticsElement, 'hidden-v3', config, eventResolver);
// NOTE: createReadyReportPromiseFunc is
// fully tested in test-visibility-manager
return root.ampdoc.whenReady().then(() => {
saveCallback.callback({totalVisibleTime: 10});
return eventPromise.then(event => {
expect(event.vars.totalVisibleTime).to.equal(10);
expect(event.type).to.equal('hidden-v3');
});
});
});

describe('should wait on correct readyPromise', () => {
const selector = '.target';

Expand Down Expand Up @@ -678,5 +717,19 @@ describes.realWin('Events', {amp: 1}, env => {
});
});
});

describe('should create correct readyReportPromise', () => {
it('with viewer hidden', () => {
const stub = sandbox.stub(tracker.root, 'getViewer', () => {
return {
isVisible: () => {return false;},
};
});
const promise = tracker.createReportReadyPromise_();
return promise.then(() => {
expect(stub).to.be.calledOnce;
});
});
});
});
});
14 changes: 14 additions & 0 deletions extensions/amp-analytics/0.1/test/test-instrumentation.js
View file
Open in desktop
Expand Up @@ -235,6 +235,20 @@ describes.realWin('InstrumentationService', {amp: 1}, env => {
analyticsElement, 'visible-v3', config, handler);
});

it('should add "visible-v3" trigger for hidden', () => {
toggleExperiment(win, 'visibility-v3', true);
group = service.createAnalyticsGroup(analyticsElement);
const config = {on: 'hidden'};
const getTrackerSpy = sandbox.spy(root, 'getTracker');
group.addTrigger(config, () => {});
expect(getTrackerSpy).to.be.calledWith('visible-v3');
const tracker = root.getTrackerOptional('visible-v3');
const unlisten = function() {};
const stub = sandbox.stub(tracker, 'add', () => unlisten);
group.addTrigger(config, () => {});
expect(stub).to.be.calledWith(analyticsElement, 'hidden-v3', config);
});

it('should use "visible-v3" for "visible" w/experiment', () => {
// TODO(dvoytenko, #8121): Cleanup visibility-v3 experiment.
toggleExperiment(win, 'visibility-v3', true);
Expand Down