diff --git a/extensions/amp-analytics/0.1/analytics-root.js b/extensions/amp-analytics/0.1/analytics-root.js index 1f13dbc7c6e75..9cd9597609737 100644 --- a/extensions/amp-analytics/0.1/analytics-root.js +++ b/extensions/amp-analytics/0.1/analytics-root.js @@ -275,6 +275,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 = this.getRoot().querySelectorAll(selector); + + // DOM search can "look" outside the boundaries of the root, thus make + // sure the result is contained. Length is not supported in all browsers + if (foundElements && foundElements.length) { + for (let i = 0; i < foundElements.length; i++) { + if (this.contains(foundElements[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 +326,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 0b3c978444378..dddab604c6a7b 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 69b8a40192e70..768f598ca5989 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 779a605050bfd..182bbddd2b7c5 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 de18753dea228..11dd356b5b978 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"> + + +