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 14 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
87 changes: 82 additions & 5 deletions extensions/amp-analytics/0.1/analytics-root.js
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,30 @@ export class AnalyticsRoot {
});
}

/**
* @param {string} selector DOM query selector.
* @return {!Promise<!Array<!Element>>} Element corresponding to the selector.
*/
getElementsByQuerySelectorAll_(selector) {
// 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])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this.contains?

Copy link
Contributor Author

@micajuine-ho micajuine-ho Mar 24, 2020

Choose a reason for hiding this comment

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

Used in the original getAmpElements(), to make sure that the element is a descendant of the analytics-root: defined here

elements.push(foundElements[i]);
}
}
userAssert(elements.length, `Element "${selector}" not found`);
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,18 +311,71 @@ 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 => {
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
);
const elementsListsPromise = [];
for (let i = 0; i < selectors.length; i++) {
elementsListsPromise.push(
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little concern about the performance here. Would it be faster to assign a unique attribute to element instead? so that we don't need to iterate the array everytime. Could you please verify?

cc @jridgewell who might know : ) What's the best way to find unique element out of an array?

Copy link
Contributor

Choose a reason for hiding this comment

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

For small lists, this will be quick enough.

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 I think the list will be relatively small. What do you think?

uniqueElements.push(elementsLists[i][j]);
}
}
}
return uniqueElements;
});
}
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(
element.classList.contains('i-amphtml-element'),
elements[i].classList.contains('i-amphtml-element'),
'Element "%s" is required to be an AMP element',
selector
);
return element;
});
}
}

/**
Expand Down
66 changes: 39 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(
this.getWaitForSpecForRootSelector_(waitForSpec, selector)
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
),
createReportReadyPromiseFunc,
this.onEvent_.bind(
this,
Expand All @@ -1506,32 +1508,37 @@ 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 =>
const selectors = Array.isArray(selector)
? selector.filter((val, index) => selectors.indexOf(val) === index)
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
: [selector];
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 Down Expand Up @@ -1641,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');
Expand Down
99 changes: 91 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,108 @@ 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 elements = await root.getAmpElementOrElements(body, '#child');
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
expect(elements[0]).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.getAmpElementOrElements(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.getAmpElementOrElements(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;
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 () => {
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
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(
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');
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
elements = await root.getAmpElementOrElements(
body,
'#myId.myClass',
null,
true
);
});

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/);
});
});

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/);
});
});
});
micajuine-ho marked this conversation as resolved.
Show resolved Hide resolved
});

describe('createSelectiveListener', () => {
Expand Down
Loading