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

Binding scrollY to svelte:window causes implicit scrolling to the top of the page #11623

Closed
Serator opened this issue May 14, 2024 · 6 comments · Fixed by #11630 or #11802
Closed

Binding scrollY to svelte:window causes implicit scrolling to the top of the page #11623

Serator opened this issue May 14, 2024 · 6 comments · Fixed by #11630 or #11802
Labels
Milestone

Comments

@Serator
Copy link

Serator commented May 14, 2024

Describe the bug

In the example from the video, I scroll and then refresh the page. In the case where scrollY is bound to svelte:window, the page scrolls to the beginning after refresh. If no binding is done, the page does not scroll.

Most likely the same thing happens with scrollX.

iShot_2024-05-15_00.44.03.mp4

Reproduction

https://stackblitz.com/edit/vitejs-vite-8xmzpm?file=src%2Froutes%2F%2Bpage.svelte

Logs

No response

System Info

"svelte": "^5.0.0-next.123"

Severity

annoyance

@dummdidumm
Copy link
Member

This is already fixed in the latest version of 5.0-next, make sure to update the version.

@dummdidumm dummdidumm closed this as not planned Won't fix, can't repro, duplicate, stale May 15, 2024
@Serator
Copy link
Author

Serator commented May 15, 2024

@dummdidumm Can you tell me if this change will be reflected in Svelte 4? If not, it might be worth mentioning it in #11400. Thanks!

@dummdidumm
Copy link
Member

Does the bug even exist in Svelte 4?

@Serator
Copy link
Author

Serator commented May 15, 2024

@dummdidumm https://stackblitz.com/edit/vitejs-vite-f8dfha?file=src%2Froutes%2F%2Bpage.svelte - no, you're right, there is no bug in Svelte 4.

@Serator
Copy link
Author

Serator commented May 15, 2024

@dummdidumm But I found another related bug:

  • Svelte 5 (runes) - initial value of the bound scrollY is undefined. 🔴
  • Svelte 5 (no runes) - initial value of the bound scrollY is undefined. 🔴
  • Svelte 4 - initial value of the bound scrollY is undefined, then 0. 🟢

dummdidumm added a commit that referenced this issue May 16, 2024
- we were scrolling to the given value, which we shouldn't for accessibility reasons (Svelte 4 didn't do it either)
- we need to notify of the value 0 if there's no scroll (#11623 (comment))
Rich-Harris pushed a commit that referenced this issue May 16, 2024
- we were scrolling to the given value, which we shouldn't for accessibility reasons (Svelte 4 didn't do it either)
- we need to notify of the value 0 if there's no scroll (#11623 (comment))
@Serator
Copy link
Author

Serator commented May 17, 2024

@dummdidumm I found the issue again in my app. The example this time is a little more complicated:

Any deletion fixes the issue (e.g. if you remove class="relative grid overflow-clip" / <div bind:contentRect></div> / <svelte:window bind:scrollY /> / scroll-behavior: smooth;...).

Svelte 5.0.0-next.135 🟢 - when refreshing, the page scrolls smoothly from the beginning to the position where it was before the refresh.

iShot_2024-05-17_17.11.02.mp4

Svelte 5.0.0-next.136 🔴 - when refreshing, the page instantly scrolls to the beginning and stays in that position.

iShot_2024-05-17_17.13.59.mp4

@dummdidumm dummdidumm reopened this May 17, 2024
@dummdidumm dummdidumm added this to the 5.0 milestone May 17, 2024
@dummdidumm dummdidumm added the bug label May 17, 2024
@dummdidumm dummdidumm added awaiting submitter needs a reproduction, or clarification and removed awaiting submitter needs a reproduction, or clarification labels May 28, 2024
@dummdidumm dummdidumm modified the milestones: 5.0, 5.x May 28, 2024
dummdidumm added a commit that referenced this issue May 28, 2024
- the previous assumption was wrong: browser don't fire a scroll event initially when the scroll isn't smooth
- the previous logic wasn't using the "is scrolling now" logic which meant the render effect fired immediately after, causing smooth scrolling to start too late to be overridden

adjusted the comment and reused the scroll handler function to guard against the race condition
fixes #11623
trueadm pushed a commit that referenced this issue May 28, 2024
…11802)

- the previous assumption was wrong: browser don't fire a scroll event initially when the scroll isn't smooth
- the previous logic wasn't using the "is scrolling now" logic which meant the render effect fired immediately after, causing smooth scrolling to start too late to be overridden

adjusted the comment and reused the scroll handler function to guard against the race condition
fixes #11623
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants