From db28aadcbf515a3e01f79fb9db7a344c01d99e60 Mon Sep 17 00:00:00 2001 From: Micajuine Ho Date: Wed, 19 Feb 2020 10:39:30 -0800 Subject: [PATCH 01/11] Impl, unit test, & manual test --- extensions/amp-analytics/0.1/amp-analytics.js | 22 ++++ extensions/amp-analytics/0.1/events.js | 64 +++++++--- .../0.1/test/test-amp-analytics.js | 69 ++++++++++ .../amp-analytics/0.1/test/test-events.js | 118 ++++++++++++++++++ .../amp-analytics-visible-repeat.amp.html | 24 ++++ tools/experiments/experiments-config.js | 6 + 6 files changed, 283 insertions(+), 20 deletions(-) diff --git a/extensions/amp-analytics/0.1/amp-analytics.js b/extensions/amp-analytics/0.1/amp-analytics.js index 38b26eb4a744..dd83212a7ddc 100644 --- a/extensions/amp-analytics/0.1/amp-analytics.js +++ b/extensions/amp-analytics/0.1/amp-analytics.js @@ -39,6 +39,7 @@ import {expandTemplate} from '../../../src/string'; import {getMode} from '../../../src/mode'; import {installLinkerReaderService} from './linker-reader'; import {isArray, isEnumValue} from '../../../src/types'; +import {isExperimentOn} from '../../../src/experiments'; import {isIframed} from '../../../src/dom'; import {isInFie} from '../../../src/iframe-helper'; import {toggle} from '../../../src/style'; @@ -105,6 +106,12 @@ export class AmpAnalytics extends AMP.BaseElement { /** @private {?boolean} */ this.isInFie_ = null; + + /** @private {boolean} */ + this.multiSelectorVisibilityOn_ = isExperimentOn( + this.win, + 'multi-selector-visibility-trigger' + ); } /** @override */ @@ -360,6 +367,21 @@ export class AmpAnalytics extends AMP.BaseElement { this.addTrigger_(trigger); } else if (trigger['selector']) { // Expand the selector using variable expansion. + if (this.multiSelectorVisibilityOn_) { + if (Array.isArray(trigger['selector'])) { + const selectorPromises = trigger['selector'].map(s => { + return this.variableService_.expandTemplate( + s, + expansionOptions, + this.element + ); + }); + return Promise.all(selectorPromises).then(selectors => { + trigger['selector'] = selectors; + this.addTrigger_(trigger); + }); + } + } return this.variableService_ .expandTemplate( trigger['selector'], diff --git a/extensions/amp-analytics/0.1/events.js b/extensions/amp-analytics/0.1/events.js index 4a3702730c1d..0b3c97844437 100644 --- a/extensions/amp-analytics/0.1/events.js +++ b/extensions/amp-analytics/0.1/events.js @@ -27,6 +27,7 @@ import {dict, hasOwn} from '../../../src/utils/object'; import {getData} from '../../../src/event-helper'; import {getDataParamsFromAttributes} from '../../../src/dom'; import {isEnumValue, isFiniteNumber} from '../../../src/types'; +import {isExperimentOn} from '../../../src/experiments'; import {startsWith} from '../../../src/string'; const SCROLL_PRECISION_PERCENT = 5; @@ -1424,7 +1425,10 @@ export class VisibilityTracker extends EventTracker { const waitForSpec = visibilitySpec['waitFor']; let reportWhenSpec = visibilitySpec['reportWhen']; let createReportReadyPromiseFunc = null; - + const multiSelectorVisibilityOn = isExperimentOn( + this.root.ampdoc.win, + 'multi-selector-visibility-trigger' + ); if (reportWhenSpec) { userAssert( !visibilitySpec['repeat'], @@ -1468,6 +1472,29 @@ export class VisibilityTracker extends EventTracker { ); } + const getUnlistenPromiseForSelector = (selector, selectionMethod) => { + return this.root + .getAmpElement( + context.parentElement || context, + selector, + selectionMethod + ) + .then(element => { + return visibilityManagerPromise.then( + visibilityManager => { + return visibilityManager.listenElement( + element, + visibilitySpec, + this.getReadyPromise(waitForSpec, selector, element), + createReportReadyPromiseFunc, + this.onEvent_.bind(this, eventType, listener, element) + ); + }, + () => {} + ); + }); + }; + let unlistenPromise; // Root selectors are delegated to analytics roots. if (!selector || selector == ':root' || selector == ':host') { @@ -1494,31 +1521,28 @@ export class VisibilityTracker extends EventTracker { // false missed searches. const selectionMethod = config['selectionMethod'] || visibilitySpec['selectionMethod']; - unlistenPromise = this.root - .getAmpElement( - context.parentElement || context, + + if (multiSelectorVisibilityOn && Array.isArray(selector)) { + unlistenPromise = Promise.all( + selector.map(s => getUnlistenPromiseForSelector(s, selectionMethod)) + ); + } else { + unlistenPromise = getUnlistenPromiseForSelector( selector, selectionMethod - ) - .then(element => { - return visibilityManagerPromise.then( - visibilityManager => { - return visibilityManager.listenElement( - element, - visibilitySpec, - this.getReadyPromise(waitForSpec, selector, element), - createReportReadyPromiseFunc, - this.onEvent_.bind(this, eventType, listener, element) - ); - }, - () => {} - ); - }); + ); + } } return function() { unlistenPromise.then(unlisten => { - unlisten(); + if (multiSelectorVisibilityOn && Array.isArray(unlisten)) { + for (let i = 0; i < unlisten.length; i++) { + unlisten[i](); + } + } else { + unlisten(); + } }); }; } diff --git a/extensions/amp-analytics/0.1/test/test-amp-analytics.js b/extensions/amp-analytics/0.1/test/test-amp-analytics.js index 03589c519ed5..92513dd5e1a7 100644 --- a/extensions/amp-analytics/0.1/test/test-amp-analytics.js +++ b/extensions/amp-analytics/0.1/test/test-amp-analytics.js @@ -34,6 +34,7 @@ import { import {installCryptoService} from '../../../../src/service/crypto-impl'; import {installUserNotificationManagerForTesting} from '../../../amp-user-notification/0.1/amp-user-notification'; import {instrumentationServiceForDocForTesting} from '../instrumentation'; +import {toggleExperiment} from '../../../../src/experiments'; describes.realWin( 'amp-analytics', @@ -817,6 +818,74 @@ describes.realWin( }); }); + describe('expand multi-selector visibility', () => { + beforeEach(() => { + toggleExperiment(win, 'multi-selector-visibility-trigger', true); + }); + + afterEach(() => { + toggleExperiment(win, 'multi-selector-visibility-trigger', false); + }); + + it('expands selector with config variable', () => { + const tracker = ins.root_.getTracker('visible', VisibilityTracker); + const addStub = env.sandbox.stub(tracker, 'add'); + const analytics = getAnalyticsTag({ + requests: {foo: 'https://example.test/bar'}, + triggers: [ + {on: 'visible', selector: ['${foo}', '${title}'], request: 'foo'}, + ], + vars: {foo: 'bar'}, + }); + return waitForNoSendRequest(analytics).then(() => { + expect(addStub).to.be.calledOnce; + const config = addStub.args[0][2]; + expect(config['selector']).to.deep.equal(['bar', 'My Test Title']); + }); + }); + + function selectorExpansionTest(selector) { + it('expand selector value: ' + selector, () => { + const tracker = ins.root_.getTracker('visible', VisibilityTracker); + const addStub = env.sandbox.stub(tracker, 'add'); + const analytics = getAnalyticsTag({ + requests: {foo: 'https://example.test/bar'}, + triggers: [ + {on: 'visible', selector: ['${foo}, ${bar}'], request: 'foo'}, + ], + vars: {foo: selector, bar: '123'}, + }); + return waitForNoSendRequest(analytics).then(() => { + expect(addStub).to.be.calledOnce; + const config = addStub.args[0][2]; + expect(config['selector']).to.deep.equal([selector + ', 123']); + }); + }); + } + + [ + '.clazz', + 'a, div', + 'a .foo', + 'a #foo', + 'a > div', + 'div + p', + 'div ~ ul', + '[target=_blank]', + '[title~=flower]', + '[lang|=en]', + 'a[href^="https"]', + 'a[href$=".pdf"]', + 'a[href="w3schools"]', + 'a:active', + 'p::after', + 'p:first-child', + 'p:lang(it)', + ':not(p)', + 'p:nth-child(2)', + ].map(selectorExpansionTest); + }); + describe('optout by function', () => { beforeEach(() => { env.sandbox.stub(AnalyticsConfig.prototype, 'loadConfig').returns( diff --git a/extensions/amp-analytics/0.1/test/test-events.js b/extensions/amp-analytics/0.1/test/test-events.js index 108ad00e7850..779a605050bf 100644 --- a/extensions/amp-analytics/0.1/test/test-events.js +++ b/extensions/amp-analytics/0.1/test/test-events.js @@ -32,6 +32,7 @@ import {AmpdocAnalyticsRoot} from '../analytics-root'; import {Deferred} from '../../../../src/utils/promise'; import {Signals} from '../../../../src/utils/signals'; import {macroTask} from '../../../../testing/yield'; +import {toggleExperiment} from '../../../../src/experiments'; describes.realWin('Events', {amp: 1}, env => { let win; @@ -40,7 +41,9 @@ describes.realWin('Events', {amp: 1}, env => { let handler; let analyticsElement; let target; + let target2; let child; + let child2; beforeEach(() => { win = env.win; @@ -55,9 +58,17 @@ describes.realWin('Events', {amp: 1}, env => { target.classList.add('target'); win.document.body.appendChild(target); + target2 = win.document.createElement('div'); + target2.classList.add('target2'); + win.document.body.appendChild(target2); + child = win.document.createElement('div'); child.classList.add('child'); target.appendChild(child); + + child2 = win.document.createElement('div'); + child2.classList.add('child2'); + target2.appendChild(child2); }); describe('AnalyticsEventType', () => { @@ -1864,6 +1875,113 @@ describes.realWin('Events', {amp: 1}, env => { }); }); + describe('multi selector visibility trigger', () => { + let targetSignals2; + let saveCallback2; + let eventsSpy; + + beforeEach(() => { + toggleExperiment(win, 'multi-selector-visibility-trigger', true); + + eventsSpy = env.sandbox.spy(tracker, 'onEvent_'); + + target2.classList.add('i-amphtml-element'); + targetSignals2 = new Signals(); + target2.signals = () => targetSignals2; + + saveCallback2 = env.sandbox.match(arg => { + if (typeof arg == 'function') { + saveCallback2.callback = arg; + return true; + } + return false; + }); + }); + + afterEach(() => { + toggleExperiment(win, 'multi-selector-visibility-trigger', false); + }); + + it('should fire event per selector', async () => { + const config = {visibilitySpec: {selector: ['.target', '.target2']}}; + const unlisten = env.sandbox.spy(); + const unlisten2 = env.sandbox.spy(); + iniLoadTrackerMock.expects('getRootSignal').twice(); + const readyPromise = Promise.resolve(); + // const readyPromise2 = Promise.resolve(); + iniLoadTrackerMock + .expects('getElementSignal') + .withExactArgs('ini-load', target) + .returns(readyPromise) + .once(); + iniLoadTrackerMock + .expects('getElementSignal') + .withExactArgs('ini-load', target2) + .returns(readyPromise) + .once(); + visibilityManagerMock + .expects('listenElement') + .withExactArgs( + target, + config.visibilitySpec, + readyPromise, + /* createReportReadyPromiseFunc */ null, + saveCallback + ) + .returns(unlisten) + .once(); + visibilityManagerMock + .expects('listenElement') + .withExactArgs( + target2, + config.visibilitySpec, + readyPromise, + /* createReportReadyPromiseFunc */ null, + saveCallback2 + ) + .returns(unlisten2) + .once(); + // Dispose function + const res = tracker.add( + analyticsElement, + 'visible', + config, + eventResolver + ); + expect(res).to.be.a('function'); + const unlistenReady = getAmpElementSpy.returnValues[0]; + const unlistenReady2 = getAmpElementSpy.returnValues[1]; + // #getAmpElement Promise + await unlistenReady; + await unlistenReady2; + // #assertMeasurable_ Promise + await macroTask(); + await macroTask(); + saveCallback.callback({totalVisibleTime: 10}); + saveCallback2.callback({totalVisibleTime: 15}); + + // Testing that visibilty manager mock sends state to onEvent_ + expect(eventsSpy.getCall(0).args[0]).to.equal('visible'); + expect(eventsSpy.getCall(0).args[1]).to.equal(eventResolver); + expect(eventsSpy.getCall(0).args[2]).to.equal(target); + expect(eventsSpy.getCall(0).args[3]).to.deep.equal({ + totalVisibleTime: 10, + }); + expect(eventsSpy.getCall(1).args[0]).to.equal('visible'); + expect(eventsSpy.getCall(1).args[1]).to.equal(eventResolver); + expect(eventsSpy.getCall(1).args[2]).to.equal(target2); + expect(eventsSpy.getCall(1).args[3]).to.deep.equal({ + totalVisibleTime: 15, + }); + + expect(unlisten).to.not.be.called; + expect(unlisten2).to.not.be.called; + await res(); + expect(unlisten).to.be.calledOnce; + expect(unlisten2).to.be.calledOnce; + }); + }); + it('should expand data params', function*() { target.setAttribute('data-vars-foo', 'bar'); diff --git a/test/manual/amp-analytics-visible-repeat.amp.html b/test/manual/amp-analytics-visible-repeat.amp.html index 39c936213aca..de18753dea22 100644 --- a/test/manual/amp-analytics-visible-repeat.amp.html +++ b/test/manual/amp-analytics-visible-repeat.amp.html @@ -56,6 +56,23 @@ } + + + diff --git a/tools/experiments/experiments-config.js b/tools/experiments/experiments-config.js index 6b44948956dc..7dce53b12da2 100644 --- a/tools/experiments/experiments-config.js +++ b/tools/experiments/experiments-config.js @@ -305,4 +305,10 @@ export const EXPERIMENTS = [ spec: 'https://github.com/ampproject/amphtml/issues/20595', cleanupIssue: 'https://github.com/ampproject/amphtml/issues/26709', }, + { + id: 'multi-selector-visibility-trigger', + name: 'AMP Analytics Multi-selector Visibility Trigger', + spec: 'https://github.com/ampproject/amphtml/issues/26823', + cleanupIssue: 'https://github.com/ampproject/amphtml/issues/26823', + }, ]; From 7ba97840b336ca07ea7edc0c7438638515b3259c Mon Sep 17 00:00:00 2001 From: Micajuine Ho Date: Thu, 20 Feb 2020 09:38:17 -0800 Subject: [PATCH 02/11] Impl, unit tests, manual tests --- .../amp-analytics/0.1/analytics-root.js | 58 ++++++- extensions/amp-analytics/0.1/events.js | 69 +++++---- .../0.1/test/test-analytics-root.js | 75 ++++++++- .../amp-analytics/0.1/test/test-events.js | 146 +++++++++++++++--- .../amp-analytics-visible-repeat.amp.html | 21 ++- 5 files changed, 308 insertions(+), 61 deletions(-) diff --git a/extensions/amp-analytics/0.1/analytics-root.js b/extensions/amp-analytics/0.1/analytics-root.js index 1f13dbc7c6e7..d0e74efa0049 100644 --- a/extensions/amp-analytics/0.1/analytics-root.js +++ b/extensions/amp-analytics/0.1/analytics-root.js @@ -22,6 +22,7 @@ import { closestAncestorElementBySelector, matches, scopedQuerySelector, + scopedQuerySelectorAll, } from '../../../src/dom'; import {dev, user, userAssert} from '../../../src/log'; import {getMode} from '../../../src/mode'; @@ -275,6 +276,45 @@ export class AnalyticsRoot { }); } + /** + * @param {string} selector DOM query selector. + * @return {!Promise>} Element corresponding to the selector. + */ + getElements_(selector) { + // Wait for document-ready to avoid false missed searches + return this.ampdoc.whenReady().then(() => { + const results = []; + const foundElements = scopedQuerySelectorAll( + dev().assertElement(this.ampdoc.getBody()), + selector + ); + + // Length is not supported in all browsers + if (foundElements && foundElements.length) { + for (let i = 0; i < foundElements.length; i++) { + results.push(foundElements[i]); + } + } + userAssert(results.length, `Element "${selector}" not found`); + return results; + }); + } + + /** + * Searches for the AMP elements that matches from the root. + * + * @param {string} selector DOM query selector. + * @return {!Promise>} Array of AMP elements corresponding to the selector if found. + */ + getAmpElements(selector) { + return this.getElements_(selector).then(elements => { + for (let i = 0; i < elements.length; i++) { + this.verifyAmpElement_(elements[i], selector); + } + return elements; + }); + } + /** * Searches the AMP element that matches the selector within the scope of the * analytics root in relationship to the specified context node. @@ -287,15 +327,23 @@ export class AnalyticsRoot { */ getAmpElement(context, selector, selectionMethod) { return this.getElement(context, selector, selectionMethod).then(element => { - userAssert( - element.classList.contains('i-amphtml-element'), - 'Element "%s" is required to be an AMP element', - selector - ); + this.verifyAmpElement_(element, selector); return element; }); } + /** + * @param {!Element} element + * @param {string} selector + */ + verifyAmpElement_(element, selector) { + userAssert( + element.classList.contains('i-amphtml-element'), + 'Element "%s" is required to be an AMP element', + selector + ); + } + /** * Creates listener-filter for DOM events to check against the specified * selector. If the node (or its ancestors) match the selector the listener diff --git a/extensions/amp-analytics/0.1/events.js b/extensions/amp-analytics/0.1/events.js index 0b3c97844437..dddab604c6a7 100644 --- a/extensions/amp-analytics/0.1/events.js +++ b/extensions/amp-analytics/0.1/events.js @@ -1472,28 +1472,19 @@ export class VisibilityTracker extends EventTracker { ); } - const getUnlistenPromiseForSelector = (selector, selectionMethod) => { - return this.root - .getAmpElement( - context.parentElement || context, - selector, - selectionMethod - ) - .then(element => { - return visibilityManagerPromise.then( - visibilityManager => { - return visibilityManager.listenElement( - element, - visibilitySpec, - this.getReadyPromise(waitForSpec, selector, element), - createReportReadyPromiseFunc, - this.onEvent_.bind(this, eventType, listener, element) - ); - }, - () => {} + const getUnlistenPromiseForElement = element => + visibilityManagerPromise.then( + visibilityManager => { + return visibilityManager.listenElement( + element, + visibilitySpec, + this.getReadyPromise(waitForSpec, selector, element), + createReportReadyPromiseFunc, + this.onEvent_.bind(this, eventType, listener, element) ); - }); - }; + }, + () => {} + ); let unlistenPromise; // Root selectors are delegated to analytics roots. @@ -1523,14 +1514,38 @@ export class VisibilityTracker extends EventTracker { config['selectionMethod'] || visibilitySpec['selectionMethod']; if (multiSelectorVisibilityOn && Array.isArray(selector)) { - unlistenPromise = Promise.all( - selector.map(s => getUnlistenPromiseForSelector(s, selectionMethod)) + userAssert( + !selectionMethod, + 'Cannot have selectionMethod defined with an array selector: ', + selector ); + unlistenPromise = Promise.all( + selector.map(individualSelector => { + return this.root.getAmpElements(individualSelector); + }) + ).then(selectorArrayOfElements => { + const uniqueElements = []; + for (let i = 0; i < selectorArrayOfElements.length; i++) { + for (let j = 0; j < selectorArrayOfElements[i].length; j++) { + if ( + uniqueElements.indexOf(selectorArrayOfElements[i][j]) === -1 + ) { + uniqueElements.push(selectorArrayOfElements[i][j]); + } + } + } + return Promise.all( + uniqueElements.map(element => getUnlistenPromiseForElement(element)) + ); + }); } else { - unlistenPromise = getUnlistenPromiseForSelector( - selector, - selectionMethod - ); + unlistenPromise = this.root + .getAmpElement( + context.parentElement || context, + selector, + selectionMethod + ) + .then(element => getUnlistenPromiseForElement(element)); } } diff --git a/extensions/amp-analytics/0.1/test/test-analytics-root.js b/extensions/amp-analytics/0.1/test/test-analytics-root.js index 69b8a40192e7..768f598ca598 100644 --- a/extensions/amp-analytics/0.1/test/test-analytics-root.js +++ b/extensions/amp-analytics/0.1/test/test-analytics-root.js @@ -333,25 +333,84 @@ describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, env => { addTestInstance(root3.getElement(body, '#target'), null); }); - it('should find an AMP element for AMP search', () => { + it('should find an AMP element for AMP search', async () => { child.classList.add('i-amphtml-element'); - return root.getAmpElement(body, '#child').then(element => { - expect(element).to.equal(child); - }); + const element = await root.getAmpElement(body, '#child'); + expect(element).to.equal(child); }); - it('should allow not-found element for AMP search', () => { - return root.getAmpElement(body, '#unknown').catch(error => { + it('should allow not-found element for AMP search', async () => { + await root.getAmpElement(body, '#unknown').catch(error => { expect(error).to.match(/Element "#unknown" not found/); }); }); - it('should fail if the found element is not AMP for AMP search', () => { + it('should fail if the found element is not AMP for AMP search', async () => { child.classList.remove('i-amphtml-element'); - return root.getAmpElement(body, '#child').catch(error => { + await root.getAmpElement(body, '#child').catch(error => { expect(error).to.match(/required to be an AMP element/); }); }); + + describe('get amp elements', () => { + let child2; + let elements; + let error; + + beforeEach(() => { + error = false; + child2 = win.document.createElement('child'); + body.appendChild(child2); + child.classList.add('i-amphtml-element'); + child2.classList.add('i-amphtml-element'); + }); + + afterEach(() => { + if (!error) { + expect(elements).to.contain(child); + expect(elements).to.contain(child2); + expect(elements.length).to.equal(2); + } + }); + + it('should find elements by ID', async () => { + child.id = 'myId'; + child2.id = 'myId'; + elements = await root.getAmpElements('#myId'); + }); + + it('should find element by class', async () => { + child.classList.add('myClass'); + child2.classList.add('myClass'); + elements = await root.getAmpElements('.myClass'); + }); + + it('should find element by tag name', async () => { + elements = await root.getAmpElements('child'); + }); + + it('should find element by selector', async () => { + child.id = 'myId'; + child2.id = 'myId'; + child.classList.add('myClass'); + child2.classList.add('myClass'); + elements = await root.getAmpElements('#myId.myClass'); + }); + + it('should allow not-found element for AMP search', async () => { + error = true; + await root.getAmpElement(body, '#unknown').catch(error => { + expect(error).to.match(/Element "#unknown" not found/); + }); + }); + + it('should fail if the found element is not AMP for AMP search', async () => { + error = true; + await root.getAmpElements('#child').catch(error => { + expect(error).to.match(/required to be an AMP element/); + }); + }); + }); }); describe('createSelectiveListener', () => { diff --git a/extensions/amp-analytics/0.1/test/test-events.js b/extensions/amp-analytics/0.1/test/test-events.js index 779a605050bf..182bbddd2b7c 100644 --- a/extensions/amp-analytics/0.1/test/test-events.js +++ b/extensions/amp-analytics/0.1/test/test-events.js @@ -1876,12 +1876,21 @@ describes.realWin('Events', {amp: 1}, env => { }); describe('multi selector visibility trigger', () => { + let unlisten; + let unlisten2; + let config; + let readyPromise; let targetSignals2; let saveCallback2; let eventsSpy; + let res; beforeEach(() => { toggleExperiment(win, 'multi-selector-visibility-trigger', true); + readyPromise = Promise.resolve(); + unlisten = env.sandbox.spy(); + unlisten2 = env.sandbox.spy(); + config = {}; eventsSpy = env.sandbox.spy(tracker, 'onEvent_'); @@ -1898,17 +1907,25 @@ describes.realWin('Events', {amp: 1}, env => { }); }); - afterEach(() => { + afterEach(async () => { + [unlisten, unlisten2].forEach(value => { + if (value) { + expect(value).to.not.be.called; + } + }); + expect(res).to.be.a('function'); + await res(); + [unlisten, unlisten2].forEach(value => { + if (value) { + expect(value).to.be.calledOnce; + } + }); + toggleExperiment(win, 'multi-selector-visibility-trigger', false); }); it('should fire event per selector', async () => { - const config = {visibilitySpec: {selector: ['.target', '.target2']}}; - const unlisten = env.sandbox.spy(); - const unlisten2 = env.sandbox.spy(); - iniLoadTrackerMock.expects('getRootSignal').twice(); - const readyPromise = Promise.resolve(); - // const readyPromise2 = Promise.resolve(); + config['visibilitySpec'] = {selector: ['.target', '.target2']}; iniLoadTrackerMock .expects('getElementSignal') .withExactArgs('ini-load', target) @@ -1942,13 +1959,7 @@ describes.realWin('Events', {amp: 1}, env => { .returns(unlisten2) .once(); // Dispose function - const res = tracker.add( - analyticsElement, - 'visible', - config, - eventResolver - ); - expect(res).to.be.a('function'); + res = tracker.add(analyticsElement, 'visible', config, eventResolver); const unlistenReady = getAmpElementSpy.returnValues[0]; const unlistenReady2 = getAmpElementSpy.returnValues[1]; // #getAmpElement Promise @@ -1973,12 +1984,109 @@ describes.realWin('Events', {amp: 1}, env => { expect(eventsSpy.getCall(1).args[3]).to.deep.equal({ totalVisibleTime: 15, }); + }); - expect(unlisten).to.not.be.called; - expect(unlisten2).to.not.be.called; - await res(); - expect(unlisten).to.be.calledOnce; - expect(unlisten2).to.be.calledOnce; + it('should fire event for all elements of the selector', async () => { + target.classList.add('sameClass'); + target2.classList.add('sameClass'); + config['visibilitySpec'] = {selector: ['.sameClass']}; + iniLoadTrackerMock + .expects('getElementSignal') + .withExactArgs('ini-load', target) + .returns(readyPromise) + .once(); + iniLoadTrackerMock + .expects('getElementSignal') + .withExactArgs('ini-load', target2) + .returns(readyPromise) + .once(); + visibilityManagerMock + .expects('listenElement') + .withExactArgs( + target, + config.visibilitySpec, + readyPromise, + /* createReportReadyPromiseFunc */ null, + saveCallback + ) + .returns(unlisten) + .once(); + visibilityManagerMock + .expects('listenElement') + .withExactArgs( + target2, + config.visibilitySpec, + readyPromise, + /* createReportReadyPromiseFunc */ null, + saveCallback2 + ) + .returns(unlisten2) + .once(); + // Dispose function + res = tracker.add(analyticsElement, 'visible', config, eventResolver); + const unlistenReady = getAmpElementSpy.returnValues[0]; + const unlistenReady2 = getAmpElementSpy.returnValues[1]; + // #getAmpElement Promise + await unlistenReady; + await unlistenReady2; + // #assertMeasurable_ Promise + await macroTask(); + await macroTask(); + saveCallback.callback({totalVisibleTime: 10}); + saveCallback2.callback({totalVisibleTime: 15}); + + // Testing that visibilty manager mock sends state to onEvent_ + expect(eventsSpy.getCall(0).args[0]).to.equal('visible'); + expect(eventsSpy.getCall(0).args[1]).to.equal(eventResolver); + expect(eventsSpy.getCall(0).args[2]).to.equal(target); + expect(eventsSpy.getCall(0).args[3]).to.deep.equal({ + totalVisibleTime: 10, + }); + expect(eventsSpy.getCall(1).args[0]).to.equal('visible'); + expect(eventsSpy.getCall(1).args[1]).to.equal(eventResolver); + expect(eventsSpy.getCall(1).args[2]).to.equal(target2); + expect(eventsSpy.getCall(1).args[3]).to.deep.equal({ + totalVisibleTime: 15, + }); + }); + + it('should only fire one event for each element', async () => { + target.classList.add('sameClass'); + target.classList.add('anotherClass'); + config['visibilitySpec'] = {selector: ['.sameClass', '.anotherClass']}; + iniLoadTrackerMock + .expects('getElementSignal') + .withExactArgs('ini-load', target) + .returns(readyPromise) + .once(); + visibilityManagerMock + .expects('listenElement') + .withExactArgs( + target, + config.visibilitySpec, + readyPromise, + /* createReportReadyPromiseFunc */ null, + saveCallback + ) + .returns(unlisten) + .once(); + res = tracker.add(analyticsElement, 'visible', config, eventResolver); + const unlistenReady = getAmpElementSpy.returnValues[0]; + // #getAmpElement Promise + await unlistenReady; + // #assertMeasurable_ Promise + await macroTask(); + saveCallback.callback({totalVisibleTime: 10}); + + // Testing that visibilty manager mock sends state to onEvent_ + expect(eventsSpy.getCall(0).args[0]).to.equal('visible'); + expect(eventsSpy.getCall(0).args[1]).to.equal(eventResolver); + expect(eventsSpy.getCall(0).args[2]).to.equal(target); + expect(eventsSpy.getCall(0).args[3]).to.deep.equal({ + totalVisibleTime: 10, + }); + expect(eventsSpy).to.be.calledOnce; + unlisten2 = null; }); }); diff --git a/test/manual/amp-analytics-visible-repeat.amp.html b/test/manual/amp-analytics-visible-repeat.amp.html index de18753dea22..11dd356b5b97 100644 --- a/test/manual/amp-analytics-visible-repeat.amp.html +++ b/test/manual/amp-analytics-visible-repeat.amp.html @@ -61,13 +61,19 @@ { "transport": {"beacon": false, "xhrpost": false}, "requests": { - "base": "fake.com/ad?multiVisible=true&adId=${id}" + "base": "fake.com/ad?multiVisible=true&adId=${id}", + "queryAll": "fake.com/ad?querySelectorAll=true&adId=${id}" }, "triggers": { "multiVisible": { "on": "visible", "request": "base", "selector": ["#ad1","#ad2"] + }, + "querySelectorAll": { + "on": "visible", + "request": "queryAll", + "selector": [".myClass"] } } } @@ -154,7 +160,18 @@ data-valid='true' data-vars-id='ad2' data-ad-width=300 - data-ad-height=250> + data-ad-height=250 + class="myClass"> + + + From 926e4bf9bffd0209e1e1c3b1a31f1c8de32a3799 Mon Sep 17 00:00:00 2001 From: Micajuine Ho Date: Fri, 20 Mar 2020 16:23:41 -0700 Subject: [PATCH 03/11] Update amp-analytics-visible-repeat.amp.html --- test/manual/amp-analytics-visible-repeat.amp.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/manual/amp-analytics-visible-repeat.amp.html b/test/manual/amp-analytics-visible-repeat.amp.html index b7523c3e0f5f..39c936213aca 100644 --- a/test/manual/amp-analytics-visible-repeat.amp.html +++ b/test/manual/amp-analytics-visible-repeat.amp.html @@ -141,4 +141,4 @@ - \ No newline at end of file + From 62ab379a1bb9601f233f6c1c6e76255ac01f584e Mon Sep 17 00:00:00 2001 From: Micajuine Ho Date: Fri, 20 Mar 2020 16:26:08 -0700 Subject: [PATCH 04/11] Update amp-analytics-visible-repeat.amp.html --- test/manual/amp-analytics-visible-repeat.amp.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/manual/amp-analytics-visible-repeat.amp.html b/test/manual/amp-analytics-visible-repeat.amp.html index b7523c3e0f5f..39c936213aca 100644 --- a/test/manual/amp-analytics-visible-repeat.amp.html +++ b/test/manual/amp-analytics-visible-repeat.amp.html @@ -141,4 +141,4 @@ - \ No newline at end of file + From beb8f58269fef32feffdebc28dfebf5a2ad874ed Mon Sep 17 00:00:00 2001 From: Micajuine Ho Date: Mon, 23 Mar 2020 15:32:22 -0700 Subject: [PATCH 05/11] keeping element list reference --- .../amp-analytics/0.1/analytics-root.js | 70 ++++++++++++------- extensions/amp-analytics/0.1/events.js | 20 +++--- 2 files changed, 55 insertions(+), 35 deletions(-) diff --git a/extensions/amp-analytics/0.1/analytics-root.js b/extensions/amp-analytics/0.1/analytics-root.js index 89988f27a380..cfaa9de43e7d 100644 --- a/extensions/amp-analytics/0.1/analytics-root.js +++ b/extensions/amp-analytics/0.1/analytics-root.js @@ -281,17 +281,15 @@ export class AnalyticsRoot { } /** + * @param {!Element} context * @param {string} selector DOM query selector. * @return {!Promise>} Element corresponding to the selector. */ - getElementsByScopedQuerySelectorAll_(selector) { + getElementsByScopedQuerySelectorAll_(context, selector) { // Wait for document-ready to avoid false missed searches return this.ampdoc.whenReady().then(() => { const results = []; - const foundElements = scopedQuerySelectorAll( - dev().assertElement(this.ampdoc.getBody()), - selector - ); + const foundElements = this.getRoot().querySelectorAll(selector); // Length is not supported in all browsers if (foundElements && foundElements.length) { @@ -304,6 +302,27 @@ export class AnalyticsRoot { }); } + /** + * Searches the AMP element that matches the selector within the scope of the + * analytics root in relationship to the specified context node. + * + * @param {!Element} context + * @param {string} selector DOM query selector. + * @param {?string=} selectionMethod Allowed values are `null`, + * `'closest'` and `'scope'`. + * @return {!Promise} AMP element corresponding to the selector if found. + */ + getAmpElement(context, selector, selectionMethod) { + return this.getElement(context, selector, selectionMethod).then(element => { + userAssert( + element.classList.contains('i-amphtml-element'), + 'Element "%s" is required to be an AMP element', + selector + ); + return element; + }); + } + /** * Searches the AMP element that matches the selector within the scope of the * analytics root in relationship to the specified context node. @@ -313,45 +332,42 @@ export class AnalyticsRoot { * @param {?string=} selectionMethod Allowed values are `null`, * `'closest'` and `'scope'`. * @param {boolean=} opt_multiSelectorOn multi-selector expriment + * @param {Array} opt_elementsReference reference to unique elements * @return {!Promise>} Array of AMP elements corresponding to the selector if found. */ getAmpElementOrElements( context, selector, selectionMethod, - opt_multiSelectorOn + opt_multiSelectorOn, + opt_elementsReference ) { - let elementsPromise = this.getElement(context, selector, selectionMethod); - // Return unique elements based upon selector - if (opt_multiSelectorOn) { + if (opt_multiSelectorOn && opt_elementsReference) { userAssert( !selectionMethod, 'Cannot have selectionMethod defined with an array selector: %s', selector ); - elementsPromise = this.getElementsByScopedQuerySelectorAll_( - selector - ).then(elements => { - const uniqueElements = []; - for (let i = 0; i < elements.length; i++) { - for (let j = 0; j < elements[i].length; j++) { - if (uniqueElements.indexOf(elements[i][j]) === -1) { - uniqueElements.push(elements[i][j]); + return this.getElementsByScopedQuerySelectorAll_(selector).then( + elements => { + const uniqueElements = []; + for (let i = 0; i < elements.length; i++) { + if (opt_elementsReference.indexOf(elements[i]) === -1) { + uniqueElements.push(elements[i]); + opt_elementsReference.push(elements[i]); } } + this.verifyAmpElements_(uniqueElements, selector); + return uniqueElements; } - return uniqueElements; - }); + ); } - - return elementsPromise.then(elementOrElements => { - const elements = Array.isArray(elementOrElements) - ? elementOrElements - : [elementOrElements]; - this.verifyAmpElements_(elements, selector); - return elements; - }); + return this.getAmpElement( + context, + selector, + selectionMethod + ).then(element => [element]); } /** diff --git a/extensions/amp-analytics/0.1/events.js b/extensions/amp-analytics/0.1/events.js index 293501613486..87b5e0af9b57 100644 --- a/extensions/amp-analytics/0.1/events.js +++ b/extensions/amp-analytics/0.1/events.js @@ -746,13 +746,13 @@ export class SignalTracker extends EventTracker { // false missed searches. const selectionMethod = config['selectionMethod']; signalsPromise = this.root - .getAmpElementOrElements( + .getAmpElement( context.parentElement || context, selector, selectionMethod ) - .then(elements => { - target = elements[0]; + .then(element => { + target = element; return this.getElementSignal(eventType, target); }); } @@ -807,13 +807,13 @@ export class IniLoadTracker extends EventTracker { // false missed searches. const selectionMethod = config['selectionMethod']; promise = this.root - .getAmpElementOrElements( + .getAmpElement( context.parentElement || context, selector, selectionMethod ) - .then(elements => { - target = elements[0]; + .then(element => { + target = element; return this.getElementSignal('ini-load', target); }); } @@ -1506,14 +1506,18 @@ export class VisibilityTracker extends EventTracker { // Array selectors do not suppor the special cases: ':host' & ':root' const selectionMethod = config['selectionMethod'] || visibilitySpec['selectionMethod']; - const selectors = Array.isArray(selector) ? selector : [selector]; + const selectors = Array.isArray(selector) + ? selector.filter((val, index) => selectors.indexOf(val) === index) + : [selector]; + const elementsReference = []; for (let i = 0; i < selectors.length; i++) { this.root .getAmpElementOrElements( context.parentElement || context, selectors[i], selectionMethod, - multiSelectorVisibilityOn + multiSelectorVisibilityOn, + elementsReference ) .then(elements => { for (let j = 0; j < elements.length; j++) { From e9efdfe71404bd487391a5ae76b0d63665689aa8 Mon Sep 17 00:00:00 2001 From: Micajuine Ho Date: Tue, 24 Mar 2020 15:45:15 -0700 Subject: [PATCH 06/11] Make reduce getReadyPromise --- .../amp-analytics/0.1/analytics-root.js | 37 ++++---- extensions/amp-analytics/0.1/events.js | 88 ++++++++++--------- 2 files changed, 63 insertions(+), 62 deletions(-) diff --git a/extensions/amp-analytics/0.1/analytics-root.js b/extensions/amp-analytics/0.1/analytics-root.js index fae1b5eb27f6..21ebd89bf28c 100644 --- a/extensions/amp-analytics/0.1/analytics-root.js +++ b/extensions/amp-analytics/0.1/analytics-root.js @@ -326,41 +326,40 @@ export class AnalyticsRoot { * the specified context node. * * @param {!Element} context - * @param {string} selector DOM query selector. + * @param {!Array} selectors Array of DOM query selector. * @param {?string=} selectionMethod Allowed values are `null`, * `'closest'` and `'scope'`. * @param {boolean=} opt_multiSelectorOn multi-selector expriment - * @param {Array=} opt_elementsReference reference to unique elements * @return {!Promise>} Array of AMP elements corresponding to the selector if found. */ - getAmpElementOrElements( - context, - selector, - selectionMethod, - opt_multiSelectorOn, - opt_elementsReference - ) { - if (opt_multiSelectorOn && opt_elementsReference) { + getAmpElements(context, selectors, selectionMethod, opt_multiSelectorOn) { + if (opt_multiSelectorOn) { userAssert( !selectionMethod, - 'Cannot have selectionMethod defined with an array selector: %s', - selector + 'Cannot have selectionMethod defined with an array selector.' ); - return this.getElementsByQuerySelectorAll_(selector).then(elements => { + const elementsListsPromise = []; + for (let i = 0; i < selectors.length; i++) { + elementsListsPromise.push( + this.getElementsByQuerySelectorAll_(selectors[i]) + ); + } + return Promise.all(elementsListsPromise).then(elementsLists => { const uniqueElements = []; - for (let i = 0; i < elements.length; i++) { - if (opt_elementsReference.indexOf(elements[i]) === -1) { - uniqueElements.push(elements[i]); - opt_elementsReference.push(elements[i]); + for (let i = 0; i < elementsLists.length; i++) { + this.verifyAmpElements_(elementsLists[i], selectors[i]); + for (let j = 0; j < elementsLists[i].length; j++) { + if (uniqueElements.indexOf(elementsLists[i][j]) === -1) { + uniqueElements.push(elementsLists[i][j]); + } } } - this.verifyAmpElements_(uniqueElements, selector); return uniqueElements; }); } return this.getAmpElement( context, - selector, + selectors[0], selectionMethod ).then(element => [element]); } diff --git a/extensions/amp-analytics/0.1/events.js b/extensions/amp-analytics/0.1/events.js index 87b5e0af9b57..0156f4cc0860 100644 --- a/extensions/amp-analytics/0.1/events.js +++ b/extensions/amp-analytics/0.1/events.js @@ -1487,7 +1487,9 @@ export class VisibilityTracker extends EventTracker { visibilityManager => { return visibilityManager.listenRoot( visibilitySpec, - this.getReadyPromise(waitForSpec, selector), + this.getReadyPromise( + this.getWaitForSpecForRootSelector_(waitForSpec, selector) + ), createReportReadyPromiseFunc, this.onEvent_.bind( this, @@ -1509,39 +1511,34 @@ export class VisibilityTracker extends EventTracker { const selectors = Array.isArray(selector) ? selector.filter((val, index) => selectors.indexOf(val) === index) : [selector]; - const elementsReference = []; - for (let i = 0; i < selectors.length; i++) { - this.root - .getAmpElementOrElements( - context.parentElement || context, - selectors[i], - selectionMethod, - multiSelectorVisibilityOn, - elementsReference - ) - .then(elements => { - for (let j = 0; j < elements.length; j++) { - unlistenPromises.push( - visibilityManagerPromise.then( - visibilityManager => { - return visibilityManager.listenElement( - elements[j], - visibilitySpec, - this.getReadyPromise( - waitForSpec, - selectors[i], - elements[j] - ), - createReportReadyPromiseFunc, - this.onEvent_.bind(this, eventType, listener, elements[j]) - ); - }, - () => {} - ) - ); - } - }); - } + this.root + .getAmpElements( + context.parentElement || context, + selectors, + selectionMethod, + multiSelectorVisibilityOn + ) + .then(elements => { + for (let i = 0; i < elements.length; i++) { + unlistenPromises.push( + visibilityManagerPromise.then( + visibilityManager => { + return visibilityManager.listenElement( + elements[i], + visibilitySpec, + this.getReadyPromise( + waitForSpec || 'ini-load', + elements[i] + ), + createReportReadyPromiseFunc, + this.onEvent_.bind(this, eventType, listener, elements[i]) + ); + }, + () => {} + ) + ); + } + }); } return function() { @@ -1651,22 +1648,27 @@ export class VisibilityTracker extends EventTracker { } /** + * Selector being null is special case just like :host and :root. + * If it's null, then don't wait for anything, otherwise + * wait for the AMP element's load. * @param {string|undefined} waitForSpec * @param {string|undefined} selector + * @return {string|undefined} + */ + getWaitForSpecForRootSelector_(waitForSpec, selector) { + return waitForSpec || (selector ? 'ini-load' : null); + } + + /** + * @param {string|undefined} waitForSpec * @param {Element=} opt_element * @return {?Promise} * @visibleForTesting */ - getReadyPromise(waitForSpec, selector, opt_element) { + getReadyPromise(waitForSpec, opt_element) { if (!waitForSpec) { - // Default case: - if (!selector) { - // waitFor selector is not defined, wait for nothing - return null; - } else { - // otherwise wait for ini-load by default - waitForSpec = 'ini-load'; - } + // Default case, waitFor selector is not defined, wait for nothing + return null; } const trackerWhitelist = getTrackerTypesForParentType('visible'); From b4228b8ec1e822137249410684c3ce75674dc0ac Mon Sep 17 00:00:00 2001 From: Micajuine Ho Date: Thu, 26 Mar 2020 10:48:06 -0700 Subject: [PATCH 07/11] Optimizing and testing --- extensions/amp-analytics/0.1/amp-analytics.js | 5 ++- extensions/amp-analytics/0.1/events.js | 34 ++++++++++--------- .../amp-analytics-multi-selector.html | 2 +- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/extensions/amp-analytics/0.1/amp-analytics.js b/extensions/amp-analytics/0.1/amp-analytics.js index 38b26eb4a744..86e8c99c0e5d 100644 --- a/extensions/amp-analytics/0.1/amp-analytics.js +++ b/extensions/amp-analytics/0.1/amp-analytics.js @@ -358,7 +358,10 @@ export class AmpAnalytics extends AMP.BaseElement { trigger['selector'] = this.element.parentElement.tagName; trigger['selectionMethod'] = 'closest'; this.addTrigger_(trigger); - } else if (trigger['selector']) { + } else if ( + trigger['selector'] && + !Array.isArray(trigger['selector']) + ) { // Expand the selector using variable expansion. return this.variableService_ .expandTemplate( diff --git a/extensions/amp-analytics/0.1/events.js b/extensions/amp-analytics/0.1/events.js index 0156f4cc0860..6acc6cb712a6 100644 --- a/extensions/amp-analytics/0.1/events.js +++ b/extensions/amp-analytics/0.1/events.js @@ -1488,7 +1488,7 @@ export class VisibilityTracker extends EventTracker { return visibilityManager.listenRoot( visibilitySpec, this.getReadyPromise( - this.getWaitForSpecForRootSelector_(waitForSpec, selector) + waitForSpec || (selector ? 'ini-load' : null) ), createReportReadyPromiseFunc, this.onEvent_.bind( @@ -1508,9 +1508,8 @@ export class VisibilityTracker extends EventTracker { // Array selectors do not suppor the special cases: ':host' & ':root' const selectionMethod = config['selectionMethod'] || visibilitySpec['selectionMethod']; - const selectors = Array.isArray(selector) - ? selector.filter((val, index) => selectors.indexOf(val) === index) - : [selector]; + const selectors = Array.isArray(selector) ? selector : [selector]; + this.assertUniqueSelectors_(selectors); this.root .getAmpElements( context.parentElement || context, @@ -1550,6 +1549,21 @@ export class VisibilityTracker extends EventTracker { }; } + /** + * Assert that the selectors are all unique + * @param {!Array} selectors + */ + assertUniqueSelectors_(selectors) { + const filtered = selectors.filter( + (val, index) => selectors.indexOf(val) === index + ); + userAssert( + selectors.length === filtered.length, + 'Cannot have duplicate selectors in selectors list: %s', + selectors + ); + } + /** * Assert that the setting is measurable with host API * @param {string=} selector @@ -1647,18 +1661,6 @@ export class VisibilityTracker extends EventTracker { return 'onpagehide' in this.root.ampdoc.win; } - /** - * Selector being null is special case just like :host and :root. - * If it's null, then don't wait for anything, otherwise - * wait for the AMP element's load. - * @param {string|undefined} waitForSpec - * @param {string|undefined} selector - * @return {string|undefined} - */ - getWaitForSpecForRootSelector_(waitForSpec, selector) { - return waitForSpec || (selector ? 'ini-load' : null); - } - /** * @param {string|undefined} waitForSpec * @param {Element=} opt_element diff --git a/test/manual/amp-analytics/amp-analytics-multi-selector.html b/test/manual/amp-analytics/amp-analytics-multi-selector.html index ec58719dc4dc..dce0de3b94b3 100644 --- a/test/manual/amp-analytics/amp-analytics-multi-selector.html +++ b/test/manual/amp-analytics/amp-analytics-multi-selector.html @@ -46,7 +46,7 @@ "multiVisible": { "on": "visible", "request": "base", - "selector": ["#ad1","#ad2"] + "selector": ["#ad1","#ad2","#ad1"] }, "querySelectorAll": { "on": "visible", From 97c92a9b678cc5c13687e5aa4ded086e79cd7f05 Mon Sep 17 00:00:00 2001 From: Micajuine Ho Date: Mon, 30 Mar 2020 14:33:36 -0700 Subject: [PATCH 08/11] Fixing tests & suggested changes --- extensions/amp-analytics/0.1/amp-analytics.js | 5 +- .../amp-analytics/0.1/analytics-root.js | 53 ++++---- .../0.1/test/test-analytics-root.js | 122 +++++++++--------- .../amp-analytics/0.1/test/test-events.js | 49 ++----- 4 files changed, 95 insertions(+), 134 deletions(-) diff --git a/extensions/amp-analytics/0.1/amp-analytics.js b/extensions/amp-analytics/0.1/amp-analytics.js index 86e8c99c0e5d..38b26eb4a744 100644 --- a/extensions/amp-analytics/0.1/amp-analytics.js +++ b/extensions/amp-analytics/0.1/amp-analytics.js @@ -358,10 +358,7 @@ export class AmpAnalytics extends AMP.BaseElement { trigger['selector'] = this.element.parentElement.tagName; trigger['selectionMethod'] = 'closest'; this.addTrigger_(trigger); - } else if ( - trigger['selector'] && - !Array.isArray(trigger['selector']) - ) { + } else if (trigger['selector']) { // Expand the selector using variable expansion. return this.variableService_ .expandTemplate( diff --git a/extensions/amp-analytics/0.1/analytics-root.js b/extensions/amp-analytics/0.1/analytics-root.js index 21ebd89bf28c..8bb1292af45d 100644 --- a/extensions/amp-analytics/0.1/analytics-root.js +++ b/extensions/amp-analytics/0.1/analytics-root.js @@ -280,25 +280,31 @@ export class AnalyticsRoot { } /** - * @param {string} selector DOM query selector. + * @param {!Array} selectors Array of DOM query selectors. * @return {!Promise>} Element corresponding to the selector. */ - getElementsByQuerySelectorAll_(selector) { + getElementsByQuerySelectorAll_(selectors) { // Wait for document-ready to avoid false missed searches return this.ampdoc.whenReady().then(() => { - let foundElements; - try { - foundElements = this.getRoot().querySelectorAll(selector); - } catch (e) { - userAssert(false, `Invalid query selector ${selector}`); - } - const elements = []; - for (let i = 0; i < foundElements.length; i++) { - if (this.contains(foundElements[i])) { - elements.push(foundElements[i]); + let elements = []; + for (let i = 0; i < selectors.length; i++) { + let nodeList; + const elementArray = []; + const selector = selectors[i]; + try { + nodeList = this.getRoot().querySelectorAll(selector); + } catch { + userAssert(false, `Invalid query selector ${selector}`); + } + for (let j = 0; j < nodeList.length; j++) { + if (this.contains(nodeList[j])) { + elementArray.push(nodeList[j]); + } } + userAssert(elementArray.length, `Element "${selector}" not found`); + this.verifyAmpElements_(elementArray, selector); + elements = elements.concat(elementArray); } - userAssert(elements.length, `Element "${selector}" not found`); return elements; }); } @@ -338,24 +344,9 @@ export class AnalyticsRoot { !selectionMethod, 'Cannot have selectionMethod defined with an array selector.' ); - const elementsListsPromise = []; - for (let i = 0; i < selectors.length; i++) { - elementsListsPromise.push( - this.getElementsByQuerySelectorAll_(selectors[i]) - ); - } - return Promise.all(elementsListsPromise).then(elementsLists => { - const uniqueElements = []; - for (let i = 0; i < elementsLists.length; i++) { - this.verifyAmpElements_(elementsLists[i], selectors[i]); - for (let j = 0; j < elementsLists[i].length; j++) { - if (uniqueElements.indexOf(elementsLists[i][j]) === -1) { - uniqueElements.push(elementsLists[i][j]); - } - } - } - return uniqueElements; - }); + return this.getElementsByQuerySelectorAll_(selectors).then(elements => + elements.filter((element, index) => elements.indexOf(element) === index) + ); } return this.getAmpElement( context, diff --git a/extensions/amp-analytics/0.1/test/test-analytics-root.js b/extensions/amp-analytics/0.1/test/test-analytics-root.js index 9aa7d1cbed41..21933ce37502 100644 --- a/extensions/amp-analytics/0.1/test/test-analytics-root.js +++ b/extensions/amp-analytics/0.1/test/test-analytics-root.js @@ -335,19 +335,19 @@ describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, env => { it('should find an AMP element for AMP search', async () => { child.classList.add('i-amphtml-element'); - const elements = await root.getAmpElementOrElements(body, '#child'); - expect(elements[0]).to.equal(child); + const element = await root.getAmpElement(body, '#child'); + expect(element).to.equal(child); }); it('should allow not-found element for AMP search', async () => { - await root.getAmpElementOrElements(body, '#unknown').catch(error => { + await root.getAmpElement(body, '#unknown').catch(error => { expect(error).to.match(/Element "#unknown" not found/); }); }); it('should fail if the found element is not AMP for AMP search', async () => { child.classList.remove('i-amphtml-element'); - await root.getAmpElementOrElements(body, '#child').catch(error => { + await root.getAmpElement(body, '#child').catch(error => { expect(error).to.match(/required to be an AMP element/); }); }); @@ -355,84 +355,43 @@ describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, env => { describe('get amp elements', () => { let child2; let elements; - let error; beforeEach(() => { - error = false; child2 = win.document.createElement('child'); body.appendChild(child2); child.classList.add('i-amphtml-element'); child2.classList.add('i-amphtml-element'); }); - afterEach(() => { - if (!error) { - expect(elements).to.contain(child); - expect(elements).to.contain(child2); - expect(elements.length).to.equal(2); - } - }); - - it('should find elements by ID', async () => { + it('should find all elements by selector', async () => { child.id = 'myId'; child2.id = 'myId'; - elements = await root.getAmpElementOrElements( - body, - '#myId', - null, - true - ); - }); - - it('should find element by class', async () => { child.classList.add('myClass'); child2.classList.add('myClass'); - elements = await root.getAmpElementOrElements( + elements = await root.getAmpElements( body, - '.myClass', - null, - true - ); - }); - - it('should find element by tag name', async () => { - elements = await root.getAmpElementOrElements( - body, - 'child', - null, - true - ); - }); - - it('should find element by selector', async () => { - child.id = 'myId'; - child2.id = 'myId'; - child.classList.add('myClass'); - child2.classList.add('myClass'); - elements = await root.getAmpElementOrElements( - body, - '#myId.myClass', + ['#myId.myClass'], null, true ); + expect(elements).to.contain(child); + expect(elements).to.contain(child2); + expect(elements.length).to.equal(2); }); it('should allow not-found element for AMP search', async () => { - error = true; - await root - .getAmpElementOrElements(body, '#unknown', null, true) - .catch(error => { - expect(error).to.match(/Element "#unknown" not found/); - }); + expectAsyncConsoleError(/Element "#unknown" not found/, 1); + await expect( + root.getAmpElements(body, ['#unknown'], null, true) + ).to.be.rejectedWith(/Element "#unknown" not found​​​/); }); it('should fail if the found element is not AMP for AMP search', async () => { - error = true; - await root - .getAmpElementOrElements(body, '#child', null, true) - .catch(error => { - expect(error).to.match(/required to be an AMP element/); - }); + expectAsyncConsoleError(/required to be an AMP element/, 1); + child.classList.remove('i-amphtml-element'); + await expect( + root.getAmpElements(body, ['#child'], null, true) + ).to.be.rejectedWith(/required to be an AMP element/); }); }); }); @@ -724,6 +683,49 @@ describes.realWin( }); }); + describe('get amp elements', () => { + let child2; + let elements; + + beforeEach(() => { + child2 = win.document.createElement('child'); + body.appendChild(child2); + child.classList.add('i-amphtml-element'); + child2.classList.add('i-amphtml-element'); + }); + + it('should find all elements by selector', async () => { + child.id = 'myId'; + child2.id = 'myId'; + child.classList.add('myClass'); + child2.classList.add('myClass'); + elements = await root.getAmpElements( + body, + ['#myId.myClass'], + null, + true + ); + expect(elements).to.contain(child); + expect(elements).to.contain(child2); + expect(elements.length).to.equal(2); + }); + + it('should allow not-found element for AMP search', async () => { + expectAsyncConsoleError(/Element "#unknown" not found/, 1); + await expect( + root.getAmpElements(body, ['#unknown'], null, true) + ).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); + child.classList.remove('i-amphtml-element'); + await expect( + root.getAmpElements(body, ['#child'], null, true) + ).to.be.rejectedWith(/required to be an AMP element/); + }); + }); + describe('createSelectiveListener', () => { function matches(context, target, selector, selectionMethod) { const listener = env.sandbox.spy(); diff --git a/extensions/amp-analytics/0.1/test/test-events.js b/extensions/amp-analytics/0.1/test/test-events.js index 97781b657646..a1ed3a229664 100644 --- a/extensions/amp-analytics/0.1/test/test-events.js +++ b/extensions/amp-analytics/0.1/test/test-events.js @@ -2237,17 +2237,15 @@ describes.realWin('Events', {amp: 1}, env => { }); describe('should wait on correct readyPromise', () => { - const selector = '.target'; - it('with waitFor default value', () => { // Default case: selector is not specified - expect(tracker.getReadyPromise(undefined, undefined)).to.be.null; + expect(tracker.getReadyPromise(undefined)).to.be.null; // Default case: waitFor is not specified, no AMP element selected iniLoadTrackerMock .expects('getRootSignal') .returns(Promise.resolve()) .once(); - const waitForTracker1 = tracker.getReadyPromise(undefined, ':root'); + const waitForTracker1 = tracker.getReadyPromise('ini-load'); return waitForTracker1.then(() => { iniLoadTrackerMock .expects('getElementSignal') @@ -2255,17 +2253,14 @@ describes.realWin('Events', {amp: 1}, env => { .returns(Promise.resolve()) .once(); // Default case: waitFor is not specified, AMP element selected - const promise2 = tracker.getReadyPromise(undefined, selector, target); + const promise2 = tracker.getReadyPromise('ini-load', target); target.signals().signal('ini-load'); return promise2; }); }); it('with waitFor NONE', () => { - expect(tracker.getReadyPromise('none', undefined, undefined)).to.be - .null; - expect(tracker.getReadyPromise('none', ':root', undefined)).to.be.null; - expect(tracker.getReadyPromise('none', selector, target)).to.be.null; + expect(tracker.getReadyPromise('none')).to.be.null; }); it('with waitFor INI_LOAD', () => { @@ -2273,28 +2268,16 @@ describes.realWin('Events', {amp: 1}, env => { .expects('getRootSignal') .returns(Promise.resolve()) .twice(); - const promise = tracker.getReadyPromise( - 'ini-load', - undefined, - undefined - ); + const promise = tracker.getReadyPromise('ini-load'); return promise.then(() => { - const promise1 = tracker.getReadyPromise( - 'ini-load', - ':root', - undefined - ); + const promise1 = tracker.getReadyPromise('ini-load'); return promise1.then(() => { iniLoadTrackerMock .expects('getElementSignal') .withExactArgs('ini-load', target) .returns(Promise.resolve()) .once(); - const promise2 = tracker.getReadyPromise( - 'ini-load', - selector, - target - ); + const promise2 = tracker.getReadyPromise('ini-load', target); return promise2; }); }); @@ -2313,28 +2296,16 @@ describes.realWin('Events', {amp: 1}, env => { .withExactArgs('render-start') .returns(Promise.resolve()) .twice(); - const promise = tracker.getReadyPromise( - 'render-start', - undefined, - undefined - ); + const promise = tracker.getReadyPromise('render-start'); return promise.then(() => { - const promise1 = tracker.getReadyPromise( - 'render-start', - ':root', - undefined - ); + const promise1 = tracker.getReadyPromise('render-start'); return promise1.then(() => { signalTrackerMock .expects('getElementSignal') .withExactArgs('render-start', target) .returns(Promise.resolve()) .once(); - const promise2 = tracker.getReadyPromise( - 'render-start', - selector, - target - ); + const promise2 = tracker.getReadyPromise('render-start', target); return promise2; }); }); From 2108ac72f41c3c65fd0ecfe6d0e4b4b044e8f5ec Mon Sep 17 00:00:00 2001 From: Micajuine Ho Date: Thu, 2 Apr 2020 17:26:12 -0700 Subject: [PATCH 09/11] Suggested changes --- .../amp-analytics/0.1/analytics-root.js | 16 +- extensions/amp-analytics/0.1/events.js | 41 ++- .../0.1/test/test-analytics-root.js | 101 +++++--- .../amp-analytics/0.1/test/test-events.js | 105 -------- .../amp-analytics-multi-selector.html | 239 +++++++++++++++++- 5 files changed, 334 insertions(+), 168 deletions(-) diff --git a/extensions/amp-analytics/0.1/analytics-root.js b/extensions/amp-analytics/0.1/analytics-root.js index e8dadd7970cd..ac5131207cb4 100644 --- a/extensions/amp-analytics/0.1/analytics-root.js +++ b/extensions/amp-analytics/0.1/analytics-root.js @@ -305,7 +305,10 @@ export class AnalyticsRoot { this.verifyAmpElements_(elementArray, selector); elements = elements.concat(elementArray); } - return elements; + // Return unique + return elements.filter( + (element, index) => elements.indexOf(element) === index + ); }); } @@ -334,7 +337,7 @@ export class AnalyticsRoot { * the specified context node. * * @param {!Element} context - * @param {!Array} selectors Array of DOM query selector. + * @param {!Array|string} selectors DOM query selector(s). * @param {?string=} selectionMethod Allowed values are `null`, * `'closest'` and `'scope'`. * @param {boolean=} opt_multiSelectorOn multi-selector expriment @@ -344,15 +347,14 @@ export class AnalyticsRoot { if (opt_multiSelectorOn) { userAssert( !selectionMethod, - 'Cannot have selectionMethod defined with an array selector.' - ); - return this.getElementsByQuerySelectorAll_(selectors).then((elements) => - elements.filter((element, index) => elements.indexOf(element) === index) + 'Cannot have selectionMethod %s defined with an array selector.', + selectionMethod ); + return this.getElementsByQuerySelectorAll_(selectors); } return this.getAmpElement( context, - selectors[0], + selectors, selectionMethod ).then((element) => [element]); } diff --git a/extensions/amp-analytics/0.1/events.js b/extensions/amp-analytics/0.1/events.js index d015ca5204cb..15107bd51e45 100644 --- a/extensions/amp-analytics/0.1/events.js +++ b/extensions/amp-analytics/0.1/events.js @@ -26,7 +26,7 @@ import {dev, devAssert, user, userAssert} from '../../../src/log'; import {dict, hasOwn} from '../../../src/utils/object'; import {getData} from '../../../src/event-helper'; import {getDataParamsFromAttributes} from '../../../src/dom'; -import {isEnumValue, isFiniteNumber} from '../../../src/types'; +import {isArray, isEnumValue, isFiniteNumber} from '../../../src/types'; import {isExperimentOn} from '../../../src/experiments'; import {startsWith} from '../../../src/string'; @@ -1429,12 +1429,13 @@ 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; const unlistenPromises = []; const multiSelectorVisibilityOn = isExperimentOn(this.root.ampdoc.win, 'visibility-trigger-improvements') && - Array.isArray(selector); + isArray(selector); if (reportWhenSpec) { userAssert( !visibilitySpec['repeat'], @@ -1482,14 +1483,13 @@ 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); unlistenPromises.push( visibilityManagerPromise.then( (visibilityManager) => { return visibilityManager.listenRoot( visibilitySpec, - this.getReadyPromise( - waitForSpec || (selector ? 'ini-load' : null) - ), + this.getReadyPromise(readyPromiseWaitForSpec), createReportReadyPromiseFunc, this.onEvent_.bind( this, @@ -1508,12 +1508,12 @@ export class VisibilityTracker extends EventTracker { // Array selectors do not suppor the special cases: ':host' & ':root' const selectionMethod = config['selectionMethod'] || visibilitySpec['selectionMethod']; - const selectors = Array.isArray(selector) ? selector : [selector]; - this.assertUniqueSelectors_(selectors); + readyPromiseWaitForSpec = waitForSpec || 'ini-load'; + this.assertUniqueSelectors_(selector); this.root .getAmpElements( context.parentElement || context, - selectors, + selector, selectionMethod, multiSelectorVisibilityOn ) @@ -1525,10 +1525,7 @@ export class VisibilityTracker extends EventTracker { return visibilityManager.listenElement( elements[i], visibilitySpec, - this.getReadyPromise( - waitForSpec || 'ini-load', - elements[i] - ), + this.getReadyPromise(readyPromiseWaitForSpec, elements[i]), createReportReadyPromiseFunc, this.onEvent_.bind(this, eventType, listener, elements[i]) ); @@ -1551,17 +1548,19 @@ export class VisibilityTracker extends EventTracker { /** * Assert that the selectors are all unique - * @param {!Array} selectors + * @param {!Array|string} selectors */ assertUniqueSelectors_(selectors) { - const filtered = selectors.filter( - (val, index) => selectors.indexOf(val) === index - ); - userAssert( - selectors.length === filtered.length, - 'Cannot have duplicate selectors in selectors list: %s', - selectors - ); + if (isArray(selectors)) { + const filtered = selectors.filter( + (val, index) => selectors.indexOf(val) === index + ); + userAssert( + selectors.length === filtered.length, + 'Cannot have duplicate selectors in selectors list: %s', + selectors + ); + } } /** diff --git a/extensions/amp-analytics/0.1/test/test-analytics-root.js b/extensions/amp-analytics/0.1/test/test-analytics-root.js index fa5a7ab8e54e..1afe1cd6b41a 100644 --- a/extensions/amp-analytics/0.1/test/test-analytics-root.js +++ b/extensions/amp-analytics/0.1/test/test-analytics-root.js @@ -339,7 +339,7 @@ describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => { expect(element).to.equal(child); }); - it('should allow not-found element for AMP search', async () => { + it('should handle missing selector for AMP search', async () => { await root.getAmpElement(body, '#unknown').catch((error) => { expect(error).to.match(/Element "#unknown" not found/); }); @@ -354,38 +354,67 @@ describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => { describe('get amp elements', () => { let child2; - let elements; + let child3; beforeEach(() => { child2 = win.document.createElement('child'); + child3 = win.document.createElement('child'); body.appendChild(child2); + body.appendChild(child3); child.classList.add('i-amphtml-element'); child2.classList.add('i-amphtml-element'); + child3.classList.add('i-amphtml-element'); }); - it('should find all elements by selector', async () => { - child.id = 'myId'; - child2.id = 'myId'; + it('should find element and elements by selector', async () => { child.classList.add('myClass'); child2.classList.add('myClass'); - elements = await root.getAmpElements( - body, - ['#myId.myClass'], - null, - true - ); - expect(elements).to.contain(child); - expect(elements).to.contain(child2); - expect(elements.length).to.equal(2); + child3.classList.add('notMyClass'); + expect( + await root.getAmpElements(body, ['.myClass'], null, true) + ).to.deep.equal([child, child2]); + expect( + await root.getAmpElements(body, '.notMyClass', null, false) + ).to.deep.equal([child3]); + }); + + it('should remove duplicate elements found', async () => { + child.id = 'myId'; + child.classList.add('myClass'); + expect( + await root.getAmpElements(body, ['.myClass', '#myId'], null, true) + ).to.deep.equal([child]); }); - it('should allow not-found element for AMP search', async () => { + it('should remove duplicate selectors', async () => { + child.classList.add('myClass'); + expect( + await root.getAmpElements(body, ['.myClass', '.myClass'], null, true) + ).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, true) + ).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, true) ).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, true) + ).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); child.classList.remove('i-amphtml-element'); @@ -393,6 +422,16 @@ describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => { root.getAmpElements(body, ['#child'], null, true) ).to.be.rejectedWith(/required to be an AMP element/); }); + + it('should fail if selection method is found', async () => { + try { + await root.getAmpElements(body, ['#child'], 'scope', true); + } catch (e) { + expect(e).to.match( + /Cannot have selectionMethod scope defined with an array selector/ + ); + } + }); }); }); @@ -684,33 +723,29 @@ describes.realWin( }); describe('get amp elements', () => { - let child2; - let elements; - beforeEach(() => { - child2 = win.document.createElement('child'); - body.appendChild(child2); child.classList.add('i-amphtml-element'); - child2.classList.add('i-amphtml-element'); }); it('should find all elements by selector', async () => { - child.id = 'myId'; - child2.id = 'myId'; + const child2 = win.document.createElement('child'); + const child3 = win.document.createElement('child'); + body.appendChild(child2); + body.appendChild(child3); child.classList.add('myClass'); child2.classList.add('myClass'); - elements = await root.getAmpElements( - body, - ['#myId.myClass'], - null, - true - ); - expect(elements).to.contain(child); - expect(elements).to.contain(child2); - expect(elements.length).to.equal(2); + child3.classList.add('notMyClass'); + child2.classList.add('i-amphtml-element'); + child3.classList.add('i-amphtml-element'); + expect( + await root.getAmpElements(body, ['.myClass'], null, true) + ).to.deep.equals([child, child2]); + expect( + await root.getAmpElements(body, '.notMyClass', null, false) + ).to.deep.equals([child3]); }); - it('should allow not-found element for AMP search', async () => { + it('should handle missing selector for AMP search', async () => { expectAsyncConsoleError(/Element "#unknown" not found/, 1); await expect( root.getAmpElements(body, ['#unknown'], null, true) diff --git a/extensions/amp-analytics/0.1/test/test-events.js b/extensions/amp-analytics/0.1/test/test-events.js index ab8484108bad..34e743838bb3 100644 --- a/extensions/amp-analytics/0.1/test/test-events.js +++ b/extensions/amp-analytics/0.1/test/test-events.js @@ -2052,111 +2052,6 @@ describes.realWin('Events', {amp: 1}, (env) => { totalVisibleTime: 15, }); }); - - it('should fire event for all elements of the selector', async () => { - target.classList.add('sameClass'); - target2.classList.add('sameClass'); - config['visibilitySpec'] = {selector: ['.sameClass']}; - iniLoadTrackerMock - .expects('getElementSignal') - .withExactArgs('ini-load', target) - .returns(readyPromise) - .once(); - iniLoadTrackerMock - .expects('getElementSignal') - .withExactArgs('ini-load', target2) - .returns(readyPromise) - .once(); - visibilityManagerMock - .expects('listenElement') - .withExactArgs( - target, - config.visibilitySpec, - readyPromise, - /* createReportReadyPromiseFunc */ null, - saveCallback - ) - .returns(unlisten) - .once(); - visibilityManagerMock - .expects('listenElement') - .withExactArgs( - target2, - config.visibilitySpec, - readyPromise, - /* createReportReadyPromiseFunc */ null, - saveCallback2 - ) - .returns(unlisten2) - .once(); - // Dispose function - res = tracker.add(analyticsElement, 'visible', config, eventResolver); - const unlistenReady = getAmpElementSpy.returnValues[0]; - const unlistenReady2 = getAmpElementSpy.returnValues[1]; - // #getAmpElement Promise - await unlistenReady; - await unlistenReady2; - // #assertMeasurable_ Promise - await macroTask(); - await macroTask(); - saveCallback.callback({totalVisibleTime: 10}); - saveCallback2.callback({totalVisibleTime: 15}); - - // Testing that visibilty manager mock sends state to onEvent_ - expect(eventsSpy.getCall(0).args[0]).to.equal('visible'); - expect(eventsSpy.getCall(0).args[1]).to.equal(eventResolver); - expect(eventsSpy.getCall(0).args[2]).to.equal(target); - expect(eventsSpy.getCall(0).args[3]).to.deep.equal({ - totalVisibleTime: 10, - }); - expect(eventsSpy.getCall(1).args[0]).to.equal('visible'); - expect(eventsSpy.getCall(1).args[1]).to.equal(eventResolver); - expect(eventsSpy.getCall(1).args[2]).to.equal(target2); - expect(eventsSpy.getCall(1).args[3]).to.deep.equal({ - totalVisibleTime: 15, - }); - }); - - it('should only fire one event for each element', async () => { - target.classList.add('sameClass'); - target.classList.add('anotherClass'); - config['visibilitySpec'] = { - selector: ['.sameClass', '.anotherClass'], - }; - iniLoadTrackerMock - .expects('getElementSignal') - .withExactArgs('ini-load', target) - .returns(readyPromise) - .once(); - visibilityManagerMock - .expects('listenElement') - .withExactArgs( - target, - config.visibilitySpec, - readyPromise, - /* createReportReadyPromiseFunc */ null, - saveCallback - ) - .returns(unlisten) - .once(); - res = tracker.add(analyticsElement, 'visible', config, eventResolver); - const unlistenReady = getAmpElementSpy.returnValues[0]; - // #getAmpElement Promise - await unlistenReady; - // #assertMeasurable_ Promise - await macroTask(); - saveCallback.callback({totalVisibleTime: 10}); - - // Testing that visibilty manager mock sends state to onEvent_ - expect(eventsSpy.getCall(0).args[0]).to.equal('visible'); - expect(eventsSpy.getCall(0).args[1]).to.equal(eventResolver); - expect(eventsSpy.getCall(0).args[2]).to.equal(target); - expect(eventsSpy.getCall(0).args[3]).to.deep.equal({ - totalVisibleTime: 10, - }); - expect(eventsSpy).to.be.calledOnce; - unlisten2 = null; - }); }); }); diff --git a/test/manual/amp-analytics/amp-analytics-multi-selector.html b/test/manual/amp-analytics/amp-analytics-multi-selector.html index dce0de3b94b3..4b80835a2245 100644 --- a/test/manual/amp-analytics/amp-analytics-multi-selector.html +++ b/test/manual/amp-analytics/amp-analytics-multi-selector.html @@ -40,13 +40,14 @@ { "transport": {"beacon": false, "xhrpost": false}, "requests": { - "base": "fake.com/ad?multiVisible=true&adId=${id}" + "base": "fake.com/ad?multiVisible=true&adId=${id}", + "queryAll": "fake.com/ad?queryAll=true" }, "triggers": { "multiVisible": { "on": "visible", "request": "base", - "selector": ["#ad1","#ad2","#ad1"] + "selector": ["#ad1","#ad2"] }, "querySelectorAll": { "on": "visible", @@ -86,6 +87,240 @@ data-ad-height=250 class="myClass"> + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +