Skip to content
This repository has been archived by the owner on Jul 18, 2022. It is now read-only.

Why does visibilitychange fire after pagehide in the unload flow? #39

Closed
philipwalton opened this issue Apr 26, 2018 · 14 comments
Closed
Milestone

Comments

@philipwalton
Copy link
Member

The current ordering of pagehide and visibilitychange during a page unload complicates our proposed flow of Lifecycle states for the Lifecycle API.

The reason is the ordering of pagehide and visibilitychange events can be inverted, depending on what the user does. Consider these two examples:

  1. The user loads a page in Tab A and then switches to a new tab B. Later, they close tab A without refocusing it. In this case the ordering of events fired on tab A is visibilitychangepagehide.

  2. The user loads a page in Tab A and then navigates to a new page from a link. In this case the ordering of events fired on tab A is pagehidevisibilitychange.

Wouldn't it make more sense if visibilitychange always fired before pagehide?

Since pagehide is associated with the FROZEN state (for bfcache supporting browsers), and visiblitychange is associated with the HIDDEN state, the current ordering of events suggests that the lifecycle state of a page can transition from ACTIVE to FROZEN to HIDDEN, which doesn't make sense since we only want to freeze hidden tabs.

Does anyone here remember why this ordering was chosen? Would anyone object to changing the ordering in the spec?

https://html.spec.whatwg.org/multipage/browsing-the-web.html#unloading-documents

(Related dicussion WICG/page-lifecycle#7).

@igrigorik
Copy link
Member

I don't recall any particular reason for why this ordering was chosen. My guess is that it simply wasn't a constraint or criteria when we specced this.

If we wanted to update this logic, you'd have to update the HTML spec and swap order of steps 5 and 6.

  • Need to make make sure this change doesn't affect any other specs relying on "unloading document visibility change steps" hook.
  • Could this break any existing clients / analytics libraries?

@philipwalton
Copy link
Member Author

My guess is this wouldn't break many (or any) sites or analytics libraries based on the fact that, until recently, not all browsers actually fired visibilitychange when the page was unloading (@toddreifsteck actually made this point when we discussed this issue on the last call).

My sense is most code has to determine page unload status by listening to a number of different events (pagehide, visibilitychange, unload), so it's unlikely that changing one of them would cause breakage.

@igrigorik
Copy link
Member

Fair enough. I don't have any objections to making this change. That said, this is an update to the HTML spec, so I'll defer to the editors there.

@spanicker
Copy link

@domenic could we send a patch to update the ordering in the HTML spec? or do we need a new bug on html repo first?

@domenic
Copy link

domenic commented May 7, 2018

Sending a patch to HTML sounds great! It'll need publicly-expressed multi-implementer interest to land; you can gather that on the PR or in this issue. (Also it will need web platform tests.)

@spanicker
Copy link

@smaug--- @toddreifsteck - could you comment for implementer interest.
If there is agreement, I will make the change.

@spanicker
Copy link

spanicker commented May 15, 2018

(Oops, I had the wrong git handle for Olli)
@smaug---- any thoughts on this?

@smaug----
Copy link

smaug---- commented May 16, 2018

I think it should be fine, and looking at the Gecko implementation, the change should be easy https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/dom/base/nsDocument.cpp#8548,8561

But that code made me look at fullscreen spec, and perhaps we'll need to update that too "Whenever the unloading document cleanup steps run with a document, fully exit fullscreen document." That happens quite late during unload.

Basically someone needs to take a look at the whole unload algorithm https://html.spec.whatwg.org/multipage/browsing-the-web.html#unload-a-document and figure out what all needs to be changed and whether changes are actually feasible.

@spanicker
Copy link

Would it make sense to make the code change in Chrome (before spec change) and see if anything breaks, although I'm fine with proceeding with spec change based on comments in this thread.

@toddreifsteck toddreifsteck added this to the Level 3 milestone Aug 20, 2018
@toddreifsteck
Copy link
Member

@philipwalton Per WG call, Could you work with folks to move this issue to HTML5 spec as it tracks the ordering of events.

@philipwalton
Copy link
Member Author

Filed whatwg/html#3957.

domenic pushed a commit to whatwg/html that referenced this issue Sep 23, 2020
Part of #3957. See also discussion in
w3c/page-visibility#39.

Note that we're already running visibility change steps before firing pageshow,
so after this change we have up-to-date visibility when either pagehide or
pageshow is fired.
mfreed7 pushed a commit to mfreed7/html that referenced this issue Sep 25, 2020
Part of whatwg#3957. See also discussion in
w3c/page-visibility#39.

Note that we're already running visibility change steps before firing pageshow,
so after this change we have up-to-date visibility when either pagehide or
pageshow is fired.
mfreed7 pushed a commit to mfreed7/html that referenced this issue Sep 25, 2020
Part of whatwg#3957. See also discussion in
w3c/page-visibility#39.

Note that we're already running visibility change steps before firing pageshow,
so after this change we have up-to-date visibility when either pagehide or
pageshow is fired.
mfreed7 pushed a commit to mfreed7/html that referenced this issue Sep 25, 2020
Part of whatwg#3957. See also discussion in
w3c/page-visibility#39.

Note that we're already running visibility change steps before firing pageshow,
so after this change we have up-to-date visibility when either pagehide or
pageshow is fired.
@sideshowbarker
Copy link

Could this break any existing clients / analytics libraries?

My guess is this wouldn't break many (or any) sites or analytics libraries based on the fact that, until recently, not all browsers actually fired visibilitychange when the page was unloading (@toddreifsteck actually made this point when we discussed this issue on the last call).

But in the more-than-two-years since this issue was first raised, I think Gecko and Blink have for quite a while now been shipping with support for visibilitychange firing when the page was unloading. It’s true that WebKit has not — but in that case the fairly well-known workaround that’s been used is to have additional code to listen for the pagehide event.

Given that, I wonder what’s the current level of confidence among the Chrome team and Firefox team that this change isn’t going to break/regress existing sites and libraries?

My sense is most code has to determine page unload status by listening to a number of different events (pagehide, visibilitychange, unload)

OK, yes, it does seem likely that sites would be doing that if they needed to make things work across current browsers.

so it's unlikely that changing one of them would cause breakage

…but I think from the assumption that existing sites are listening to a number of different events, it doesn’t necessarily follow that changing the firing order of pagehide and visibilitychange wouldn’t break their code. I would think that whether or not it’ll break their code can in practice depend in part on exactly how they’re handling the conditional branching.

But regardless, it’s all just speculation unless there’s some plan to try to evaluate whether in fact the change breaks any existing sites. Given the nature of the change, it seems like there’s still a non-insignificant risk it could actually break some.

@rakina
Copy link
Member

rakina commented Sep 28, 2020

But in the more-than-two-years since this issue was first raised, I think Gecko and Blink have for quite a while now been shipping with support for visibilitychange firing when the page was unloading. It’s true that WebKit has not — but in that case the fairly well-known workaround that’s been used is to have additional code to listen for the pagehide event.
Given that, I wonder what’s the current level of confidence among the Chrome team and Firefox team that this change isn’t going to break/regress existing sites and libraries?

Looking at https://www.chromestatus.com/metrics/feature/timeline/popularity/196, it looks like the usage number hasn't changed much between 2018-2020. Other than that, it's already possible for visibilityState to be hidden and visibilitychange to be fired before pagehide, if a tab is already backgrounded, so sites should already handle this case. And as you mentioned, Safari only very recently added visibilitychange support on navigations.

Anyways, it's probably a good idea to measure the impact by how often visibilityState is accessed within a pagehide handler. Do you have other suggestions on what might help here?

@sideshowbarker
Copy link

sideshowbarker commented Sep 28, 2020

Looking at https://www.chromestatus.com/metrics/feature/timeline/popularity/196, it looks like the usage number hasn't changed much between 2018-2020.

ah OK yeah, that’s good data to know about

Other than that, it's already possible for visibilityState to be hidden and visibilitychange to be fired before pagehide, if a tab is already backgrounded, so sites should already handle this case.

That makes sense, yeah

And as you mentioned, Safari only very recently added visibilitychange support on navigations.

Right. And it’s not actually shipped yet — not even in a Technology Preview release.

Anyways, it's probably a good idea to measure the impact by how often visibilityState is accessed within a pagehide handler. Do you have other suggestions on what might help here?

I don’t have any other specific suggestions — but if others with more insight than me have ideas that would require somebody spending time — whether it’s manual searching of some kind, or writing some tooling to help with — I’d certainly be willing to put time into it needed to do those kind of things.

sideshowbarker added a commit to whatwg/html that referenced this issue Oct 1, 2020
This change reverts 386360c (#3957).
Specifically, it reorders two steps in the “unload a document” algorithm such
that the spec once again requires UAs to wait to run any “unloading document
visibility change steps” from other applicable specifications (in particular,
the Page Visibility spec) until *after* firing the pagehide event.

In practice, in combination with requirements in the Page Visibility spec,
this change has the effect of stating that UAs must wait to fire the
visibilitychange event until after the pagehide event has fired. That
ordering matches the behavior current implemented in all engines.

Otherwise, without this change, the ordering of firing for the events
doesn’t match what implementations actually do.

See also w3c/page-visibility#39
See also #5949 (comment)
annevk pushed a commit to whatwg/html that referenced this issue Oct 1, 2020
This change reverts 386360c (#3957).
Specifically, it reorders two steps in the “unload a document” algorithm such
that the spec once again requires UAs to wait to run any “unloading document
visibility change steps” from other applicable specifications (in particular,
the Page Visibility spec) until *after* firing the pagehide event.

In practice, in combination with requirements in the Page Visibility spec,
this change has the effect of stating that UAs must wait to fire the
visibilitychange event until after the pagehide event has fired. That
ordering matches the behavior current implemented in all engines.

Otherwise, without this change, the ordering of firing for the events
doesn’t match what implementations actually do.

See also:
* w3c/page-visibility#39
* #5949 (comment)
mfreed7 pushed a commit to mfreed7/html that referenced this issue Oct 2, 2020
This change reverts 386360c (whatwg#3957).
Specifically, it reorders two steps in the “unload a document” algorithm such
that the spec once again requires UAs to wait to run any “unloading document
visibility change steps” from other applicable specifications (in particular,
the Page Visibility spec) until *after* firing the pagehide event.

In practice, in combination with requirements in the Page Visibility spec,
this change has the effect of stating that UAs must wait to fire the
visibilitychange event until after the pagehide event has fired. That
ordering matches the behavior current implemented in all engines.

Otherwise, without this change, the ordering of firing for the events
doesn’t match what implementations actually do.

See also:
* w3c/page-visibility#39
* whatwg#5949 (comment)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants