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.

Sign up for GitHub

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(Select): HiddenSelect Component Auto-Completion Issue #7670

Merged
merged 6 commits into from
Jan 29, 2025
Merged

fix(Select): HiddenSelect Component Auto-Completion Issue #7670

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e752670
address autofill issue
igorlima Jan 24, 2025
8cdc137
lint
igorlima Jan 25, 2025
4a2615e
remove unnecessary change
igorlima Jan 25, 2025
bffedc9
Update packages/@react-aria/select/src/HiddenSelect.tsx
igorlima Jan 29, 2025
d9293d3
remove unnecessary variable declaration
igorlima Jan 29, 2025
9224e82
Merge branch 'main' into issue-7660
reidbarber Jan 29, 2025
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
3 changes: 2 additions & 1 deletion packages/@react-aria/select/src/HiddenSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ export function useHiddenSelect<T>(props: AriaHiddenSelectOptions, state: Select
let {autoComplete, name = data.name, isDisabled = data.isDisabled} = props;
let {validationBehavior, isRequired} = data;
let {visuallyHiddenProps} = useVisuallyHidden();
let selectRefValue = props.selectRef?.current?.value;
Copy link
Member

Choose a reason for hiding this comment

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

Reading from a ref during render is typically unsafe and unfortunately it's hard to tell when it potentially breaks React's concurrent mode since they don't expose any of that for tests.

We should try to find another way to handle this.

does the hidden select fire an onChange that we aren't listening to? I'm worried that the state doesn't know about this value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for raising this concern!

This repository already has a lint rule to detect issues related to reading from refs during rendering. In my initial proposal, the lint failed because it did indeed read from a ref during render. However, I addressed this, and the lint has now passed.

  • screenshot lint fixed image

Regarding the use of selectRef, there are existing references to it in this function:

@snowystinger given that the lint passes for this case and considering the function's existing use of refs, I believe it’s safe to use selectRef here. 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The concern that the state might not be aware of the value is valid. I added a debugger to the demo link to check everything is still working as intended. By opening the DevTools and triggering the native browser autocomplete, we can see that the value is set. Additionally, we can check all values when we submit the form.

  • screenshots image

    image

    image

Copy link
Member

@snowystinger snowystinger Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

ok, I had a chance today to look into this. I think the behavior is actually related to the controlled value being initially set. I'm making a recommendation below for how to handle it so we don't need to read the ref during render and we can rely on the state itself.

the onChange fired by autofill will have as value whatever 'controlled' value is already there. It doesn't fire an onChange with the new value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything seems to work as intended, and the code looks much more straightforward and cleaner now. Thanks for the help!


useFormReset(props.selectRef, state.selectedKey, state.setSelectedKey);
useFormValidation({
Expand Down Expand Up @@ -99,7 +100,7 @@ export function useHiddenSelect<T>(props: AriaHiddenSelectOptions, state: Select
disabled: isDisabled,
required: validationBehavior === 'native' && isRequired,
name,
value: state.selectedKey ?? '',
value: state.selectedKey ?? (selectRefValue || ''),
onChange: (e: React.ChangeEvent<HTMLSelectElement>) => state.setSelectedKey(e.target.value)
}
};
Expand Down