[Select][material-ui] Add name to hidden input element

[DarhkVoyd]

Changes Made:

• Updated TextField.js to have a conditional htmlFor which generates id for the input based on select prop.
• The generated id is assigned to the SelectInput through SelectDisplayProps.id in SelectInput.js.
• Removed an old test which ensured that there is no id on the input.

Testing:

I have added tests to the TextField.test.js file to ensure that the changes work as expected. The tests include matching the id of the input with the label's htmlFor.

• I have followed (at least) the PR section of the contributing guide.

Edit: The above discussed changes were discarded after further assessment instead a name was added to the hidden input element with useId() method. The incorrect use of label for error that could be resolved by passing span as component would also bring some styling issues. Since, setting span as the default component in this case would be a breaking change, it was noted for Port to v6 and just the style fix was added for now.

Closes #38869

[mui-bot]

Netlify deploy preview

https://deploy-preview-39414--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 37fc6c8

[Owenleo778]

If we're changing the logic to include the id, perhaps we should also add the label-id to the aria-describedby as well?

[Owenleo778]

Nevermind, I just re-read and realised you used the htmlFor attribute on the label instead

Though this may actually cause an accessibility issue when the screenreader finds the label but can't find the associated input (because it's aria-hidden)

[DarhkVoyd]

So what do you suggest I do? Also, I just realised that this might still be happening because current htmlFor addresses the div.

[Owenleo778]

I'm not sure, unfortunately I'm not super well versed in improving accessibility. At the very least you could check out the chrome issues tab with your changes in place, to see what (if any) issues will occur around the Textfield with the select prop set to true.

I still have a suspicion that it will cause users of screenreaders to get confused from the label pointing to a hidden input though

[DarhkVoyd]

@Owenleo778 I completely understand your point. I'm not very experienced with accessibility either. I did check the Chrome issues tab, and it seems that there are no reported issues related to the changes made. If you have any specific recommendations or resources on accessibility best practices, I'd be more than happy to learn and implement them.

[Owenleo778]

After thinking about it more, I do feel like it would be best to change how the label connection is defined. Rather than setting the label's htmlFor attribute, I feel like it would be better to set the input's aria-labelledby attribute to the label id instead. This way, screenreaders wont see the for attribute leading to an invisible input from their perspective.

Hopefully, it resolves the chrome issues without causing any additional accessibility problems

(Though honestly, it does feel redundant to set up the id / htmlFor / aria-labeledby etc attributes when they're going to be hidden from the common usages anyway. But at least we can quieten the chrome issues. This is most likely the reason it wasn't set to something else when removed initially)

[Owenleo778]

As far as I can tell, this at minimum maintains the same level of accessibility as before and (presumably) removes the related chrome flagged issues.

You'll still have to wait for someone else to review first though

[DarhkVoyd]

@Owenleo778 After some more testing I noticed that the accessibility issue is only shown in Chrome (still), I am unable to reproduce the issue in Safari and Firefox. I believe this might actually be an issue with Chrome DevTools as suggested in the original issue. Here, setting aria-labeledby of the input to the id of the label, does not silence the accessibility issue. It should also be reverted because the div that uses label's htmlFor in id also uses label's id as aria-labeledby.

There were other accessibility issue for this select, one such was that input should have an id or name attribute which I have resolved by adding an new id to the input.

[DarhkVoyd]

The chrome throws an error:

Incorrect use of <label for=FORM_ELEMENT>
The label's for attribute doesn't match any element id. This might prevent the browser from correctly autofilling the form and accessibility tools from working correctly.
To fix this issue, make sure the label's for attribute references the correct id of a form field.

It seems that there's an issue with MUI's current practice of setting the label's htmlFor to a div and the div's aria-labelledby to the label's id. Upon further research into accessibility practices, I've found that setting a label's for attribute to a non-form field is generally considered a bad practice in terms of accessibility. Which leads to unexpected behavior in autofill and accessibility tools. That is why, there are multiple similar issues regarding this in the devtools.

Now, I'm leaning towards the idea that the label's for attribute should not point to the div. However, I'm not entirely certain if this issue can be easily resolved without changing the overall accessibility practices within the components.

I'd really appreciate your thoughts on this matter.

[Owenleo778]

The only accessibility issue would be the label without a linked input field I believe (since the input is hidden to screenreaders). I agree, due to the implementation of the component I don't see any easy solutions.

Unfortunately, without the knowledge of the history of the component / reasoning behind certain decisions, I can't really comment much further on where to take this.

We could potentially swap the input label for a different text component, but i'm not sure of the ramifications of that change on accessibility, and could just make the situation worse.
For example, linking the input label to a div might be discouraged, however how screenreaders would actually interpret that to the user, I have no idea on; meaning that this discouraged method may actually benefit screenreader users more than a method such as: changing the label to a p tag and adding an aria-label attribute to the current linked div with the same text as the label itself

[DarhkVoyd]

@Owenleo778 How should I proceed now? Should I contact member signed in the implementation commit?

[Owenleo778]

@DarhkVoyd I'm not sure on the etiquette of reaching out directly, but potentially simply just leaving this issue until they view this PR and respond could be the best path of action for now

[mj12albert]

Sorry for not getting back to this sooner, I will review this PR this week 🙏

[mj12albert]

@DarhkVoyd I don't think adding aria-labelledby really helps anything 😅

Moreover, I believe the input intentionally didn't have the id because there exists the test for the same here.

But to get rid of the A form field element should have an id or name attribute issue we can pass the generated id as the name attribute instead without affecting the test

diff --git a/packages/mui-material/src/Select/SelectInput.js b/packages/mui-material/src/Select/SelectInput.js
index 9c36c98f14..75cd39dd11 100644
--- a/packages/mui-material/src/Select/SelectInput.js
+++ b/packages/mui-material/src/Select/SelectInput.js
@@ -489,6 +489,8 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
 
   const listboxId = useId();
 
+  const hiddenInputId = useId();
+
   return (
     <React.Fragment>
       <SelectSelect
@@ -523,7 +525,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
       <SelectNativeInput
         aria-invalid={error}
         value={Array.isArray(value) ? value.join(',') : value}
-        name={name}
+        name={name ?? hiddenInputId}
         ref={inputRef}
         aria-hidden
         onChange={handleChange}

[mj12albert]

We could potentially swap the input label for a different text component, but i'm not sure of the ramifications of that change on accessibility, and could just make the situation worse.

@Owenleo778 I think this is fine and doesn't harm a11y, see my comment here: #38869 (comment)

[DarhkVoyd]

@mj12albert Thank you, adding generated Id to name does resolve the A form field element should have an id or name attribute. warning but how do you suggest resolving the Incorrect use of <label for=FORM ELEMENT> because the div is still used as label's for attribute.

[DarhkVoyd]

@mj12albert Hey, I have made the necessary changes, fixing it from the users perspective which lead to a small styling error which I also have solved. Kindly review the PR.

[mj12albert]

Thanks @DarhkVoyd for working through this and @Owenleo778 for assisting with the review 🙏

I've made a note to port the fix and the breaking change to v6 which we are ramping up!

[Owenleo778]

Nice one!

[joshkel]

I believe this change has unintended side effects; see #40128.