✨Manual management of amp-next-page document visibility#25388
Merged
wassgha merged 17 commits intoampproject:masterfrom Nov 8, 2019
Merged
✨Manual management of amp-next-page document visibility#25388wassgha merged 17 commits intoampproject:masterfrom
amp-next-page document visibility#25388wassgha merged 17 commits intoampproject:masterfrom
Conversation
sparhami
suggested changes
Nov 5, 2019
Contributor
|
@wassgha qq:
Can you clarify this? Are we bumping version? Is this component really launched? |
dvoytenko
reviewed
Nov 6, 2019
dvoytenko
reviewed
Nov 6, 2019
Contributor
Author
|
@dvdyakonov let's talk about versioning offline but here's the (very incomplete) design doc https://docs.google.com/document/d/1x-TD4qWr3hWm2BzqUfrv4hHJyTJ4HBb6H3EcJUDDmyY/edit |
c586180 to
346e3d8
Compare
sparhami
reviewed
Nov 6, 2019
1dc9bd6 to
986ed28
Compare
sparhami
suggested changes
Nov 8, 2019
sparhami
approved these changes
Nov 8, 2019
dvoytenko
approved these changes
Nov 8, 2019
Contributor
Ping on this question. |
micajuine-ho
pushed a commit
to micajuine-ho/amphtml
that referenced
this pull request
Dec 27, 2019
…#25388) * Manual management of document visibility * Edge cases and tests * Fixed types * Requested changes * Fixed types * Removed unnecessary cast * Switched to using refs instead of indices * Fixed non-nullable type * Manual management of document visibility * Edge cases and tests * Fixed types * Requested changes * Fixed types * Removed unnecessary cast * Switched to using refs instead of indices * Fixed non-nullable type
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #25069 , Partial for #15807 & #14059
Changes
amp-next-page v0.1(the change will land inv0.2) to allow it to override document visibility for the added shadow docs, fixing interactions withamp-pixeland possiblyamp-analytics(previously all added "next" documents would be given the "visible" state even during pre-rendering which would immediately trigger anamp-pixelhit before the user reaches the next page).e2etesting is implemented, see Adde2etests foramp-next-page#24610 )ShadowDocwhich defines the Shadow document object (already generated by the AMP runtime but typed asObject)Implementation details
Previous to this change, shadow docs appended by
amp-next-pagewould follow their parent's visibility state which in most cases would bevisibleas the user is scrolling down the page. This causes multiple problems during the time between the pre-render phase (user is scrolling and the next page is loaded) and the visible phase (the user scrolled to the next page and is reading it).One of the problems that have surfaced is the interaction with
amp-pixelwhere the analytics call is gated by the document's visibility state. In this case, as the next page has been loaded (and thus adopts its parent'svisiblestate), theamp-pixeltriggers preemptively, before the user actually scrolls to the next page.This change corrects the behavior by having
amp-next-pagemanually override the visibility state of the next pages as follows:PRERENDER(as opposed to the parent's visibility state)VISIBLEwhile other documents (excluding the host/first document) are either in thePRERENDERorHIDDENstatesHIDDEN,PAUSEDorINACTIVEbut restore the previous states when the parent/first document becomesVISIBLETodo
e2e)PRERENDERvisibility state when scrolling back/cc @nainar @kristoferbaxter