✨ Visibility trigger support for querySelectorAll #26886

micajuine-ho · 2020-02-20T18:11:28Z

Task 2 of #26823

getAmpElement() => getAmpElementOrElements
- If experiment is on, returns unique elements of scopedQuerySelectorAll() for given selector
  - Error if selectionMethod is provided
- Otherwise, return getElement() of selector
Also changed what promise gets pushed into the unlistenPromises

Flame Charts:
Before for processConfigs and addTriggers:

After for processConfigs and addTriggers:

micajuine-ho

@zhouyx Should the tests in test-analytics-root.js also be applied to theEmbedAnalyticsRoot within that file?

micajuine-ho · 2020-03-20T23:22:27Z

extensions/amp-analytics/0.1/events.js

+        this.root
+          .getAmpElementOrElements(
+            context.parentElement || context,
+            selectors[i],
+            selectionMethod,
+            multiSelectorVisibilityOn
+          )
+          .then(elements => {
+            for (let j = 0; j < elements.length; j++) {
+              unlistenPromises.push(
+                visibilityManagerPromise.then(


Does this seem correct?

…to querySelectorAll

zhouyx · 2020-03-21T01:20:23Z

extensions/amp-analytics/0.1/analytics-root.js

+    return this.ampdoc.whenReady().then(() => {
+      const results = [];
+      const foundElements = scopedQuerySelectorAll(
+        dev().assertElement(this.ampdoc.getBody()),


Could you confirm that if embed would get its own ampdoc? If not I think we still need to pass in the context

Current implantation uses this.getRoot().querySelector(selector), which I think is a better approach.

extensions/amp-analytics/0.1/analytics-root.js

…to querySelectorAll

micajuine-ho · 2020-03-23T22:55:31Z

PTAL @zhouyx

zhouyx · 2020-03-24T02:00:46Z

extensions/amp-analytics/0.1/analytics-root.js

+      }
+      const elements = [];
+      for (let i = 0; i < foundElements.length; i++) {
+        if (this.contains(foundElements[i])) {


this.contains?

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

extensions/amp-analytics/0.1/analytics-root.js

…to querySelectorAll

micajuine-ho · 2020-03-25T18:32:52Z

@zhouyx Ready for another round of review

zhouyx · 2020-03-25T22:41:49Z

extensions/amp-analytics/0.1/analytics-root.js

+        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) {


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?

For small lists, this will be quick enough.

@zhouyx I think the list will be relatively small. What do you think?

extensions/amp-analytics/0.1/analytics-root.js

extensions/amp-analytics/0.1/events.js

extensions/amp-analytics/0.1/test/test-analytics-root.js

micajuine-ho · 2020-03-31T20:10:06Z

@zhouyx Ready for another round of review

Changed getReadyPromise()
Condensed getAmpElements()'s work into getElementsByQuerySelectorAll_()
Fixed tests (including tests for getReadyPromise())

Changes in the next PR:

Add additional restriction to getElementsByQuerySelectorAll_() to only return elements with data-vars-*
Change intersectionObserver polyfill to support non-amp elements (add asyncGetLayoutRect()), remove verifyAmpElements() from getElementsByQuerySelectorAll_()

These changes will not change the code flow/structure seen in this PR.

extensions/amp-analytics/0.1/events.js

extensions/amp-analytics/0.1/analytics-root.js

zhouyx · 2020-03-31T23:48:20Z

extensions/amp-analytics/0.1/analytics-root.js

+        'Cannot have selectionMethod defined with an array selector.'
+      );
+      return this.getElementsByQuerySelectorAll_(selectors).then((elements) =>
+        elements.filter((element, index) => elements.indexOf(element) === index)


Can this goes into getElementsByQuerySelectorAll_ as well?

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.

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)

extensions/amp-analytics/0.1/test/test-analytics-root.js

extensions/amp-analytics/0.1/test/test-events.js

test/manual/amp-analytics/amp-analytics-multi-selector.html

micajuine-ho · 2020-04-03T00:26:55Z

@zhouyx Ready for review

zhouyx

I can't reply to the original comment thread, but re https://github.com/ampproject/amphtml/pull/26886/files#discussion_r402566861
Thanks for running the benchmark, I can only think about apply special attribute and remove it afterwards as the way to achieve O(n).
SG, let's go with the your approach.

extensions/amp-analytics/0.1/analytics-root.js

zhouyx · 2020-04-03T03:32:32Z

extensions/amp-analytics/0.1/test/test-analytics-root.js

        expect(error).to.match(/required to be an AMP element/);
      });
    });
+
+    describe('get amp elements', () => {


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

extensions/amp-analytics/0.1/test/test-analytics-root.js

zhouyx · 2020-04-03T19:29:27Z

extensions/amp-analytics/0.1/test/test-analytics-root.js

+        child2.classList.add('i-amphtml-element');
+      });
+
+      it('should find all elements by selector', async () => {


Ping on this comment : )

extensions/amp-analytics/0.1/test/test-events.js

test/manual/amp-analytics/amp-analytics-multi-selector.html

extensions/amp-analytics/0.1/test/test-analytics-root.js

micajuine-ho · 2020-04-20T19:26:05Z

@zhouyx Can you approve again for the bundle size check?

Impl, unit test, & manual test

db28aad

googlebot added the cla: yes label Feb 20, 2020

micajuine-ho requested a review from zhouyx February 20, 2020 18:12

micajuine-ho mentioned this pull request Feb 20, 2020

I2I: Multi-selector support for visibility trigger #26823

Closed

8 tasks

micajuine-ho force-pushed the querySelectorAll branch from 1b947bf to ec0e4ae Compare February 20, 2020 19:17

micajuine-ho commented Feb 20, 2020

View reviewed changes

micajuine-ho mentioned this pull request Feb 21, 2020

✨ Require visibility trigger selector's to have data-vars-* #26902

Merged

micajuine-ho force-pushed the querySelectorAll branch 2 times, most recently from 9b8dc50 to c978db5 Compare March 2, 2020 21:09

Impl, unit tests, manual tests

7ba9784

micajuine-ho force-pushed the querySelectorAll branch from c978db5 to 7ba9784 Compare March 2, 2020 21:50

Merge branch 'master' into querySelectorAll

74eaef3

micajuine-ho force-pushed the querySelectorAll branch from 3d508c6 to 74eaef3 Compare March 20, 2020 23:18

micajuine-ho commented Mar 20, 2020

View reviewed changes

Micajuine Ho added 4 commits March 20, 2020 16:23

Update amp-analytics-visible-repeat.amp.html

926e4bf

Merge branch 'master' into querySelectorAll

cbdb344

Merge branch 'querySelectorAll' of github.com:micajuine-ho/amphtml in…

1003500

…to querySelectorAll

Update amp-analytics-visible-repeat.amp.html

62ab379

zhouyx suggested changes Mar 21, 2020

View reviewed changes

Micajuine Ho added 5 commits March 23, 2020 15:32

keeping element list reference

beb8f58

Merge branch 'querySelectorAll' of github.com:micajuine-ho/amphtml in…

7a43664

…to querySelectorAll

Merge branch 'querySelectorAll' of github.com:micajuine-ho/amphtml in…

df942d2

…to querySelectorAll

Merge branch 'querySelectorAll' of github.com:micajuine-ho/amphtml in…

7456e8d

…to querySelectorAll

Merge branch 'querySelectorAll' of github.com:micajuine-ho/amphtml in…

813992f

…to querySelectorAll

micajuine-ho requested a review from zhouyx March 23, 2020 22:55

zhouyx suggested changes Mar 24, 2020

View reviewed changes

Merge branch 'querySelectorAll' of github.com:micajuine-ho/amphtml in…

7683e34

…to querySelectorAll

micajuine-ho force-pushed the querySelectorAll branch from 489b467 to 7683e34 Compare March 24, 2020 20:56

Make reduce getReadyPromise

e9efdfe

zhouyx suggested changes Mar 25, 2020

View reviewed changes

Micajuine Ho added 2 commits March 26, 2020 10:48

Optimizing and testing

b4228b8

Fixing tests & suggested changes

97c92a9

micajuine-ho force-pushed the querySelectorAll branch from 4efc09f to 97c92a9 Compare March 30, 2020 21:36

micajuine-ho force-pushed the querySelectorAll branch from d30b758 to 0b1f226 Compare March 31, 2020 20:26

Merge branch 'master' into querySelectorAll

c4b9cc5

micajuine-ho force-pushed the querySelectorAll branch from 0b1f226 to c4b9cc5 Compare March 31, 2020 22:41

zhouyx reviewed Apr 1, 2020

View reviewed changes

Suggested changes

2108ac7

zhouyx reviewed Apr 3, 2020

View reviewed changes

Test changes

b6ea443

micajuine-ho commented Apr 3, 2020

View reviewed changes

extensions/amp-analytics/0.1/test/test-analytics-root.js Outdated Show resolved Hide resolved

micajuine-ho force-pushed the querySelectorAll branch from 457130d to 215a15b Compare April 14, 2020 21:29

amp-bundle-size bot requested a review from ajwhatson April 14, 2020 22:30

zhouyx approved these changes Apr 16, 2020

View reviewed changes

Fixes to FIE test

71c6b80

micajuine-ho force-pushed the querySelectorAll branch from 215a15b to 71c6b80 Compare April 17, 2020 19:23

micajuine-ho requested review from zhouyx and removed request for zhouyx April 20, 2020 19:25

zhouyx approved these changes Apr 20, 2020

View reviewed changes

micajuine-ho merged commit e1233a7 into ampproject:master Apr 20, 2020

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Apr 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Visibility trigger support for querySelectorAll #26886

✨ Visibility trigger support for querySelectorAll #26886

micajuine-ho commented Feb 20, 2020 •

edited

Loading

micajuine-ho left a comment

micajuine-ho Mar 20, 2020

zhouyx Mar 21, 2020

micajuine-ho Mar 23, 2020

micajuine-ho commented Mar 23, 2020

zhouyx Mar 24, 2020

micajuine-ho Mar 24, 2020 •

edited

Loading

micajuine-ho commented Mar 25, 2020

zhouyx Mar 25, 2020

jridgewell Mar 26, 2020

micajuine-ho Mar 27, 2020

micajuine-ho commented Mar 31, 2020 •

edited

Loading

zhouyx Mar 31, 2020

zhouyx Mar 31, 2020

micajuine-ho Apr 2, 2020

micajuine-ho commented Apr 3, 2020

zhouyx left a comment

zhouyx Apr 3, 2020

zhouyx Apr 3, 2020

micajuine-ho commented Apr 20, 2020

✨ Visibility trigger support for querySelectorAll #26886

✨ Visibility trigger support for querySelectorAll #26886

Conversation

micajuine-ho commented Feb 20, 2020 • edited Loading

micajuine-ho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micajuine-ho commented Mar 23, 2020

Choose a reason for hiding this comment

micajuine-ho Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

micajuine-ho commented Mar 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micajuine-ho commented Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micajuine-ho commented Apr 3, 2020

zhouyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micajuine-ho commented Apr 20, 2020

micajuine-ho commented Feb 20, 2020 •

edited

Loading

micajuine-ho Mar 24, 2020 •

edited

Loading

micajuine-ho commented Mar 31, 2020 •

edited

Loading