fix: add scrollable area check and scrollable area attribute

[eltigerchino]

fixes #2733

Adds a check to see if html or body is scrollable.
Otherwise, it looks for an element with the attribute data-sveltekit-main-scrollable to determine which element to scroll during navigation.

Looking for feedback as this solution still feels quite rough. @dummdidumm

TODO

• add tests
• docs
• changeset

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]

⚠️ No Changeset found

Latest commit: a8669e7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[dummdidumm]

The idea is tempting, although I'm wondering how much code people really save by doing that. I'm also wondering if they would always prefer the explicitly set scrollable area to be used for scrolling. Also, how often will be html element be non-scrollable while the body will be? Feels like both won't have that if someone does that.

[eltigerchino]

The idea is tempting, although I'm wondering how much code people really save by doing that. I'm also wondering if they would always prefer the explicitly set scrollable area to be used for scrolling. Also, how often will be html element be non-scrollable while the body will be? Feels like both won't have that if someone does that.

I agree. The thread showed that many people were able to solve it with their own workarounds.
It might be enough to document the behaviour and/or provide a warning when html isn't scrollable.

The common patterns I've gathered from the issue thread are:

1. html is not scrollable and its height is set to the window height. The body is the scrolling area.
2. html and/or body are not scrollable and both have height set to window height. A div or main is the scrolling area. The element location varies.

[petem-methods]

I think warning + documenting is the most important thing - it is very non-obvious that this will happen, mainly because as a user you're not thinking about how the magic happens.

The thread showed that many people were able to solve it with their own workarounds.

But I really like the idea of providing a proper solution - as a mortal Sveltekit user, I don't feel like I understand the workaround we have done enough to know that I've done it right. Like, I added the check for when navigation.type === "link"... because otherwise goto didn't work properly.... I want to be able to just say, "hey Sveltekit, treat this as the scrollable area" and have it work in the same way as it does for the html element.

Please let me know if I can help testing this - and if so, how. 🙏

[eltigerchino]

But I really like the idea of providing a proper solution - as a mortal Sveltekit user, I don't feel like I understand the workaround we have done enough to know that I've done it right. Like, I added the check for when navigation.type === "link"... because otherwise goto didn't work properly.... I want to be able to just say, "hey Sveltekit, treat this as the scrollable area" and have it work in the same way as it does for the html element.

In this case, it would be preferable to always require the explicit scrolling area set. I hesitated earlier about this when I recalled a conversation about the issues of having too many framework specific attributes.

Please let me know if I can help testing this - and if so, how. 🙏

I had the idea to add tests for the new attribute following the procedure:

1. Scroll down.
2. Click a link.
3. Check scroll position of the main scrolling element.

However, I'm waiting for more feedback from everyone before continuing this PR (and probably some help with refining the code).

[Rich-Harris]

I wonder if #8710 is an adequate solution to this, if properly documented? It's a tiny bit more work than adding an attribute, but it uses existing API surface area and teaches people a technique that they can then generalise to other problems.

[eltigerchino]

It would also need to address the issue of correctly scrolling to the top when navigating to a new page.

My current understanding of snapshots is that it only restores scroll when navigating backwards / forwards to visited pages.

[petem-methods]

It also doesn't stop the mistake being made, I don't think? We only discovered it late in a cycle when testing on real mobile devices, so the buggy scrolling went into production. So we need something like this DEV check / warn.

As an aside, I'm not sure I've seen an explanation here of what the relevant difference is in mobile browsers that causes this behaviour to manifest (or - rather - not manifest reliably on desktop). 🙏

[Rich-Harris]

t would also need to address the issue of correctly scrolling to the top when navigating to a new page.

Ah, that's true. You need a combination of snapshot and afterNavigate:

/** @type {HTMLElement} */
let container;

afterNavigate(() => {
  container.scrollTo(0, 0);
});

/** @type {import('./$types').Snapshot<number>} */
export const snapshot = {
  capture: () => container.scrollTop,
  restore: (y) => container.scrollTo(0, y)
};

(Snapshots are restored immediately after afterNavigate callbacks, so there's no need to filter out popstate navigations.)

An attribute would definitely be easier, though I do worry slightly about adding too many more moving parts to client.js, especially if they're only relevant to a subset of users. The pattern above would need to be properly documented though (perhaps a 'scroll management' section in 'advanced').

It also doesn't stop the mistake being made, I don't think? We only discovered it late in a cycle when testing on real mobile devices, so the buggy scrolling went into production.

IIUC the bugginess is something separate, related to scroll-behavior? That was my takeaway from #2733 (comment) at least.

[eltigerchino]

I think documenting that recipe would be a nice solution for now.
UI library maintainers that provide a container component should export their own capture / restore logic that the user can easily add to their layout snapshot exports.

[samal-rasmussen]

@Rich-Harris said:

Snapshots are restored immediately after afterNavigate callbacks

Unfortunately I'm seeing afterNavigate being called after snapshot restore, so this doesn't work.

I can just workaround with a setTimeout/requestAnimationFrame on the restore... but that's a workaround for a workaround so I'm starting to feel a bit icky.

[dvlpr91]

I was very confused that this was not in the sveltekit docs.