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

Proposal: change the ordering of visibilitychange and pagehide when unloading a page #3957

Open
philipwalton opened this issue Aug 23, 2018 · 5 comments

Comments

@philipwalton
Copy link

(Moving the discussion from page-visibility #39 to here.)

The current ordering of pagehide and visibilitychange during a document unload complicates our proposed Lifecycle states/event flow because the ordering of lifecycle events can be different depending on whether the page was unloaded from the foreground or the background.

Given the assumption that the flow through Page Lifecycle states generally takes this path:

ACTIVE → PASSIVE → HIDDEN → TERMINATED/FROZEN

In the foreground case, if the visibilitychange event fires after the pagehide event (currently spec'ed behavior), then the implication is either:

  • The TERMINATED/FROZEN states can come before the HIDDEN state in some cases
  • The visibilitychange and pagehide events do not necessarily signal changes to the HIDDEN and TERMINATED/FROZEN states (respectively).

This complicates things for developers.

If we update the order in the spec, these complications go way and developers can rely on a consistent event firing order in all unload situations, which allows them to more easily follow best-practice guidance for the type of work to do in each state.

Furthermore, the new freeze event is required to run after the visibilitychange event when a document is unloading, and for future compatibility it would make sense that pagehide (persisted:true) fires immediately before freeze.

/cc @spanicker

@domenic
Copy link
Member

domenic commented Aug 23, 2018

Relevant implementer thoughts:

So the biggest TODO is the concerns around unload.

@rakina
Copy link
Member

rakina commented Sep 15, 2020

I recently looked at the unload a Document steps and this change sounds simple. We probably need some changes to the steps in the Page Visibility spec to not reference the "unload" event as it does right now, and instead refer to "unload a Document" which covers bfcache too (we won't fire the unload event in bfcache's case - yes, it is confusing and we'll probably rename "unload a Document" to something like "navigate away from Document" or something)

The fullscreen unload step that @smaug---- mentioned should probably be changed to mention unload a Document instead (also unless I'm missing something this seems orthogonal to the change we wanted for this issue, so can be fixed separately).

So the changes needed are:

  1. Swap the order of pagehide and "unloading document visibility change steps" in HTML spec.
  2. Rename "unload a Document" to a better name in HTML spec
  3. Change Page Visibility spec to refer to the newly renamed "unload a Document" steps
  4. Change fullscreen spec to refer to the newly renamed "unload a Document" steps

domenic pushed a commit 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.
@domenic
Copy link
Member

domenic commented Sep 23, 2020

Merged #5928! I'll leave this open to track items (2)-(4) in @rakina's list.

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
Contributor

We probably need some changes to the steps in the Page Visibility spec to not reference the "unload" event as it does right now, and instead refer to "unload a Document" which covers bfcache too

That sounds necessary. It came up when chatting with @cdumez about https://webkit.org/b/151234 — he said:

I am also firing the visibilitychange event when coming out of the back/forward cache. Not sure if this is spec’d but this makes sense and this is Firefox’s behavior

So I’d be happy to raise a PR or bug myself against the Page Visibility spec about this. Should I?

@rakina
Copy link
Member

rakina commented Sep 28, 2020

So I’d be happy to raise a PR or bug myself against the Page Visibility spec about this. Should I?

That would be great, thank you!

Although, I think the part that @cdumez talked about in that comment is not related to unload (it's about recovering a document on bfcache/history traversal) which is already covered by session history document visibility change steps

sideshowbarker added a commit 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 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 join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants