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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions packages/@headlessui-react/src/components/combobox/combobox.tsx
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.

Original file line number Diff line number Diff line change
Expand Up @@ -1729,6 +1729,12 @@ function OptionFn<
if (disabled || data.virtual?.disabled(value)) return event.preventDefault()
select()

if (data.mode === ValueMode.Single) {
requestAnimationFrame(() => actions.closeCombobox())
}
})

let handleFocus = useEvent(() => {
// We want to make sure that we don't accidentally trigger the virtual keyboard.
//
// This would happen if the input is focused, the options are open, you select an option (which
Expand All @@ -1745,12 +1751,6 @@ function OptionFn<
requestAnimationFrame(() => data.inputRef.current?.focus({ preventScroll: true }))
}

if (data.mode === ValueMode.Single) {
requestAnimationFrame(() => actions.closeCombobox())
}
})

let handleFocus = useEvent(() => {
if (disabled || data.virtual?.disabled(value)) {
return actions.goToOption(Focus.Nothing)
}
Expand Down Expand Up @@ -1787,7 +1787,7 @@ function OptionFn<
id,
ref: optionRef,
role: 'option',
tabIndex: disabled === true ? undefined : -1,
tabIndex: -1,
'aria-disabled': disabled === true ? true : undefined,
// According to the WAI-ARIA best practices, we should use aria-checked for
// multi-select,but Voice-Over disagrees. So we use aria-checked instead for
Expand Down
18 changes: 14 additions & 4 deletions packages/@headlessui-react/src/test-utils/interactions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ export async function rawClick(
if (!cancelled) {
let next: HTMLElement | null = element as HTMLElement | null
while (next !== null) {
if (next.matches(focusableSelector)) {
if (next.matches(focusableSelectorWithNegativeTabindex)) {
next.focus()
break
}
Expand Down Expand Up @@ -463,9 +463,7 @@ function focusNext(event: Partial<KeyboardEvent>) {
return innerFocusNext()
}

// Credit:
// - https://stackoverflow.com/a/30753870
let focusableSelector = [
let _focusableSelector = [
'[contentEditable=true]',
'[tabindex]',
'a[href]',
Expand All @@ -476,6 +474,10 @@ let focusableSelector = [
'select:not([disabled])',
'textarea:not([disabled])',
]

// Credit:
// - https://stackoverflow.com/a/30753870
let focusableSelector = _focusableSelector
.map(
process.env.NODE_ENV === 'test'
? // TODO: Remove this once JSDOM fixes the issue where an element that is
Expand All @@ -486,6 +488,14 @@ let focusableSelector = [
)
.join(',')

let focusableSelectorWithNegativeTabindex = _focusableSelector
.map(
process.env.NODE_ENV === 'test'
? (selector) => `${selector}:not([style*='display: none'])`
: (selector) => selector
)
.join(',')
Comment on lines +491 to +497
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.


function getFocusableElements(container = document.body) {
if (!container) return []
return Array.from(container.querySelectorAll(focusableSelector))
Expand Down
14 changes: 7 additions & 7 deletions packages/@headlessui-vue/src/components/combobox/combobox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1462,6 +1462,12 @@ export let ComboboxOption = defineComponent({
if (props.disabled || api.virtual.value?.disabled(props.value)) return event.preventDefault()
api.selectOption(id)

if (api.mode.value === ValueMode.Single) {
requestAnimationFrame(() => api.closeCombobox())
}
}

function handleFocus() {
// We want to make sure that we don't accidentally trigger the virtual keyboard.
//
// This would happen if the input is focused, the options are open, you select an option
Expand All @@ -1478,12 +1484,6 @@ export let ComboboxOption = defineComponent({
requestAnimationFrame(() => dom(api.inputRef)?.focus({ preventScroll: true }))
}

if (api.mode.value === ValueMode.Single) {
requestAnimationFrame(() => api.closeCombobox())
}
}

function handleFocus() {
if (props.disabled || api.virtual.value?.disabled(props.value)) {
return api.goToOption(Focus.Nothing)
}
Expand Down Expand Up @@ -1520,7 +1520,7 @@ export let ComboboxOption = defineComponent({
id,
ref: internalOptionRef,
role: 'option',
tabIndex: disabled === true ? undefined : -1,
tabIndex: -1,
'aria-disabled': disabled === true ? true : undefined,
// According to the WAI-ARIA best practices, we should use aria-checked for
// multi-select,but Voice-Over disagrees. So we use aria-selected instead for
Expand Down
18 changes: 14 additions & 4 deletions packages/@headlessui-vue/src/test-utils/interactions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ export async function click(
if (!cancelled) {
let next: HTMLElement | null = element as HTMLElement | null
while (next !== null) {
if (next.matches(focusableSelector)) {
if (next.matches(focusableSelectorWithNegativeTabindex)) {
next.focus()
break
}
Expand Down Expand Up @@ -456,9 +456,7 @@ function focusNext(event: Partial<KeyboardEvent>) {
return innerFocusNext()
}

// Credit:
// - https://stackoverflow.com/a/30753870
let focusableSelector = [
let _focusableSelector = [
'[contentEditable=true]',
'[tabindex]',
'a[href]',
Expand All @@ -469,6 +467,10 @@ let focusableSelector = [
'select:not([disabled])',
'textarea:not([disabled])',
]

// Credit:
// - https://stackoverflow.com/a/30753870
let focusableSelector = _focusableSelector
.map(
process.env.NODE_ENV === 'test'
? // TODO: Remove this once JSDOM fixes the issue where an element that is
Expand All @@ -479,6 +481,14 @@ let focusableSelector = [
)
.join(',')

let focusableSelectorWithNegativeTabindex = _focusableSelector
.map(
process.env.NODE_ENV === 'test'
? (selector) => `${selector}:not([style*='display: none'])`
: (selector) => selector
)
.join(',')

function getFocusableElements(container = document.body) {
if (!container) return []
return Array.from(container.querySelectorAll(focusableSelector))
Expand Down
16 changes: 7 additions & 9 deletions packages/@headlessui-vue/src/utils/render.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,16 +218,14 @@ function mergeProps(...listOfProps: Record<any, any>[]) {
}
}

// Do not attach any event handlers when there is a `disabled` or `aria-disabled` prop set.
// 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?.()]
}
}
Comment on lines +221 to +228
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

}

// Merge event handlers
Expand Down