-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: RAC Autocomplete audit for release #7475
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
Changes from all commits
5e88a7d
515092a
216dbbc
83baefd
4bc93a7
a7ca39a
3ac236e
1842f2e
36d49f0
6fa6f26
675c41f
1bd7004
571c4dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,11 +13,11 @@ | |
import {AriaLabelingProps, BaseEvent, DOMProps, RefObject} from '@react-types/shared'; | ||
import {AutocompleteProps, AutocompleteState} from '@react-stately/autocomplete'; | ||
import {ChangeEvent, InputHTMLAttributes, KeyboardEvent as ReactKeyboardEvent, useCallback, useEffect, useMemo, useRef} from 'react'; | ||
import {CLEAR_FOCUS_EVENT, FOCUS_EVENT, mergeProps, mergeRefs, UPDATE_ACTIVEDESCENDANT, useEffectEvent, useId, useLabels, useObjectRef} from '@react-aria/utils'; | ||
import {CLEAR_FOCUS_EVENT, FOCUS_EVENT, isCtrlKeyPressed, mergeProps, mergeRefs, UPDATE_ACTIVEDESCENDANT, useEffectEvent, useId, useLabels, useObjectRef} from '@react-aria/utils'; | ||
// @ts-ignore | ||
import intlMessages from '../intl/*.json'; | ||
import {useFilter, useLocalizedStringFormatter} from '@react-aria/i18n'; | ||
import {useKeyboard} from '@react-aria/interactions'; | ||
import {useLocalizedStringFormatter} from '@react-aria/i18n'; | ||
|
||
export interface CollectionOptions extends DOMProps, AriaLabelingProps { | ||
/** Whether the collection items should use virtual focus instead of being focused directly. */ | ||
|
@@ -27,10 +27,10 @@ export interface CollectionOptions extends DOMProps, AriaLabelingProps { | |
} | ||
export interface AriaAutocompleteProps extends AutocompleteProps { | ||
/** | ||
* The filter function used to determine if a option should be included in the autocomplete list. | ||
* @default contains | ||
* An optional filter function used to determine if a option should be included in the autocomplete list. | ||
* Include this if the items you are providing to your wrapped collection aren't filtered by default. | ||
*/ | ||
defaultFilter?: (textValue: string, inputValue: string) => boolean | ||
filter?: (textValue: string, inputValue: string) => boolean | ||
} | ||
|
||
export interface AriaAutocompleteOptions extends Omit<AriaAutocompleteProps, 'children'> { | ||
|
@@ -48,7 +48,7 @@ export interface AutocompleteAria { | |
/** Ref to attach to the wrapped collection. */ | ||
collectionRef: RefObject<HTMLElement | null>, | ||
/** A filter function that returns if the provided collection node should be filtered out of the collection. */ | ||
filterFn: (nodeTextValue: string) => boolean | ||
filterFn?: (nodeTextValue: string) => boolean | ||
} | ||
|
||
/** | ||
|
@@ -57,27 +57,34 @@ export interface AutocompleteAria { | |
* @param props - Props for the autocomplete. | ||
* @param state - State for the autocomplete, as returned by `useAutocompleteState`. | ||
*/ | ||
export function useAutocomplete(props: AriaAutocompleteOptions, state: AutocompleteState): AutocompleteAria { | ||
export function UNSTABLE_useAutocomplete(props: AriaAutocompleteOptions, state: AutocompleteState): AutocompleteAria { | ||
let { | ||
collectionRef, | ||
defaultFilter, | ||
filter, | ||
inputRef | ||
} = props; | ||
|
||
let collectionId = useId(); | ||
let timeout = useRef<ReturnType<typeof setTimeout> | undefined>(undefined); | ||
let delayNextActiveDescendant = useRef(false); | ||
let queuedActiveDescendant = useRef(null); | ||
let lastCollectionNode = useRef<HTMLElement>(null); | ||
|
||
let updateActiveDescendant = useEffectEvent((e) => { | ||
let {target} = e; | ||
if (queuedActiveDescendant.current === target.id) { | ||
return; | ||
} | ||
|
||
clearTimeout(timeout.current); | ||
e.stopPropagation(); | ||
|
||
if (target !== collectionRef.current) { | ||
if (delayNextActiveDescendant.current) { | ||
queuedActiveDescendant.current = target.id; | ||
timeout.current = setTimeout(() => { | ||
state.setFocusedNodeId(target.id); | ||
queuedActiveDescendant.current = null; | ||
}, 500); | ||
} else { | ||
state.setFocusedNodeId(target.id); | ||
|
@@ -130,20 +137,18 @@ export function useAutocomplete(props: AriaAutocompleteOptions, state: Autocompl | |
collectionRef.current?.dispatchEvent(clearFocusEvent); | ||
}); | ||
|
||
// Tell wrapped collection to focus the first element in the list when typing forward and to clear focused key when deleting text | ||
// for screen reader announcements | ||
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) { | ||
focusFirstItem(); | ||
} else { | ||
clearVirtualFocus(); | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be handled in follow up |
||
let onChange = (e: ChangeEvent<HTMLInputElement>) => { | ||
// Tell wrapped collection to focus the first element in the list when typing forward and to clear focused key when deleting text | ||
// for screen reader announcements | ||
if (state.inputValue !== e.target.value && state.inputValue.length <= e.target.value.length) { | ||
focusFirstItem(); | ||
} else { | ||
clearVirtualFocus(); | ||
} | ||
}, [state.inputValue, focusFirstItem, clearVirtualFocus]); | ||
|
||
state.setInputValue(e.target.value); | ||
}; | ||
|
||
// For textfield specific keydown operations | ||
let onKeyDown = (e: BaseEvent<ReactKeyboardEvent<any>>) => { | ||
|
@@ -152,11 +157,21 @@ export function useAutocomplete(props: AriaAutocompleteOptions, state: Autocompl | |
} | ||
|
||
switch (e.key) { | ||
case 'a': | ||
if (isCtrlKeyPressed(e)) { | ||
return; | ||
} | ||
break; | ||
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 | ||
// 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; | ||
Comment on lines
+169
to
175
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As noted in the comment, we currently apply the tracked There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean it now happens for textfields in addition to search field? Might be unexpected?
yeah maybe. I guess there might be more too - anything that uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
Yep, it would need to be anything that uses |
||
} | ||
break; | ||
|
@@ -242,19 +257,18 @@ export function useAutocomplete(props: AriaAutocompleteOptions, state: Autocompl | |
'aria-label': stringFormatter.format('collectionLabel') | ||
}); | ||
|
||
let {contains} = useFilter({sensitivity: 'base'}); | ||
let filterFn = useCallback((nodeTextValue: string) => { | ||
if (defaultFilter) { | ||
return defaultFilter(nodeTextValue, state.inputValue); | ||
if (filter) { | ||
return filter(nodeTextValue, state.inputValue); | ||
} | ||
|
||
return contains(nodeTextValue, state.inputValue); | ||
}, [state.inputValue, defaultFilter, contains]) ; | ||
return true; | ||
}, [state.inputValue, filter]); | ||
|
||
return { | ||
inputProps: { | ||
value: state.inputValue, | ||
onChange: (e: ChangeEvent<HTMLInputElement>) => state.setInputValue(e.target.value), | ||
onChange, | ||
...keyboardProps, | ||
autoComplete: 'off', | ||
'aria-haspopup': 'listbox', | ||
|
@@ -273,6 +287,6 @@ export function useAutocomplete(props: AriaAutocompleteOptions, state: Autocompl | |
disallowTypeAhead: true | ||
}), | ||
collectionRef: mergedCollectionRef, | ||
filterFn | ||
filterFn: filter != null ? filterFn : undefined | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,7 +73,9 @@ export function useSearchField( | |
} | ||
|
||
if (key === 'Escape') { | ||
if (state.value === '') { | ||
// 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 === '')) { | ||
Comment on lines
+76
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
e.continuePropagation(); | ||
} else { | ||
state.setValue(''); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,13 +10,13 @@ | |
* governing permissions and limitations under the License. | ||
*/ | ||
|
||
import {CLEAR_FOCUS_EVENT, FOCUS_EVENT, focusWithoutScrolling, mergeProps, scrollIntoView, scrollIntoViewport, UPDATE_ACTIVEDESCENDANT, useEvent, useRouter} from '@react-aria/utils'; | ||
import {CLEAR_FOCUS_EVENT, FOCUS_EVENT, focusWithoutScrolling, isCtrlKeyPressed, mergeProps, scrollIntoView, scrollIntoViewport, UPDATE_ACTIVEDESCENDANT, useEffectEvent, useEvent, useRouter, useUpdateLayoutEffect} from '@react-aria/utils'; | ||
import {DOMAttributes, FocusableElement, FocusStrategy, Key, KeyboardDelegate, RefObject} from '@react-types/shared'; | ||
import {flushSync} from 'react-dom'; | ||
import {FocusEvent, KeyboardEvent, useEffect, useRef} from 'react'; | ||
import {focusSafely, getFocusableTreeWalker} from '@react-aria/focus'; | ||
import {getInteractionModality} from '@react-aria/interactions'; | ||
import {isCtrlKeyPressed, isNonContiguousSelectionModifier} from './utils'; | ||
import {isNonContiguousSelectionModifier} from './utils'; | ||
import {MultipleSelectionManager} from '@react-stately/selection'; | ||
import {useLocale} from '@react-aria/i18n'; | ||
import {useTypeSelect} from './useTypeSelect'; | ||
|
@@ -391,6 +391,10 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions | |
} | ||
}; | ||
|
||
// 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); | ||
Comment on lines
+394
to
+397
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
// Add event listeners for custom virtual events. These handle updating the focused key in response to various keyboard events | ||
// at the autocomplete level | ||
// TODO: fix type later | ||
|
@@ -401,21 +405,50 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions | |
|
||
// If the user is typing forwards, autofocus the first option in the list. | ||
if (detail?.focusStrategy === 'first') { | ||
let keyToFocus = delegate.getFirstKey?.() ?? null; | ||
// If no focusable items exist in the list, make sure to clear any activedescendant that may still exist | ||
if (keyToFocus == null) { | ||
ref.current?.dispatchEvent( | ||
new CustomEvent(UPDATE_ACTIVEDESCENDANT, { | ||
cancelable: true, | ||
bubbles: true | ||
}) | ||
); | ||
} | ||
shouldVirtualFocusFirst.current = true; | ||
} | ||
}); | ||
|
||
let updateActiveDescendant = useEffectEvent(() => { | ||
let keyToFocus = delegate.getFirstKey?.() ?? null; | ||
|
||
// If no focusable items exist in the list, make sure to clear any activedescendant that may still exist | ||
if (keyToFocus == null) { | ||
ref.current?.dispatchEvent( | ||
new CustomEvent(UPDATE_ACTIVEDESCENDANT, { | ||
cancelable: true, | ||
bubbles: true | ||
}) | ||
); | ||
} else { | ||
manager.setFocusedKey(keyToFocus); | ||
// Only set shouldVirtualFocusFirst to false if we've successfully set the first key as the focused key | ||
// If there wasn't a key to focus, we might be in a temporary loading state so we'll want to still focus the first key | ||
// after the collection updates after load | ||
shouldVirtualFocusFirst.current = false; | ||
} | ||
}); | ||
|
||
useUpdateLayoutEffect(() => { | ||
if (shouldVirtualFocusFirst.current) { | ||
updateActiveDescendant(); | ||
} | ||
|
||
}, [manager.collection, updateActiveDescendant]); | ||
|
||
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; | ||
Comment on lines
+439
to
+444
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
}); | ||
|
||
useUpdateLayoutEffect(() => { | ||
resetFocusFirstFlag(); | ||
LFDanLu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}, [manager.focusedKey, resetFocusFirstFlag]); | ||
|
||
useEvent(ref, CLEAR_FOCUS_EVENT, !shouldUseVirtualFocus ? undefined : (e) => { | ||
e.stopPropagation(); | ||
manager.setFocused(false); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,8 +12,8 @@ | |
|
||
import {DOMAttributes, DOMProps, FocusableElement, Key, LongPressEvent, PointerType, PressEvent, RefObject} from '@react-types/shared'; | ||
import {focusSafely} from '@react-aria/focus'; | ||
import {isCtrlKeyPressed, isNonContiguousSelectionModifier} from './utils'; | ||
import {mergeProps, openLink, UPDATE_ACTIVEDESCENDANT, useId, useRouter} from '@react-aria/utils'; | ||
import {isCtrlKeyPressed, mergeProps, openLink, UPDATE_ACTIVEDESCENDANT, useId, useRouter} from '@react-aria/utils'; | ||
import {isNonContiguousSelectionModifier} from './utils'; | ||
import {MultipleSelectionManager} from '@react-stately/selection'; | ||
import {PressProps, useLongPress, usePress} from '@react-aria/interactions'; | ||
import {useEffect, useRef} from 'react'; | ||
|
@@ -160,6 +160,9 @@ export function useSelectableItem(options: SelectableItemOptions): SelectableIte | |
}; | ||
|
||
// Focus the associated DOM node when this item becomes the focusedKey | ||
// 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(() => { | ||
Comment on lines
+163
to
166
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
let isFocused = key === manager.focusedKey; | ||
if (isFocused && manager.isFocused) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
/* | ||
* Copyright 2024 Adobe. All rights reserved. | ||
* This file is licensed to you under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. You may obtain a copy | ||
* of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software distributed under | ||
* the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS | ||
* OF ANY KIND, either express or implied. See the License for the specific language | ||
* governing permissions and limitations under the License. | ||
*/ | ||
|
||
import {isMac} from './platform'; | ||
|
||
interface Event { | ||
altKey: boolean, | ||
ctrlKey: boolean, | ||
metaKey: boolean | ||
} | ||
|
||
export function isCtrlKeyPressed(e: Event) { | ||
if (isMac()) { | ||
return e.metaKey; | ||
} | ||
|
||
return e.ctrlKey; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
/* | ||
* Copyright 2020 Adobe. All rights reserved. | ||
* This file is licensed to you under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. You may obtain a copy | ||
* of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software distributed under | ||
* the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS | ||
* OF ANY KIND, either express or implied. See the License for the specific language | ||
* governing permissions and limitations under the License. | ||
*/ | ||
|
||
import {EffectCallback, useRef} from 'react'; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added to get rid of some flickering when auto focusing the first element in the list |
||
const isInitialMount = useRef(true); | ||
const lastDeps = useRef<any[] | null>(null); | ||
|
||
useLayoutEffect(() => { | ||
isInitialMount.current = true; | ||
return () => { | ||
isInitialMount.current = false; | ||
}; | ||
}, []); | ||
|
||
useLayoutEffect(() => { | ||
if (isInitialMount.current) { | ||
isInitialMount.current = false; | ||
} else if (!lastDeps.current || dependencies.some((dep, i) => !Object.is(dep, lastDeps[i]))) { | ||
effect(); | ||
} | ||
lastDeps.current = dependencies; | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, dependencies); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 settingaria-activedescendant