search

techniq / svelte-ux

Collection of Svelte components, actions, stores, and utilities to build highly interactive applications.
https://svelte-ux.techniq.dev/
MIT License
663 stars 33 forks source link

Fix scrolling away when opening menu #400

Closed myieye closed 4 weeks ago

myieye commented 4 weeks ago

I put a my reproduction from the recordings followed by the bug fix in this branch. (i.e. revert the fix to see the bug)

Before: broken-scrolling

After: fixed-scrolling

I'm not sure how to create an example for this, because I can only reproduce it when / are larger than the viewport. Somehow focusing on the element too early makes the browser scroll to the wrong place.

changeset-bot[bot] commented 4 weeks ago

🦋 Changeset detected

Latest commit: 455dc4bed7a5ea849fa88d9ecb9488913afbfe8c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | --------- | ----- | | svelte-ux | 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

vercel[bot] commented 4 weeks ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
svelte-ux ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 20, 2024 0:27am
techniq commented 4 weeks ago

Hmm, that's interesting, and fix works for me. Someone reported a similar isolated issue on Discord and wonder if this was the cause as well. Thanks!

I think the way AppLayout has main overflow hides these kinds of issues a lot of times, and is something I would like to change. Currently since main overflows, it breaks initial browser scrolling of link fragments (#here).

techniq commented 4 weeks ago

@myieye Hmm, I just noticed, but it looks like this breaks the focus on QuickSearch. Here's a build from yesterday that works, and reverting locally restores the behavior.

Currently QuickSearch uses autoFocus which is an earlier version of focusMove.

Going to look into this, but wanted to let you know as I'll wait to cut a release (also looked at your other PR and was writing up some comments when I spotted this)

techniq commented 4 weeks ago

Fixed via https://github.com/techniq/svelte-ux/commit/7d9f09cc999eea8d8244edffe818a36220680621.

I unified the implementations by calling focusMove() within autoFocus(). I considered removing autoFocus() at first, but the best solution I could come up with was having a longer delay for those use case (TextField within Dialog, where Dialog is calling focusMove()). I think having the distinction between autoFocus (used for inputs) and focusMove (more lower level, but mostly for modals/etc, makes sense. What do you think?