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
2 changes: 1 addition & 1 deletion packages/@react-stately/combobox/src/useComboBoxState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ export function useComboBoxState<T extends object, M extends SelectionMode = 'si

let validation = useFormValidationState({
...props,
value: useMemo(() => Array.isArray(displayValue) && displayValue.length === 0 ? null : ({inputValue, value: displayValue as any, selectedKey}), [inputValue, selectedKey, displayValue])
value: useMemo(() => ({inputValue, value: displayValue as any, selectedKey}), [inputValue, selectedKey, displayValue])
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is correct. If the value is empty, validate should not be called. This should be handled by isRequired.

});

// Revert input value and close menu
Expand Down
18 changes: 12 additions & 6 deletions packages/react-aria-components/src/ComboBox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,10 @@ function ComboBoxInner<T extends object>({props, collection, comboBoxRef: ref}:
let [labelRef, label] = useSlot(
!props['aria-label'] && !props['aria-labelledby']
);
let comboBoxProps = removeDataAttributes(props);
if (props.selectionMode === 'multiple') {
delete comboBoxProps.isRequired;
Copy link
Member

Choose a reason for hiding this comment

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

Can we handle this inside the useComboBox hook?

}
let {
buttonProps,
inputProps,
Expand All @@ -162,7 +166,7 @@ function ComboBoxInner<T extends object>({props, collection, comboBoxRef: ref}:
valueProps,
...validation
} = useComboBox({
...removeDataAttributes(props),
...comboBoxProps,
label,
inputRef,
buttonRef,
Expand Down Expand Up @@ -210,12 +214,14 @@ function ComboBoxInner<T extends object>({props, collection, comboBoxRef: ref}:
if (name && formValue === 'key') {
let values: (Key | null)[] = Array.isArray(state.value) ? state.value : [state.value];
if (values.length === 0) {
values = [null];
// For multiple mode, add required to the hidden input when no values selected
let required = props.selectionMode === 'multiple' && props.isRequired;
inputs = [<input key="empty" type="hidden" name={name} form={props.form} value="" required={required} />];
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 actually prevent form submission? I thought type="hidden" did not support required. We may need to use a type="text" with display: none here.

} else {
inputs = values.map((value, i) => (
<input key={i} type="hidden" name={name} form={props.form} value={value ?? ''} />
));
}

inputs = values.map((value, i) => (
<input key={i} type="hidden" name={name} form={props.form} value={value ?? ''} />
));
}

return (
Expand Down
45 changes: 41 additions & 4 deletions packages/react-aria-components/test/ComboBox.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -614,17 +614,14 @@ describe('ComboBox', () => {
expect(listbox).toBeInTheDocument();
expect(listbox).toBeVisible();

// Verify we can still interact with options
let options = comboboxTester.options();
expect(options.length).toBeGreaterThan(0);

// Click an option
await user.click(options[0]);
act(() => {
jest.runAllTimers();
});

// Verify the combobox is closed and the value is updated
expect(tree.queryByRole('listbox')).toBeNull();
expect(comboboxTester.combobox).toHaveValue('Apple');
});
Expand Down Expand Up @@ -718,7 +715,6 @@ describe('ComboBox', () => {
});

it('should support multi-select with custom value', async () => {
// allowsCustomValue doesn't really make sense to use with multi-selection, but test it anyway.
let {container} = render(<TestComboBox selectionMode="multiple" allowsCustomValue />);
let comboboxTester = testUtilUser.createTester('ComboBox', {root: container});

Expand Down Expand Up @@ -762,6 +758,47 @@ describe('ComboBox', () => {
expect(onChange).toHaveBeenLastCalledWith(['1']);
});

it('should support isRequired with multiple selection', async () => {
let {container, getByTestId} = render(
<Form data-testid="form">
<TestComboBox name="combobox" selectionMode="multiple" isRequired />
<input type="reset" />
</Form>
);
let comboboxTester = testUtilUser.createTester('ComboBox', {root: container});
let combobox = comboboxTester.combobox;

expect(combobox).not.toHaveAttribute('required');

act(() => {getByTestId('form').checkValidity();});
expect(combobox).toHaveAttribute('aria-describedby');
expect(container.querySelector('.react-aria-ComboBox')).toHaveAttribute('data-invalid');

await comboboxTester.open();
let options = comboboxTester.options();
await user.click(options[0]);

act(() => {getByTestId('form').checkValidity();});
expect(combobox).not.toHaveAttribute('aria-describedby');
expect(container.querySelector('.react-aria-ComboBox')).not.toHaveAttribute('data-invalid');

let hiddenInputs = container.querySelectorAll('input[type="hidden"]');
expect(hiddenInputs).toHaveLength(1);
expect(hiddenInputs[0]).toHaveAttribute('name', 'combobox');
expect(hiddenInputs[0]).toHaveAttribute('value', '1');
expect(hiddenInputs[0]).not.toHaveAttribute('required');

await user.click(options[0]);
act(() => {getByTestId('form').checkValidity();});
expect(combobox).toHaveAttribute('aria-describedby');

hiddenInputs = container.querySelectorAll('input[type="hidden"]');
expect(hiddenInputs).toHaveLength(1);
expect(hiddenInputs[0]).toHaveAttribute('name', 'combobox');
expect(hiddenInputs[0]).toHaveAttribute('value', '');
expect(hiddenInputs[0]).toHaveAttribute('required');
});

it('should not close the combobox when clicking on the input', async () => {
let onOpenChange = jest.fn();
let {container, getByRole} = render(<TestComboBox onOpenChange={onOpenChange} />);
Expand Down
Loading