-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: Allow selectedKey=null
to clear value in HiddenSelect
#8330
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
Conversation
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.
Appears to leave behaviour for autofill intact for Dashlane. @yihuiliao can you check with chrome's autofill?
Verified it allows the hidden input to be reset to null as expected
@@ -1715,6 +1715,7 @@ describe('Picker', function () { | |||
expect(options.length).toBe(60); | |||
options.forEach((option, index) => index > 0 && expect(option).toHaveTextContent(states[index - 1].name)); | |||
|
|||
fireEvent.input(hiddenSelect, {target: {value: 'CA'}}); |
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.
just double checking, is this just testing that onSelectionChange isn't called twice? asking because it seems it was passing before the changes in HIddenSelect
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.
Yeah
@@ -99,8 +102,9 @@ export function useHiddenSelect<T>(props: AriaHiddenSelectOptions, state: Select | |||
disabled: isDisabled, | |||
required: validationBehavior === 'native' && isRequired, | |||
name, | |||
value: state.selectedKey ?? undefined, | |||
onChange: (e: React.ChangeEvent<HTMLSelectElement>) => state.setSelectedKey(e.target.value) | |||
value: state.selectedKey ?? '', |
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.
This was changed previously in #7670 to fix a different issue. @snowystinger do you remember what was going on there?
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.
Yep, that was to fix the autocomplete. If I recall correctly, it viewed the ''
as a controlled value for the field and wouldn't autofill as a result, React would just overwrite it with the empty string again as there was no onChange event. There is apparently an onInput change for it though, so we can use that to update with the value for auto fill.
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.
ah, I missed the onInput. Does having both cause setSelectedKey to run twice then?
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.
good question, i doubt any of our tests check that, so will need to verify
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.
oh looks like the added test covers it.
Closes #8032
react-spectrum/packages/@react-aria/form/src/useFormValidation.ts
Lines 74 to 76 in 9786f69
runs before
react-spectrum/packages/@react-aria/select/src/HiddenSelect.tsx
Line 103 in 9786f69
, which leads to the autofill failing.
See here for more information
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: