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

[amp-analytics] More visibilitySpec vars. #3371

Merged
merged 3 commits into from
Jun 2, 2016
Merged

[amp-analytics] More visibilitySpec vars. #3371

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e488866
[amp-analytics] More visibilitySpec vars.
avimehta May 27, 2016
540f3c8
Added dep whitelist. Visibility-impl needs to know about visibility
avimehta May 27, 2016
a31a4b0
Updated some tests, review feedback.
avimehta Jun 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions build-system/dep-check-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ exports.rules = [
{
filesMatching: 'extensions/**/*.js',
mustNotDependOn: 'src/service/**/*.js',
whitelist: [
'extensions/amp-analytics/0.1/visibility-impl.js->src/service/viewer-impl.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? This shouldn't be needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually this is very dangerous. Let's remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.

],
},
{
filesMatching: 'extensions/**/*.js',
Expand Down
2 changes: 1 addition & 1 deletion examples/analytics.amp.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
"base": "https://example.com/?domain=${canonicalHost}&path=${canonicalPath}&title=${title}&time=${timestamp}&tz=${timezone}&pid=${pageViewId}&_=${random}",
"pageview": "${base}&name=${eventName}&type=${eventId}&screenSize=${screenWidth}x${screenHeight}",
"event": "${base}&name=${eventName}&scrollY=${scrollTop}&scrollX=${scrollLeft}&height=${availableScreenHeight}&width=${availableScreenWidth}&scrollBoundV=${verticalScrollBoundary}&scrollBoundH=${horizontalScrollBoundary}",
"visibility": "https://example.com/visibility?a=${maxContinuousTime}&b=${totalVisibleTime}&c=${firstSeenTime}&d=${lastSeenTime}&e=${fistVisibleTime}&f=${lastVisibleTime}&g=${minVisiblePercentage}&h=${maxVisiblePercentage}"
"visibility": "https://example.com/visibility?a=${maxContinuousTime}&b=${totalVisibleTime}&c=${firstSeenTime}&d=${lastSeenTime}&e=${fistVisibleTime}&f=${lastVisibleTime}&g=${minVisiblePercentage}&h=${maxVisiblePercentage}&i=${elementX}&j=${elementY}&k=${elementWidth}&l=${elementHeight}&m=${totalTime}&n=${loadTimeVisibility}&o=${backgroundedAtStart}&p=${backgrounded}"
},
"vars": {
"title": "Example Request"
Expand Down
110 changes: 98 additions & 12 deletions extensions/amp-analytics/0.1/visibility-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,34 @@

import {dev} from '../../../src/log';
import {getService} from '../../../src/service';
import {rectIntersection} from '../../../src/layout-rect';
import {resourcesFor} from '../../../src/resources';
import {timer} from '../../../src/timer';
import {user} from '../../../src/log';
import {viewportFor} from '../../../src/viewport';
import {viewerFor} from '../../../src/viewer';
import {VisibilityState} from '../../../src/service/viewer-impl';
Copy link
Contributor

Choose a reason for hiding this comment

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

We should either move it into viewer.js or into its own visibility-state.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.


/** @const {number} */
const LISTENER_INITIAL_RUN_DELAY_ = 20;

// Variables that are passed to the callback.
const MAX_CONTINUOUS_TIME = 'maxContinuousTime';
const TOTAL_TIME = 'totalVisibleTime';
const TOTAL_VISIBLE_TIME = 'totalVisibleTime';
const FIRST_SEEN_TIME = 'firstSeenTime';
const LAST_SEEN_TIME = 'lastSeenTime';
const FIRST_VISIBLE_TIME = 'fistVisibleTime';
const LAST_VISIBLE_TIME = 'lastVisibleTime';
const MIN_VISIBLE = 'minVisiblePercentage';
const MAX_VISIBLE = 'maxVisiblePercentage';
const ELEMENT_X = 'elementX';
const ELEMENT_Y = 'elementY';
const ELEMENT_WIDTH = 'elementWidth';
const ELEMENT_HEIGHT = 'elementHeight';
const TOTAL_TIME = 'totalTime';
const LOAD_TIME_VISIBILITY = 'loadTimeVisibility';
const BACKGROUNDED = 'backgrounded';
const BACKGROUNDED_AT_START = 'backgroundedAtStart';

// Variables that are not exposed outside this class.
const CONTINUOUS_TIME = 'cT';
Expand Down Expand Up @@ -169,9 +180,15 @@ export class Visibility {
/** @private @const {function} */
this.boundScrollListener_ = this.scrollListener_.bind(this);

/** @private @const {function} */
this.boundVisibilityListener_ = this.visibilityListener_.bind(this);

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

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

/** @private {!Resources} */
this.resourcesService_ = resourcesFor(this.win_);

Expand All @@ -183,11 +200,29 @@ export class Visibility {

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

/** @private {Viewer} */
this.viewer_ = viewerFor(this.win_);

/** @private {boolean} */
this.backgroundedAtStart_ = !this.viewer_.isVisible();

/** @private {boolean} */
this.backgrounded_ = this.backgroundedAtStart_;
}

/** @private */
registerForVisibilityEvents_() {
if (!this.visibilityListenerRegistered_) {
this.viewer_.onVisibilityChanged(this.boundVisibilityListener_);
this.visibilityListenerRegistered_ = true;
this.visibilityListener_();
}
}

/** @private */
registerForViewportEvents_() {
if (!this.scrollListenerRegistered__) {
if (!this.scrollListenerRegistered_) {
const viewport = viewportFor(this.win_);

// Currently unlistens are not being used. In the event that no resources
Expand All @@ -196,7 +231,6 @@ export class Visibility {
viewport.onChanged(this.boundScrollListener_);
this.scrollListenerRegistered_ = true;
}

}

/**
Expand All @@ -210,6 +244,7 @@ export class Visibility {
const resId = res.getId();

this.registerForViewportEvents_();
this.registerForVisibilityEvents_();

this.listeners_[resId] = (this.listeners_[resId] || []);
this.listeners_[resId].push({
Expand All @@ -226,6 +261,16 @@ export class Visibility {
}
}

/** @private */
visibilityListener_() {
// viewer.getVisibilityState works when the page is loaded inside a viewer.
Copy link
Contributor

Choose a reason for hiding this comment

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

It should work in all other cases as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it wasn't working at some point and I had more code in this function. since that code is removed, this comment did not make sense. removed.

const state = this.viewer_.getVisibilityState();
if (state == VisibilityState.HIDDEN || state == VisibilityState.PAUSED ||
state == VisibilityState.INACTIVE) {
this.backgrounded_ = true;
}
}

/** @private */
scrollListener_() {
if (this.scheduledRunId_ != null) {
Expand All @@ -252,10 +297,8 @@ export class Visibility {
for (let c = listeners.length - 1; c >= 0; c--) {
if (this.updateCounters_(visible, listeners[c])) {

// Remove the state that need not be public and call callback.
delete listeners[c]['state'][CONTINUOUS_TIME];
delete listeners[c]['state'][LAST_UPDATE];
delete listeners[c]['state'][IN_VIEWPORT];
this.prepareStateForCallback_(listeners[c]['state'],
change.rootBounds, br, ir);
listeners[c].callback(listeners[c]['state']);
listeners.splice(c, 1);
}
Expand Down Expand Up @@ -316,7 +359,7 @@ export class Visibility {
state[CONTINUOUS_TIME] + timeSinceLastUpdate);

state[LAST_UPDATE] = -1;
state[TOTAL_TIME] += timeSinceLastUpdate;
state[TOTAL_VISIBLE_TIME] += timeSinceLastUpdate;
state[CONTINUOUS_TIME] = 0; // Clear only after max is calculated above.
state[LAST_VISIBLE_TIME] = Date.now() - state[TIME_LOADED];
} else if (state[IN_VIEWPORT] && !wasInViewport) {
Expand All @@ -335,7 +378,7 @@ export class Visibility {
? config[CONTINUOUS_TIME_MIN] - state[CONTINUOUS_TIME]
: Infinity;
const waitForTotalTime = config[TOTAL_TIME_MIN]
? config[TOTAL_TIME_MIN] - state[TOTAL_TIME]
? config[TOTAL_TIME_MIN] - state[TOTAL_VISIBLE_TIME]
: Infinity;

// Wait for minimum of (previous timeToWait, positive values of
Expand All @@ -346,9 +389,9 @@ export class Visibility {
listener['state'] = state;
return state[IN_VIEWPORT] &&
(config[TOTAL_TIME_MIN] === undefined ||
state[TOTAL_TIME] >= config[TOTAL_TIME_MIN]) &&
state[TOTAL_VISIBLE_TIME] >= config[TOTAL_TIME_MIN]) &&
(config[TOTAL_TIME_MAX] === undefined ||
state[TOTAL_TIME] <= config[TOTAL_TIME_MAX]) &&
state[TOTAL_VISIBLE_TIME] <= config[TOTAL_TIME_MAX]) &&
(config[CONTINUOUS_TIME_MIN] === undefined ||
state[CONTINUOUS_TIME] >= config[CONTINUOUS_TIME_MIN]) &&
(config[CONTINUOUS_TIME_MAX] === undefined ||
Expand Down Expand Up @@ -378,7 +421,8 @@ export class Visibility {
/** @private */
setState_(s, visible, sinceLast) {
s[LAST_UPDATE] = Date.now();
s[TOTAL_TIME] = s[TOTAL_TIME] !== undefined ? s[TOTAL_TIME] + sinceLast : 0;
s[TOTAL_VISIBLE_TIME] = s[TOTAL_VISIBLE_TIME] !== undefined
? s[TOTAL_VISIBLE_TIME] + sinceLast : 0;
s[CONTINUOUS_TIME] = s[CONTINUOUS_TIME] !== undefined
? s[CONTINUOUS_TIME] + sinceLast : 0;
s[MAX_CONTINUOUS_TIME] = s[MAX_CONTINUOUS_TIME] !== undefined
Expand All @@ -387,6 +431,48 @@ export class Visibility {
s[MAX_VISIBLE] = s[MAX_VISIBLE] ? Math.max(s[MAX_VISIBLE], visible) : -1;
s[LAST_VISIBLE_TIME] = Date.now() - s[TIME_LOADED];
}

/**
* Sets variable values for callback. Cleans up existing values.
* @param {Object<string, *>} state The state object to populate
* @param {!LayoutRect} rb Bounds of Root object. (the viewport in this case)
* @param {!LayoutRect} br The bounding rectangle for the element
* @param {!LayoutRect} ir The intersection between element and the viewport
* @private
*/
prepareStateForCallback_(state, rb, br, ir) {
const perf = this.win_.performance;
state[ELEMENT_X] = rb.left + br.left;
state[ELEMENT_Y] = rb.top + br.top;
state[ELEMENT_WIDTH] = br.width;
state[ELEMENT_HEIGHT] = br.height;
state[TOTAL_TIME] = perf && perf.timing && perf.timing.domInteractive
? Date.now() - perf.timing.domInteractive
: '';
const intersection = rectIntersection(
{top: 0, height: rb.height, left: 0, width: rb.width},
{top: ir.top, left: ir.left, width: br.width, height: br.height});
state[LOAD_TIME_VISIBILITY] = intersection != null
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please comment here the meaning of this formula?

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 comment.

? Math.round(intersection.width * intersection.height * 10000
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we do anything when br.width or br.height are 0?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why 10,000?

Copy link
Contributor Author

@avimehta avimehta Jun 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This allows for two significant digits after decimal. So 12.344 becomes 12.34 and 12.346 becomes 12.35

Regarding br.width and br.height: I'll need to add checks for that. Currently that would be a problem.

(in practical cases, tracking 0 width/height elements does not make sense.)

/ (br.width * br.height)) / 100
: 0;
state[MIN_VISIBLE] = Math.round(state[MIN_VISIBLE] * 100) / 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite understand rounding rules. Could you please clarify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows for two significant digits after decimal. So 12.344 becomes 12.34 and 12.346 becomes 12.35

state[MAX_VISIBLE] = Math.round(state[MAX_VISIBLE] * 100) / 100;
state[BACKGROUNDED] = this.backgrounded_ ? '1' : '0';
state[BACKGROUNDED_AT_START] = this.backgroundedAtStart_ ? '1' : '0';

// Remove the state that need not be public and call callback.
delete state[CONTINUOUS_TIME];
delete state[LAST_UPDATE];
delete state[IN_VIEWPORT];
delete state[TIME_LOADED];

for (const k in state) {
if (state.hasOwnProperty(k)) {
state[k] = String(state[k]);
}
}
}
}

/**
Expand Down
16 changes: 10 additions & 6 deletions test/functional/test-visibility.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
installVisibilityService,
} from '../../extensions/amp-analytics/0.1/visibility-impl';
import {installResourcesService} from '../../src/service/resources-impl';
import {layoutRectLtwh} from '../../src/layout-rect';
import {visibilityFor} from '../../src/visibility';
import * as sinon from 'sinon';

Expand All @@ -35,12 +36,14 @@ describe('Visibility (tag: amp-analytics)', () => {
let callbackStub;

const INTERSECTION_0P = {
intersectionRect: {width: 0, height: 0},
boundingClientRect: {height: 100, width: 100},
intersectionRect: layoutRectLtwh(0, 0, 0, 0),
boundingClientRect: layoutRectLtwh(100, 100, 100, 100),
rootBounds: layoutRectLtwh(0, 0, 100, 100),
};
const INTERSECTION_50P = {
intersectionRect: {width: 50, height: 100},
boundingClientRect: {height: 100, width: 100},
intersectionRect: layoutRectLtwh(50, 0, 50, 100),
boundingClientRect: layoutRectLtwh(50, 0, 100, 100),
rootBounds: layoutRectLtwh(0, 0, 100, 100),
};

beforeEach(() => {
Expand Down Expand Up @@ -94,8 +97,9 @@ describe('Visibility (tag: amp-analytics)', () => {

it('fires for non-trivial config', () => {
listen({
intersectionRect: {height: 100, width: 49},
boundingClientRect: {height: 100, width: 100},
intersectionRect: layoutRectLtwh(0, 0, 49, 100),
Copy link
Contributor

Choose a reason for hiding this comment

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

More tests for the actual visibility times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated some tests. I'll continue to write more tests.

boundingClientRect: layoutRectLtwh(0, 0, 49, 100),
rootBounds: layoutRectLtwh(0, 0, 100, 100),
}, {visiblePercentageMin: 49, visiblePercentageMax: 80}, 0);

verifyChange(INTERSECTION_50P, 1);
Expand Down