track scroll position without scroll listener

[Rich-Harris]

Supersedes #3936 (which stores scroll state in history.state) and (I think) #3873.

Instead of calling replaceState frequently as a result of scrolling, this updates scroll position when we navigate or leave the page. It does so by storing scroll_positions in memory and persisting it to sessionStorage in beforeunload (which allows us to recover scroll positions when the page is reloaded without needing to manipulate history).

Contra #3936, I think it may be possible to add tests that use page.reload() — will investigate. Added a test, which makes me feel much more comfortable about this change.

Thanks to @PH4NTOMiki for pointing in the direction of using sessionStorage rather than history.state.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: f304f0d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[mrkishi]

It might be worth investigating if visibilitychange events with fallbacks are still preferred over plain beforeunload.

As for sessionStorage, what does it give us over history.state? I didn't catch the issues that was trying to solve, but browsers save history and scroll restoration across sessions natively, so maybe localStorage with some long-tail invalidation would match it better?

[Rich-Harris]

It might be worth investigating if visibilitychange events with fallbacks are still preferred over plain beforeunload.

Based on my reading of that article, I think we do want to use beforeunload, since the problem we're trying to solve is what happens when you do Cmd-W followed by Cmd-Shift-T. It looks like visibilitychange is unreliable for that.

As for sessionStorage, what does it give us over history.state?

Before this PR, we were replacing the current state after every scroll. Instead, we're now tracking scroll position when navigation occurs from a to b. In the case where navigation occurs as a result of an intercepted link click, history.state belongs to a, but when it occurs as a result of popstate it belongs to b instead — we would be replacing the wrong state. By using sessionStorage, we can reliably update the scroll state belonging to a.

[benmccann]

Nice! I think the explanation from your last comment could go in a code comment because I had the same exact question (#3873 (comment)) on the PR that inspired this one

[mrkishi]

Based on my reading of that article, I think we do want to use beforeunload, since the problem we're trying to solve is what happens when you do Cmd-W followed by Cmd-Shift-T. It looks like visibilitychange is unreliable for that.

I've looked a bit more and those marked as (bug) were eventually fixed, and the spec has been updated to reinforce the behavior, so unless other bugs have been reintroduced that should still be more reliable on browsers from 2020 onwards (when the fix landed in webkit). I mentioned fallbacks for legacy browsers, but that could be a differential builds concern.

For what it's worth, I just tested locally and it was triggered apparently reliably, although I don't have access to many devices (win, android) and didn't check all use cases (I don't know all the different ways to close/sleep an app on Android, for instance).

In the case where navigation occurs as a result of an intercepted link click, history.state belongs to a, but when it occurs as a result of popstate it belongs to b instead — we would be replacing the wrong state.

I see, that makes sense, then!

e: To clarify, it makes sense beforeunload is required, but an added visibilitychange to save the scroll position on mobile would be nice.

[Rich-Harris]

I'll confess I don't totally understand what the difference is — what cases would visibilitychange catch that beforeunload wouldn't?

[Rich-Harris]

I added an explanatory comment in d538ac6

[mrkishi]

I'll confess I don't totally understand what the difference is — what cases would visibilitychange catch that beforeunload wouldn't?

Basically situations where kill browser happens after switching away from a page—or, well, regular mobile usage. beforeunload isn't triggered when you switch away from a browser since the page's still in ram and you can come back. Last I checked, however, killing the browser afterwards (eg. from the task switcher or some device health monitor) won't guarantee beforeunload gets called.

Force-quitting might be rare on a desktop, but it's surprisingly common on battery-restricted devices, though I'm not sure how aggressive devices/os versions are on average.

[PH4NTOMiki]

Awesome stuff @Rich-Harris definitely supersedes my PR, much cleaner than my PR, actually one of my first commits in that PR was using variable approach(like you are doing in this PR, and Remix is doing almost the same) and only storing the states in beforeunload, but I made it too complicated, so I dropped the idea, and went for the individual stores(like Next.js does),
Few things I want to point out:

1. You should call scroll_positions[this.current_history_index] = scroll_state(); in that hash if block(where we are setting hash_navigation = true) before or after that, because that is specific case, it is navigation, but it's not being handled by _navigate method(the only place you're saving scroll state, other than beforeunload but that's irrelevant to that.)
2. I think you should move that const scroll_positions = JSON.parse(sessionStorage.getItem(SCROLL_KEY) || '{}'); that's currently outside the Router, to the inside of it's constructor, because it's safer, and we lose nothing by doing it.
3. We should wrap those 2 calls to sessionStorage (in beforeunload and that getItem) with try/catch block, that fails 1 in a million, but still why not do it.
4. I don't know why you're doing that history.scrollRestoration = 'auto'; inside beforeunload block. It's kinda pointless since we're storing value to return to, and we are doing it(line 76-77).
5. You should add a type def for scroll_positions something like this /** @type {{[key: string]: {x: number, y: number}}} */
6. You don't need that load event that sets history.scrollRestoration = 'manual', it's a duplicate.
7. You should move that if ('scrollRestoration' in history) {history.scrollRestoration = 'manual';} from init_listeners to Router construuctor so it runs as soon as possible.

[Rich-Harris]

Thanks, I made some updates:

• now using visibilitychange
• updating scroll positions when clicking a hash link
• put sessionStorage read/write inside try-catch
• added types

Some of the other stuff is deliberate. Resetting scrollRestoration to auto in theory means that scroll will be recovered on page reload before hydration kicks in. I haven't been able to verify that it actually works, but as in other cases related to scroll handling I think a conservative approach is preferable.

Setting scrollRestoration to manual should happen inside init_listeners, because we don't want to use it on pages that don't use the client-side router.