-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
scroll restoration overhaul #3873
Conversation
🦋 Changeset detectedLatest commit: 0b9e769 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
✔️ Deploy Preview for kit-demo canceled. 🔨 Explore the source changes: 8d44831 🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/620bf7f3a2c8200008f9da89 |
Afraid I'm seeing some wildly inconsistent behaviour here: Screen.Recording.2022-02-12.at.3.55.18.PM.mov |
// need this in case it was set in previous state | ||
if (new_state['sveltekit:scroll'].x === 0 && new_state['sveltekit:scroll'].y === 0) { | ||
delete new_state['sveltekit:scroll']; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand the purpose of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if an x and y coordinates are 0, that's the default position so we don't need to store it.
@Rich-Harris @benmccann we have a decision to make, we are currently decision-less about this thing: if (hash !== undefined && base === location.href.split('#')[0]) {
event.preventDefault();
this.save_scroll_state();
const info = this.parse(url);
if (info) {
await this.renderer.update(info, [], false);
location.hash = hash; // this will automatically pushState and set the state to null and then trigger hashchange event
this.save_scroll_state(); // by calling this method, it will also replaceState with object instead of null, we could also use history.replaceState({}, '', url.href);
}
return;
} or even simple(but without re-rendering): if (hash !== undefined && base === location.href.split('#')[0]) {
event.preventDefault();
this.save_scroll_state();
const info = this.parse(url);
if (info) {
location.hash = hash; // this will automatically pushState and set the state to null and then trigger hashchange event
this.save_scroll_state(); // by calling this method, it will also replaceState with object instead of null, we could also use history.replaceState({}, '', url.href);
}
return;
} |
1@Rich-Harris also scrolling test are wrong, at least about hash handling, they go success but they should fail, also I'll implement the fix for scrolling now(option B) and remove the listener tomorrow, because it's more complicated than I originally though, and It's 10pm where I live (London time + 1 :) ) so I'm tired now. I'll implement it tomorrow. |
Can you update the PR title and description to reflect what this change does? It's hard to understand the context for it right now since I don't think it removes the need for the scroll event listener and the description is unclear as to whether it fixes the duplicate history entry |
// Call `pushState` to add url to history so going back works. | ||
// Also make a delay, otherwise the browser default behaviour would not kick in | ||
setTimeout(() => history.pushState({}, '', url.href)); | ||
event.preventDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would disable the focus management done by the browser. Making #3105 a problem again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh never mind. It's handled by hash setter and I tested with the wrong commit. But I notice the focus test is wrong. It actually won't fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll deal with it. thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean this one
kit/packages/kit/test/apps/basics/test/test.js
Line 1732 in 2fefa5e
test('focus works when navigating to a hash on the same page', async ({ page }) => { |
There should be another focusable element between <p id="p1">
and <p id="p2">
here
<p id="p2">paragraph 2</p> |
I think it's somewhat unrelated to this PR. So you don't have to deal with it. I could also create another PR for this later.
@Rich-Harris fixed, please test now, it should work in Chrome/Edge and FF |
Thanks. It turns out I can get the test to pass with just the most recent change, so I've opened a separate PR with just that: #3931 Is there a separate issue that this PR is also solving? Given how volatile scroll handling has turned out to be, I'd favour a cautious (and test-driven) approach to making changes |
Yeah, to be honest I thought of that as well, but this solution is much safer, than relaying on the scroll event, both Remix(https://github.com/remix-run/remix/blob/1fd70960e4d88740df5bf407a6ba2cd2b9549459/packages/remix-react/scroll-restoration.tsx) and Next.js(https://github.com/vercel/next.js/blob/81b537c04e694930a2fb9e6870ba9c5805269683/packages/next/shared/lib/router/router.ts) are using a solution similar to this. I based this solution on theirs. because scroll event is pretty bad to rely on, and on low powered devices(and Svelte is great for those) scroll event can cause throttling(lagging) of scrolling. I've seen it happen many many times. So consider this, but that PR is okay too, if that's what you want. |
In this PR we are fully using our own scroll handling, so in the future if we decide to add some scroll behavior that's more complicated, we can do it with PR, but not with that with scroll event listener. |
and there is also that Ctrl+shift+t bug that's not present in this PR, I know it's not that big of deal, but still, one of many upsides of this PR comparing to that one. |
For the sake of the git history I think what I'll do is merge #3931 first, so that this PR becomes purely about eliminating the scroll handler and the ctrl+shift+t bug (which we'd ideally have a test for — not yet sure if that's possible with playwright) |
Can we add a comment explaining why the |
|
#3936 addresses the Cmd-Shift-T bug separately from everything else |
Ignore #3936, I've opened #3938 which supersedes it. I think that PR covers everything here, but more simply — as far as I can see we don't need to track |
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpx changeset
and following the prompts. All changesets should bepatch
until SvelteKit 1.0Should fix double recording navigation issue(still checking on that) and removes the need for
scroll
event listener.Related to #3835