✨ Implements `amp-analytics` support for `amp-next-page` by wassgha · Pull Request #26451 · ampproject/amphtml

wassgha · 2020-01-23T06:50:49Z

Changes

Fixes amp-analytics calculates wrong scroll triggers for embedded shadow documents #18735 by introducing ignoreResize to the scrollSpec of <amp-analytics>. This prevents counting subsequent loaded document heights as part of the host/parent page's scrollable area, which means that at 100%, the first/parent document has been fully read and the user is now transitioning to the next document.
Fixes scroll trigger calculations for embedded documents (next pages) by using their viewport as a point of reference as opposed to the host/parent page's viewport (now uses viewport.getLayoutRect() instead of viewport.getScrollWidth/viewport.getScrollHeight)
Verifies that visibility and timer triggers have been fixed as part of ✨Manual management of amp-next-page document visibility #25388
Changes the visibility management policy to prerender partially visible pages in order to prevent loaders and placeholders from being shown until the page is fully scrolled into view
Adds analytics to the manual tests

Assumptions (will be enforced by validator changes):

Only one amp-next-page element per AMP document
amp-next-page is always the last child of the body so there is no content after it (footers will be declared inside the amp-next-page element)
The footer's visibility is controlled solely by amp-next-page and the footer is not counted as part of the host document's scrollable area

wassgha · 2020-01-30T06:27:32Z

@zhouyx this is ready for review but I'd be extra careful with it, might have some unexpected implications

caroqliu

EDIT: I realized that many of these changes rely on synchronously measuring the layout, such as scrollTop and scrollWidth, which could cause layout thrashing. Please refactor to use measureMutateElement/mutateElement where appropriate. Happy to discuss further if you'd like.

~~Approving amp-next-page changes once all comments are addressed. Deferring to @zhouyx for overall approvals for merge. :)~~

extensions/amp-analytics/0.1/events.js

extensions/amp-analytics/0.1/scroll-manager.js

caroqliu · 2020-01-31T20:41:01Z

extensions/amp-analytics/0.1/scroll-manager.js

    const scrollEvent = {
-      top: e.top,
-      left: e.left,
+      top: e.top - layoutRect.top,


Why isn't this expected to be a breaking change?

same question, could you please expand a bit on what are the this.viewport_.getScrollTop(), layoutRect.top. thanks

This shouldn't be a breaking change for the non-shadow case because for the top-level page, this.root_ is equivalent to the document body which means that for the existing case, layoutRect.top and layoutRect.left would be 0 (so it doesn't change the existing behavior). For the ShadowDOM case, this.root_ becomes the ShadowDOM's container, in which case the layoutRect.top and layoutRect.left would serve to offset the calculations to become relative to the ShadowDOM's container as opposed to the top level container.

@zhouyx are there any test suites / manual tests we can do here to ensure that nothing breaks?

nothing that tests shadow dom that I'm aware of. Please feel free to create and add new ones. Thanks!

Thanks for the explanation. One more question on the viewport.getScrollTop() for the non-shadow case. Does the shadow doc share the same viewport service as the parent? What is the scrollTop value here?

If I understand correctly, it does not, the shadow docs will have a viewport service for their own ampdoc instance ( AmpDocShadow as opposed to AmpDocSingle) so viewport.getScrollTop will yield the distance between top of the page and the position of the shadow doc.

extensions/amp-next-page/1.0/service.js

extensions/amp-next-page/amp-next-page.md

test/manual/amp-next-page/1.0/articles/article-long.html

caroqliu

Removing approvals for now.

extensions/amp-analytics/0.1/events.js

extensions/amp-analytics/0.1/scroll-manager.js

zhouyx · 2020-01-31T22:14:22Z

extensions/amp-analytics/0.1/scroll-manager.js

    const scrollEvent = {
-      top: e.top,
-      left: e.left,
+      top: e.top - layoutRect.top,


same question, could you please expand a bit on what are the this.viewport_.getScrollTop(), layoutRect.top. thanks

wassgha · 2020-02-06T09:46:22Z

Added @dvoytenko for sanity checking

alanorozco

Refactor and rename request.

(No strong preference on specific nouns btw, as long as the structure/positive boolean is kept).

extensions/amp-analytics/amp-analytics.md

extensions/amp-analytics/0.1/events.js

extensions/amp-analytics/0.1/scroll-manager.js

dvoytenko

LG from amp-next-page code itself. I'm leaving analytics review to the respective wg

extensions/amp-analytics/0.1/scroll-manager.js

zhouyx · 2020-02-07T02:31:52Z

extensions/amp-analytics/0.1/scroll-manager.js

+        } = layoutRect;
+        /** {./scroll-manager.ScrollEventDef} */
+        const scrollEvent = {
+          top: e.top - scrollTop,


If I understand correctly, it does not, the shadow docs will have a viewport service for their own ampdoc instance ( AmpDocShadow as opposed to AmpDocSingle) so viewport.getScrollTop will yield the distance between top of the page and the position of the shadow doc.

I'm a bit lost : ) If that's the case, e.top is the distance of the element to the shadow doc top. Do we still need to minus the scrollTop here?

Yes :)) So that an element placed at the top of the shadow doc would be considered at 0% of the top (where top refers to the top of the embedded document, not the entire page)

extensions/amp-analytics/0.1/scroll-manager.js

extensions/amp-analytics/0.1/events.js

extensions/amp-analytics/0.1/scroll-manager.js

extensions/amp-analytics/amp-analytics.md

extensions/amp-analytics/0.1/scroll-manager.js

wassgha · 2020-02-24T23:34:55Z

Are we waiting for anything on this one?

zhouyx · 2020-02-24T23:55:22Z

No, didn't get a chance to review the change. Will do today.

zhouyx · 2020-02-25T00:00:47Z

one more unresolved comment #26451 (comment)

zhouyx

LGTM. Thanks!

wassgha · 2020-02-26T04:06:49Z

@alanorozco @caroqliu ping

caroqliu

LGTM

* master: Launch `amp-next-page` v2 & clean up experiment (ampproject#27035) ✨Implement Display Locking on amp-accordion (ampproject#25867) 📖<amp-next-page> documentation and validation (ampproject#26841) Improve visibility event tests (ampproject#26847) amp-geo: Fall back to API for country (ampproject#26407) ✨ Add customization options to <amp-story-quiz> (ampproject#26714) navigation: Minor test improvements (ampproject#26926) ♻️ Alias video.fullscreen action for symmetry with interface names (ampproject#27017) ✨ Implements `amp-analytics` support for `amp-next-page` (ampproject#26451) ✨ amp-video-iframe integration: global jwplayer() singleton by default (ampproject#26969) Fix unit tests broken by ampproject#26687 (ampproject#27000) Filter redirect info from emails (ampproject#27012) 📖 Make amp-bind doc valid, fix amp-form stories filter (ampproject#27003) url-replacements: Minor test improvement (ampproject#26930) viewport: Minor test improvement (ampproject#26931) amp-consent fullscreen warning (ampproject#26467) dep-check: USE CAPS FOR IMPORTANCE (ampproject#26993) fix img url (ampproject#26987) # Conflicts: # extensions/amp-next-page/amp-next-page.md

Added manual tests for analytics usage in amp-next-page

e5fc76a

googlebot added the cla: yes label Jan 23, 2020

wassgha added 3 commits January 27, 2020 17:23

Add setting to the amp-analytics scroll triggers

66ff2d1

Changed visibility management to prerender partially visible pages

58d9391

Merge branch 'master' into amp-next-page-analytics

956e0c4

amp-bundle-size bot requested a review from zhouyx January 28, 2020 23:56

Fixes scroll triggers for shadow docs

70be916

wassgha marked this pull request as ready for review January 30, 2020 06:26

wassgha requested review from alanorozco and caroqliu January 30, 2020 06:28

caroqliu approved these changes Jan 31, 2020

View reviewed changes

caroqliu requested changes Jan 31, 2020

View reviewed changes

zhouyx reviewed Jan 31, 2020

View reviewed changes

wassgha added 4 commits February 3, 2020 01:31

Suggested changes (1/2)

8baa1f8

Merge branch 'master' into amp-next-page-analytics

c7b0986

Merge branch 'master' into amp-next-page-analytics

e83e9ba

Suggested changes (2/2)

dc06733

wassgha requested a review from dvoytenko February 6, 2020 09:45

wassgha requested a review from caroqliu February 6, 2020 18:09

alanorozco requested changes Feb 6, 2020

View reviewed changes

dvoytenko reviewed Feb 6, 2020

View reviewed changes

Review feedback

07eadce

wassgha requested a review from alanorozco February 7, 2020 00:28

Type checking fix

c445ee1

wassgha force-pushed the amp-next-page-analytics branch from 0603aa1 to c445ee1 Compare February 7, 2020 00:39

zhouyx suggested changes Feb 7, 2020

View reviewed changes

alanorozco reviewed Feb 7, 2020

View reviewed changes

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

alanorozco reviewed Feb 8, 2020

View reviewed changes

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

alanorozco reviewed Feb 8, 2020

View reviewed changes

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

alanorozco reviewed Feb 8, 2020

View reviewed changes

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

alanorozco reviewed Feb 8, 2020

View reviewed changes

extensions/amp-analytics/amp-analytics.md Outdated Show resolved Hide resolved

wassgha added 4 commits February 11, 2020 16:12

Suggested changes

ad66689

Merge branch 'master' into amp-next-page-analytics

cd4a4a1

Documentation nits

61667a7

Fixed type check

f0dcce9

alanorozco reviewed Feb 12, 2020

View reviewed changes

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

alanorozco reviewed Feb 12, 2020

View reviewed changes

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

Suggested change

5417314

wassgha requested review from alanorozco and zhouyx February 13, 2020 07:46

zhouyx reviewed Feb 14, 2020

View reviewed changes

Added comments

9c8bfa8

wassgha requested a review from zhouyx February 19, 2020 06:33

zhouyx approved these changes Feb 26, 2020

View reviewed changes

caroqliu approved these changes Feb 26, 2020

View reviewed changes

alanorozco approved these changes Feb 27, 2020

View reviewed changes

wassgha merged commit 7785d74 into ampproject:master Feb 27, 2020

wassgha deleted the amp-next-page-analytics branch February 27, 2020 22:56

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Mar 10, 2020

Conversation

wassgha commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Assumptions (will be enforced by validator changes):

Uh oh!

wassgha commented Jan 30, 2020

Uh oh!

caroqliu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wassgha Feb 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

caroqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wassgha commented Feb 6, 2020

Uh oh!

alanorozco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dvoytenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wassgha commented Feb 24, 2020

Uh oh!

wassgha commented Jan 23, 2020 •

edited

Loading

caroqliu left a comment •

edited

Loading

wassgha Feb 5, 2020 •

edited

Loading

zhouyx commented Feb 25, 2020 •

edited

Loading