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

fix: avoid using the problematic beforeunload event

Merged
merged 14 commits into from Mar 11, 2020

Conversation

hedgepigdaniel
Copy link
Contributor

@hedgepigdaniel hedgepigdaniel commented Dec 18, 2019

Use of the beforeunload event is problematic because when it is used, browsers generally avoid using the back/forward cache or freezing the page, because the (historically assumed) semantics of the beforeunload event are incompatible with a page unloading and then loading again. The unload event also has this problem, and is not triggered reliably on mobile browsers.

So currently, any site that uses scroll-behavior and instantiates it at least once cannot be cached in a browsers back/forward cache, and cannot be frozen to save memory.

See https://developers.google.com/web/updates/2018/07/page-lifecycle-api for details

The pagehide and pageshow events don't suffer from this problem and are well supported. To use them we must account for the possibility that an unloaded page might be loaded again (e.g. if it was frozen, or put in a browsers back/forward cache).

@taion
Copy link
Owner

taion commented Dec 24, 2019

Huh – from the document, it seems like pagehide -> freeze -> resume without going through pageshow again is a possible sequence of events. Should we just look for event.persisted on the pagehide event?

Seems like it might be more robust to listen for pagevisibility.

@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Dec 24, 2019

It looks like that, but my assumption (given the description for transition i: "system freezes the page to conserve CPU") is that the only time a page can be unfrozen without the pageshow event is if it was also hidden without the pagehide event, and the tab is still open. If the user closed the tab or similar, I'd hope that the page would defrost in order to do the pagehide event...

I don't think using the persisted property of the pagehide event would work, because even if the page is persisted when it is frozen, that does not guarantee that it will ever be unfrozen again (e.g. if the user clears their cache).

I agree though, the visibility is probably a better way to do it, given the description of the hidden state:

This means you should treat the hidden state as the likely end to the user's session. In other words, persist any unsaved application state and send any unsent analytics data.

You should also stop making UI updates (since they won't be seen by the user), and you should stop any tasks that a user wouldn't want running in the background.

My only hesitation with that is that I think in the hidden state JS still runs, so in theory the user might be running code that scrolls the page (even though the user can't see it), and expecting scroll-behavior to work normally.

If you're happy with that compromise I'll change to to use the pagevisibility events.

@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Dec 24, 2019

My guess would be that using the pageshow/pagehide events would behave similarly to the unload event in the sense that it would fail to clean up in the same situations (e.g. mobile browsers), while not breaking back/forward caches. The page visibility events would make the cleanup more reliable at the expense of making scroll restoration work incorrectly if the app actively scrolled the page when the tab was hidden.

@taion
Copy link
Owner

taion commented Jan 11, 2020

Ah, shoot, forgot I had to reply to this.

Hmm, the other thing I'm concerned about is the freeze -> DISCARDED -> reloaded flow, which seems like it might be possible?

Perhaps the easy answer is to just listen to freeze and resume as well? The page can't scroll if it's frozen, right?

@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Jan 12, 2020

Yeah, I think you're right. I've changed it to use page-lifecycle to detect the transitions instead of manually listening to the events. This is what is recommended in that documentation, and it means we don't have to worry about browser differences (e.g. the freeze event not working on browsers other than chrome afaik).

@@ -6,6 +6,7 @@ import scrollLeft from 'dom-helpers/query/scrollLeft';
import scrollTop from 'dom-helpers/query/scrollTop';
import requestAnimationFrame from 'dom-helpers/util/requestAnimationFrame';
import invariant from 'invariant';
import PageLifecycle from 'page-lifecycle/dist/lifecycle.es5';
Copy link
Contributor Author

@hedgepigdaniel hedgepigdaniel Jan 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test/ScrollBehavior.test.js Show resolved Hide resolved
@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Jan 12, 2020

I'm a bit stumped on the test failures - I can't seem to increase the timeout. don't think the error has anything to do with the changes.

Copy link
Owner

@taion taion left a comment

hmm, the regular dependabot CI is passing, though. i'm not sure what it could be other than some oddness with the polyfill?

do things fail if we just remove the beforeunload handler, i.e. were things depending on bad side effects of having installed that handler?

src/index.js Show resolved Hide resolved
}
this._hasSetScrollRestoration = true;
};

_restoreScrollRestoration = () => {
/* istanbul ignore if: not supported by any browsers on Travis */
if (this._oldScrollRestoration) {
Copy link
Owner

@taion taion Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so just bail if this._hasSetScrollRestoration is false?

Copy link
Contributor Author

@hedgepigdaniel hedgepigdaniel Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ive replaced this with just _oldScrollRestoration as you suggested - does that look good?

src/index.js Outdated Show resolved Hide resolved
@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Feb 14, 2020

Tests were failing to to various things - should be fixed now :)

@taion taion merged commit 6a7a8b4 into taion:master Mar 11, 2020
1 check passed
@taion
Copy link
Owner

taion commented Mar 11, 2020

really sorry for the delay here; it should not have taken me so long

@taion
Copy link
Owner

taion commented Mar 11, 2020

published as v0.10.0

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

Successfully merging this pull request may close these issues.

None yet

2 participants