Fix: Only call enable/disableScroll on open/close

[andrew-mc]

Background

I have a Svelte app that keeps track of a variable in App, and passes it down to multiple child components. When the variable is updated I'm encountering some odd scrolling behaviour.

Observed Behaviour

I've created a minimal REPL example with a basic counter variable: https://svelte.dev/repl/24df7129ba5c45c4903f87f9a8d0d36e?version=3.38.3

Two odd behaviours occur (seen in latest Firefox/Chrome/Edge on Windows 10):

1. If the page is scrolled before the counter is incremented, the page jumps back to the top
2. If the counter is incremented while the pop-up is open, scrolling remains disabled after the modal is closed.

Cause

Svelte will update the Modal component when the counter is updated, even if the state of the Modal itself hasn't changed. Because open and close are called in svelte.afterUpdate, this means enableScroll or disableScroll may be called multiple times without the modal actually opening or closing.

This means:

1. When the counter is updated while the pop-up is closed enableScroll is called, which in turn calls window.scrollTo(0, scrollY) where scrollY is undefined and treated as 0.
2. When the counter is updated while the pop-up is open disableScroll gets run a second time, which means the cached values (like previousBodyPosition) get overwritten with the identical current values. This means e.g. when the modal is closed the the body keeps position: fixed.

Fix

Explicitly calling disable/enable during open/close fixes the issue for me. I do appreciate the elegance of using afterUpdate but it seems Svelte has other ideas :)

[flekschas]

Thank you for bringing this up and the detailed explanation! 🎉 This is a great example of an excellent PR!

afterupdate was recently introduced to support SvelteKit or other server-side rendering approaches. See #45. However, it seems we have to revert the change. Any ideas how we could fix this and still avoid the document is not defined in SvelteKit. Maybe @truongvan can comment on this as well since he proposed to use afterUpdate in the first place.

[andrew-mc]

I'm afraid I'm not super familiar with svelte kit, but the FAQ suggests you can explicitly check if you're in the browser, which if I understand the issue seems like it might be on the right track?

import { browser } from '$app/env';

if (browser) {
	// client-only code here
}

[flekschas]

I did a bit of testing in SvelteKit and found a solution that should make it work without causing scrolling issues and using afterUpdate.

@andrew-mc Could you give the branch another quick shot? Here's the updated demo: https://svelte.dev/repl/9b056ff538f743cf82846d772cc1407d?version=3.38.3 Does it still solve all the issues you noticed? If so I'll go ahead and merge your PR!

[flekschas]

Looks good to me. 🎉 I added the isMounted check to avoid the document not defined error in SvelteKit. Please give it another test if you have the chance.

[andrew-mc]

Tested and working for me 🥳

Thanks for responding so quickly and also for putting this library together, very glad I don't have to figure all of it out myself :)

[flekschas]

Merged and released! Thanks again for your PR! 🙇