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

Merged
merged 6 commits into from
Jan 29, 2025

Conversation

igorlima
Copy link
Contributor

@igorlima igorlima commented Jan 25, 2025

Closes #7660

Summary

This pull request addresses the issue #7660, focusing on fixing the value assignment in the HiddenSelect component within the @react-aria/select package. The changes ensure that the value attribute now defaults to the current value of selectRef when state.selectedKey isn't set. This should enhance the native browser autocomplete.

Changes Made

  • File: packages/@react-aria/select/src/HiddenSelect.tsx
    • Updated the value property in the useHiddenSelect function to default to props.selectRef?.current?.value if state.selectedKey is unavailable.

Verification

  • Checked that the HiddenSelect component now assigns the value attribute, and the fallback mechanism works as intended!
  • Ensured that all existing functionality and behaviors are still intact and not affected by these changes.

Additional Context

When the browser triggers autocomplete, it first fills out the hidden select component before passing the value to the visible React Aria Select component. Unfortunately, there was a hiccup where the values weren't being passed down properly, which meant the select form field wasn't being filled out correctly by the native browser autocomplete.

Feel free to share thoughts!

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

There isn't a specific React Aria form story to test this issue, but I've included a demo link to check it out. It was inspired by the sandbox that @nabanitabania shared in a previous PR.

🧢 Your Project:

Sorry, something went wrong.

@felipeac
Copy link

LGTM

reidbarber
reidbarber previously approved these changes Jan 27, 2025
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

LGTM, and thanks for the demo link!

@@ -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

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!

@@ -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

@snowystinger snowystinger Jan 28, 2025

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.

packages/@react-aria/select/src/HiddenSelect.tsx Outdated Show resolved Hide resolved
Co-authored-by: Robert Snow <snowystinger@gmail.com>
@igorlima
Copy link
Contributor Author

@reidbarber, the previous approval was dismissed after I applied one reviewer's suggestion. If you have a moment, could you take a look and let me know if anything else needs attention? The library requires at least two approvals before moving forward. Thanks. 🙏

  • approval was dismissed

    😥
    image
    😞

@reidbarber reidbarber changed the title Fix for HiddenSelect Component Auto-Completion Issue fix(Select): HiddenSelect Component Auto-Completion Issue Jan 29, 2025
@reidbarber reidbarber added this pull request to the merge queue Jan 29, 2025
Merged via the queue into adobe:main with commit beb8bd5 Jan 29, 2025
30 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Browser autofill for React Aria Select is not working.
6 participants