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

Fix focus loss from Combobox.Input when disabled option clicked #3008

Closed

Conversation

taro-28
Copy link

@taro-28 taro-28 commented Feb 23, 2024

Fixed an issue with the Combobox component where clicking a disabled Combobox.Option would take focus away from Combobox.Input.

sample codesandbox

Copy link

vercel bot commented Feb 23, 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 Feb 23, 2024 6:31am
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 23, 2024 6:31am

Copy link
Author

Choose a reason for hiding this comment

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

Set tabIndex=-1 for Combobox.Option when it is disabled.

Moved data.inputRef.current?.focus({ preventScroll: true }) from handleClick to handleFocus and ensured it is called even when the option is disabled.

The reason for moving it into handleFocus is that the onClick event handler for disabled options is ignored by the mergePropsAdvanced function.

Comment on lines +491 to +497
let focusableSelectorWithNegativeTabindex = _focusableSelector
.map(
process.env.NODE_ENV === 'test'
? (selector) => `${selector}:not([style*='display: none'])`
: (selector) => selector
)
.join(',')
Copy link
Author

Choose a reason for hiding this comment

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

An element with tabIndex=-1 can be focused on through means other than keyboard navigation, such as clicking.

Comment on lines +221 to +228
// Ensure event listeners are not called if `disabled` or `aria-disabled` is true
if (target.disabled || target['aria-disabled']) {
return Object.assign(
target,
// Set all event listeners that we collected to `undefined`. This is
// important because of the `cloneElement` from above, which merges the
// existing and new props, they don't just override therefore we have to
// explicitly nullify them.
Object.fromEntries(Object.keys(eventHandlers).map((eventName) => [eventName, undefined]))
)
for (let eventName in eventHandlers) {
// Prevent default events for `onClick`, `onMouseDown`, `onKeyDown`, etc.
if (/^(on(?:Click|Pointer|Mouse|Key)(?:Down|Up|Press)?)$/.test(eventName)) {
eventHandlers[eventName] = [(e: any) => e?.preventDefault?.()]
}
}
Copy link
Author

Choose a reason for hiding this comment

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

Fixed it in the same way as the mergePropsAdvanced function in @headlessui-react.

#2890

@taro-28 taro-28 marked this pull request as ready for review February 23, 2024 06:33
@RobinMalfait RobinMalfait self-assigned this Apr 4, 2024
@RobinMalfait
Copy link
Collaborator

Hey!

We recently made some changes to how the ComboboxInput, ComboboxButton and ComboboxOption work behind the scenes. The short story is that we reworked the internals such that the focus doesn't leave the ComboboxInput at all. More info: #3073

This means that in the latest insiders version clicking a disabled option should already work as expected.

Going to close this PR because it's not needed anymore, but I really appreciate your work on this!

Thanks!

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.

None yet

2 participants