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

Should hidden fire on unload if page is visible when unloaded? #18

Closed
toddreifsteck opened this Issue Oct 27, 2015 · 16 comments

Comments

Projects
None yet
6 participants
@toddreifsteck
Member

toddreifsteck commented Oct 27, 2015

@igrigorik has done some testing demonstrating that Firefox fires the hidden event when a visible page is unloaded. The spec should be cleaned up to clarify whether this behavior is expected.

@igrigorik

This comment has been minimized.

Member

igrigorik commented Nov 5, 2015

For the record, prior to landing #16 we had some non-normative text for hidden that stated:

As examples, on getting, the hidden attribute returns true when:

  • ...
  • The user agent is about to unload the page.

We did not explicitly indicate that the user agent must fire a visibilitychange event when page is being unloaded, and unloaded state was marked as optional. As such, I'm guessing FF's implementation decided to skip unloaded but wired up visibilitychange event to fire hidden as suggested by the non-normative text. In my testing...

image

  • FF is the only implementation that fires visibilitychange on unload.
  • Safari has a bug (I believe) where pagehide does not fire when active desktop tab is closed.

In terms of where we are and next steps:

Q: is there a concrete use case to fire visibilitychange -> hidden on unload?

Practically, the combination of visibilitychange and pagehide covers all the cases I can think of and already has good cross-browser support (modulo Safari bug on pagehide)...

  1. Developers should subscribe to visiblitychange to account for background tab/app scenarios
    • once the tab/app goes into background pagehide (and any other event) is not guaranteed to fire
  2. Developers should subscribe to pagehide to account for navigation scenarios
    • if you get pagehide the visiblitychange is implicit (and redundant, I think?)

As such, I propose we stay with what we have currently and do not fire visibilityChange on unload. The current FF behavior does not break anything, and perhaps that's something that can be removed to avoid any confusion moving forward? This also allows us to stay with PV1 and avoid revving the spec, etc..

/cc @annevk @bzbarsky @eliperelman @ojanvafai @toddreifsteck @plehegar @slightlyoff

@bzbarsky

This comment has been minimized.

bzbarsky commented Nov 6, 2015

Practically, the combination of visibilitychange and pagehide covers all the cases

That's true, but now you have to watch two separate events to figure out whether you're visible, no? I don't know why you're thinking about his in terms of "scenarios" when it's really about knowing whether you're visible, whatever the reasons.

I'm going to stop wasting my time with this because this is an exact rehash of a discussion we already had, already decided years ago, and I really don't think it's useful to have again. As far as I can tell #16 just randomly changed long-agreed-on behavior for reasons that escape me. But just to summarize those old conclusions:

  1. The visibilityState should become some variant of hidden when unloading, and then go back to visible again when showing the same document again.
  2. It's pretty bizarre to change visibilityState without firing the corresponding change event.

It sounds like you disagree with one or both of them. I disagree with you, in that case. But as I said, I'm washing my hands of this whole mess, because it's just been a huge waste of my time all along.

@bzbarsky

This comment has been minimized.

bzbarsky commented Nov 6, 2015

For the record, prior to landing #16 we had some non-normative text for hidden that stated:

For the record, what we have in http://www.w3.org/TR/2013/REC-page-visibility-20131029/ is normative steps in http://www.w3.org/TR/2013/REC-page-visibility-20131029/#sec-processing-model that clearly describe the behavior that Firefox implements.

@igrigorik

This comment has been minimized.

Member

igrigorik commented Nov 11, 2015

A couple of different threads here, let me try to unpack...

  1. You're right, the "if the user agent is to unload the Document, run the now hidden algorithm" language was there in the 2013 REC document, and has carried over into the newest draft as well (step 3). From what I can see, only FF has implemented this behavior -- wonder why that's the case?

  2. The unloaded state in visibilityState enum was marked as optional and in my testing has not been implemented by any browser. We dropped that in #16.


Practically, the combination of visibilitychange and pagehide covers all the cases

That's true, but now you have to watch two separate events to figure out whether you're visible, no? I don't know why you're thinking about his in terms of "scenarios" when it's really about knowing whether you're visible, whatever the reasons.

I can imagine that some developers might want to distinguish between page going into background and unloading the document, which is why, by the looks of it, we kept unloaded but marked it as optional. But given that nobody has implemented (2), and the the fact that we're effectively duplicating pagehide/pageshow... it seems like (at best) a low priority and (at worst) an unnecessary feature?

As a developer I can subscribe to pagehide and observe unloads. pagehide is specced to fire before PV fires hidden. So it's easy enough to check if pagehide already fired in my visibility handler. Otherwise, my PV handler gets notifications for all other visibility transitions, which keeps everything nice and tidy.

The other major benefit here is that the above already works today... Which has to count for something :)

@bzbarsky

This comment has been minimized.

bzbarsky commented Nov 11, 2015

wonder why that's the case?

You're asking me? Or the people who didn't implement the agreed-on behavior in other browsers after we had all agreed on it?

I can imagine that some developers might want to distinguish between page going into background
and unloading the document

No, you have it backward. The use case I care about is that someone wants to know when a page goes into the background, whatever the reason: whether it's because the page is unloaded, or goes to a background tab, or minimized window, or background app on a phone, or whatever. What you're saying is that this person should have to register for two separate events to deal with this, and one more to then find out when the page comes out of the background. What I'm saying is that making someone register for three separate events when they could just register for one which already exists and is supposed to mean exactly the thing we care about is ridiculous.

I have no particular opinion on "unloaded". Marking unloaded documents as "hidden" is perfectly fine from my point of view for the actually important use case here: being told when a document stops being visible.

@bzbarsky

This comment has been minimized.

bzbarsky commented Nov 11, 2015

Or put another way, what you're saying is "Yeah, none of us except for you bothered to implement the spec we all agreed on, but there's a complicated workaround, so let's just make everyone else use the complicated workaround and change the spec to require inconsistent behavior so the workaround has to be used." Or you could, you know, try actually implementing the spec.

Maybe I'm missing something, but other than someone on your team having to spend a bit of time (and it's not very much time, speaking from personal experience) what are the drawbacks of implementing the provisions of the 2013 REC, exactly?

@igrigorik

This comment has been minimized.

Member

igrigorik commented Nov 11, 2015

I'm not against firing visibility change (to hidden) when unloading -- and per spec, we should be! My hunch is that many developers will end up also subscribing to pagehide because they'll want to differentiate unloads and transition-to-background states.. as such, they have to deal with two different events anyway, and this is really a debate over convenience and not enabling new or critical functionality. With that in mind, I'm happy to be convinced either way.. but from a purely practical standpoint (for developers and UAs), we already have a workable solution.

@toddreifsteck @rniwa any thoughts on this from your side?

@bzbarsky

This comment has been minimized.

bzbarsky commented Nov 11, 2015

because they'll want to differentiate unloads and transition-to-background states

This seems to be the fundamental assumption we disagree on, right. I think it's reasonably common to not care about the difference.

@toddreifsteck

This comment has been minimized.

Member

toddreifsteck commented Nov 11, 2015

Per call on 11/11/2015, the hidden state was shipped as part of the spec and all UAs should be firing event on the change of visibility. We'll add a note to spec to ensure this is clear to all readers.

@igrigorik

This comment has been minimized.

Member

igrigorik commented Nov 12, 2015

I think this should do the trick: #19 -- nothing new there, just adding explicit callout for visibilityState to report hidden when unloading the doc.

With above in place, I believe we can resolve this issue? Well, modulo everyone implementing the visibility transition on unload.. :)

@igrigorik

This comment has been minimized.

Member

igrigorik commented Nov 12, 2015

@toddreifsteck

This comment has been minimized.

Member

toddreifsteck commented Nov 12, 2015

I've opened a bug at Microsoft to track add the change to hidden state and ensuring the visbilitychange event is fired on unload of a visible page. @rniwa or @yoavweiss may want to create a Webkit bug for this.

As we've addressed the original spec concerns and merged Ilya's pull, I'm closing this issue.

@ojanvafai

This comment has been minimized.

ojanvafai commented Nov 13, 2015

Glad this resolved the way it did. Some extra context on why for the benefit of future folks reading this issue.

Authors should never try to distinguish between visibility-change:hidden and unload/pagehide. Specifically, on iOS/Android, we cannot implement unload/pagehide in a number of scenarios (as per igrigorik's data above), so any code that uses unload/pagehide will be fundamentally broken. The only way to have a reliable system is to listen to visibility-change:hidden, but handle the fact that you can get multiple of those for a give page.

The core insight is that, in practice, browsers and OSes never kill visible apps/pages, so visibility-change is reliable (modulo crash bugs).

@ojanvafai

This comment has been minimized.

ojanvafai commented Nov 13, 2015

@matthiasg

This comment has been minimized.

matthiasg commented Nov 25, 2015

@toddreifsteck too sad that the bug tracking of Edge is not in the open like Chromium and WebKit are ..

@jfsiii

This comment has been minimized.

jfsiii commented Apr 6, 2016

@toddreifsteck Is the ticket you created accessible on the public Edge bug tracker?

If so, could you add the link here so we can track it? If not, could you add one? I see that the Page Visibility API is supported but it'd be great to know when this specific behavior is implemented /cc @matthiasg

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