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

Display-observer: a hidden tab should still be displayable for playback #32478

Merged
merged 2 commits into from
Feb 8, 2021

Conversation

dvoytenko
Copy link
Contributor

@dvoytenko dvoytenko commented Feb 6, 2021

Partial for #31915.
Partial for #31540.
Closes #32487.

These rules come from the Resources class. Notice that only "paused" and "inactive" cause transitions to pause().

// The document is still considered "displayed" or at least "displayable"
// when it's hidden (tab is switched). Only prerender/paused/inactive
// states require pause of resources.
visibilityState == VisibilityState.HIDDEN
Copy link
Contributor

Choose a reason for hiding this comment

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

This state is also used when a viewer swipes to the next doc. Shouldn't that be marked as not displayed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not this one. I checked and in the viewer, the sequence of states is: "visible" -> "paused" -> "inactive".

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, tested and this works correctly. Viewer swipe leads to INACTIVE, and tab change leads to HIDDEN.

Should PAUSED be included here? I think yes. And what about PRERENDER? I think that could be likely no, because someone might depend on it and accidentally leak privacy.

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 believe, yes, because PAUSE is explicitly made for this use-case. I think the way this happens is that PAUSED is issued immediately and INACTIVE some time later to avoid too many unlayouts while the new page is loaded.

src/utils/display-observer.js Outdated Show resolved Hide resolved
// The document is still considered "displayed" or at least "displayable"
// when it's hidden (tab is switched). Only prerender/paused/inactive
// states require pause of resources.
visibilityState == VisibilityState.HIDDEN
Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, tested and this works correctly. Viewer swipe leads to INACTIVE, and tab change leads to HIDDEN.

Should PAUSED be included here? I think yes. And what about PRERENDER? I think that could be likely no, because someone might depend on it and accidentally leak privacy.

@dvoytenko dvoytenko merged commit 4a43478 into ampproject:master Feb 8, 2021
@dvoytenko dvoytenko deleted the run3/display-pause2 branch February 8, 2021 21:06
ampprojectbot pushed a commit that referenced this pull request Feb 8, 2021
…ck (#32478)

* Display-observer: a hidden tab should still be displayable for playback

* review fixes

(cherry picked from commit 4a43478)
ampprojectbot pushed a commit that referenced this pull request Feb 8, 2021
…ck (#32478)

* Display-observer: a hidden tab should still be displayable for playback

* review fixes

(cherry picked from commit 4a43478)
ampprojectbot pushed a commit that referenced this pull request Feb 9, 2021
…ck (#32478)

* Display-observer: a hidden tab should still be displayable for playback

* review fixes

(cherry picked from commit 4a43478)
Copy link
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

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

Just catching up on this. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Iframe refreshes automatically when tab change.
4 participants