sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.44k stars 1.89k forks source link

Scrolling to hash links is broken on Chrome #4216

Closed Rich-Harris closed 2 years ago

Rich-Harris commented 2 years ago

Describe the bug

Clicking a link like https://kit.svelte.dev/docs/configuration#version should take you to that part of the page. It works in Firefox...

image

...but not Chrome:

image

Reproduction

Go to https://kit.svelte.dev/docs/configuration#version in Chrome

Logs

No response

System Info

System:
    OS: macOS 12.0.1
    CPU: (10) arm64 Apple M1 Max
    Memory: 69.61 MB / 32.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.13.1 - ~/.nvm/versions/node/v16.13.1/bin/node
    Yarn: 1.22.17 - ~/.nvm/versions/node/v16.13.1/bin/yarn
    npm: 8.1.2 - ~/.nvm/versions/node/v16.13.1/bin/npm
  Browsers:
    Chrome: 98.0.4758.109
    Firefox: 97.0.1
    Safari: 15.1
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.30 
    @sveltejs/adapter-static: next => 1.0.0-next.28 
    @sveltejs/kit: next => 1.0.0-next.291 
    svelte: ^3.46.0 => 3.46.4

Severity

annoyance

Additional Information

No response

PH4NTOMiki commented 2 years ago

Testing fix now

mrkishi commented 2 years ago

You're never going to believe it, but it was introduced in #4155 somehow.

PH4NTOMiki commented 2 years ago

Makes sense, because it scrolls to the correct position temporary and then returns to 0

PH4NTOMiki commented 2 years ago

It's probably something to do with that clientHeight/Width bind, some Chrome edge-case/bug, but no idea how to fix it, maybe there is some other way to do that banner without that bind

Conduitry commented 2 years ago

Most of the more complicated stuff with the banner is no longer necessary with the shorter message on mobile that doesn't need to wrap. The whole thing should be able to be replaced with an HTML- and CSS-only banner that swaps out messages with media queries.

PH4NTOMiki commented 2 years ago

4219 it looks like it works.

PH4NTOMiki commented 2 years ago

https://kit-svelte-dev-aigplkaoi-svelte.vercel.app/docs/configuration#version

PH4NTOMiki commented 2 years ago

I think I actually figured the root cause of the issue

PH4NTOMiki commented 2 years ago

Closed #4219 it was just hiding the issue, need to investigate deeper.

Rich-Harris commented 2 years ago

Before this block of code, the page is correctly scrolled: https://github.com/sveltejs/kit/blob/cdc32a9f4f17d69be6ca1cae58bf3961155545a2/packages/kit/src/runtime/client/client.js#L330-L334

After, it's reset. So it's something to do with the hydration. Trying to create a minimal repro.

PH4NTOMiki commented 2 years ago

I think the browser tries to scroll, but the max scroll(full page height) changes slightly when hydrated(maybe just 1 pixel) and that causes it, but how to solve it is beyond me.

Rich-Harris commented 2 years ago

Ok I've managed to at least make a simpler repro: https://stackblitz.com/edit/sveltejs-kit-template-default-pm2sdq?file=src%2Froutes%2Ffails.svelte&terminal=dev

Visit https://sveltejs-kit-template-default-pm2sdq--3000.local.webcontainer.io/ and try going to works and fails. There's something about the combination of {@html ...} and bind:clientHeight that causes the scroll to be reset on hydration.

benzara-tahar commented 2 years ago

Just wanted to tell you guys, you are doing great job, I recently tried svelte (kit) and felt like am home 🚀 ( no jsx, simplicity, chrome lighthouse is very happy ..etc) I want to help with this but am still a noob, am learning a lot reading what you're doing guys. Keep it up.

And good luck with this scrolling bug 😊

Rich-Harris commented 2 years ago

Ok, so the bug isn't visible in the Svelte REPL for obvious reasons, but I think I have a basic understanding of why it's happening:

image

On line 32, in the l (cLaim) method, the children of the <main> are being detached. The HTML isn't put back until line 42 in the m (Mount) method. Between claim and mount, the addition of the resize listener is somehow causing Chrome to reset the scroll (presumably Firefox waits until work is complete before assessing whether it needs to). I'm not sure why innerHTML = html doesn't happen during claim. Will raise an issue on the Svelte repo.

So we have lots of fixes to choose from:

PH4NTOMiki commented 2 years ago

@Rich-Harris I'm on my phone now, but one solution I can think of is to save scroll position(inside client.js) to const before calling new Root and after it just do scrollTo(pos.x, pos.y)

I don't it's good to just fix it for the docs site because that bug could be happening on some other sites we don't know about. That's just one of the most simplest solutions but your solutions are definitely better.

Rich-Harris commented 2 years ago

Opened https://github.com/sveltejs/svelte/issues/7341

PH4NTOMiki commented 2 years ago

@Rich-Harris It's happening again for me on the docs site, for ex. link https://kit.svelte.dev/docs/routing#advanced-routing-fallthrough-routes you posted in https://github.com/sveltejs/kit/issues/4291#issue-1165916525 Interestingly enough, it happens most of the loads, but not always.

frederikhors commented 2 years ago

Interestingly enough, it happens most of the loads, but not always.

Same here! Crazy! 😄

PH4NTOMiki commented 2 years ago

@Rich-Harris any idea why this could be happening? because I have none, one workaround I can think of is to check if hash is present and scrollTop is 0, If both are true then we do document.getElementById(location.hash.slice(1)).scrollIntoView() or something like that in client.js but I understand it's bad design.

cliftonlabrum commented 2 years ago

Hash links are working fine for me in Chrome Version 99.0.4844.84 on a Mac. Am I supposed to do something in particular to reproduce the issue?

https://kit.svelte.dev/docs/configuration#version
HashLink
p-lau commented 2 years ago

It is working fine for me...

Version 1.37.109 Chromium: 100.0.4896.60 (Official Build) (64-bit)

winston0410 commented 2 years ago

Didn't work for me(Will attach the right version tomorrow)

henken commented 2 years ago

Works for me using Chrome Version 100.0.4896.75 (Official Build) (x86_64)

psytrx commented 2 years ago

Can not reproduce the behavior when default-clicking a link from another page (external or internal), i.e. the links in the Github comments above.

However, when middle-mouse-clicking, ctrl-clicking, or pasting the URL into a new tab, the scroll offset doesn't apply just as described in the issue. The same happens when I modify a link in this issue by adding target="_blank".

I'm not sure if that's supposed to work like that, due to some browser limitations or something, so I'm not sure that helps.

Version 1.37.111 Chromium: 100.0.4896.79 (Official Build) (64-bit)

moritzebeling commented 2 years ago

Used to be broken in Safari as well. But seems to work from version 324

nubpro commented 2 years ago

Faced this bug while I was learning sveltekit.

Click on this link Generated Types, it doesnt scroll to that position on Chrome. But sometimes it does, very finicky.

whataboutpereira commented 2 years ago

I might have happened on a related issue - I'm finding that Chrome 102.0.5005.63 doesn't scroll to hash when the page is doing an inertial scroll from scrolling with a mouse. Hash links work only when the page has completely stopped moving from inertial scrolling.

Scrolling when inertial movement is in effect seems to be working on SvelteKit docs site though.

dummdidumm commented 2 years ago

Can't reproduce this anymore, therefore closing this. The PR that would fix the underlying issue is https://github.com/sveltejs/svelte/pull/7426

nilslindemann commented 1 year ago

Please reopen, middleclicking the link does not scroll to the hash in recent chromium based browsers.