Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Visibility API #2212

Merged
merged 2 commits into from
Feb 26, 2016
Merged

Conversation

jridgewell
Copy link
Contributor

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

@jridgewell
Copy link
Contributor Author

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

/**
* TODO
*/
UNLOADED: 'unloaded',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

PTAL.

@dvoytenko
Copy link
Contributor

Looking...

@@ -445,7 +452,40 @@ export class Viewer {
* @return {!VisibilityState}
*/
getVisibilityState() {
return this.visibilityState_;
// The viewer has informed us that the doc is no longer active, meaning
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dvoytenko
Copy link
Contributor

Looks great! Some comments...

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

PTAL.

this.visibilityState_;
log.fine(TAG_, '- visibilityState:', this.visibilityState_);
this.setVisibilityState_(this.params_['visibilityState'],
/* opt_fix_unloaded */ true, /* opt_skip_fire */ true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opt_camelCase

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dvoytenko
Copy link
Contributor

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

  1. Add PRERENDER state distinctly.
  2. Clarify PAUSE vs UNLOADED
  3. Create a spec doc to update viewer's visibilityState as the result.
  4. 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
Copy link
Contributor Author

  1. Add PRERENDER state distinctly.
    • Done.
  2. 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.
  3. Create a spec doc to update viewer's visibilityState as the result.
    • What?
  4. 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.

*/
VISIBLE: 'visible',

/**
* Viewer has indicated that AMP document is hidden.
* The AMP document is active but the browser tab is not.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@dvoytenko
Copy link
Contributor

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

@jridgewell jridgewell force-pushed the visibility-state branch 2 times, most recently from 40270a7 to 69e8e2a Compare February 25, 2016 23:13
@@ -251,7 +268,7 @@ export class Viewer {
this.hasBeenVisible_ = this.isVisible();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@dvoytenko
Copy link
Contributor

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

@jridgewell jridgewell force-pushed the visibility-state branch 2 times, most recently from 238307a to 1c63939 Compare February 25, 2016 23:23
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add PAUSED VisibilityState
2 participants