[material-ui][select] Fix assigning of id to Select #47498
Conversation
sai6855
added
type: bug
Netlify deploy previewhttps://deploy-preview-47498--material-ui.netlify.app/ Bundle size report
|
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix an issue where the id attribute from a TextField component wasn't being passed to the hidden input element when using select mode. The fix modifies how the id is passed to the Select component - changing from a direct prop (id={id}) to being nested in the inputProps object (inputProps={{ id }}).
Key Changes
- Changed the TextField to pass
idthroughinputPropsinstead of as a direct prop to Select - Removed a test assertion that verified the hidden input has no
idattribute
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/mui-material/src/TextField/TextField.js | Modified how TextField passes the id prop to Select component when in select mode |
| packages/mui-material/src/TextField/TextField.test.js | Removed assertion checking that hidden input lacks an id attribute |
Critical Issue Found: While this change successfully passes the id to the hidden input element, it introduces a regression where the visible combobox element loses its id attribute. The Select component uses the direct id prop to populate SelectDisplayProps.id, which becomes the id of the visible combobox. By only passing id through inputProps, the combobox will either have no id or fall back to a name-based generated id, breaking accessibility and label associations. The correct fix should pass id both as a direct prop AND in inputProps.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <SelectSlot | ||
| aria-describedby={helperTextId} | ||
| id={id} | ||
| labelId={inputLabelId} | ||
| value={value} | ||
| input={InputElement} | ||
| inputProps={{ id }} | ||
| {...selectProps} |
There was a problem hiding this comment.
The removal of the id prop from SelectSlot will break the visible combobox element's id attribute.
In the Select component, the id prop is used to populate SelectDisplayProps.id, which becomes the id of the visible combobox element (via buttonId in SelectInput.js line 480). By only passing id through inputProps, it ends up on the hidden input element but NOT on the visible combobox.
The fix should pass BOTH:
id={id}as a direct prop (for the visible combobox via SelectDisplayProps)inputProps={{ id }}(for the hidden input element)
This ensures the visible combobox maintains its proper id for accessibility (label association, etc.) while also fixing the original issue of the hidden input not receiving the id.
| </TextField>, | ||
| ); | ||
|
|
||
| const input = container.querySelector('input[aria-hidden]'); |
There was a problem hiding this comment.
While this assertion removal is correct (the hidden input should now have an id attribute after the fix), a new assertion should be added to verify that the visible combobox still maintains its correct id. This would help catch the regression where the combobox loses its id attribute.
Consider adding an assertion like:
expect(screen.getByRole('combobox')).to.have.attribute('id');
Additionally, you should verify that both the visible combobox and the hidden input have the SAME id value when an explicit id prop is passed to TextField, to ensure proper form submission behavior.
| @@ -257,10 +257,10 @@ const TextField = React.forwardRef(function TextField(inProps, ref) { | |||
| {select ? ( | |||
| <SelectSlot | |||
| aria-describedby={helperTextId} | |||
| id={id} | |||
There was a problem hiding this comment.
removed this id as it was sending to div role='combobox'
There was a problem hiding this comment.
This would be a breaking change. We cannot remove the id of the combobox. The id passed on Select is of div combobox.
See the comment #38869 (comment):
I would not remove or change the id on the div itself, though.
sai6855
changed the title
3 participants
closes #47489
Before
After