-
-
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
[fix] remove scroll handling code #3835
Conversation
🦋 Changeset detectedLatest commit: 24333aa 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: 24333aa 🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/62070409234d8f0007066705 |
21f2341
to
dc69d32
Compare
dc69d32
to
29f70e1
Compare
29f70e1
to
507b9a7
Compare
507b9a7
to
2f7fe45
Compare
2f7fe45
to
480754a
Compare
480754a
to
8e95071
Compare
Would this allow us to close #3629 as well? |
@Rich-Harris I don't think so, I think it works only for navigating forward/back in history, not for navigating forward with pushState |
8e95071
to
7541847
Compare
7541847
to
e7b85ae
Compare
e7b85ae
to
5e5571d
Compare
5e5571d
to
24333aa
Compare
Unfortunately I'm not sure this works? If I take the default template and add a tall div to the index page with a link below it, clicking the link then returning to the index page resets the scroll: Screen.Recording.2022-02-12.at.9.06.33.AM.movWhat am I missing? |
It should work(on few of my vanilla web apps (SPAs) it works, and as @benmccann said, it works on React Router), maybe there is some left over code that sets |
Weird. @Rich-Harris, what browser are you testing in? |
Firefox |
But I'm seeing the same thing in Chrome |
I think I know the reason, it(browser) tries to scrollTo that position before the Kit has the time to render the page completely, and when it fails is stays on the top. |
It's possible that it's not possible to kick out the scroll handling code @benmccann |
Yeah, I just checked History API docs on Google Developers and this the reason, |
https://github.com/remix-run/remix/blob/1fd70960e4d88740df5bf407a6ba2cd2b9549459/packages/remix-react/scroll-restoration.tsx#L22 |
But I think that we can actually remove that |
That shouldn't be the case in this PR. The only |
No @benmccann I mean the internal scrollTo, not |
Ah, yeah, I think it's only working if you navigate to a longer page and then hit back, but if you navigate to a shorter page and then it back it won't work. That's why Remix requires you to put the But the react-router-hash-link demo somehow does not have this problem and I wonder why... |
I would guess it's all down to timing. If you're using vanilla React Router then changing routes is basically synchronous. If it's async then the relationship between scroll position and history state is presumably severed. I wonder if delaying |
I think it could be, but I'm currently working on removing need for scroll event, l think that's the step in the right direction, if it works, and I think we don't need to do this if that works out, because the scroll event listener is the biggest pain in the current approach. |
Svelte is synchronous on the server, right? But you're saying it's async on the client? Just curious if there's a reason for that?
I think that's unlikely to help because the |
#3873 check this out |
Navigating to a new route involves importing code and fetching data
Right, but if |
Hmm. Surely people must fetch data when using React Router as well. I assume their router would still work when doing so, but maybe not? It looks like Next does manual scroll restoration behind an experimental flag |
I started wondering why we need some of the scroll handling stuff and dug into what React Router does. It looks like they have zero scroll handling and rely on the browser to handle it. SvelteKit's scroll handling was originally inspired by Nuxt. I think that older browsers at some point before 2017 didn't have built-in scroll handling with
pushState
, but have since added it and so there's no longer a reason to include this codeFixes #3636
Fixes #3621
With this PR, the forward button seems to only scroll to the correct location for every other page. Though given that it fixes two more serious issues, that's probably something we can live with for now
I left
disableScrollHandling
for now as it wasn't clear to me that we can get rid of it. React Router does not handle deep links by default and you have to include another package for it, but I think it's better that we include it by default