Downshift Voice Over Accessibility

[ghost]

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

When using Voice Over it should read out currently selected MenuItem.

Current Behavior 😯

Navigating through MenuItem's doesn't trigger Voice Over.

Steps to Reproduce 🕹

Link: https://material-ui.com/demos/autocomplete/#downshift

1. Enable Voice Over (I have used built in VoiceOver in macOS)
2. Type character a into input.
3. Navigate through MenuItem's with keyboard arrows

Context 🔦

In Downshift we have to use getInputProps to create necessary aria attributes for accessibility. Those attributes have to be passed to an input, currently they are passed to a div wrapping that input.

Easy solution to fix this is to pass those attributes via inputProps (rather than InputProps) but this could cause another issue if we want to take an advantage of stuff like startAdornment.

We could follow up with solution above by using both inputProps and InputProps at the same time, but ESLint is warning us about duplicate props...

Ideal solution would be to do something similar we are doing here InputBase.js line 322 (those aria's are in other).

Anyway, I fixed this issue in my project by using inputProps. I would be more than happy to create a PR to fix this here.. I am just not sure what would be the best approach here.

Your Environment 🌎

Tech	Version
Material-UI	v4.0.0-alpha.0
React	16.8
Browser	Chrome
TypeScript
etc.

[oliviertassinari]

@michal9909 I confirm the issue. I can reproduce it. It's specific to our demo. Downshift demos are OK. I could fix the issue with this simple change:

--- a/docs/src/pages/demos/autocomplete/IntegrationDownshift.js
+++ b/docs/src/pages/demos/autocomplete/IntegrationDownshift.js
@@ -172,7 +172,12 @@ class DownshiftMultiple extends React.Component {
             {renderInput({
               fullWidth: true,
               classes,
-              InputProps: getInputProps({
+              inputProps: getInputProps({
+                placeholder: 'Select multiple countries',
+                onChange: this.handleInputChange,
+                onKeyDown: this.handleKeyDown,
+              }),
+              InputProps: {
                 startAdornment: selectedItem.map(item => (
                   <Chip
                     key={item}
@@ -182,10 +187,7 @@ class DownshiftMultiple extends React.Component {
                     onDelete={this.handleDelete(item)}
                   />
                 )),
-                onChange: this.handleInputChange,
-                onKeyDown: this.handleKeyDown,
-                placeholder: 'Select multiple countries',
-              }),
+              },
               label: 'Label',
             })}
             {isOpen ? (
@@ -264,7 +266,7 @@ function IntegrationDownshift(props) {
             {renderInput({
               fullWidth: true,
               classes,
-              InputProps: getInputProps({
+              inputProps: getInputProps({
                 placeholder: 'Search a country (start with a)',
               }),
             })}
@@ -303,7 +305,7 @@ function IntegrationDownshift(props) {
             {renderInput({
               fullWidth: true,
               classes,
-              InputProps: getInputProps({
+              inputProps: getInputProps({
                 placeholder: 'With Popper',
               }),
               ref: node => {

Can you confirm? Do you want to submit a pull request to fix our demos? :)

[oliviertassinari]

We could follow up with solution above by using both inputProps and InputProps at the same time, but ESLint is warning us about duplicate props...

Yes, we are exploring the solutions in #10064.

Ideal solution would be to do something similar we are doing here InputBase.js line 322 (those aria's are in other).

There are 4 properties to handle: (aria-activedescendant, aria-autocomplete, aria-controls, aria-labelledby). This is an interesting proposal. We could forward all the aria- properties to the input. 🤔

[oliviertassinari]

The alternative solution:

--- a/packages/material-ui/src/InputBase/InputBase.js
+++ b/packages/material-ui/src/InputBase/InputBase.js
@@ -139,6 +139,8 @@ export const styles = theme => {
   };
 };

+const ariaRegExp = /^aria-/;
+
 /**
  * `InputBase` contains as few styles as possible.
  * It aims to be a simple building block for creating an input.
@@ -319,8 +321,14 @@ class InputBase extends React.Component {
       ...other
     } = this.props;

-    const ariaDescribedby = other['aria-describedby'];
-    delete other['aria-describedby'];
+    // Forward the aria properties to the input.
+    const aria = {};
+    Object.keys(other).forEach(key => {
+      if (ariaRegExp.test(key)) {
+        aria[key] = other[key];
+        delete other[key];
+      }
+    });

     const fcs = formControlState({
       props: this.props,
@@ -407,7 +415,7 @@ class InputBase extends React.Component {
         <FormControlContext.Provider value={null}>
           <InputComponent
             aria-invalid={fcs.error}
-            aria-describedby={ariaDescribedby}
+            {...aria}
             autoComplete={autoComplete}
             autoFocus={autoFocus}
             className={inputClassName}

[ghost]

Looks like this would work 👍 Since you have this working nicely feel free to create a PR for this!

[eps1lon]

Ideal solution would be to do something similar we are doing here InputBase.js line 322 (those aria's are in other).

The ideal solution would be to fix the inputProps <-> inputProps ambiguity instead of adding more complexity that will cause this exact same issue if somebody thinks some other html attribute should be spread to the input instead of the div.

We shouldn't bloat implementation to accommodate for lint rules but rather fix the actual issue that the lint rule reports.

[oliviertassinari]

The problem was solved in #15780.