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

Keep focus inside of the <ComboboxInput /> component #3073

Merged
merged 16 commits into from
Apr 3, 2024

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Apr 2, 2024

This PR improves the UX of the Combobox component.

When you open the combobox via the button or when you select an option from the list, then the focus is briefly moved to the button or the option respectively and then we restore the focus to the input itself.

We improved this behaviour in #3065 where we ensure that the selection and cursor position inside of the input will be preserved when the focus is moved around as-if nothing happened.

However, this can result in a tiny selection flicker (because when the focus is moved to the button or the option, the selection is removed).

This PR improves on that behaviour by using the mousedown event instead of the click event. If we use event.preventDefault() in the mousedown event, then the focus will not be moved to the button or the option and the focus remains in the input. This also means that the selection remains in the input.

We now use the mousedown event, which means that we have to do a tiny bit more work compared to the click event. For example:

  • We have to check that the left mouse button was used. The mousedown event will also be triggered when using the right mouse button (the click event doesn't do this)
  • We have to manually check for space or enter keydown events on the button — this was previously handled by the click event as well.
Screen.Recording.2024-04-02.at.00.10.59.mov

The `mousedown` event happens before the `focus` event. When we
`e.preventDefault()` in this listener, the `focus` event will not fire.

This also means that the focus is not lost on the actual `input`
component which in turn means that we can maintain the selection /
cursor position inside the `input`.

We still use the `refocusInput()` as a fallback in case something else
goes wrong.
Now that we use the `mousedown` event instead of the `click` event, we
have to make sure that we handle the `enter` and `space` keys
explicitly.

This used to be covered by the `click` event, but not for the `mousedown` event.
…oboxButton`

We go to the last one on `ArrownUp`, but we forgot to do this on
`ArrowDown`.
Not related to this PR, but noticed it and fixed it anyway.
Copy link

vercel bot commented Apr 2, 2024

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

Name Status Preview Comments Updated (UTC)
headlessui-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 3, 2024 0:46am
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 3, 2024 0:46am

While we are typing, the flag can remain true. But once we stop typing,
the `nextFrame` handler will kick in and set it to `false` again.

It currently behaves as a debounce-like function such that the
`nextFrame` callbacks are cancelled once a new event is fired.
This allows us to keep re-adding dispose functions and only register the
callbacks once.

Ideally we can use a `Set`, but we also want to remove a single callback
if the callback is disposed on its own instead of disposing the whole
group. For this we do require an `idx` which is not available in a
`Set` unless you are looping over all disposable functions.
@@ -1077,8 +1078,21 @@ function InputFn<
})
})

let dd = useDisposables()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dd variable here is confusing to me — what does it mean? I know we typically use d for disposables, so is it just because you have two disposables in this case? Wondering if we can come up with a better name here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep we already had d but that had a different use case. To solve this, I created a useFrameDebounce() hook (we can improve this name if we want). But now we can do this instead:

image

RobinMalfait and others added 2 commits April 3, 2024 14:09
Co-authored-by: Jonathan Reinink <jonathan@reinink.ca>
Co-authored-by: Jonathan Reinink <jonathan@reinink.ca>
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.

2 participants