Implement Visibility API by jridgewell · Pull Request #2212 · ampproject/amphtml

jridgewell · 2016-02-22T23:42:52Z

Implements a “standardized” visibility API with control from the viewer
and browser document.

Allows the Viewer to send the paused state when the users swipes
between AMP docs. (closes Add PAUSED VisibilityState #1655, add a 'paused' visibility state #1538)
Adds the inactive visibility state when the AMP document is not
active
Changes the hidden visibility to mean the AMP doc is active but the
browser tab is not visible.

jridgewell · 2016-02-22T23:43:58Z

Assigning to @dvoytenko, but we're waiting to merge this until later this week.

jridgewell · 2016-02-23T23:07:08Z

src/service/viewer-impl.js

I think a better term for this might be "background". This state only triggers when the browser tab that displays the viewer/doc is not active (another tab is).

jridgewell · 2016-02-24T20:00:11Z

PTAL.

dvoytenko · 2016-02-24T20:34:53Z

Looking...

dvoytenko · 2016-02-24T21:02:18Z

src/service/viewer-impl.js

Let's store a single and correct this.visibilityState_ (point of reference) and recalculate it only when something changes.

dvoytenko · 2016-02-24T21:05:46Z

Looks great! Some comments...

jridgewell · 2016-02-24T22:08:02Z

PTAL.

dvoytenko · 2016-02-25T18:29:20Z

src/service/viewer-impl.js

opt_camelCase

We don't need to worry about skipFire in this case (or maybe never?) - here we know that the list of listeners is empty :)

Is fixedUnloaded really true here? Seems like the initial hidden would indicate instead the prerendering state, not unloaded.

dvoytenko · 2016-02-25T18:46:34Z

@jridgewell this looks awesome! I just would like to do these few steps:

Add PRERENDER state distinctly.
Clarify PAUSE vs UNLOADED
Create a spec doc to update viewer's visibilityState as the result.
Make it super clear in the code where we workaround current viewer incompatibility and where it's a meaningful code for the future (perhaps it's already done, but the 1/2 above might change that a bit).

jridgewell · 2016-02-25T19:56:51Z

Add PRERENDER state distinctly.
- Done.
Clarify PAUSE vs UNLOADED
- PAUSED is sent when the user begins to swipe the doc. UNLOADED is sent when the user finishes swiping away. VISIBLE may be sent if the user half-swipes but ends stays on the same doc. Essentially, PAUSED is the state in between VISIBLE and UNLOADED.
Create a spec doc to update viewer's visibilityState as the result.
- What?
Make it super clear in the code where we workaround current viewer incompatibility and where it's a meaningful code for the future (perhaps it's already done, but the 1/2 above might change that a bit).
- Done in #setVisibilityState_.

I had to backtrack a bit and do a bit of computation in #getVisibilityState. To do everything in #set..., I would have to keep track of too much to properly transition on when cycling through the browser tab.

dvoytenko · 2016-02-25T21:34:41Z

src/service/viewer-impl.js

"but the browser tab or an AMP app is not"

dvoytenko · 2016-02-25T21:42:03Z

Great! Some doc changes, clear up getter and setter and confirm if we need flush and we're at lgtm.

dvoytenko · 2016-02-25T23:16:09Z

src/service/viewer-impl.js

This line has to be called after this.recheckVisibilityState_() call, otherwise it will misreport. Probably just better to call recheckVisibilityState_() as soon as viewerVisibilityState_ and docState_ are initialized?

dvoytenko · 2016-02-25T23:17:41Z

Great! It looks like there's one small bug to be fixed with the sequence of initializations. But otherwise LGTM!

Implements a “standardized” visibility API with control from the viewer and browser document. - Allows the Viewer to send the `paused` state when the users swipes between AMP docs. (closes ampproject#1655, ampproject#1538) - Adds the `prerender` visibility state when the AMP document has not been shown yet. - Adds the `inactive` visibility state when the AMP document is not active. - Changes the `hidden` visibility to mean the AMP doc is active but the browser tab is not visible.

Implement Visibility API

jridgewell assigned dvoytenko Feb 22, 2016

jridgewell force-pushed the visibility-state branch from 2013b58 to 40bd5f7 Compare February 23, 2016 19:20

jridgewell reviewed Feb 23, 2016
View reviewed changes

jridgewell force-pushed the visibility-state branch from 40bd5f7 to aac902a Compare February 24, 2016 19:59

jridgewell added the NEEDS REVIEW label Feb 24, 2016

dvoytenko reviewed Feb 24, 2016
View reviewed changes

jridgewell force-pushed the visibility-state branch 2 times, most recently from f28fb6c to dbda708 Compare February 24, 2016 22:07

jridgewell force-pushed the visibility-state branch from dbda708 to bffaf1a Compare February 24, 2016 22:27

dvoytenko reviewed Feb 25, 2016
View reviewed changes

jridgewell force-pushed the visibility-state branch from bffaf1a to cc90d19 Compare February 25, 2016 19:50

Separate Document Ready from Document State

c09dcc2

jridgewell force-pushed the visibility-state branch from cc90d19 to a5b7a0f Compare February 25, 2016 19:51

dvoytenko reviewed Feb 25, 2016
View reviewed changes

jridgewell force-pushed the visibility-state branch 2 times, most recently from 40270a7 to 69e8e2a Compare February 25, 2016 23:13

dvoytenko reviewed Feb 25, 2016
View reviewed changes

dvoytenko added LGTM and removed NEEDS REVIEW labels Feb 25, 2016

jridgewell force-pushed the visibility-state branch 2 times, most recently from 238307a to 1c63939 Compare February 25, 2016 23:23

jridgewell force-pushed the visibility-state branch from 1c63939 to 0b66e51 Compare February 26, 2016 20:22

jridgewell added a commit that referenced this pull request Feb 26, 2016

Merge pull request #2212 from jridgewell/visibility-state

dedcd4f

Implement Visibility API

jridgewell merged commit dedcd4f into ampproject:master Feb 26, 2016

jridgewell deleted the visibility-state branch February 26, 2016 22:27

This was referenced Feb 29, 2016

add a 'paused' visibility state #1538

Closed

Allow audio in inactive windows/background #2361

Closed

ampprojectbot mentioned this pull request Mar 8, 2022

🚨 Error: Cannot read properties of null (reading 'setState') ampproject/error-reporting#152

Open

Conversation

jridgewell commented Feb 22, 2016

Uh oh!

jridgewell commented Feb 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jridgewell commented Feb 24, 2016

Uh oh!

dvoytenko commented Feb 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvoytenko commented Feb 24, 2016

Uh oh!

jridgewell commented Feb 24, 2016

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!

dvoytenko commented Feb 25, 2016

Uh oh!

jridgewell commented Feb 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvoytenko commented Feb 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvoytenko commented Feb 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants