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

Async _navigate() unintuitively prevents scrolling or focusing with onMount() or actions #2477

Closed
Theo-Steiner opened this issue Sep 23, 2021 · 2 comments · Fixed by #2489
Closed
Labels
bug Something isn't working p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc.

Comments

@Theo-Steiner
Copy link
Contributor

Theo-Steiner commented Sep 23, 2021

Describe the bug

Since the _navigate() function that is called after clicking an anchor tag is asynchronous, part of it runs after the code inside the onMount() or use:action of the target route is executed.
If one were to use either of those to for example focus an input field on navigation, this would not work, as the default scroll and focus behavior of the router are run later and therefore overwrite the focus/scroll changes made inside the onMount/action.
I think it is fair to classify this as a bug since

  1. this is completely undocumented behavior that works without hiccups in non-sveltekit svelte:
    (REPL for reference)

  2. since the _navigate() function is not called when refreshing manually, the onMount/action code works as expected on refresh, possibly causing confusion for why this doesn't work. (Check the reproduction repository to see this behavior in action)

I see two possible solutions to addressing this unexpected behavior:

  1. Document the behavior and tell users to work around this by wrapping the focus/scroll inside setTimeout as async code has priority to whatever is on the eventLoop and therefore focus will only be pushed onto the browsers main thread after _navigate() completes. As you can see in the reproduction, this does work but is not very intuitive. The end user does not now that navigate is async and therefore won't understand why setTimeout is required here.

  2. Fix the behavior. To my surprise, adding the attribute of sveltekit:noscroll to anchor tags did not fix this behavior. I thought this could be addressed by simply adding sveltekit:keepfocus as an attribute and telling users to use either of those modifiers if they want to focus/scroll in their onMount/actions. I will be happy to help with a fix for this, but I currently have no other idea for how to fix this.

Reproduction

https://github.com/Theo-Steiner/onMountFocus

Logs

No response

System Info

all browsers

Severity

serious, but I can work around it

I assume many people would want to scroll/focus inside onMount / actions. I just stumbled upon this behavior in the first place because people in the discord were struggling with this.

Additional Information

No response

@benmccann
Copy link
Member

I wonder if this line is related at all? There's no explanation of why it's there

You could try removing it to see if it helps at all. At least some portion of routing / rendering needs to be async though because we need to call the user's load function which may be async. Maybe the focus / scrolling needs to move inside renderer.update?

@benmccann
Copy link
Member

It's worth noting that both your /workaround and /not-working links work if you open them in a new tab. It's only if you navigate directly that the /not-working link doesn't work

@benmccann benmccann added bug Something isn't working p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc. labels Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants