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

[Select] Prop aria-describedby is passed down to hidden input element #24019

Closed
2 tasks done
HVish opened this issue Dec 15, 2020 · 3 comments · Fixed by #24027
Closed
2 tasks done

[Select] Prop aria-describedby is passed down to hidden input element #24019

HVish opened this issue Dec 15, 2020 · 3 comments · Fixed by #24027
Labels
accessibility a11y component: select This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@HVish
Copy link
Contributor

HVish commented Dec 15, 2020

I am trying to use Select component with accessibility support. It works pretty well except for the usage with FormHelperText . When the select toggle button is focused the screen reader does not read the contents of FormHelperText. I have also tried using the TextField component with select prop for better support as mentioned in docs.

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The screen reader does not reads the helper text provided using aria-describedby prop to Select component or simply by passing id prop to TextField component with select prop. Instead the prop is passed down to the hidden (aria-hidden) input element. I have narrowed down it to following piece of code:

https://github.com/mui-org/material-ui/blob/174b64f8a3adffb8df3c13285f4edc2437a8547a/packages/material-ui/src/TextField/TextField.js#L176

https://github.com/mui-org/material-ui/blob/174b64f8a3adffb8df3c13285f4edc2437a8547a/packages/material-ui/src/Select/SelectInput.js#L398

Expected Behavior 🤔

It should pass the aria-describedby prop to wrapper div of select component. May be in following line:

https://github.com/mui-org/material-ui/blob/174b64f8a3adffb8df3c13285f4edc2437a8547a/packages/material-ui/src/Select/SelectInput.js#L372

Steps to Reproduce 🕹

I am implementing it as follows:

<FormControl
  fullWidth
  error={hasError}
>
  <InputLabel id="select-label">Please select an option</InputLabel>
  <Select
    labelId="select-label"
    id="select-field"
    value={value}
    aria-describedby="select-field-helper-text"
    onChange={onChange}
  >
    {options.map((option) => (
      <MenuItem key={option.value} value={option.value}>
        {option.title}
      </MenuItem>
    ))}
  </Select>
  {hasError && (
    <FormHelperText id="select-field-helper-text">
      {errorMessage}
    </FormHelperText>
  )}
</FormControl>;

Context 🔦

I would like to make screen readers read the helper text for form validation purpose.

Your Environment 🌎

`npx @material-ui/envinfo`
  System:
    OS: macOS Mojave 10.14.6
  Binaries:
    Node: 14.15.0 - ~/.nvm/versions/node/v14.15.0/bin/node
    Yarn: 1.22.5 - ~/.yarn/bin/yarn
    npm: 6.14.8 - ~/.nvm/versions/node/v14.15.0/bin/npm
  Browsers:
    Chrome: 87.0.4280.88
    Edge: Not Found
    Firefox: 72.0.2
    Safari: 13.0.5
  npmPackages:
    @material-ui/core: ^4.11.0 => 4.11.0 
    @material-ui/icons: ^4.9.1 => 4.9.1 
    @material-ui/styles:  4.10.0 
    @material-ui/system:  4.9.14 
    @material-ui/types:  5.1.0 
    @material-ui/utils:  4.10.2 
    @types/react: ^16.9.0 => 16.9.49 
    react: ^16.13.1 => 16.13.1 
    react-dom: ^16.13.1 => 16.13.1 
    typescript: ^4.0.5 => 4.0.5 
   
@HVish HVish added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Dec 15, 2020
@eps1lon eps1lon added accessibility a11y component: select This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Dec 15, 2020
@oliviertassinari oliviertassinari changed the title A11y: prop aria-describedby is passed down to hidden input element in Select component [Select] Prop aria-describedby is passed down to hidden input element Dec 16, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 16, 2020

@HVish Thanks for reporting the issue. I could reproduce on VoiceOver with this demo https://next.material-ui.com/components/text-fields/#select. It works with the native select but fails with the nonnative implementation.

What do you think about this fix? Do you want to work on a pull request? :)

diff --git a/docs/src/pages/components/text-fields/SelectTextFields.tsx b/docs/src/pages/components/text-fields/SelectTextFields.tsx
index 124b6085cf..5f00b6278b 100644
--- a/docs/src/pages/components/text-fields/SelectTextFields.tsx
+++ b/docs/src/pages/components/text-fields/SelectTextFields.tsx
@@ -33,7 +33,7 @@ const useStyles = makeStyles((theme: Theme) =>
   }),
 );

-export default function MultilineTextFields() {
+export default function SelectTextFields() {
   const classes = useStyles();
   const [currency, setCurrency] = React.useState('EUR');

diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js
index 34244bb0ef..4ba999d576 100644
--- a/packages/material-ui/src/Select/SelectInput.js
+++ b/packages/material-ui/src/Select/SelectInput.js
@@ -28,6 +28,7 @@ function isEmpty(display) {
  */
 const SelectInput = React.forwardRef(function SelectInput(props, ref) {
   const {
+    'aria-describedby': ariaDescribedby,
     'aria-label': ariaLabel,
     autoFocus,
     autoWidth,
@@ -367,6 +368,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
         aria-haspopup="listbox"
         aria-label={ariaLabel}
         aria-labelledby={[labelId, buttonId].filter(Boolean).join(' ') || undefined}
+        aria-describedby={ariaDescribedby}
         onKeyDown={handleKeyDown}
         onMouseDown={disabled || readOnly ? null : handleMouseDown}
         onBlur={handleBlur}
@@ -439,6 +441,10 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
 });

 SelectInput.propTypes = {
+  /**
+   * @ignore
+   */
+  'aria-describedby': PropTypes.string,
   /**
    * @ignore
    */

At first, I got confused with the difference with aria-labelledby. This blog post clarified it.

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Dec 16, 2020
@HVish
Copy link
Contributor Author

HVish commented Dec 16, 2020

@oliviertassinari Of course! Actually I was about to send the pull request :D

I have one more question. Like other form components could we have aria-invalid attribute for Select component? Obviously I tried implementing it before sending the pull request but one test failed with message:

The attribute aria-invalid is not supported by the role button jsx-a11y/role-supports-aria-props

So I was wondering if we could do some work around with non-native select component?

HVish added a commit to HVish/material-ui that referenced this issue Dec 16, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 16, 2020

I'm not familiar with this aria-invalid attribute, maybe @eps1lon has more context on it? It's probably something to consider for a different pull request anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: select This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants