Repository: uber-web/baseui
Pull request #305 (2018-09-25 16:28:58)

fix(modal): use role=dialog

schnerd: Seeing what some others do:
- [react-aria-modal](http://davidtheclark.github.io/react-aria-modal/demo/) uses role="dialog" on dialog body and aria-hidden="true" on body
- [Antd](https://ant.design/components/modal/) uses role="dialog" on mask and role="document" on modal body, no aria-hidden/aria-modal 
- [Atlaskit](https://atlaskit.atlassian.com/packages/core/modal-dialog) uses role="dialog" on dialog body, no aria-hidden/aria-modal
- [Material](https://material-ui.com/utils/modal/) uses role="document", no aria-hidden/aria-modal
- [Blueprint](https://blueprintjs.com/docs/#core/components/dialog) has no roles or aria attributes

[Latest w3c aria spec for modals](https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html) recommends role="dialog" and aria-modal="true", so I'll stick with that for now given the inconsistency among all the other implementations.

[Some more details on aria-modal](http://www.maxability.co.in/2018/04/aria-modal-property/)

Patch:
diff --git a/src/modal/modal.js b/src/modal/modal.js
index 74b351d76..548e0ae30 100644
--- a/src/modal/modal.js
+++ b/src/modal/modal.js
@@ -251,7 +251,6 @@ class Modal extends React.Component<ModalPropsT, ModalStateT> {
 
     return (
       <Root
-        role={role}
         $ref={this.getRef('Root')}
         {...sharedProps}
         {...getOverrideProps(RootOverride)}
@@ -273,11 +272,7 @@ class Modal extends React.Component<ModalPropsT, ModalStateT> {
               // https://www.w3.org/TR/wai-aria-practices-1.1/examples/dialog-modal/dialog.html
               'true'
             }
-            role={
-              // Adding role="document" to dialogs in modals is recommended
-              // https://github.com/twbs/bootstrap/issues/15875
-              'document'
-            }
+            role={role}
             $ref={this.getRef('Dialog')}
             {...sharedProps}
             {...getOverrideProps(DialogOverride)}


Test patch:
diff --git a/src/modal/__tests__/__snapshots__/modal.test.js.snap b/src/modal/__tests__/__snapshots__/modal.test.js.snap
index 4e16d04cd..3d3f8fd87 100644
--- a/src/modal/__tests__/__snapshots__/modal.test.js.snap
+++ b/src/modal/__tests__/__snapshots__/modal.test.js.snap
@@ -26,10 +26,8 @@ exports[`Modal renders portal when open: Rendered Modal 1`] = `
       }
       $role="dialog"
       $size="default"
-      role="dialog"
     >
       <div
-        role="dialog"
         styled-component="true"
       >
         <MockStyledComponent
@@ -70,12 +68,12 @@ exports[`Modal renders portal when open: Rendered Modal 1`] = `
               $role="dialog"
               $size="default"
               aria-modal="true"
-              role="document"
+              role="dialog"
               tabIndex="-1"
             >
               <div
                 aria-modal="true"
-                role="document"
+                role="dialog"
                 styled-component="true"
                 tabIndex="-1"
               >
diff --git a/src/modal/__tests__/modal.test.js b/src/modal/__tests__/modal.test.js
index 94302d067..502e84631 100644
--- a/src/modal/__tests__/modal.test.js
+++ b/src/modal/__tests__/modal.test.js
@@ -18,7 +18,6 @@ import {
   StyledBackdrop,
   StyledClose,
   StyledDialog,
-  StyledRoot,
   CLOSE_SOURCE,
 } from '../index';
 
@@ -161,7 +160,6 @@ describe('Modal', () => {
       </Modal>,
     );
 
-    expect(wrapper.find(StyledRoot)).toHaveProp('role', 'mycustomrole');
-    expect(wrapper.find(StyledDialog)).toHaveProp('role', 'document');
+    expect(wrapper.find(StyledDialog)).toHaveProp('role', 'mycustomrole');
   });
 });
diff --git a/src/modal/e2e.js b/src/modal/e2e.js
index ae2532c67..a6317cea5 100644
--- a/src/modal/e2e.js
+++ b/src/modal/e2e.js
@@ -14,7 +14,7 @@ const suite = 'Modal Test Suite';
 const selectors = {
   closeButton: 'button[aria-label="Close"]',
   openModal: '.open-modal-button',
-  dialog: '[role="document"]',
+  dialog: '[role="dialog"]',
 };
 
 function getUrl({launchUrl, suite, test}) {