Skip to content

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

Merged
merged 13 commits into from
Jan 6, 2025
Merged
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
2 changes: 1 addition & 1 deletion packages/@react-aria/autocomplete/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
* governing permissions and limitations under the License.
*/
export {useSearchAutocomplete} from './useSearchAutocomplete';
export {useAutocomplete} from './useAutocomplete';
export {UNSTABLE_useAutocomplete} from './useAutocomplete';

export type {AriaSearchAutocompleteOptions, SearchAutocompleteAria} from './useSearchAutocomplete';
export type {AriaSearchAutocompleteProps} from '@react-types/autocomplete';
Expand Down
70 changes: 42 additions & 28 deletions packages/@react-aria/autocomplete/src/useAutocomplete.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand All @@ -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'> {
Expand All @@ -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
}

/**
Expand All @@ -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);
Copy link
Member Author

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 setting aria-activedescendant

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);
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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>>) => {
Expand All @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

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)?

Copy link
Member

Choose a reason for hiding this comment

The 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?

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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

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

}
break;
Expand Down Expand Up @@ -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',
Expand All @@ -273,6 +287,6 @@ export function useAutocomplete(props: AriaAutocompleteOptions, state: Autocompl
disallowTypeAhead: true
}),
collectionRef: mergedCollectionRef,
filterFn
filterFn: filter != null ? filterFn : undefined
};
}
4 changes: 3 additions & 1 deletion packages/@react-aria/searchfield/src/useSearchField.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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 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

e.continuePropagation();
} else {
state.setValue('');
Expand Down
57 changes: 45 additions & 12 deletions packages/@react-aria/selection/src/useSelectableCollection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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
Expand All @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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();
}, [manager.focusedKey, resetFocusFirstFlag]);

useEvent(ref, CLEAR_FOCUS_EVENT, !shouldUseVirtualFocus ? undefined : (e) => {
e.stopPropagation();
manager.setFocused(false);
Expand Down
7 changes: 5 additions & 2 deletions packages/@react-aria/selection/src/useSelectableItem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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 useUpdateLayoutEffect in useSelectableCollection. However, this breaks menus in dialogs where keyboard opening the menu causes the dialog itself to close...

let isFocused = key === manager.focusedKey;
if (isFocused && manager.isFocused) {
Expand Down
10 changes: 1 addition & 9 deletions packages/@react-aria/selection/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
* governing permissions and limitations under the License.
*/

import {isAppleDevice, isMac} from '@react-aria/utils';
import {isAppleDevice} from '@react-aria/utils';

interface Event {
altKey: boolean,
Expand All @@ -23,11 +23,3 @@ export function isNonContiguousSelectionModifier(e: Event) {
// On Windows and Ubuntu, Alt + Space has a system wide meaning.
return isAppleDevice() ? e.altKey : e.ctrlKey;
}

export function isCtrlKeyPressed(e: Event) {
if (isMac()) {
return e.metaKey;
}

return e.ctrlKey;
}
2 changes: 2 additions & 0 deletions packages/@react-aria/utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export {useGlobalListeners} from './useGlobalListeners';
export {useLabels} from './useLabels';
export {useObjectRef} from './useObjectRef';
export {useUpdateEffect} from './useUpdateEffect';
export {useUpdateLayoutEffect} from './useUpdateLayoutEffect';
export {useLayoutEffect} from './useLayoutEffect';
export {useResizeObserver} from './useResizeObserver';
export {useSyncRef} from './useSyncRef';
Expand All @@ -43,4 +44,5 @@ export {useDeepMemo} from './useDeepMemo';
export {useFormReset} from './useFormReset';
export {useLoadMore} from './useLoadMore';
export {CLEAR_FOCUS_EVENT, FOCUS_EVENT, UPDATE_ACTIVEDESCENDANT} from './constants';
export {isCtrlKeyPressed} from './keyboard';
export {useEnterAnimation, useExitAnimation} from './animation';
27 changes: 27 additions & 0 deletions packages/@react-aria/utils/src/keyboard.tsx
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;
}
37 changes: 37 additions & 0 deletions packages/@react-aria/utils/src/useUpdateLayoutEffect.ts
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[]) {
Copy link
Member Author

Choose a reason for hiding this comment

The 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);
}
2 changes: 1 addition & 1 deletion packages/@react-stately/autocomplete/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@
* governing permissions and limitations under the License.
*/

export {useAutocompleteState} from './useAutocompleteState';
export {UNSTABLE_useAutocompleteState} from './useAutocompleteState';

export type {AutocompleteProps, AutocompleteStateOptions, AutocompleteState} from './useAutocompleteState';
Loading
Loading