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

fix: Properly clear Autocomplete wrapped SearchField when "x" button is pressed #7606

Merged
merged 6 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions packages/@react-aria/autocomplete/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
"@react-aria/interactions": "^3.22.5",
"@react-aria/listbox": "^3.13.6",
"@react-aria/searchfield": "^3.7.11",
"@react-aria/textfield": "^3.15.0",
"@react-aria/utils": "^3.26.0",
"@react-stately/autocomplete": "3.0.0-alpha.1",
"@react-stately/combobox": "^3.10.1",
Expand Down
27 changes: 12 additions & 15 deletions packages/@react-aria/autocomplete/src/useAutocomplete.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@
*/

import {AriaLabelingProps, BaseEvent, DOMProps, RefObject} from '@react-types/shared';
import {AriaTextFieldProps} from '@react-aria/textfield';
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, isCtrlKeyPressed, mergeProps, mergeRefs, UPDATE_ACTIVEDESCENDANT, useEffectEvent, useId, useLabels, useObjectRef} from '@react-aria/utils';
import {InputHTMLAttributes, KeyboardEvent as ReactKeyboardEvent, useCallback, useEffect, useMemo, useRef} from 'react';
// @ts-ignore
import intlMessages from '../intl/*.json';
import {useKeyboard} from '@react-aria/interactions';
import {useLocalizedStringFormatter} from '@react-aria/i18n';

export interface CollectionOptions extends DOMProps, AriaLabelingProps {
Expand All @@ -43,6 +43,8 @@ export interface AriaAutocompleteOptions extends Omit<AriaAutocompleteProps, 'ch
export interface AutocompleteAria {
/** Props for the autocomplete input element. */
inputProps: InputHTMLAttributes<HTMLInputElement>,
/** Props for the autocomplete textfield/searchfield element. These should be passed to the textfield/searchfield aria hooks respectively. */
textFieldProps: AriaTextFieldProps,
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 bit non-standard to have both inputProps and textFieldProps (at least I don't think we have something like this in other hooks), but some props should be passed to the hooks (onKeyDown, onChange, value) so that SearchField's clear button works properly and the controlled input value is synced between this hook and useSearchField. Other stuff like aria-controls needs to go directly on the input and aren't regarded as valid props in AriaTextFieldProps hence the split. I could expand AriaTextFieldProps to accepts those attributes instead if want to go that direction

Copy link
Member

@snowystinger snowystinger Jan 14, 2025

Choose a reason for hiding this comment

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

Wouldn't SearchField/TextField's Contexts interfere with InputProps anyways? so really there are three components we're potentially targeting, two of them just happen to use the same set of props?

Copy link
Member

Choose a reason for hiding this comment

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

hmm maybe we should add those props to AriaTextfieldProps. Then we could remove the weird merging with InputContext too. Looks like just aria-controls, autoCorrect, and spellCheck are missing? Those seem like reasonable props for TextField to support.

/** Props for the collection, to be passed to collection's respective aria hook (e.g. useMenu). */
collectionProps: CollectionOptions,
/** Ref to attach to the wrapped collection. */
Expand Down Expand Up @@ -138,16 +140,16 @@ export function UNSTABLE_useAutocomplete(props: AriaAutocompleteOptions, state:
});

// TODO: update to see if we can tell what kind of event (paste vs backspace vs typing) is happening instead
let onChange = (e: ChangeEvent<HTMLInputElement>) => {
let onChange = (value: string) => {
// 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) {
if (state.inputValue !== value && state.inputValue.length <= value.length) {
focusFirstItem();
} else {
clearVirtualFocus();
}

state.setInputValue(e.target.value);
state.setInputValue(value);
};

// For textfield specific keydown operations
Expand All @@ -166,12 +168,7 @@ export function UNSTABLE_useAutocomplete(props: AriaAutocompleteOptions, state:
// 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()) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed?

state.setInputValue('');
return;
}
break;
Expand Down Expand Up @@ -249,8 +246,6 @@ export function UNSTABLE_useAutocomplete(props: AriaAutocompleteOptions, state:
};
}, [inputRef, onKeyUpCapture]);

let {keyboardProps} = useKeyboard({onKeyDown});

let stringFormatter = useLocalizedStringFormatter(intlMessages, '@react-aria/autocomplete');
let collectionProps = useLabels({
id: collectionId,
Expand All @@ -267,9 +262,6 @@ export function UNSTABLE_useAutocomplete(props: AriaAutocompleteOptions, state:

return {
inputProps: {
value: state.inputValue,
onChange,
...keyboardProps,
autoComplete: 'off',
'aria-haspopup': 'listbox',
'aria-controls': collectionId,
Expand All @@ -281,6 +273,11 @@ export function UNSTABLE_useAutocomplete(props: AriaAutocompleteOptions, state:
// This disable's the macOS Safari spell check auto corrections.
spellCheck: 'false'
},
textFieldProps: {
value: state.inputValue,
onChange,
onKeyDown
},
collectionProps: mergeProps(collectionProps, {
// TODO: shouldFocusOnHover? shouldFocusWrap? Should it be up to the wrapped collection?
shouldUseVirtualFocus: true,
Expand Down
5 changes: 5 additions & 0 deletions packages/react-aria-components/src/Autocomplete.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import {InputContext} from './Input';
import {mergeProps} from '@react-aria/utils';
import {Provider, removeDataAttributes, SlotProps, SlottedContextValue, useSlottedContext} from './utils';
import React, {createContext, RefObject, useRef} from 'react';
import {SearchFieldContext} from './SearchField';
import {TextFieldContext} from './TextField';

export interface AutocompleteProps extends AriaAutocompleteProps, SlotProps {}

Expand Down Expand Up @@ -44,6 +46,7 @@ export function UNSTABLE_Autocomplete(props: AutocompleteProps) {

let {
inputProps,
textFieldProps,
collectionProps,
collectionRef: mergedCollectionRef,
filterFn
Expand All @@ -58,6 +61,8 @@ export function UNSTABLE_Autocomplete(props: AutocompleteProps) {
<Provider
values={[
[UNSTABLE_AutocompleteStateContext, state],
[SearchFieldContext, {...textFieldProps}],
[TextFieldContext, {...textFieldProps}],
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't need the spread?

[InputContext, {...inputProps, ref: inputRef}],
[UNSTABLE_InternalAutocompleteContext, {
filterFn,
Expand Down
23 changes: 22 additions & 1 deletion packages/react-aria-components/test/Autocomplete.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
*/

import {AriaAutocompleteTests} from './AriaAutocomplete.test-util';
import {Header, Input, Label, ListBox, ListBoxItem, ListBoxSection, Menu, MenuItem, MenuSection, SearchField, Separator, Text, UNSTABLE_Autocomplete} from '..';
import {Button, Header, Input, Label, ListBox, ListBoxItem, ListBoxSection, Menu, MenuItem, MenuSection, SearchField, Separator, Text, UNSTABLE_Autocomplete} from '..';
import {pointerMap, render} from '@react-spectrum/test-utils-internal';
import React, {ReactNode} from 'react';
import {useAsyncList} from 'react-stately';
Expand Down Expand Up @@ -110,6 +110,7 @@ let AutocompleteWrapper = ({autocompleteProps = {}, inputProps = {}, children}:
<SearchField {...inputProps}>
<Label style={{display: 'block'}}>Test</Label>
<Input />
<Button>✕</Button>
<Text style={{display: 'block'}} slot="description">Please select an option below.</Text>
</SearchField>
{children}
Expand Down Expand Up @@ -199,6 +200,26 @@ describe('Autocomplete', () => {
expect(onKeyDown).not.toHaveBeenCalled();
onKeyDown.mockReset();
});

it('should prevent clear the input field when clicking on the clear button', async () => {
let {getByRole} = render(
<AutocompleteWrapper>
<StaticMenu />
</AutocompleteWrapper>
);

let input = getByRole('searchbox');
await user.tab();
expect(document.activeElement).toBe(input);
await user.keyboard('Foo');
expect(input).toHaveValue('Foo');

let button = getByRole('button');
expect(button).toHaveAttribute('aria-label', 'Clear search');
await user.click(button);

expect(input).toHaveValue('');
});
});

AriaAutocompleteTests({
Expand Down
1 change: 1 addition & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5916,6 +5916,7 @@ __metadata:
"@react-aria/interactions": "npm:^3.22.5"
"@react-aria/listbox": "npm:^3.13.6"
"@react-aria/searchfield": "npm:^3.7.11"
"@react-aria/textfield": "npm:^3.15.0"
"@react-aria/utils": "npm:^3.26.0"
"@react-stately/autocomplete": "npm:3.0.0-alpha.1"
"@react-stately/combobox": "npm:^3.10.1"
Expand Down
Loading