Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Visibility trigger support for querySelectorAll #26886

Merged
merged 20 commits into from
Apr 20, 2020
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 75 additions & 7 deletions extensions/amp-analytics/0.1/analytics-root.js
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,36 @@ export class AnalyticsRoot {
});
}

/**
* @param {!Array<string>} selectors Array of DOM query selectors.
* @return {!Promise<!Array<!Element>>} 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 elements;
});
}

/**
* Searches the AMP element that matches the selector within the scope of the
* analytics root in relationship to the specified context node.
Expand All @@ -287,22 +317,60 @@ 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<!AmpElement>} 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 Array of DOM query selector.
* @param {?string=} selectionMethod Allowed values are `null`,
* `'closest'` and `'scope'`.
* @param {boolean=} opt_multiSelectorOn multi-selector expriment
* @return {!Promise<!Array<!AmpElement>>} Array of AMP elements corresponding to the selector if found.
*/
getAmpElements(context, selectors, selectionMethod, opt_multiSelectorOn) {
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
if (opt_multiSelectorOn) {
userAssert(
!selectionMethod,
'Cannot have selectionMethod defined with an array selector.'
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
);
return this.getElementsByQuerySelectorAll_(selectors).then((elements) =>
elements.filter((element, index) => elements.indexOf(element) === index)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this goes into getElementsByQuerySelectorAll_ as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't make a much of a difference when the elements list is small, but it does take O(n^2) time... I'm a bit concerned when one try to select every single <p> of a document. could you please bench mark the performance with a reasonable element count? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a webpage with 30 amp-ad elements and no throttle:

  • 0.024ms avg with O(n^2)
 elements.filter((element, index) => elements.indexOf(element) === index)
  • 0.058ms avg with O(n)
for (let i = 0; i < elements.length; i++) {
   if (!elements[i].hasAttribute('special')) {
      elements[i].setAttribute('special', 'yes');
      returnElements.push(elements[i]);
   }
}

Not sure if there's. better way to do O(n)

);
}
return this.getAmpElement(
context,
selectors[0],
selectionMethod
).then((element) => [element]);
}

/**
* @param {!Array<Element>} 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
Expand Down
68 changes: 41 additions & 27 deletions extensions/amp-analytics/0.1/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -1487,7 +1487,9 @@ export class VisibilityTracker extends EventTracker {
(visibilityManager) => {
return visibilityManager.listenRoot(
visibilitySpec,
this.getReadyPromise(waitForSpec, selector),
this.getReadyPromise(
waitForSpec || (selector ? 'ini-load' : null)
),
createReportReadyPromiseFunc,
this.onEvent_.bind(
this,
Expand All @@ -1507,31 +1509,35 @@ export class VisibilityTracker extends EventTracker {
const selectionMethod =
config['selectionMethod'] || visibilitySpec['selectionMethod'];
const selectors = Array.isArray(selector) ? selector : [selector];
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
for (let i = 0; i < selectors.length; i++) {
unlistenPromises.push(
this.root
.getAmpElement(
context.parentElement || context,
selectors[i],
selectionMethod,
multiSelectorVisibilityOn
)
.then((element) =>
this.assertUniqueSelectors_(selectors);
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(
element,
elements[i],
visibilitySpec,
this.getReadyPromise(waitForSpec, selectors[i], element),
this.getReadyPromise(
waitForSpec || 'ini-load',
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
elements[i]
),
createReportReadyPromiseFunc,
this.onEvent_.bind(this, eventType, listener, element)
this.onEvent_.bind(this, eventType, listener, elements[i])
);
},
() => {}
)
)
);
}
);
}
});
}

return function () {
Expand All @@ -1543,6 +1549,21 @@ export class VisibilityTracker extends EventTracker {
};
}

/**
* Assert that the selectors are all unique
* @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
);
}

/**
* Assert that the setting is measurable with host API
* @param {string=} selector
Expand Down Expand Up @@ -1642,21 +1663,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');
Expand Down
101 changes: 93 additions & 8 deletions extensions/amp-analytics/0.1/test/test-analytics-root.js
Original file line number Diff line number Diff line change
Expand Up @@ -333,25 +333,67 @@ 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 () => {
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
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', () => {
Copy link
Contributor

@zhouyx zhouyx Apr 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm, what will be the API provided after all changes.
analyticsRoot.getElements() and analyticsRoot.getElement()
or a single analyticsRoot.getElements() or anything?
The design would affect tests here.

Copy link
Contributor Author

@micajuine-ho micajuine-ho Apr 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will have analyticsRoot.getElements() (added in this pr) and analyticsRoot.getElement(), previous tests cover getElement().

Adding test coverage for getElements(Array<string>|string)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we deprecate usage of analyticsRoot.getElement() afterwards? If so is the plan to fix related test in the following PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, getElement() is still used by getAmpElement() which is staying (non-experiment case).

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');
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
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);
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
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);
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
child.classList.remove('i-amphtml-element');
await expect(
root.getAmpElements(body, ['#child'], null, true)
).to.be.rejectedWith(/required to be an AMP element/);
});
});
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
});

describe('createSelectiveListener', () => {
Expand Down Expand Up @@ -641,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 () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you try one thing, add another element to the embed host and make sure it won't be selected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping on this comment : )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhouyx Do you mean add another element that is non-amp?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant add element with the className to the env.parentWin and then verify that they don't get selected.
https://github.com/ampproject/amphtml/blob/master/testing/describes.js#L805

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Thanks for the explanation. Added an element called parentChild that is attached to the the parentWin.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks

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();
Expand Down
Loading