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

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

Open
rakina opened this issue Oct 5, 2020 · 3 comments
Open

Comments

@rakina
Copy link
Member

rakina commented Oct 5, 2020

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 #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: #5748, #4611

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

@annevk
Copy link
Member

annevk commented Oct 5, 2020

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

  1. Solve Spec for iframe removal from the document doesn't match browser behavior #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
Copy link
Member Author

rakina commented Oct 12, 2020

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:

  • We have beforeunload which uses the first definition of "unload", unlike the unload event.
  • There's an additional case where unload won't be fired even when we will destroy the Document: e.g. when a page initially gets bfcached (we successfully navigate away from it), but due to some reason it gets dropped (due to memory pressure, etc).

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

@annevk
Copy link
Member

annevk commented Oct 12, 2020

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.

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

No branches or pull requests

2 participants