feat: RAC Autocomplete audit for release #7475
Conversation
LFDanLu
changed the title
|
Marked a bunch of things as UNSTABLE, what do people think about including this in the test apps? Usually we don't have RAC components in the test apps, and I don't think it would be good to put this in the tailwind example apps until it exits alpha/gets docs. |
| @@ -16,7 +16,7 @@ import 'client-only'; | |||
|
|
|||
| export {CheckboxContext, ColorAreaContext, ColorFieldContext, ColorSliderContext, ColorWheelContext, HeadingContext} from './RSPContexts'; | |||
|
|
|||
| export {Autocomplete, AutocompleteContext, AutocompleteStateContext, InternalAutocompleteContext} from './Autocomplete'; | |||
| export {UNSTABLE_Autocomplete, UNSTABLE_AutocompleteContext, UNSTABLE_AutocompleteStateContext, UNSTABLE_InternalAutocompleteContext} from './Autocomplete'; | |||
There was a problem hiding this comment.
If it's named InternalAutocompleteContext, is it meant to be exported? I think eventually we'd want to export it but probably with a different name in that case.
There was a problem hiding this comment.
I'll remove it from the exports from now and add it to the followup ticket. Perhaps a name like AutocompleteCollectionContext or something might appropriate
| } | ||
|
|
||
| lastInputValue.current = state.inputValue; | ||
| // TODO: update to see if we can tell what kind of event (paste vs backspace vs typing) is happening instead |
There was a problem hiding this comment.
To be handled in follow up
| // Ref to track whether the first item in the collection should be automatically focused. Specifically used for autocomplete when user types | ||
| // to focus the first key AFTER the collection updates. | ||
| // TODO: potentially expand the usage of this | ||
| let shouldVirtualFocusFirst = useRef(false); |
There was a problem hiding this comment.
A flag has been added to let the collection know if it should focus the first item in the list after various user interactions. This may have other potential use cases (perhaps fixes the delay rendering RAC menu only until the collection is populated) but to be determined later, for now keeping changes scoped to Autocomplete
| let resetFocusFirstFlag = useEffectEvent(() => { | ||
| // If user causes the focused key to change in any other way, clear shouldVirtualFocusFirst so we don't | ||
| // accidentally move focus from under them. Skip this if the collection was empty because we might be in a load | ||
| // state and will still want to focus the first item after load | ||
| if (manager.collection.size > 0) { | ||
| shouldVirtualFocusFirst.current = false; |
There was a problem hiding this comment.
This is a bit interesting since there are cases where the collection is temporarily empty due to a loading state after a user types in the field. We still want focus the first item after the load so we don't want to reset the flag. I believe the check against size should be correct since loading state nodes don't count towards the size of the collection (at least for table/menu/etc) but will need to make sure that remains true moving forward
| args: { | ||
| includeLoadState: true | ||
| } |
There was a problem hiding this comment.
When testing the async story, note this control. This will toggle the existance of a intermediate loading state as the list is filtered
| <Menu | ||
| items={list.items} | ||
| onAction={onAction} | ||
| onSelectionChange={onSelectionChange}> | ||
| {item => <MenuItem id={item.id}>{item.name}</MenuItem>} | ||
| </Menu> |
There was a problem hiding this comment.
We may want to look into giving Menu/Listbox a loading indicator component like Table has. Menu itself doesn't even have the notion of an empty state
There was a problem hiding this comment.
yeah idk if the loader should be on the menu or inside the searchfield like ComboBox/SearchAutocomplete. We might need to talk to design about that one
There was a problem hiding this comment.
I'll make a note to ask them
the collection might change to a loading state and thus have a size of 0. Dont reset focus first flag in that case
LFDanLu
force-pushed
the
autocomplete_audit_for_release
branch
from
ab188cd to
83baefd
Compare
for some reason listbox doesnt have the custom events fire properly when typing the word as a whole
| <Menu | ||
| items={list.items} | ||
| onAction={onAction} | ||
| onSelectionChange={onSelectionChange}> | ||
| {item => <MenuItem id={item.id}>{item.name}</MenuItem>} | ||
| </Menu> |
There was a problem hiding this comment.
yeah idk if the loader should be on the menu or inside the searchfield like ComboBox/SearchAutocomplete. We might need to talk to design about that one
|
totally tiny nit I just noticed when testing, can be fixed later: there seems like there is a slight delay sometimes between when the filtered list updates and when the first item is focused (can be seen in the virtualized list most easily). we may need a |
|
@devongovett I'm not quite sure I'm seeing the delay you are mentioning 😅. I tried the virtualized story and a couple of the other ones, was there a particular sequence of interactions/browsers/etc? |
|
Can't wait for this to be released! Y'all are amazing! 🙌🏻 |
LFDanLu
changed the title
kinda gross that we need to be careful about useLayoutEffect vs useEffect here...
| inputRef | ||
| } = props; | ||
|
|
||
| let collectionId = useId(); | ||
| let timeout = useRef<ReturnType<typeof setTimeout> | undefined>(undefined); | ||
| let delayNextActiveDescendant = useRef(false); | ||
| let queuedActiveDescendant = useRef(null); |
There was a problem hiding this comment.
Added so that useSelectableItem's multiple calls of the UPDATE_ACTIVEDESCENDANT event when auto focusing the first item in the list doesn't remove the delay from setting aria-activedescendant
| // Also set the inputValue to '' to cover Firefox case where Esc doesn't actually clear searchfields. Normally we already | ||
| // handle this in useSearchField, but we are directly setting the inputValue on the input element in RAC Autocomplete instead of | ||
| // passing it to the SearchField via props. This means that a controlled value set on the Autocomplete isn't synced up with the | ||
| // SearchField until the user makes a change to the field's value via typing | ||
| if (e.isPropagationStopped()) { | ||
| state.setInputValue(''); | ||
| return; |
There was a problem hiding this comment.
As noted in the comment, we currently apply the tracked inputValue directly on to the Autocomplete wrapped input element rather than providing it via props to TextField/SearchField so that we can just consume the inputValue from the input context once rather than needing to add it to every input field component in RAC. This means that the input value tracked here in useAutocomplete isn't synced with the input value tracked in useSearchField until the user types into the field to cause the chained onChange to fire. Since Firefox doesn't also clear the native search input fields on Esc we won't get a onChange fired that clears the field so we'll need to do this here. Kinda gross, maybe reason enough to pass value to the input components via props (aka via TextFieldContext/SearchFieldContext) instead of applying it directly to the input element (aka via InputContext like we are doing right now)?
There was a problem hiding this comment.
Does this mean it now happens for textfields in addition to search field? Might be unexpected?
maybe reason enough to pass value to the input components via props (aka via TextFieldContext/SearchFieldContext) instead of applying it directly to the input element (aka via InputContext like we are doing right now)?
yeah maybe. I guess there might be more too - anything that uses <Input>? or would we limit it to those two?
There was a problem hiding this comment.
Does this mean it now happens for textfields in addition to search field? Might be unexpected?
The value is still only cleared for search field for now because we only trigger this if the event coming from the input field has isPropagationStopped.
yeah maybe. I guess there might be more too - anything that uses ? or would we limit it to those two?
Yep, it would need to be anything that uses Input that we could reasonably expect to be used within the Autocomplete which is a bit annoying since we'd need to remember to update if for any new components in the future. For now I'm leaning towards keeping it as is for now unless we find any edge cases/bugs, but will file something to track this
| // Also check the inputRef value for the case where the value was set directly on the input element instead of going through | ||
| // the hook | ||
| if (state.value === '' && (!inputRef.current || inputRef.current.value === '')) { |
There was a problem hiding this comment.
As mentioned above, currently there can be a situation where the value of the input field isn't passed in via props but is instead applied directly onto the input element and thus state.value here could be '' but the value applied on the input is "blah" or something and thus we'd erroneously would continuePropagation. Check the ref's value as a fallback
| // TODO: can't make this useLayoutEffect bacause it breaks menus inside dialogs | ||
| // However, if this is a useEffect, it runs twice and dispatches two UPDATE_ACTIVEDESCENDANT and immediately sets | ||
| // aria-activeDescendant in useAutocomplete... I've worked around this for now | ||
| useEffect(() => { |
There was a problem hiding this comment.
Bit of an interesting thing I discovered. If this is a useLayoutEffect then it won't run twice when auto focusing the first item of a newly filtered collection presumably because it runs in the same phase as the useUpdateLayoutEffect in useSelectableCollection. However, this breaks menus in dialogs where keyboard opening the menu causes the dialog itself to close...
| import {useLayoutEffect} from './useLayoutEffect'; | ||
|
|
||
| // Like useLayoutEffect, but only called for updates after the initial render. | ||
| export function useUpdateLayoutEffect(effect: EffectCallback, dependencies: any[]) { |
There was a problem hiding this comment.
Added to get rid of some flickering when auto focusing the first element in the list
…ete_audit_for_release
|
Hey friends 👋🏻 Any rough estimation on when this feature might get released? Thanks again for all your amazing work! |
|
@romansndlr Hopefully within this month with the next release! |
There was a problem hiding this comment.
LGTM. One behavior we may want to consider revisiting is when you press Left/Right arrow while an item is focused and the input is empty. Focus goes back up to the input, but we might want to keep it on the item in this case. Not a huge deal, just something to consider.
…ete_audit_for_release
## 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-components:UNSTABLE_Autocomplete+UNSTABLE_Autocomplete {
+ children: ReactNode
+ defaultInputValue?: string
+ filter?: (string, string) => boolean
+ inputValue?: string
+ onInputChange?: (string) => void
+ slot?: string | null
+}/react-aria-components:UNSTABLE_AutocompleteContext+UNSTABLE_AutocompleteContext {
+ UNTYPED
+}/react-aria-components:UNSTABLE_AutocompleteStateContext+UNSTABLE_AutocompleteStateContext {
+ 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
+ filter?: (string, string) => boolean
inputValue?: string
onInputChange?: (string) => void
}/@react-aria/autocomplete:AriaAutocompleteOptions AriaAutocompleteOptions {
collectionRef: RefObject<HTMLElement | null>
- defaultFilter?: (string, string) => boolean = contains
defaultInputValue?: string
+ filter?: (string, string) => boolean
inputRef: RefObject<HTMLInputElement | null>
inputValue?: string
onInputChange?: (string) => void
}/@react-aria/autocomplete:AutocompleteAria AutocompleteAria {
collectionProps: CollectionOptions
collectionRef: RefObject<HTMLElement | null>
- filterFn: (string) => boolean
+ filterFn?: (string) => boolean
inputProps: InputHTMLAttributes<HTMLInputElement>
}/@react-aria/autocomplete:UNSTABLE_useAutocomplete+UNSTABLE_useAutocomplete {
+ props: AriaAutocompleteOptions
+ state: AutocompleteState
+ returnVal: undefined
+}@react-aria/utils/@react-aria/utils:useUpdateLayoutEffect+useUpdateLayoutEffect {
+ effect: EffectCallback
+ dependencies: Array<any>
+ returnVal: undefined
+}/@react-aria/utils:isCtrlKeyPressed+isCtrlKeyPressed {
+ e: Event
+ returnVal: undefined
+}@react-stately/autocomplete/@react-stately/autocomplete:useAutocompleteState-useAutocompleteState {
- props: AutocompleteStateOptions
- returnVal: undefined
-}/@react-stately/autocomplete:UNSTABLE_useAutocompleteState+UNSTABLE_useAutocompleteState {
+ props: AutocompleteStateOptions
+ returnVal: undefined
+}@react-stately/selection/@react-stately/selection:MultipleSelectionManager MultipleSelectionManager {
canSelectItem: (Key) => boolean
childFocusStrategy: FocusStrategy | null
clearSelection: () => void
+ collection: Collection<Node<unknown>>
disabledBehavior: DisabledBehavior
disabledKeys: Set<Key>
disallowEmptySelection?: boolean
extendSelection: (Key) => void
focusedKey: Key | null
getItemProps: (Key) => any
isDisabled: (Key) => boolean
isEmpty: boolean
isFocused: boolean
isLink: (Key) => boolean
isSelectAll: boolean
isSelected: (Key) => boolean
isSelectionEqual: (Set<Key>) => boolean
lastSelectedKey: Key | null
replaceSelection: (Key) => void
select: (Key, PressEvent | LongPressEvent | PointerEvent) => void
selectAll: () => void
selectedKeys: Set<Key>
selectionBehavior: SelectionBehavior
selectionMode: SelectionMode
setFocused: (boolean) => void
setFocusedKey: (Key | null, FocusStrategy) => void
setSelectedKeys: (Iterable<Key>) => void
setSelectionBehavior: (SelectionBehavior) => void
toggleSelectAll: () => void
toggleSelection: (Key) => void
} |
Closes
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project:
RSP