Skip to content

fix: iOS scroll-behavior: smooth breaks navigation scrolling - #8724

Closed
teemingc wants to merge 4 commits into
sveltejs:masterfrom
teemingc:fix-scroll-behavior
Closed

fix: iOS scroll-behavior: smooth breaks navigation scrolling#8724
teemingc wants to merge 4 commits into
sveltejs:masterfrom
teemingc:fix-scroll-behavior

Conversation

@teemingc

Copy link
Copy Markdown
Member

fixes #2733 (partially)

The issue is mentioned here #2733 (comment)

Sets the scroll-behavior to auto when scrolling from navigation, then resets it.

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 pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

@changeset-bot

changeset-bot Bot commented Jan 25, 2023

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 02a4ded

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

@teemingc teemingc changed the title fix: iOS scroll-behavior: smooth breaks navigation scroll fix: iOS scroll-behavior: smooth breaks navigation scrolling Jan 25, 2023
@Rich-Harris

Copy link
Copy Markdown
Member

Could this be considered a breaking change, if people are currently using smooth scrolling? It feels like it might be.

@teemingc

teemingc commented Feb 9, 2023

Copy link
Copy Markdown
Member Author

Could this be considered a breaking change, if people are currently using smooth scrolling? It feels like it might be.

Yes, it's a breaking change for users that intentionally wanted smooth scrolling when navigating between different pages.

At the same time, it's a fix for users that wanted smooth scrolling but didn't want the effect to happen when navigating across different pages (the browser's default behaviour).

The current behaviour can be seen here. Scrolling down and navigating to a new page causes a long scroll upwards instead of an instant scroll to the top.

This fix has no effect on hash navigation, since it uses .scrollIntoView() that doesn't have the same issues as scrollTo()

@Rich-Harris

Copy link
Copy Markdown
Member

I feel like if someone has set scroll-behaviour: smooth then it was probably intentional, so I'm not sure we can make this change. We should probably just document this approach instead:

/** @type {string}*/
let scroll_behaviour;

beforeNavigate(() => {
  scroll_behaviour = getComputedStyle(document.documentElement).scrollBehavior;
  document.documentElement.style.scrollBehavior = 'auto';
});

afterNavigate(() => {
  if (scroll_behaviour) {
    document.documentElement.style.scrollBehavior = scroll_behaviour;
  }
});

Is there an iOS bug distinct from the smooth scrolling behaviour?

@teemingc

teemingc commented Feb 9, 2023

Copy link
Copy Markdown
Member Author

I've just tested and it seems that I can no longer reproduce the issue in iOS 15 and 16.
The bug is still present in iOS 12, but that's quite old.
If it's okay, I think I'll close this.

@jasongitmail

Copy link
Copy Markdown

FWIW, this bug currently exists in Chrome, Safari, and Firefox. I also encountered it and wrote repro steps.

Rich's workaround above works. But it'd seem better to solve it in the framework for everybody if possible.

Yes, it's a breaking change for users that intentionally wanted smooth scrolling when navigating between different pages.

I'd consider it a bug fix of unexpected behavior, not a breaking feature change, b/c I don't think anybody would intentionally want this behavior: For example, if scrolled to the bottom of page A, it will navigate to page B at the bottom of it, and then immediately smooth scroll to the top of page B. The expected behavior would be for smooth scrolling to occur only on the same page, not after navigation.

@whataboutpereira

Copy link
Copy Markdown
Contributor

FWIW, this bug currently exists in Chrome, Safari, and Firefox. I also encountered it and wrote repro steps.

Rich's workaround above works. But it'd seem better to solve it in the framework for everybody if possible.

Yes, it's a breaking change for users that intentionally wanted smooth scrolling when navigating between different pages.

Rich's workaround helped me as well.

I'm using data-sveltekit-noscroll on the body, but still some pages fail to scroll properly in afterNavigate() - scroll to top is initiated before afterNavigate() even though noscroll has been specified and the scrollIntoView() I have in afterNavigate() will fail/stop halfway through.

Unfortunately I haven't been able to narrow it down why it happens on certain plain pages.

@xxfogs

xxfogs commented Nov 7, 2023

Copy link
Copy Markdown

The issue still persists even on the latest SvelteKit version, is there an official solution to this problem? Workarounds don't seem to help me on FireFox and even if they did it wouldn't be the most reliable way how to solve an unexpected behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Page scroll position not reset to top on navigation (regression)

6 participants