Skip to content

Commit

Permalink
Visibility Trigger for Non AMP Elements (#29172)
Browse files Browse the repository at this point in the history 
* init

* Manual test

* Make default behavior

* WaitFor changes

* Suggested changes

* Clean and add error msg
  • Loading branch information
Micajuine Ho committed Sep 9, 2020
1 parent 6407ca8 commit 6874d96
Show file tree
Hide file tree
Showing 6 changed files with 633 additions and 337 deletions.
9 changes: 4 additions & 5 deletions extensions/amp-analytics/0.1/analytics-root.js
View file
Expand Up @@ -266,7 +266,6 @@ export class AnalyticsRoot {
}
elementArray = this.getDataVarsElements_(elementArray, selector);
userAssert(elementArray.length, `Element "${selector}" not found`);
this.verifyAmpElements_(elementArray, selector);
elements = elements.concat(elementArray);
}
// Return unique
Expand Down Expand Up @@ -331,17 +330,17 @@ export class AnalyticsRoot {
}

/**
* Searches for the AMP element(s) that matches the selector
* Searches for the element(s) that matches the selector
* within the scope of the analytics root in relationship to
* the specified context node.
*
* @param {!Element} context
* @param {!Array<string>|string} selectors DOM query selector(s).
* @param {?string=} selectionMethod Allowed values are `null`,
* `'closest'` and `'scope'`.
* @return {!Promise<!Array<!AmpElement>>} Array of AMP elements corresponding to the selector if found.
* @return {!Promise<!Array<!Element>>} Array of elements corresponding to the selector if found.
*/
getAmpElements(context, selectors, selectionMethod) {
getElements(context, selectors, selectionMethod) {
if (
isExperimentOn(this.ampdoc.win, 'visibility-trigger-improvements') &&
isArray(selectors)
Expand All @@ -355,7 +354,7 @@ export class AnalyticsRoot {
/** @type {!Array<string>} */ (selectors)
);
}
return this.getAmpElement(
return this.getElement(
context,
/** @type {string} */ (selectors),
selectionMethod
Expand Down
33 changes: 20 additions & 13 deletions extensions/amp-analytics/0.1/events.js
View file
Expand Up @@ -25,7 +25,7 @@ import {
import {deepMerge, dict, hasOwn} from '../../../src/utils/object';
import {dev, devAssert, user, userAssert} from '../../../src/log';
import {getData} from '../../../src/event-helper';
import {getDataParamsFromAttributes} from '../../../src/dom';
import {getDataParamsFromAttributes, isAmpElement} from '../../../src/dom';
import {isArray, isEnumValue, isFiniteNumber} from '../../../src/types';
import {startsWith} from '../../../src/string';

Expand Down Expand Up @@ -1446,7 +1446,6 @@ export class VisibilityTracker extends EventTracker {
const visibilitySpec = config['visibilitySpec'] || {};
const selector = config['selector'] || visibilitySpec['selector'];
const waitForSpec = visibilitySpec['waitFor'];
let readyPromiseWaitForSpec;
let reportWhenSpec = visibilitySpec['reportWhen'];
let createReportReadyPromiseFunc = null;
if (reportWhenSpec) {
Expand Down Expand Up @@ -1489,7 +1488,8 @@ export class VisibilityTracker extends EventTracker {
if (!selector || selector == ':root' || selector == ':host') {
// When `selector` is specified, we always use "ini-load" signal as
// a "ready" signal.
readyPromiseWaitForSpec = waitForSpec || (selector ? 'ini-load' : null);
const readyPromiseWaitForSpec =
waitForSpec || (selector ? 'ini-load' : 'none');
return visibilityManager.listenRoot(
visibilitySpec,
this.getReadyPromise(readyPromiseWaitForSpec),
Expand All @@ -1503,27 +1503,22 @@ export class VisibilityTracker extends EventTracker {
);
}

// An AMP-element. Wait for DOM to be fully parsed to avoid
// An element. Wait for DOM to be fully parsed to avoid
// false missed searches.
// Array selectors do not suppor the special cases: ':host' & ':root'
const selectionMethod =
config['selectionMethod'] || visibilitySpec['selectionMethod'];
readyPromiseWaitForSpec = waitForSpec || 'ini-load';
this.assertUniqueSelectors_(selector);
const unlistenPromise = this.root
.getAmpElements(
context.parentElement || context,
selector,
selectionMethod
)
.getElements(context.parentElement || context, selector, selectionMethod)
.then((elements) => {
const unlistenCallbacks = [];
for (let i = 0; i < elements.length; i++) {
unlistenCallbacks.push(
visibilityManager.listenElement(
elements[i],
visibilitySpec,
this.getReadyPromise(readyPromiseWaitForSpec, elements[i]),
this.getReadyPromise(waitForSpec, elements[i]),
createReportReadyPromiseFunc,
this.onEvent_.bind(this, eventType, listener, elements[i])
)
Expand Down Expand Up @@ -1645,14 +1640,26 @@ export class VisibilityTracker extends EventTracker {
* @visibleForTesting
*/
getReadyPromise(waitForSpec, opt_element) {
if (!waitForSpec) {
if (opt_element) {
if (!isAmpElement(opt_element)) {
userAssert(
!waitForSpec || waitForSpec == 'none',
'waitFor for non-AMP elements must be none or null. Found %s',
waitForSpec
);
} else {
waitForSpec = waitForSpec || 'ini-load';
}
}

if (!waitForSpec || waitForSpec == 'none') {
// Default case, waitFor selector is not defined, wait for nothing
return null;
}

const trackerAllowlist = getTrackerTypesForParentType('visible');
userAssert(
waitForSpec == 'none' || trackerAllowlist[waitForSpec] !== undefined,
trackerAllowlist[waitForSpec] !== undefined,
'waitFor value %s not supported',
waitForSpec
);
Expand Down
81 changes: 51 additions & 30 deletions extensions/amp-analytics/0.1/test/test-analytics-root.js
View file
Expand Up @@ -323,7 +323,7 @@ describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => {
});
});

describe('get amp elements', () => {
describe('get elements', () => {
let child2;
let child3;

Expand All @@ -350,13 +350,14 @@ describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => {
child.classList.add('myClass');
child2.classList.add('myClass');
child3.classList.add('notMyClass');
expect(
await root.getAmpElements(body, ['.myClass'], null)
).to.deep.equal([child, child2]);
expect(await root.getElements(body, ['.myClass'], null)).to.deep.equal([
child,
child2,
]);
// Check that non-experiment works
toggleExperiment(win, 'visibility-trigger-improvements', false);
expect(
await root.getAmpElements(body, '.notMyClass', null)
await root.getElements(body, '.notMyClass', null)
).to.deep.equal([child3]);
});

Expand All @@ -368,7 +369,7 @@ describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => {
child3.classList.add('myClass');

child3.removeAttribute('data-vars-id');
const children = await root.getAmpElements(body, ['.myClass']);
const children = await root.getElements(body, ['.myClass']);
expect(spy).callCount(1);
expect(spy).to.have.been.calledWith(
'amp-analytics/analytics-root',
Expand All @@ -383,43 +384,55 @@ describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => {
child.id = 'myId';
child.classList.add('myClass');
expect(
await root.getAmpElements(body, ['.myClass', '#myId'], null)
await root.getElements(body, ['.myClass', '#myId'], null)
).to.deep.equal([child]);
});

it('should ignore special selectors', async () => {
child.classList.add('myClass');
expectAsyncConsoleError(/Element ":host" not found/, 1);
await expect(
root.getAmpElements(body, [':host'], null)
root.getElements(body, [':host'], null)
).to.be.rejectedWith(/Element ":host" not found​​​/);
});

it('should handle missing selector for AMP search', async () => {
expectAsyncConsoleError(/Element "#unknown" not found/, 1);
await expect(
root.getAmpElements(body, ['#unknown'], null)
root.getElements(body, ['#unknown'], null)
).to.be.rejectedWith(/Element "#unknown" not found​​​/);
});

it('should handle invalid selector', async () => {
expectAsyncConsoleError(/Invalid query selector 12345/, 1);
await expect(
root.getAmpElements(body, [12345], null)
).to.be.rejectedWith(/Invalid query selector 12345​​​/);
await expect(root.getElements(body, [12345], null)).to.be.rejectedWith(
/Invalid query selector 12345​​​/
);
});

it('should fail if the found element is not AMP for AMP search', async () => {
expectAsyncConsoleError(/required to be an AMP element/, 1);
it('should find both AMP and non AMP elements within array selector', async () => {
child.classList.remove('i-amphtml-element');
await expect(
root.getAmpElements(body, ['#child'], null)
).to.be.rejectedWith(/required to be an AMP element/);
child.classList.add('myClass');
child2.classList.add('myClass');
expect(await root.getElements(body, ['.myClass'], null)).to.deep.equal([
child,
child2,
]);
});

it('should find non AMP element with single selector', async () => {
toggleExperiment(win, 'visibility-trigger-improvements', false);
child.classList.remove('i-amphtml-element');
child.removeAttribute('data-vars-id');
child.classList.add('myClass');
expect(await root.getElements(body, '.myClass', null)).to.deep.equal([
child,
]);
});

it('should fail if selection method is found', async () => {
try {
await root.getAmpElements(body, ['#child'], 'scope');
await root.getElements(body, ['#child'], 'scope');
} catch (e) {
expect(e).to.match(
/Cannot have selectionMethod scope defined with an array selector/
Expand Down Expand Up @@ -716,7 +729,7 @@ describes.realWin(
});
});

describe('get amp elements', () => {
describe('get elements', () => {
let child2;
let child3;

Expand Down Expand Up @@ -751,13 +764,13 @@ describes.realWin(
});

it('should find all elements by selector', async () => {
const elements = await root.getAmpElements(body, ['.myClass'], null);
const elements = await root.getElements(body, ['.myClass'], null);

expect(elements).to.deep.equals([child, child2]);
// Check that non-experiment version works
toggleExperiment(win, 'visibility-trigger-improvements', false);
expect(
await root.getAmpElements(body, '.notMyClass', null)
await root.getElements(body, '.notMyClass', null)
).to.deep.equals([child3]);
});

Expand All @@ -768,7 +781,7 @@ describes.realWin(
parentChild.classList.add('i-amphtml-element');
parentChild.setAttribute('data-vars-id', 'abc');

const elements = await root.getAmpElements(body, ['.myClass'], null);
const elements = await root.getElements(body, ['.myClass'], null);
expect(elements).to.deep.equals([child, child2]);
});

Expand All @@ -778,7 +791,7 @@ describes.realWin(
child3.classList.add('myClass');
child3.removeAttribute('data-vars-id');

const children = await root.getAmpElements(body, ['.myClass']);
const children = await root.getElements(body, ['.myClass']);
expect(spy).callCount(1);
expect(spy).to.have.been.calledWith(
'amp-analytics/analytics-root',
Expand All @@ -796,24 +809,32 @@ describes.realWin(
child2.classList.add('myClass');
// Each selector should find both elements, but only report once
expect(
await root.getAmpElements(body, ['.myClass', '#myId'], null)
await root.getElements(body, ['.myClass', '#myId'], null)
).to.deep.equal([child, child2]);
});

it('should handle missing selector for AMP search', async () => {
expectAsyncConsoleError(/Element "#unknown" not found/, 1);
await expect(
root.getAmpElements(body, ['#unknown'], null)
root.getElements(body, ['#unknown'], null)
).to.be.rejectedWith(/Element "#unknown" not found​​​/);
});

it('should fail if the found element is not AMP for AMP search', async () => {
expectAsyncConsoleError(/required to be an AMP element/, 1);
it('should find both AMP and non AMP elements', async () => {
child.classList.remove('i-amphtml-element');
child.setAttribute('data-vars-id', '123');
await expect(
root.getAmpElements(body, ['#child'], null)
).to.be.rejectedWith(/required to be an AMP element/);
expect(await root.getElements(body, ['.myClass'], null)).to.deep.equal([
child,
child2,
]);
});

it('should find non AMP element with single selector', async () => {
toggleExperiment(win, 'visibility-trigger-improvements', false);
child.classList.remove('i-amphtml-element');
expect(await root.getElements(body, '.myClass', null)).to.deep.equal([
child,
]);
});
});

Expand Down

0 comments on commit 6874d96

Please sign in to comment.