Material-ui: [Drawer] Demos not able to access it via keyboard

[tiagodreis]

Hey guys,

I'm having a bit of an issue using the Drawer. It seems that it's not accessible via keyboard (we need it to be, for WCAG AA compliance).

Expected Behavior 🤔

I would expect the Drawer to be accessed either via tab after showing it (or the first focusable item to be focused upon showing)

Current Behavior 😯

When the Drawer shows, if I press tab once, it focus the whole elements inside the Drawer (with no use whatsoever), if I press tab another time, the Drawer hides

Steps to Reproduce 🕹

Link: https://codesandbox.io/s/m2lyk2n89

1. Go to https://codesandbox.io/s/m2lyk2n89
2. Click the IconButton
3. Drawer is opened but not accessible via keyboard

Your Environment 🌎

| Tech | Version |
|--------------|---------|
| Material-UI | v3.9.2 |
| React | v16.8.3 |
| Browser | Chrome |

[oliviertassinari]

@tiagodreis I don't understand. We don't document this implementation strategy. Remove this element, it's wrong:

        <div
          tabIndex={drawerOpened ? 0 : -1}
          role="button"
          onClick={() => setDrawerOpened(false)}
          onKeyDown={() => setDrawerOpened(false)}
        >

[tiagodreis]

Please re-open this issue. This is a real issue.

Please check updated sandbox: https://codesandbox.io/s/m2lyk2n89 (exactly as you document it - except that I'm using a functional component with hooks - but it's the same!)

Also, check your example: https://material-ui.com/demos/drawers/:

1. Click "Open Left"
2. It's not accessible!

[joshwooding]

It's due to the onKeyDown, it's triggered when you try to tab to things. If you filter tab presses out or remove it from your example it works.

[oliviertassinari]

check your example: https://material-ui.com/demos/drawers/

@tiagodreis Thank you for linking the example 😱, I couldn't find the source of your codesanbox :). We can fix the demos like this:

--- a/docs/src/pages/demos/drawers/TemporaryDrawer.js
+++ b/docs/src/pages/demos/drawers/TemporaryDrawer.js
@@ -28,7 +28,11 @@ class TemporaryDrawer extends React.Component {
     right: false,
   };

-  toggleDrawer = (side, open) => () => {
+  toggleDrawer = (side, open) => event => {
+    if (event.type === 'keydown' && event.key === 'Tab') {
+      return;
+    }
+
     this.setState({
       [side]: open,
     });
@@ -38,7 +42,12 @@ class TemporaryDrawer extends React.Component {
     const { classes } = this.props;

     const sideList = (
-      <div className={classes.list}>
+      <div
+        className={classes.list}
+        onClick={this.toggleDrawer('left', false)}
+        onKeyDown={this.toggleDrawer('left', false)}
+        role="presentation"
+      >
         <List>
           {['Inbox', 'Starred', 'Send email', 'Drafts'].map((text, index) => (
             <ListItem button key={text}>
@@ -60,7 +69,12 @@ class TemporaryDrawer extends React.Component {
     );

     const fullList = (
-      <div className={classes.fullList}>
+      <div
+        className={classes.fullList}
+        onClick={this.toggleDrawer('left', false)}
+        onKeyDown={this.toggleDrawer('left', false)}
+        role="presentation"
+      >
         <List>
           {['Inbox', 'Starred', 'Send email', 'Drafts'].map((text, index) => (
             <ListItem button key={text}>
@@ -88,24 +102,10 @@ class TemporaryDrawer extends React.Component {
         <Button onClick={this.toggleDrawer('top', true)}>Open Top</Button>
         <Button onClick={this.toggleDrawer('bottom', true)}>Open Bottom</Button>
         <Drawer open={this.state.left} onClose={this.toggleDrawer('left', false)}>
-          <div
-            tabIndex={0}
-            role="button"
-            onClick={this.toggleDrawer('left', false)}
-            onKeyDown={this.toggleDrawer('left', false)}
-          >
-            {sideList}
-          </div>
+          {sideList}
         </Drawer>
         <Drawer anchor="top" open={this.state.top} onClose={this.toggleDrawer('top', false)}>
-          <div
-            tabIndex={0}
-            role="button"
-            onClick={this.toggleDrawer('top', false)}
-            onKeyDown={this.toggleDrawer('top', false)}
-          >
-            {fullList}
-          </div>
+          {fullList}
         </Drawer>
         <Drawer
           anchor="bottom"

Do you want to give it a try? :)

[tiagodreis]

Thanks @oliviertassinari! , it works.
But I think, we should put:

if (event.type === 'keydown' && (event.key === 'Tab' || event.key === 'Shift')) {
  return;
}

Just to support going up and down. What do you think?
Would I be able to open a PR for this, or will you handle it?

[oliviertassinari]

@tiagodreis It's a great idea 👍 . If you can submit a pull request, it's perfect!

[tiagodreis]

Just submitted PR https://github.com/mui-org/material-ui/pull/14728