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

3003 autocomplete when in dialog intercepts escape keypresses and click outside #3087

8 changes: 6 additions & 2 deletions src/Autocomplete/AutocompleteOverlay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import Overlay, {OverlayProps} from '../Overlay'
import {ComponentProps} from '../utils/types'
import {AutocompleteContext} from './AutocompleteContext'
import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef'
import VisuallyHidden from '../_VisuallyHidden'

type AutocompleteOverlayInternalProps = {
/**
Expand Down Expand Up @@ -49,7 +50,7 @@ function AutocompleteOverlay({
return null
}

return (
return showMenu ? (
<Overlay
returnFocusRef={inputRef}
preventFocusOnOpen={true}
Expand All @@ -58,14 +59,17 @@ function AutocompleteOverlay({
ref={floatingElementRef as React.RefObject<HTMLDivElement>}
top={position?.top}
left={position?.left}
visibility={showMenu ? 'visible' : 'hidden'}
sx={{
overflow: 'auto',
}}
{...overlayProps}
>
{children}
</Overlay>
) : (
// HACK: This ensures AutocompleteMenu is still mounted when closing the menu and all of the hooks inside of it are still called.
// A better way to do this would be to move the hooks to AutocompleteOverlay or somewhere that won't get unmounted.
<VisuallyHidden>{children}</VisuallyHidden>
Copy link
Member

Choose a reason for hiding this comment

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

Comparing the a11y tree both production and this branch, in production (left) the listbox is not in the a11y tree until the autocomplete is opened. While, in this branch, it's in the a11y tree even when the autocomplete is closed.

because Overlay uses the css property visibility, it removes the contents from the accessibility tree, which VisuallyHidden does not.

in production (left) the listbox is not in the a11y tree until the autocomplete is opened. While, in this branch, it's in the a11y tree even when the autocomplete is closed

If we can rid of the hack, that would of course be perfect. But, if not, we probably would need to make it aria-hidden as well.

)
}

Expand Down
30 changes: 16 additions & 14 deletions src/__tests__/Autocomplete.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -272,28 +272,30 @@ describe('Autocomplete', () => {
<LabelledAutocomplete menuProps={{items: mockItems, selectedItemIds: [], selectionVariant: 'multiple'}} />,
)
const inputNode = container.querySelector('#autocompleteInput')
const itemToClickNode = getByText(mockItems[1].text)

expect(inputNode?.getAttribute('aria-expanded')).not.toBe('true')
inputNode && fireEvent.focus(inputNode)
expect(inputNode?.getAttribute('aria-expanded')).toBe('true')
fireEvent.click(itemToClickNode)
inputNode && (await user.type(inputNode, '{enter}'))
expect(inputNode?.getAttribute('aria-expanded')).toBe('true')
if (inputNode) {
expect(inputNode.getAttribute('aria-expanded')).not.toBe('true')
await user.click(inputNode)
expect(inputNode.getAttribute('aria-expanded')).toBe('true')
await user.click(getByText(mockItems[1].text))
expect(inputNode.getAttribute('aria-expanded')).toBe('true')
}
})

it('closes the menu when clicking an item in the menu if selectionVariant=single', () => {
it('closes the menu when clicking an item in the menu if selectionVariant=single', async () => {
const user = userEvent.setup()
const {getByText, container} = HTMLRender(
<LabelledAutocomplete menuProps={{items: mockItems, selectedItemIds: [], selectionVariant: 'single'}} />,
)
const inputNode = container.querySelector('#autocompleteInput')
const itemToClickNode = getByText(mockItems[1].text)

expect(inputNode?.getAttribute('aria-expanded')).not.toBe('true')
inputNode && fireEvent.focus(inputNode)
expect(inputNode?.getAttribute('aria-expanded')).toBe('true')
fireEvent.click(itemToClickNode)
expect(inputNode?.getAttribute('aria-expanded')).not.toBe('true')
if (inputNode) {
expect(inputNode.getAttribute('aria-expanded')).not.toBe('true')
await user.click(inputNode)
expect(inputNode.getAttribute('aria-expanded')).toBe('true')
await user.click(getByText(mockItems[1].text))
expect(inputNode.getAttribute('aria-expanded')).not.toBe('true')
}
})

it('calls handleAddItem with new item data when passing addNewItem', () => {
Expand Down
15 changes: 10 additions & 5 deletions src/hooks/useDialog.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {useCallback, useEffect} from 'react'
import {useOnEscapePress} from './useOnEscapePress'

const noop = () => null

Expand Down Expand Up @@ -103,19 +104,23 @@ function useDialog({
case 'Tab':
handleTab(event)
break
case 'Escape':
onDismiss()
event.stopPropagation()
break
}
},
[handleTab, onDismiss],
[handleTab],
)

const getDialogProps = () => {
return {onKeyDown}
}

useOnEscapePress(
green6erry marked this conversation as resolved.
Show resolved Hide resolved
(event: KeyboardEvent) => {
onDismiss()
event.preventDefault()
},
[onDismiss],
)

return {getDialogProps}
}

Expand Down
4 changes: 2 additions & 2 deletions src/stories/Autocomplete.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -769,8 +769,8 @@ export const InADialog = (args: FormControlArgs<AutocompleteArgs>) => {

return (
<>
<Button onClick={() => setIsDialogOpen(!isDialogOpen)}>Show dialog</Button>
<Dialog id="dialog-with-autocomplete" isOpen={isDialogOpen}>
<Button onClick={() => setIsDialogOpen(true)}>Show dialog</Button>
<Dialog id="dialog-with-autocomplete" isOpen={isDialogOpen} onDismiss={() => setIsDialogOpen(false)}>
<div ref={outerContainerRef}>
<Box as="form" sx={{p: 3}}>
{mounted ? (
Expand Down