feat: Add Autocomplete wrapper for Menu and ListBox #7181
Conversation
issues outlined in various comments, basically boils down to ideally using the wrapped collection components state
ended up going with dispatching events to the menu/menuitem so we can piggyback off of useSelectableCollection and menus press handling for submenutriggers, onAction, and link handling
| // TODO: this is pretty specific to menu, will need to check if it is generic enough | ||
| // Will need to handle varying levels I assume but will revisit after I get searchable menu working for base menu | ||
| // TODO: an alternative is to simply walk the collection and add all item nodes that match the filter and any sections/separators we encounter | ||
| // to an array, then walk that new array and fix all the next/Prev keys while adding them to the new collection | ||
| filter(filterFn: (nodeValue: string) => boolean): BaseCollection<T> { |
There was a problem hiding this comment.
Definitely specific to menu, will need to update when we handle other collection components. Collections is in alpha so that should shield us from any changes that may need to happen to this function
| let options = within(menu).getAllByRole(collectionItemRole); | ||
| expect(input).toHaveAttribute('aria-activedescendant', options[0].id); | ||
| await user.keyboard('{Enter}'); | ||
| expect(actionListener).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
better, however, this is still tied to our API.
Someone could implement an autocomplete that is self contained and fires no event (I'm not sure why, but they could, maybe they fire a custom event on the dom instead of using props)
In addition, they may not include the id of the item, maybe they've created some mapping of the ids to some database uuid or something.
There was a problem hiding this comment.
The actionListener here that they provided doesn't have to be passed to component via props though, they just need to provide us with whatever action listener mock that would get triggered on item action and we assert it has been properly called when hitting enter on the input while that item is focused right?
As for the id of the item, the id being checked above is the aria-activedescendant which needs to be tied to the dom node they are virtually focusing so that should be fine as well right?
checked in iOS 17, previous issue of voiceover moving focus to the collection doesnt seem to happen anymore. This change makes sure that users cant tab from the autocomplete to the wrapped collection
… links work in listbox
## API Changes
react-aria-components/react-aria-components:Autocomplete+Autocomplete {
+ children: ReactNode
+ defaultFilter?: (string, string) => boolean = contains
+ defaultInputValue?: string
+ inputValue?: string
+ onInputChange?: (string) => void
+ slot?: string | null
+}/react-aria-components:AutocompleteContext+AutocompleteContext {
+ UNTYPED
+}/react-aria-components:AutocompleteStateContext+AutocompleteStateContext {
+ UNTYPED
+}/react-aria-components:InternalAutocompleteContext+InternalAutocompleteContext {
+ UNTYPED
+}@react-aria/autocomplete/@react-aria/autocomplete:useAutocomplete+useAutocomplete {
+ props: AriaAutocompleteOptions
+ state: AutocompleteState
+ returnVal: undefined
+}/@react-aria/autocomplete:AriaAutocompleteProps+AriaAutocompleteProps {
+ children: ReactNode
+ defaultFilter?: (string, string) => boolean = contains
+ defaultInputValue?: string
+ inputValue?: string
+ onInputChange?: (string) => void
+}/@react-aria/autocomplete:AriaAutocompleteOptions+AriaAutocompleteOptions {
+ collectionRef: RefObject<HTMLElement | null>
+ defaultFilter?: (string, string) => boolean = contains
+ defaultInputValue?: string
+ inputRef: RefObject<HTMLInputElement | null>
+ inputValue?: string
+ onInputChange?: (string) => void
+}/@react-aria/autocomplete:AutocompleteAria+AutocompleteAria {
+ collectionProps: CollectionOptions
+ collectionRef: RefObject<HTMLElement | null>
+ filterFn: (string) => boolean
+ inputProps: InputHTMLAttributes<HTMLInputElement>
+}/@react-aria/autocomplete:CollectionOptions+CollectionOptions {
+ aria-describedby?: string
+ aria-details?: string
+ aria-label?: string
+ aria-labelledby?: string
+ disallowTypeAhead: boolean
+ id?: string
+ shouldUseVirtualFocus: boolean
+}@react-aria/collections/@react-aria/collections:BaseCollection BaseCollection <T> {
addNode: (CollectionNode<T>) => void
at: () => Node<T>
clone: () => this
commit: (Key | null, Key | null, any) => void
+ filter: ((string) => boolean) => BaseCollection<T>
getChildren: (Key) => Iterable<Node<T>>
getFirstKey: () => void
getItem: (Key) => Node<T> | null
getKeyAfter: (Key) => void
getKeys: () => void
getLastKey: () => void
removeNode: (Key) => void
size: any
undefined: () => void
}@react-aria/menu/@react-aria/menu:AriaMenuOptions AriaMenuOptions <T> {
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
autoFocus?: boolean | FocusStrategy
defaultSelectedKeys?: 'all' | Iterable<Key>
disabledKeys?: Iterable<Key>
disallowEmptySelection?: boolean
id?: string
isVirtualized?: boolean
items?: Iterable<T>
keyboardDelegate?: KeyboardDelegate
onAction?: (Key) => void
onClose?: () => void
onKeyDown?: (KeyboardEvent) => void
onKeyUp?: (KeyboardEvent) => void
onSelectionChange?: (Selection) => void
selectedKeys?: 'all' | Iterable<Key>
selectionMode?: SelectionMode
shouldFocusWrap?: boolean
+ shouldUseVirtualFocus?: boolean
}@react-aria/selection/@react-aria/selection:SelectableItemOptions SelectableItemOptions {
allowsDifferentPressOrigin?: boolean
focus?: () => void
+ id?: string
isDisabled?: boolean
isVirtualized?: boolean
key: Key
linkBehavior?: 'action' | 'selection' | 'override' | 'none' = 'action'
ref: RefObject<FocusableElement | null>
selectionManager: MultipleSelectionManager
shouldSelectOnPressUp?: boolean
shouldUseVirtualFocus?: boolean
}@react-aria/utils/@react-aria/utils:CLEAR_FOCUS_EVENT+CLEAR_FOCUS_EVENT {
+ UNTYPED
+}/@react-aria/utils:FOCUS_EVENT+FOCUS_EVENT {
+ UNTYPED
+}/@react-aria/utils:UPDATE_ACTIVEDESCENDANT+UPDATE_ACTIVEDESCENDANT {
+ UNTYPED
+}@react-stately/autocomplete/@react-stately/autocomplete:useAutocompleteState+useAutocompleteState {
+ props: AutocompleteStateOptions
+ returnVal: undefined
+}/@react-stately/autocomplete:AutocompleteProps+AutocompleteProps {
+ children: ReactNode
+ defaultInputValue?: string
+ inputValue?: string
+ onInputChange?: (string) => void
+}/@react-stately/autocomplete:AutocompleteStateOptions+AutocompleteStateOptions {
+ defaultInputValue?: string
+ inputValue?: string
+ onInputChange?: (string) => void
+}/@react-stately/autocomplete:AutocompleteState+AutocompleteState {
+ focusedNodeId: string | null
+ inputValue: string
+ setFocusedNodeId: (string | null) => void
+ setInputValue: (string) => void
+} |
| let lastInputValue = useRef<string | null>(null); | ||
| useEffect(() => { | ||
| if (state.inputValue != null) { | ||
| if (lastInputValue.current != null && lastInputValue.current !== state.inputValue && lastInputValue.current?.length <= state.inputValue.length) { |
There was a problem hiding this comment.
what if you copy and pasted? the value might be completely different but also shorter which isn't the same as backspacing I guess. what do you think should happen in that case?
There was a problem hiding this comment.
I think focus should be cleared in that case since the announcement would be a longer one (announcing the new word). TBH that should also apply when pasting a string that is longer that your current too but currently will autofocus the first option... In a ideal world screen readers would be better about not interrupting these announcements so we could just always autofocus haha. I guess I could try and add some logic to differentiate typing forward vs copy pasting by comparing just how much the input value has changed from the last value
There was a problem hiding this comment.
feels pretty gross that we'd be diffing the strings to see if it is a different word + checking the lengths just to make it not autofocus when pasting a longer/different string in the field. Perhaps we could make an assumption that a screenreader user would already know what string they are copy and pasting and make the case that interrupted announcement isn't super important?
EDIT: in addition, string comparing doesn't feel very consistent since we don't actually know how long said string would take to announce (different language/word length/etc) so we can't be sure if the 500ms delay would even give the screenreader enough time before being interrupted by the item autofocus. I could perhaps treat ANY paste event as needing to clear the virtually focused item but it already feels pretty hacky
There was a problem hiding this comment.
yeah, the native input event provides an inputType property that has all sorts of "reasons" for the change, which might be useful. We use the related beforeinput event in number field. I know you mentioned you tried onChange and it was happening before filtering, but maybe we could use that event to track the change type and apply it after the next render. Can be a future enhancement.
There was a problem hiding this comment.
sounds good, I'll make a follow up ticket to track and do some initial tinkering to see if this is feasible (thanks for the tip with inputType, forgot that was a thing)
| case 'Escape': | ||
| // Early return for Escape here so it doesn't leak the Escape event from the simulated collection event below and | ||
| // close the dialog prematurely. Ideally that should be up to the discretion of the input element hence the check | ||
| // for isPropagationStopped | ||
| if (e.isPropagationStopped()) { | ||
| return; | ||
| } | ||
| break; |
There was a problem hiding this comment.
I just realized that CMD + A in the input field (aka "select all text") collides with a selectable collection's "select all" (aka select all items in multiselect). I'm happy to special case it here, but I guess this should perhaps be stop propagated in the textfield/searchfield hooks maybe?
| let tabIndex: number | undefined = undefined; | ||
| if (!shouldUseVirtualFocus) { | ||
| tabIndex = manager.focusedKey == null ? 0 : -1; | ||
| } else { | ||
| tabIndex = -1; |
There was a problem hiding this comment.
I assume the above VoiceOver issue no longer applies?
There was a problem hiding this comment.
yeah, I wasn't able to reproduce the issues in VoiceOver in iOS 17 on my iPad but I wasn't quite sure what the actual bug was (the blame just points to the old combobox work we had years ago). From the description it sounded like focus was being erroneously moved around, but I didn't notice anything odd on my end when adding the above.
| * The filter function used to determine if a option should be included in the autocomplete list. | ||
| * @default contains | ||
| */ | ||
| defaultFilter?: (textValue: string, inputValue: string) => boolean |
There was a problem hiding this comment.
Wonder if this should just be called filter? Why did we call it defaultFilter in ComboBox? I guess it's not a public prop there, but only in the hook?
There was a problem hiding this comment.
right, I guess the naming will depend on the what we decided to do for the defaultItems/items API in Autocomplete. In ComboBox, if the user provided items then the defaultFilter provided doesn't do anything since they are responsible for the filtering, hence the "default" part of the name. However, Menu/ListBox doesn't have the concept of defaultItems so I guess this should be filter but depends on how we want to manage external filtering here
Closes
#7248 has a version that always persists focus, to test on Monday
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project:
RSP