Skip to content
Open
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
50 changes: 42 additions & 8 deletions packages/@react-aria/combobox/src/useComboBox.ts
Copy link
Member

Choose a reason for hiding this comment

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

what do you think about allowing users to deselect from the combobox menu via Enter on a selected item? Ariakit's combobox has similar behavior to that, and it would be more flexible for keyboard users in case a taggroup wasn't rendered alongside the ComboBox (though I'm not sure how common that would be)

Copy link
Member Author

Choose a reason for hiding this comment

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

mmm yeah we should support that.

Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@

import {announce} from '@react-aria/live-announcer';
import {AriaButtonProps} from '@react-types/button';
import {AriaComboBoxProps} from '@react-types/combobox';
import {AriaComboBoxProps, SelectionMode} from '@react-types/combobox';
import {ariaHideOutside} from '@react-aria/overlays';
import {AriaListBoxOptions, getItemId, listData} from '@react-aria/listbox';
import {BaseEvent, DOMAttributes, KeyboardDelegate, LayoutDelegate, PressEvent, RefObject, RouterOptions, ValidationResult} from '@react-types/shared';
import {chain, getActiveElement, getOwnerDocument, isAppleDevice, mergeProps, nodeContains, useEvent, useFormReset, useLabels, useRouter, useUpdateEffect} from '@react-aria/utils';
import {chain, getActiveElement, getOwnerDocument, isAppleDevice, mergeProps, nodeContains, useEvent, useFormReset, useId, useLabels, useRouter, useUpdateEffect} from '@react-aria/utils';
import {ComboBoxState} from '@react-stately/combobox';
import {dispatchVirtualFocus} from '@react-aria/focus';
import {FocusEvent, InputHTMLAttributes, KeyboardEvent, TouchEvent, useEffect, useMemo, useRef} from 'react';
import {FocusEvent, InputHTMLAttributes, KeyboardEvent, TouchEvent, useEffect, useMemo, useRef, useState} from 'react';
import {getChildNodes, getItemCount} from '@react-stately/collections';
// @ts-ignore
import intlMessages from '../intl/*.json';
Expand All @@ -29,7 +29,7 @@ import {useLocalizedStringFormatter} from '@react-aria/i18n';
import {useMenuTrigger} from '@react-aria/menu';
import {useTextField} from '@react-aria/textfield';

export interface AriaComboBoxOptions<T> extends Omit<AriaComboBoxProps<T>, 'children'> {
export interface AriaComboBoxOptions<T, M extends SelectionMode = 'single'> extends Omit<AriaComboBoxProps<T, M>, 'children'> {
/** The ref for the input element. */
inputRef: RefObject<HTMLInputElement | null>,
/** The ref for the list box popover. */
Expand Down Expand Up @@ -57,6 +57,8 @@ export interface ComboBoxAria<T> extends ValidationResult {
listBoxProps: AriaListBoxOptions<T>,
/** Props for the optional trigger button, to be passed to `useButton`. */
buttonProps: AriaButtonProps,
/** Props for the element representing the selected value. */
valueProps: DOMAttributes,
/** Props for the combo box description element, if any. */
descriptionProps: DOMAttributes,
/** Props for the combo box error message element, if any. */
Expand All @@ -69,7 +71,7 @@ export interface ComboBoxAria<T> extends ValidationResult {
* @param props - Props for the combo box.
* @param state - State for the select, as returned by `useComboBoxState`.
*/
export function useComboBox<T>(props: AriaComboBoxOptions<T>, state: ComboBoxState<T>): ComboBoxAria<T> {
export function useComboBox<T, M extends SelectionMode = 'single'>(props: AriaComboBoxOptions<T, M>, state: ComboBoxState<T, M>): ComboBoxAria<T> {
let {
buttonRef,
popoverRef,
Expand Down Expand Up @@ -158,7 +160,7 @@ export function useComboBox<T>(props: AriaComboBoxOptions<T>, state: ComboBoxSta
break;
case 'Escape':
if (
state.selectedKey !== null ||
!state.selectionManager.isEmpty ||
state.inputValue === '' ||
props.allowsCustomValue
) {
Expand Down Expand Up @@ -206,6 +208,7 @@ export function useComboBox<T>(props: AriaComboBoxOptions<T>, state: ComboBoxSta
state.setFocused(true);
};

let valueId = useValueId([state.selectedItems, state.selectionManager.selectionMode]);
let {isInvalid, validationErrors, validationDetails} = state.displayValidation;
let {labelProps, inputProps, descriptionProps, errorMessageProps} = useTextField({
...props,
Expand All @@ -217,10 +220,11 @@ export function useComboBox<T>(props: AriaComboBoxOptions<T>, state: ComboBoxSta
onFocus,
autoComplete: 'off',
validate: undefined,
[privateValidationStateProp]: state
[privateValidationStateProp]: state,
'aria-describedby': [valueId, props['aria-describedby']].filter(Boolean).join(' ') || undefined
}, inputRef);

useFormReset(inputRef, state.defaultSelectedKey, state.setSelectedKey);
useFormReset(inputRef, state.defaultValue, state.setValue);

// Press handlers for the ComboBox button
let onPress = (e: PressEvent) => {
Expand Down Expand Up @@ -332,6 +336,7 @@ export function useComboBox<T>(props: AriaComboBoxOptions<T>, state: ComboBoxSta
});

// Announce when a selection occurs for VoiceOver. Other screen readers typically do this automatically.
// TODO: do we need to do this for multi-select?
Copy link
Member Author

Choose a reason for hiding this comment

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

Right now I just hear "51 options available", which interrupts the default selection announcement from VO. But I also don't hear this with single select. We should discuss whether we can just remove the custom announcements entirely at this point.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense, I think ComboBox is overdue for an accessibility sweep anyways since there have been reported issues against Talkback, the whole autocomplete announcement issue with auto focusing the first option on focus, etc

let lastSelectedKey = useRef(state.selectedKey);
useEffect(() => {
if (isAppleDevice() && state.isFocused && state.selectedItem && state.selectedKey !== lastSelectedKey.current) {
Expand Down Expand Up @@ -392,10 +397,39 @@ export function useComboBox<T>(props: AriaComboBoxOptions<T>, state: ComboBoxSta
linkBehavior: 'selection' as const,
['UNSTABLE_itemBehavior']: 'action'
}),
valueProps: {
id: valueId
},
descriptionProps,
errorMessageProps,
isInvalid,
validationErrors,
validationDetails
};
}

// This is a modified version of useSlotId that uses useEffect instead of useLayoutEffect.
// Triggering re-renders from useLayoutEffect breaks useComboBoxState's useEffect logic in React 18.
// These re-renders preempt async state updates in the useEffect, which ends up running multiple times
// prior to the state being updated. This results in onSelectionChange being called multiple times.
// TODO: refactor useComboBoxState to avoid this.
function useValueId(depArray: ReadonlyArray<any> = []): string | undefined {
let id = useId();
let [exists, setExists] = useState(true);
let [lastDeps, setLastDeps] = useState(depArray);

// If the deps changed, set exists to true so we can test whether the element exists.
if (lastDeps.some((v, i) => !Object.is(v, depArray[i]))) {
setExists(true);
setLastDeps(depArray);
}

useEffect(() => {
if (exists && !document.getElementById(id)) {
// eslint-disable-next-line react-hooks/set-state-in-effect
setExists(false);
}
}, [id, exists, lastDeps]);

return exists ? id : undefined;
}
Original file line number Diff line number Diff line change
Expand Up @@ -2479,7 +2479,8 @@ describe('SearchAutocomplete', function () {
expect(input).toHaveValue('test');

let button = getByTestId('submit');
await act(async () => await user.click(button));
// For some reason, user.click() causes act warnings related to suspense...
await act(() => button.click());
expect(input).toHaveValue('hi');
});
}
Expand Down
5 changes: 3 additions & 2 deletions packages/@react-spectrum/combobox/test/ComboBox.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5286,11 +5286,12 @@ describe('ComboBox', function () {
expect(input).toHaveValue('One');

let button = getByTestId('submit');
await act(async () => await user.click(button));
// For some reason, user.click() causes act warnings related to suspense...
await act(() => button.click());
expect(input).toHaveValue('Two');

rerender(<Test formValue="key" />);
await act(async () => await user.click(button));
await user.click(button);
expect(document.querySelector('input[name=combobox]')).toHaveValue('2');
});
}
Expand Down
5 changes: 3 additions & 2 deletions packages/@react-spectrum/s2/src/ComboBox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import {
SectionProps,
Virtualizer
} from 'react-aria-components';
import {AsyncLoadable, GlobalDOMAttributes, HelpTextProps, LoadingState, SpectrumLabelableProps} from '@react-types/shared';
import {AsyncLoadable, GlobalDOMAttributes, HelpTextProps, LoadingState, SingleSelection, SpectrumLabelableProps} from '@react-types/shared';
import {AvatarContext} from './Avatar';
import {BaseCollection, CollectionNode, createLeafComponent} from '@react-aria/collections';
import {baseColor, focusRing, space, style} from '../style' with {type: 'macro'};
Expand Down Expand Up @@ -79,7 +79,8 @@ export interface ComboboxStyleProps {
size?: 'S' | 'M' | 'L' | 'XL'
}
export interface ComboBoxProps<T extends object> extends
Omit<AriaComboBoxProps<T>, 'children' | 'style' | 'className' | 'render' | 'defaultFilter' | 'allowsEmptyCollection' | keyof GlobalDOMAttributes>,
Omit<AriaComboBoxProps<T>, 'children' | 'style' | 'className' | 'render' | 'defaultFilter' | 'allowsEmptyCollection' | 'selectionMode' | 'selectedKey' | 'defaultSelectedKey' | 'onSelectionChange' | 'value' | 'defaultValue' | 'onChange' | keyof GlobalDOMAttributes>,
Omit<SingleSelection, 'disallowEmptySelection'>,
ComboboxStyleProps,
StyleProps,
SpectrumLabelableProps,
Expand Down
Loading