Visibility Trigger for Non AMP Elements #29172

micajuine-ho · 2020-07-07T19:30:44Z

Closes #20607

Overview:
Now that a universal IntersectionObserver polyfill is deployed in all AMP modes, we can rely on it to observe non amp elements under the visibility trigger for amp-analytics. We've decided to make this the default behavior for the visibility trigger.

{
  "triggers": {
    "nonAmpElements": {
      "on": "visible",
      "request": "nonAmpElements",
      "selector": "#header",
      "visibilitySpec": {
        "visiblePercentageMin": 20,
        "visiblePercentageMax": 80,
        "totalTimeMin": 5000,
        "continuousTimeMin": 2000
      }
    }
  }
}

Changes:

Remove check for AMP element when we find elements based on querySelector for array selectors (see visibility-trigger-improvements) and for non-special selectors
Change default waitFor spec; AMP elements default to ini-load and non-AMP elements default to none.
Do no support other waitFor (besides none) for non-AMP elements
Rename getAmpElements() => getElements(), and change tests accordingly
Add manual test examples
Updated docs

Notes:

We decided to not hide this change under an experiment flag because it is essentially an opt in feature, as tracking non-amp elements is not currently allowed. This maybe a breaking change in the future, as developers might expect a selector to only apply to amp-elements, but we have updated the docs to reflect this and do not believe it should be a concern.

airroot · 2020-07-10T10:02:51Z

@micajuine-ho
Hello,
Will this feature be available soon?
In our project, we are now trying to do visibility tracking for some elements that are NON-AMP-Elements.
This feature is very useful to us.

Thanks!

micajuine-ho · 2020-07-10T17:01:07Z

@airroot We are expecting a 2-week delay before I can resume work on this; we are waiting for IntersectionObserverPolyfill clean up.

airroot · 2020-07-13T04:00:58Z

@micajuine-ho That sounds great! Thanks!

zhouyx · 2020-08-08T00:15:26Z

Please remind me. Did we agree to only support non AMP element when the selector is an array?

micajuine-ho · 2020-08-10T21:06:35Z

Please remind me. Did we agree to only support non AMP element when the selector is an array?

Discussed offline; originally we did agree on this but after revisiting the decision, we believe this should be a default feature of the visibility trigger (and not just allowed within array of selectors).

zhouyx

LGTM. A few nit picks

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

zhouyx · 2020-08-14T00:09:56Z

Realized one more thing.

waitFor signal requires the element to be an AMP element, otherwise there will be no ini-load render-start signal.

What was our decision on waifFor for an array of selectors. And how do we want to handle waitFor for non AMP element?

micajuine-ho · 2020-08-17T20:12:16Z

What was our decision on waifFor for an array of selectors. And how do we want to handle waitFor for non AMP element?

Discussed offline: waitFor in an array of selectors applies the waitFor to each element found (covered in the previous PRs).

Only waitFor: none will be supported for non-AMP elements, which we will make the default if no waitFor is provided. We will throw an error if any other waitFor is used.

zhouyx · 2020-08-17T21:37:04Z

I want to review the default readyPromise for non AMP element again.

Ideally we want to send visible event that is meaningful to end users that's why we wait for ini-load by default. For non AMP element we don't have the ini-load signal unfortunately. Do you think it's worth waiting for the root's ini-load signal instead? Or waitFor: none is sufficient?

micajuine-ho · 2020-08-18T17:47:32Z

Do you think it's worth waiting for the root's ini-load signal instead? Or waitFor: none is sufficient?

For non-ads case, we are only aware of publishers wanting to track <p> tags, which do not rely on resources in the first viewport to be fully loaded before firing (we can change this accordingly in the future if the need arises).

For the ads case, I talked the ads team (cc @lannka), and we are ok with non-AMP elements visibility trigger firing potentially before the ad is fully visible or loaded.

waitFor: none seems sufficient. No changes will be needed for inabox or FIE.

zhouyx · 2020-08-20T01:53:52Z

extensions/amp-analytics/0.1/events.js

+   */
+  setDefaultWaitForElement_(waitForSpec, element) {
+    if (!waitForSpec) {
+      const isAmpElement = element.classList.contains('i-amphtml-element');


nit: can use isAmpElement

zhouyx · 2020-08-20T01:56:00Z

extensions/amp-analytics/0.1/events.js

      .then((elements) => {
        const unlistenCallbacks = [];
        for (let i = 0; i < elements.length; i++) {
          unlistenCallbacks.push(
            visibilityManager.listenElement(
              elements[i],
              visibilitySpec,
-              this.getReadyPromise(readyPromiseWaitForSpec, elements[i]),
+              this.getReadyPromise(
+                this.setDefaultWaitForElement_(waitForSpec, elements[i]),


nit: I find it cleaner to embed the logic to getReadyPromise(). There's no need to update the waitForSpec, simply return a none readyPromise when the element is not AMP.

zhouyx · 2020-08-20T02:01:15Z

extensions/amp-analytics/amp-analytics.md

@@ -839,6 +839,8 @@ not specified, it waits for element's [`ini-load`](#ini-load) signal. See
 for that signal before sending the event. This is useful, for example, in
 sending analytics events when the page is closed.

+Tracking non-AMP elements are best effort. For example, tracking a `<div>` element that contains an `<amp-iframe>`, may not accurately wait for the iframe to load before sending the signal out.


Move the paragraph to where we document waitFor.

Also I wouldn't say it's best effort, because we always track them.

extensions/amp-analytics/0.1/events.js

zhouyx

LGTM. Thanks for adding this! Worth expanding our e2e/integration test as well. Not blocking.

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

micajuine-ho · 2020-09-08T19:49:11Z

@zhouyx Should we create an e2e test for this? I currently don't see many e2e tests for analytics.

zhouyx · 2020-09-08T20:32:14Z

There're integration tests

micajuine-ho · 2020-09-08T21:02:30Z

Discussed offline, will add to integrations tests.

* init * Manual test * Make default behavior * WaitFor changes * Suggested changes * Clean and add error msg

init

ea2494f

micajuine-ho force-pushed the visibility-trigger-nonamp branch from f8db886 to ea2494f Compare July 7, 2020 19:31

google-cla bot added the cla: yes label Aug 7, 2020

merge

1584e2f

micajuine-ho force-pushed the visibility-trigger-nonamp branch from e18eb1c to 1584e2f Compare August 7, 2020 19:16

Manual test

35bbd1b

micajuine-ho requested a review from zhouyx August 7, 2020 19:43

micajuine-ho changed the title ~~[WIP] Visibility Trigger for Non AMP Elements~~ Visibility Trigger for Non AMP Elements Aug 10, 2020

micajuine-ho force-pushed the visibility-trigger-nonamp branch from f946069 to 9506ea2 Compare August 10, 2020 22:10

zhouyx reviewed Aug 12, 2020

View reviewed changes

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

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

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

Make default behavior

4d65e3c

micajuine-ho force-pushed the visibility-trigger-nonamp branch from 9506ea2 to 4d65e3c Compare August 13, 2020 19:11

micajuine-ho requested a review from zhouyx August 13, 2020 19:12

WaitFor changes

252d68d

micajuine-ho force-pushed the visibility-trigger-nonamp branch from 6aa3382 to 252d68d Compare August 17, 2020 20:07

zhouyx reviewed Aug 20, 2020

View reviewed changes

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

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

Suggested changes

6187968

micajuine-ho force-pushed the visibility-trigger-nonamp branch from 1025a32 to 6187968 Compare August 21, 2020 18:37

zhouyx reviewed Sep 2, 2020

View reviewed changes

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

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

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

micajuine-ho force-pushed the visibility-trigger-nonamp branch from 21ccd2a to 80a8fad Compare September 3, 2020 18:51

micajuine-ho force-pushed the visibility-trigger-nonamp branch from 80a8fad to 2cb04f0 Compare September 3, 2020 18:55

zhouyx approved these changes Sep 8, 2020

View reviewed changes

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

Clean and add error msg

eddda22

micajuine-ho force-pushed the visibility-trigger-nonamp branch from 2cb04f0 to eddda22 Compare September 8, 2020 19:54

Update test-events.js

7a14eae

micajuine-ho merged commit 6874d96 into ampproject:master Sep 9, 2020

ampprojectbot added the PR use: In Beta / Experimental label Sep 15, 2020

adamsilverstein mentioned this pull request Sep 15, 2020

Analytics: add NTG tracking events for newsletters Automattic/newspack-plugin#557

Closed

6 tasks

dkoo mentioned this pull request Sep 15, 2020

Refactor analytics visibility triggers Automattic/newspack-plugin#670

Closed

ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020

Visibility Trigger for Non AMP Elements (ampproject#29172)

5f6567f

* init * Manual test * Make default behavior * WaitFor changes * Suggested changes * Clean and add error msg

adamsilverstein mentioned this pull request Mar 4, 2021

Analytics events framework Automattic/newspack-plugin#531

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Visibility Trigger for Non AMP Elements #29172

Visibility Trigger for Non AMP Elements #29172

micajuine-ho commented Jul 7, 2020 •

edited

Loading

airroot commented Jul 10, 2020

micajuine-ho commented Jul 10, 2020

airroot commented Jul 13, 2020

zhouyx commented Aug 8, 2020

micajuine-ho commented Aug 10, 2020

zhouyx left a comment

zhouyx commented Aug 14, 2020

micajuine-ho commented Aug 17, 2020

zhouyx commented Aug 17, 2020

micajuine-ho commented Aug 18, 2020

zhouyx Aug 20, 2020

zhouyx Aug 20, 2020

zhouyx Aug 20, 2020

zhouyx left a comment

micajuine-ho commented Sep 8, 2020

zhouyx commented Sep 8, 2020

micajuine-ho commented Sep 8, 2020

Visibility Trigger for Non AMP Elements #29172

Visibility Trigger for Non AMP Elements #29172

Conversation

micajuine-ho commented Jul 7, 2020 • edited Loading

airroot commented Jul 10, 2020

micajuine-ho commented Jul 10, 2020

airroot commented Jul 13, 2020

zhouyx commented Aug 8, 2020

micajuine-ho commented Aug 10, 2020

zhouyx left a comment

Choose a reason for hiding this comment

zhouyx commented Aug 14, 2020

micajuine-ho commented Aug 17, 2020

zhouyx commented Aug 17, 2020

micajuine-ho commented Aug 18, 2020

zhouyx Aug 20, 2020

Choose a reason for hiding this comment

zhouyx Aug 20, 2020

Choose a reason for hiding this comment

zhouyx Aug 20, 2020

Choose a reason for hiding this comment

zhouyx left a comment

Choose a reason for hiding this comment

micajuine-ho commented Sep 8, 2020

zhouyx commented Sep 8, 2020

micajuine-ho commented Sep 8, 2020

micajuine-ho commented Jul 7, 2020 •

edited

Loading