whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.05k stars 2.64k forks source link

Clarify cases where we would fire the "unload" event, or maybe deprecate it? #6026

Open rakina opened 3 years ago

rakina commented 3 years ago

Currently it seems like there are two separate meanings for "unload" in the spec:

The first definition means that the Document does not have to be destroyed if it's unloaded, allowing cases where the back-forward cache keeps the Document alive after navigation so that the Document can be reused on back navigation, etc.

The unload event, however, does not fire every time we "unload" (using the first definition), and instead only fires whenever the Document turns out to be no longer salvageable, meaning it will get destroyed later. Even using the second definition, though, it's not always fired: notable examples include when we discard a browsing context (per https://github.com/whatwg/html/issues/4611 this is not true in practice though, so this might be a separate issue), and when a Document gets destroyed while it's not the "active document" (e.g. when it's saved in bfcache but later destroyed due to memory pressure/timeout/other reasons)

So I guess the main problem here is just that the unload event has a misleading name, as it won't always fire whenever we're "unloading" as defined by the first definition (aka whenever the term "unload" is used in the spec). Also, we already have the pagehide event, which will always be fired whenever we're unloading and it covers all cases when the unload event is fired (+ more!). This means all unload handlers should be able to be changed into a pagehide handler.

Considering the above points, I wonder if it makes sense to deprecate the unload event (this does not mean the browsers will stop firing the event entirely immediately - ~66% of page loads still uses unload handlers). This will remove any confusion of the term "unload".

If that seems too drastic, I guess we can just make the different meanings very clear? (and maybe MDN, etc. can help clarify cases where the unload event will fire vs not).

Relevant issues: https://github.com/whatwg/html/issues/5748, https://github.com/whatwg/html/issues/4611

cc @annevk @domenic @cdumez @fergald @altimin @smaug---- @mystor

annevk commented 3 years ago

I'd suggest that we approach this in a slightly different order:

  1. Solve #4611 with tests and possible specification changes so we're clear on when unload and pagehide fire in all circumstances.
  2. Stop reusing the term "unload" to mean different things. (Could perhaps be done in parallel with 1, not sure.)
  3. Recommend that developers use pagehide because of X/Y/Z. (If unload fires in a subset of cases I'm not sure it's a good idea to suggest a search & replace as sites might well rely on the differences.)
rakina commented 3 years ago

Solve #4611 with tests and possible specification changes so we're clear on when unload and pagehide fire in all circumstances.

Sounds reasonable. I have a simple test for iframe unload/pagehide here. That issue seems to talk about other things such as relation with document.open() and ordering with nulling the browsing context though, which I'm not familiar about and need their own tests. Anyways, if my current WPT looks useful, I'll send a PR in the WPT repo.

Stop reusing the term "unload" to mean different things. (Could perhaps be done in parallel with 1, not sure.)

It looks like the only "wrong" usage currently is just for the unload event. Should we add a note or something in the spec for this?

Recommend that developers use pagehide because of X/Y/Z. (If unload fires in a subset of cases I'm not sure it's a good idea to suggest a search & replace as sites might well rely on the differences.)

I think we can suggest a pretty simple replacement strategy to web authors. If they are interested in all cases of Document replacement/navigation/removal (the first definition of "unload" that I mentioned), then they should just change their unload event listeners to pagehide event listeners. If they are interested only in the original "unload" cases (the second definition, where the Document will actually get destroyed) they can change their unload event listeners to pagehide event listeners, but filter out cases where the persisted bit is true.

Do you think that's reasonable? Where's the best place to put these recommendations?


Also, two things that I realized I forgot to include in my original post:

The first point is the weirdest one, not sure what we can do about it though :(

annevk commented 3 years ago

Are browsers consistent on the iframe removal case? If so, it's probably worth adding a test. (I'd suggest trying to capture the event order at least and perhaps even see if they are all in the same task (and no timers can get inbetween).) If the nested document had its own nested document (or even more) that would allow testing the recursion a bit better.

With a recommendation to web authors I'm mainly worried that we'd be doing it without knowing about all cases.

As for naming, maybe it's best if we stop using "unload" altogether and only have it for the names of the events. (Or rename it to the page hide algorithm.) And explain with notes when they fire in relation to other things.