Skip to content

Commit

Permalink
✨ Require visibility trigger selector's to have data-vars-* (#26902)
Browse files Browse the repository at this point in the history 
* Restrict data-vars, impl, unit, manual test

* Adding warning

* Suggested changes
  • Loading branch information
Micajuine Ho authored May 1, 2020
1 parent 19f1b72 commit 5aaea99
Show file tree
Hide file tree
Showing 4 changed files with 147 additions and 34 deletions.
40 changes: 39 additions & 1 deletion extensions/amp-analytics/0.1/analytics-root.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {Services} from '../../../src/services';
import {VisibilityManagerForMApp} from './visibility-manager-for-mapp';
import {
closestAncestorElementBySelector,
getDataParamsFromAttributes,
matches,
scopedQuerySelector,
} from '../../../src/dom';
Expand All @@ -34,6 +35,7 @@ import {tryResolve} from '../../../src/utils/promise';
import {whenContentIniLoad} from '../../../src/ini-load';

const TAG = 'amp-analytics/analytics-root';
const VARIABLE_DATA_ATTRIBUTE_KEY = /^vars(.+)/;

/**
* An analytics root. Analytics can be scoped to either ampdoc, embed or
Expand Down Expand Up @@ -291,7 +293,7 @@ export class AnalyticsRoot {
let elements = [];
for (let i = 0; i < selectors.length; i++) {
let nodeList;
const elementArray = [];
let elementArray = [];
const selector = selectors[i];
try {
nodeList = this.getRoot().querySelectorAll(selector);
Expand All @@ -303,6 +305,7 @@ export class AnalyticsRoot {
elementArray.push(nodeList[j]);
}
}
elementArray = this.getDataVarsElements_(elementArray, selector);
userAssert(elementArray.length, `Element "${selector}" not found`);
this.verifyAmpElements_(elementArray, selector);
elements = elements.concat(elementArray);
Expand All @@ -314,6 +317,41 @@ export class AnalyticsRoot {
});
}

/**
* Return all elements that have a data-vars attribute.
* @param {!Array<!Element>} elementArray
* @param {string} selector
* @return {!Array<!Element>}
*/
getDataVarsElements_(elementArray, selector) {
let removedCount = 0;
const dataVarsArray = [];
for (let i = 0; i < elementArray.length; i++) {
const dataVarKeys = Object.keys(
getDataParamsFromAttributes(
elementArray[i],
/* computeParamNameFunc */ undefined,
VARIABLE_DATA_ATTRIBUTE_KEY
)
);
if (dataVarKeys.length) {
dataVarsArray.push(elementArray[i]);
} else {
removedCount++;
}
}
if (removedCount) {
user().warn(
TAG,
'%s element(s) ommited from selector "%s"' +
' because no data-vars-* attribute was found.',
removedCount,
selector
);
}
return dataVarsArray;
}

/**
* Searches the AMP element that matches the selector within the scope of the
* analytics root in relationship to the specified context node.
Expand Down
103 changes: 86 additions & 17 deletions extensions/amp-analytics/0.1/test/test-analytics-root.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
} from '../visibility-manager';
import {VisibilityManagerForMApp} from '../visibility-manager-for-mapp';
import {toggleExperiment} from '../../../../src/experiments';
import {user} from '../../../../src/log';

describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => {
let win;
Expand Down Expand Up @@ -365,6 +366,10 @@ describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => {
child.classList.add('i-amphtml-element');
child2.classList.add('i-amphtml-element');
child3.classList.add('i-amphtml-element');

child.setAttribute('data-vars-id', 'child1');
child2.setAttribute('data-vars-id', 'child2');
child3.setAttribute('data-vars-id', 'child3');
toggleExperiment(win, 'visibility-trigger-improvements', true);
});

Expand All @@ -386,6 +391,25 @@ describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => {
).to.deep.equal([child3]);
});

it('should only find elements with data-vars-*', async () => {
const spy = env.sandbox.spy(user(), 'warn');

child.classList.add('myClass');
child2.classList.add('myClass');
child3.classList.add('myClass');

child3.removeAttribute('data-vars-id');
const children = await root.getAmpElements(body, ['.myClass']);
expect(spy).callCount(1);
expect(spy).to.have.been.calledWith(
'amp-analytics/analytics-root',
'%s element(s) ommited from selector "%s" because no data-vars-* attribute was found.',
1,
'.myClass'
);
expect(children).to.deep.equal([child, child2]);
});

it('should remove duplicate elements found', async () => {
child.id = 'myId';
child.classList.add('myClass');
Expand Down Expand Up @@ -724,8 +748,27 @@ describes.realWin(
});

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('myClass');
child2.classList.add('myClass');
child3.classList.add('notMyClass');

child.classList.add('i-amphtml-element');
child2.classList.add('i-amphtml-element');
child3.classList.add('i-amphtml-element');

child.setAttribute('data-vars-id', '123');
child2.setAttribute('data-vars-id', '456');
child3.setAttribute('data-vars-id', '789');

toggleExperiment(
parentRoot.ampdoc.win,
'visibility-trigger-improvements',
Expand All @@ -739,30 +782,55 @@ describes.realWin(
});

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]);
const elements = await root.getAmpElements(body, ['.myClass'], null);

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

it('should not find elements from parent doc', async () => {
const parentChild = env.parentWin.document.createElement('child');
env.parentWin.document.body.appendChild(parentChild);
parentChild.classList.add('myClass');
parentChild.classList.add('i-amphtml-element');
parentChild.setAttribute('data-vars-id', 'abc');

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

it('should only find elements with data-vars-*', async () => {
const spy = env.sandbox.spy(user(), 'warn');

child3.classList.add('myClass');
child3.removeAttribute('data-vars-id');

const children = await root.getAmpElements(body, ['.myClass']);
expect(spy).callCount(1);
expect(spy).to.have.been.calledWith(
'amp-analytics/analytics-root',
'%s element(s) ommited from selector "%s" because no data-vars-* attribute was found.',
1,
'.myClass'
);
expect(children).to.deep.equal([child, child2]);
});

it('should remove duplicate elements found', async () => {
child.id = 'myId';
child2.id = 'myId';
child.classList.add('myClass');
child2.classList.add('myClass');
// Each selector should find both elements, but only report once
expect(
await root.getAmpElements(body, ['.myClass', '#myId'], null)
).to.deep.equal([child, child2]);
});

it('should handle missing selector for AMP search', async () => {
expectAsyncConsoleError(/Element "#unknown" not found/, 1);
await expect(
Expand All @@ -773,6 +841,7 @@ describes.realWin(
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');
child.setAttribute('data-vars-id', '123');
await expect(
root.getAmpElements(body, ['#child'], null)
).to.be.rejectedWith(/required to be an AMP element/);
Expand Down
5 changes: 5 additions & 0 deletions extensions/amp-analytics/0.1/test/test-events.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1965,6 +1965,9 @@ describes.realWin('Events', {amp: 1}, (env) => {
targetSignals2 = new Signals();
target2.signals = () => targetSignals2;

target.setAttribute('data-vars-id', '123');
target2.setAttribute('data-vars-id', '123');

saveCallback2 = env.sandbox.match((arg) => {
if (typeof arg == 'function') {
saveCallback2.callback = arg;
Expand Down Expand Up @@ -2049,12 +2052,14 @@ describes.realWin('Events', {amp: 1}, (env) => {
expect(eventsSpy.getCall(0).args[2]).to.equal(target);
expect(eventsSpy.getCall(0).args[3]).to.deep.equal({
totalVisibleTime: 10,
id: '123',
});
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,
id: '123',
});
});

Expand Down
33 changes: 17 additions & 16 deletions test/manual/amp-analytics/amp-analytics-multi-selector.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@
"transport": {"beacon": false, "xhrpost": false},
"requests": {
"base": "fake.com/ad?multiVisible=true&adId=${id}",
"queryAll": "fake.com/ad?queryAll=true"
"queryAll": "fake.com/ad?queryAll=true",
"restrictDataVars": "fake.com/ad?restrictDataVars=true&adId=${id}"
},
"triggers": {
"multiVisible": {
Expand All @@ -53,6 +54,11 @@
"on": "visible",
"request": "queryAll",
"selector": [".myClass"]
},
"restrictDataVars": {
"on": "visible",
"request": "restrictDataVars",
"selector": [".restrictDataVars"]
}
}
}
Expand Down Expand Up @@ -238,7 +244,7 @@
data-vars-id='ad20'
data-ad-width=300
data-ad-height=250
class="myClass">
class="myClass restrictDataVars">
</amp-ad>
<amp-ad id='ad21' width=300 height=250
type="_ping_"
Expand All @@ -247,7 +253,7 @@
data-vars-id='ad21'
data-ad-width=300
data-ad-height=250
class="myClass">
class="myClass restrictDataVars">
</amp-ad>
<amp-ad id='ad22' width=300 height=250
type="_ping_"
Expand All @@ -256,7 +262,7 @@
data-vars-id='ad22'
data-ad-width=300
data-ad-height=250
class="myClass">
class="myClass restrictDataVars">
</amp-ad>
<amp-ad id='ad23' width=300 height=250
type="_ping_"
Expand All @@ -265,7 +271,7 @@
data-vars-id='ad23'
data-ad-width=300
data-ad-height=250
class="myClass">
class="myClass restrictDataVars">
</amp-ad>
<amp-ad id='ad24' width=300 height=250
type="_ping_"
Expand All @@ -274,52 +280,47 @@
data-vars-id='ad24'
data-ad-width=300
data-ad-height=250
class="myClass">
class="myClass restrictDataVars">
</amp-ad>
<amp-ad id='ad25' width=300 height=250
type="_ping_"
data-url='https://lh3.googleusercontent.com/pSECrJ82R7-AqeBCOEPGPM9iG9OEIQ_QXcbubWIOdkY=w400-h300-no-n'
data-valid='true'
data-vars-id='ad25'
data-ad-width=300
data-ad-height=250
class="myClass">
class="myClass restrictDataVars">
</amp-ad>
<amp-ad id='ad26' width=300 height=250
type="_ping_"
data-url='https://lh3.googleusercontent.com/pSECrJ82R7-AqeBCOEPGPM9iG9OEIQ_QXcbubWIOdkY=w400-h300-no-n'
data-valid='true'
data-vars-id='ad26'
data-ad-width=300
data-ad-height=250
class="myClass">
class="myClass restrictDataVars">
</amp-ad>
<amp-ad id='ad27' width=300 height=250
type="_ping_"
data-url='https://lh3.googleusercontent.com/pSECrJ82R7-AqeBCOEPGPM9iG9OEIQ_QXcbubWIOdkY=w400-h300-no-n'
data-valid='true'
data-vars-id='ad27'
data-ad-width=300
data-ad-height=250
class="myClass">
class="myClass restrictDataVars">
</amp-ad>
<amp-ad id='ad28' width=300 height=250
type="_ping_"
data-url='https://lh3.googleusercontent.com/pSECrJ82R7-AqeBCOEPGPM9iG9OEIQ_QXcbubWIOdkY=w400-h300-no-n'
data-valid='true'
data-vars-id='ad28'
data-ad-width=300
data-ad-height=250
class="myClass">
class="myClass restrictDataVars">
</amp-ad>
<amp-ad id='ad29' width=300 height=250
type="_ping_"
data-url='https://lh3.googleusercontent.com/pSECrJ82R7-AqeBCOEPGPM9iG9OEIQ_QXcbubWIOdkY=w400-h300-no-n'
data-valid='true'
data-vars-id='ad29'
data-ad-width=300
data-ad-height=250
class="myClass">
class="myClass restrictDataVars">
</amp-ad>

</div>
Expand Down

0 comments on commit 5aaea99

Please sign in to comment.