diff --git a/extensions/amp-analytics/0.1/analytics-root.js b/extensions/amp-analytics/0.1/analytics-root.js index cd18da4071ea..58617b0b3833 100644 --- a/extensions/amp-analytics/0.1/analytics-root.js +++ b/extensions/amp-analytics/0.1/analytics-root.js @@ -25,6 +25,8 @@ import { } from '../../../src/dom'; import {dev, user, userAssert} from '../../../src/log'; import {getMode} from '../../../src/mode'; +import {isArray} from '../../../src/types'; +import {isExperimentOn} from '../../../src/experiments'; import {layoutRectLtwh} from '../../../src/layout-rect'; import {map} from '../../../src/utils/object'; import {provideVisibilityManager} from './visibility-manager'; @@ -279,6 +281,39 @@ export class AnalyticsRoot { }); } + /** + * @param {!Array} selectors Array of DOM query selectors. + * @return {!Promise>} Element corresponding to the selector. + */ + getElementsByQuerySelectorAll_(selectors) { + // Wait for document-ready to avoid false missed searches + return this.ampdoc.whenReady().then(() => { + 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 (e) { + 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); + } + // Return unique + return elements.filter( + (element, index) => elements.indexOf(element) === index + ); + }); + } + /** * Searches the AMP element that matches the selector within the scope of the * analytics root in relationship to the specified context node. @@ -287,22 +322,63 @@ export class AnalyticsRoot { * @param {string} selector DOM query selector. * @param {?string=} selectionMethod Allowed values are `null`, * `'closest'` and `'scope'`. - * @param {boolean=} opt_multiSelectorOn multi-selector expriment * @return {!Promise} AMP element corresponding to the selector if found. */ - getAmpElement(context, selector, selectionMethod, opt_multiSelectorOn) { + 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.verifyAmpElements_([element], selector); return element; } ); } + /** + * Searches for the AMP 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} selectors DOM query selector(s). + * @param {?string=} selectionMethod Allowed values are `null`, + * `'closest'` and `'scope'`. + * @return {!Promise>} Array of AMP elements corresponding to the selector if found. + */ + getAmpElements(context, selectors, selectionMethod) { + if ( + isExperimentOn(this.ampdoc.win, 'visibility-trigger-improvements') && + isArray(selectors) + ) { + userAssert( + !selectionMethod, + 'Cannot have selectionMethod %s defined with an array selector.', + selectionMethod + ); + return this.getElementsByQuerySelectorAll_( + /** @type {!Array} */ (selectors) + ); + } + return this.getAmpElement( + context, + /** @type {string} */ (selectors), + selectionMethod + ).then((element) => [element]); + } + + /** + * @param {!Array} elements + * @param {string} selector + */ + verifyAmpElements_(elements, selector) { + for (let i = 0; i < elements.length; i++) { + userAssert( + elements[i].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 9e53bf310eb1..48c7dacb6ffe 100644 --- a/extensions/amp-analytics/0.1/events.js +++ b/extensions/amp-analytics/0.1/events.js @@ -26,8 +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 {isExperimentOn} from '../../../src/experiments'; +import {isArray, isEnumValue, isFiniteNumber} from '../../../src/types'; import {startsWith} from '../../../src/string'; const SCROLL_PRECISION_PERCENT = 5; @@ -1429,12 +1428,10 @@ 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); if (reportWhenSpec) { userAssert( !visibilitySpec['repeat'], @@ -1482,12 +1479,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), + this.getReadyPromise(readyPromiseWaitForSpec), createReportReadyPromiseFunc, this.onEvent_.bind( this, @@ -1506,32 +1504,32 @@ 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]; - for (let i = 0; i < selectors.length; i++) { - unlistenPromises.push( - this.root - .getAmpElement( - context.parentElement || context, - selectors[i], - selectionMethod, - multiSelectorVisibilityOn - ) - .then((element) => + readyPromiseWaitForSpec = waitForSpec || 'ini-load'; + this.assertUniqueSelectors_(selector); + this.root + .getAmpElements( + context.parentElement || context, + selector, + selectionMethod + ) + .then((elements) => { + for (let i = 0; i < elements.length; i++) { + unlistenPromises.push( visibilityManagerPromise.then( (visibilityManager) => { return visibilityManager.listenElement( - element, + elements[i], visibilitySpec, - this.getReadyPromise(waitForSpec, selectors[i], element), + this.getReadyPromise(readyPromiseWaitForSpec, elements[i]), createReportReadyPromiseFunc, - this.onEvent_.bind(this, eventType, listener, element) + this.onEvent_.bind(this, eventType, listener, elements[i]) ); }, () => {} ) - ) - ); - } + ); + } + }); } return function () { @@ -1543,6 +1541,24 @@ export class VisibilityTracker extends EventTracker { }; } + /** + * Assert that the selectors are all unique + * @param {!Array|string} selectors + */ + assertUniqueSelectors_(selectors) { + if (isArray(selectors)) { + const map = {}; + for (let i = 0; i < selectors.length; i++) { + userAssert( + !map[selectors[i]], + 'Cannot have duplicate selectors in selectors list: %s', + selectors + ); + map[selectors[i]] = selectors[i]; + } + } + } + /** * Assert that the setting is measurable with host API * @param {string=} selector @@ -1642,21 +1658,14 @@ export class VisibilityTracker extends EventTracker { /** * @param {string|undefined} waitForSpec - * @param {string|undefined} selector * @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'); 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 a96a04c66352..9e328c68c038 100644 --- a/extensions/amp-analytics/0.1/test/test-analytics-root.js +++ b/extensions/amp-analytics/0.1/test/test-analytics-root.js @@ -25,6 +25,7 @@ import { VisibilityManagerForEmbed, } from '../visibility-manager'; import {VisibilityManagerForMApp} from '../visibility-manager-for-mapp'; +import {toggleExperiment} from '../../../../src/experiments'; describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => { let win; @@ -333,25 +334,106 @@ 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 handle missing selector 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 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'); + toggleExperiment(win, 'visibility-trigger-improvements', true); + }); + + afterEach(() => { + toggleExperiment(win, 'visibility-trigger-improvements', false); + }); + + it('should find element and elements by selector', async () => { + child.classList.add('myClass'); + child2.classList.add('myClass'); + child3.classList.add('notMyClass'); + expect( + await root.getAmpElements(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) + ).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) + ).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) + ).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) + ).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​​​/); + }); + + 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) + ).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'); + } catch (e) { + expect(e).to.match( + /Cannot have selectionMethod scope defined with an array selector/ + ); + } + }); + }); }); describe('createSelectiveListener', () => { @@ -641,6 +723,62 @@ describes.realWin( }); }); + describe('get amp elements', () => { + beforeEach(() => { + child.classList.add('i-amphtml-element'); + toggleExperiment( + parentRoot.ampdoc.win, + 'visibility-trigger-improvements', + true + ); + }); + + afterEach(() => { + child.classList.add('i-amphtml-element'); + toggleExperiment(win, 'visibility-trigger-improvements', false); + }); + + it('should find all elements by selector', async () => { + const child2 = win.document.createElement('child'); + const child3 = win.document.createElement('child'); + // Parent child attached to parent doc should not be captured + const parentChild = env.parentWin.document.createElement('child'); + body.appendChild(child2); + body.appendChild(child3); + env.parentWin.document.body.appendChild(parentChild); + child.classList.add('myClass'); + child2.classList.add('myClass'); + child3.classList.add('notMyClass'); + parentChild.classList.add('myClass'); + child2.classList.add('i-amphtml-element'); + child3.classList.add('i-amphtml-element'); + parentChild.classList.add('i-amphtml-element'); + expect( + await root.getAmpElements(body, ['.myClass'], null) + ).to.deep.equals([child, child2]); + // Check that non-experiment version works + toggleExperiment(win, 'visibility-trigger-improvements', false); + expect( + await root.getAmpElements(body, '.notMyClass', null) + ).to.deep.equals([child3]); + }); + + it('should handle missing selector for AMP search', async () => { + expectAsyncConsoleError(/Element "#unknown" not found/, 1); + await expect( + root.getAmpElements(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); + child.classList.remove('i-amphtml-element'); + await expect( + root.getAmpElements(body, ['#child'], null) + ).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 995d273f14f0..a95c8de2c6cf 100644 --- a/extensions/amp-analytics/0.1/test/test-events.js +++ b/extensions/amp-analytics/0.1/test/test-events.js @@ -1950,6 +1950,7 @@ describes.realWin('Events', {amp: 1}, (env) => { let saveCallback2; let eventsSpy; let res; + let error; beforeEach(() => { toggleExperiment(win, 'visibility-trigger-improvements', true); @@ -1974,24 +1975,28 @@ describes.realWin('Events', {amp: 1}, (env) => { }); 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; - } - }); + if (!error) { + [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, 'visibility-trigger-improvements', false); }); it('should fire event per selector', async () => { - config['visibilitySpec'] = {selector: ['.target', '.target2']}; + config['visibilitySpec'] = { + selector: ['.target', '.target2'], + }; iniLoadTrackerMock.expects('getRootSignal').twice(); iniLoadTrackerMock .expects('getElementSignal') @@ -2052,6 +2057,18 @@ describes.realWin('Events', {amp: 1}, (env) => { totalVisibleTime: 15, }); }); + + it('should error on duplicate selectors', async () => { + error = true; + config['visibilitySpec'] = { + selector: ['.target', '.target'], + }; + expect(() => { + tracker.add(analyticsElement, 'visible', config, eventResolver); + }).to.throw( + /Cannot have duplicate selectors in selectors list: .target/ + ); + }); }); }); @@ -2126,69 +2143,27 @@ describes.realWin('Events', {amp: 1}, (env) => { }); describe('should wait on correct readyPromise', () => { - const selector = '.target'; + it('with waitFor NONE', () => { + expect(tracker.getReadyPromise('none')).to.be.null; + }); - it('with waitFor default value', () => { - // Default case: selector is not specified - expect(tracker.getReadyPromise(undefined, undefined)).to.be.null; - // Default case: waitFor is not specified, no AMP element selected + it('with waitFor INI_LOAD', () => { iniLoadTrackerMock .expects('getRootSignal') .returns(Promise.resolve()) - .once(); - const waitForTracker1 = tracker.getReadyPromise(undefined, ':root'); - return waitForTracker1.then(() => { + .twice(); + const promise = tracker.getReadyPromise('ini-load'); + return promise.then(() => { iniLoadTrackerMock .expects('getElementSignal') .withExactArgs('ini-load', target) .returns(Promise.resolve()) .once(); - // Default case: waitFor is not specified, AMP element selected - const promise2 = tracker.getReadyPromise(undefined, selector, target); - target.signals().signal('ini-load'); + const promise2 = tracker.getReadyPromise('ini-load', target); 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; - }); - - it('with waitFor INI_LOAD', () => { - iniLoadTrackerMock - .expects('getRootSignal') - .returns(Promise.resolve()) - .twice(); - const promise = tracker.getReadyPromise( - 'ini-load', - undefined, - undefined - ); - return promise.then(() => { - const promise1 = tracker.getReadyPromise( - 'ini-load', - ':root', - undefined - ); - return promise1.then(() => { - iniLoadTrackerMock - .expects('getElementSignal') - .withExactArgs('ini-load', target) - .returns(Promise.resolve()) - .once(); - const promise2 = tracker.getReadyPromise( - 'ini-load', - selector, - target - ); - return promise2; - }); - }); - }); - it('with waitFor RENDER_START', () => { tracker.waitForTrackers_['render-start'] = root.getTracker( 'render-start', @@ -2202,30 +2177,15 @@ 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 - ); - return promise1.then(() => { - signalTrackerMock - .expects('getElementSignal') - .withExactArgs('render-start', target) - .returns(Promise.resolve()) - .once(); - const promise2 = tracker.getReadyPromise( - 'render-start', - selector, - target - ); - return promise2; - }); + signalTrackerMock + .expects('getElementSignal') + .withExactArgs('render-start', target) + .returns(Promise.resolve()) + .once(); + const promise2 = tracker.getReadyPromise('render-start', target); + return promise2; }); }); }); diff --git a/test/manual/amp-analytics/amp-analytics-multi-selector.html b/test/manual/amp-analytics/amp-analytics-multi-selector.html index 59ef5f96073c..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,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?queryAll=true" }, "triggers": { "multiVisible": { "on": "visible", "request": "base", "selector": ["#ad1","#ad2"] + }, + "querySelectorAll": { + "on": "visible", + "request": "queryAll", + "selector": [".myClass"] } } } @@ -72,6 +78,249 @@ data-ad-height=250> + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +