Make beforeunload not affect 'salvageable' & fire unload event only if document is no longer salvageable #5889
Conversation
There was a problem hiding this comment.
Thanks for working on this! @smaug---- could you maybe review this as well?
rakina
changed the title
|
Waiting for @smaug----'s review but here's the commit message (let me know if it needs more work): Make beforeunload not affect 'salvageable' & fire unload event only if document is no longer salvageable The existence of the beforeunload or unload event listener should not |
|
Pinging @smaug---- for review here. Maybe @cdumez wants to review this also, since this is updating the spec to allow Safari's behavior. (can't add @cdumez directly as a reviewer somehow) |
|
The commit message is confusing |
There was a problem hiding this comment.
Looks reasonable. Super scary and regression prone of course to not care about beforeunload or unload event listeners, but if other browsers have succeeded with that approach...
And beforeunload does become even more weird now that it may fire multiple times.
But not sure what else could be done.
|
Here is my draft commit message: I was about to merge, however, I'm unsure whether @rakina wants to finish up the comment thread at #5889 (comment) in this PR or a separate one. Also, I'm unsure whether this closes #5748, or just helps with it. Thoughts, @rakina? |
…true If we've dispatched the 'pagehide' event with the 'persisted' property set to true, we should not dispatch the unload event after that. This is because the 'persisted' property is set to the 'salvageable' status of the page, which also determines whether the unload event is fired. If 'salvageable' is true, 'persisted' will be true and 'unload' won't be dispatched. If 'salvageable' is false, 'persisted' will be false and 'unload' should be dispatched. Relevant spec PR: whatwg/html#5889 More context: https://groups.google.com/a/google.com/g/chrome-bfcache/c/L-ZreZDY4n0/m/jna_jQJkCQAJ Bug: 1110744 Change-Id: I54b44cfdcb6c2922ca57071d695df6c4c2d77d77 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2455510 Commit-Queue: Rakina Zata Amni <[email protected]> Reviewed-by: Kentaro Hara <[email protected]> Reviewed-by: Alexander Timin <[email protected]> Cr-Commit-Position: refs/heads/master@{#815898}
|
Thanks @domenic and @smaug----! The draft commit message LGTM, and I've removed the "fired unload" flag as well now per #5889 (comment). This should close #5748, as I think the remaining issues related to this part of the spec are tracked in #6026 and #5879. |
…onal Currently firing beforeunload on a document will set its 'salvageable' bit to false, which is not true in practice in some user agents. Also, firing the 'unload' event should be optional if the user agent decides that it will keep the document alive in a session history entry, to be used later.
…true If we've dispatched the 'pagehide' event with the 'persisted' property set to true, we should not dispatch the unload event after that. This is because the 'persisted' property is set to the 'salvageable' status of the page, which also determines whether the unload event is fired. If 'salvageable' is true, 'persisted' will be true and 'unload' won't be dispatched. If 'salvageable' is false, 'persisted' will be false and 'unload' should be dispatched. Relevant spec PR: whatwg/html#5889 More context: https://groups.google.com/a/google.com/g/chrome-bfcache/c/L-ZreZDY4n0/m/jna_jQJkCQAJ Bug: 1110744 Change-Id: I54b44cfdcb6c2922ca57071d695df6c4c2d77d77 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2455510 Commit-Queue: Rakina Zata Amni <[email protected]> Reviewed-by: Kentaro Hara <[email protected]> Reviewed-by: Alexander Timin <[email protected]> Cr-Commit-Position: refs/heads/master@{#815898} GitOrigin-RevId: 67d49f2adf8c7b9dbe6166b152a7901addb02d5c
As discussed in #5748 (comment), the existence of the
beforeunloadevent listener should not affect aDocument'ssalvageablebit. Also, if the user agent decides that it will keep aDocumentalive in a session history entry (i.e. preserving it in the back-forward cache), it should not dispatch theunloadevent./browsing-the-web.html ( diff )