From 2134c43a3bf6067ed50107d71d094fec9c29dc2f Mon Sep 17 00:00:00 2001
From: Christopher Deutsch <cd@cdeutsch.com>
Date: Thu, 7 Jun 2018 10:54:23 -0500
Subject: [PATCH 1/2] On MenuItem, set the `role` to `none` if the role is
 `null` or `none`.

This will remove the implied `listitem` role of the `<li />`.

https://www.w3.org/TR/wai-aria-practices-1.1/examples/menubar/menubar-1/menubar-1.html
---
 src/MenuItem.jsx                          |  7 +++++--
 tests/MenuItem.spec.js                    |  8 +++++++-
 tests/__snapshots__/MenuItem.spec.js.snap | 12 +++++++++++-
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/src/MenuItem.jsx b/src/MenuItem.jsx
index 20631ffe..a6f36b0d 100644
--- a/src/MenuItem.jsx
+++ b/src/MenuItem.jsx
@@ -161,10 +161,13 @@ export class MenuItem extends React.Component {
         role: 'option',
         'aria-selected': props.isSelected,
       };
-    } else if (props.role === null) {
+    } else if (props.role === null || props.role === 'none') {
       // sometimes we want to specify role inside <li/> element
       // <li><a role='menuitem'>Link</a></li> would be a good example
-      delete attrs.role;
+      // in this case the role on <li/> should be "none" to
+      // remove the implied listitem role.
+      // https://www.w3.org/TR/wai-aria-practices-1.1/examples/menubar/menubar-1/menubar-1.html
+      attrs.role = 'none';
     }
     // In case that onClick/onMouseLeave/onMouseEnter is passed down from owner
     const mouseEvent = {
diff --git a/tests/MenuItem.spec.js b/tests/MenuItem.spec.js
index bfba11a2..790b2744 100644
--- a/tests/MenuItem.spec.js
+++ b/tests/MenuItem.spec.js
@@ -117,12 +117,18 @@ describe('MenuItem', () => {
   });
 
   describe('overwrite default role', () => {
-    it('should set empty role', () => {
+    it('should set role to none if null', () => {
       const wrapper = shallow(<NakedMenuItem role={null}>test</NakedMenuItem>);
 
       expect(wrapper.render()).toMatchSnapshot();
     });
 
+    it('should set role to none if none', () => {
+      const wrapper = shallow(<NakedMenuItem role="none">test</NakedMenuItem>);
+
+      expect(wrapper.render()).toMatchSnapshot();
+    });
+
     it('should set specific role', () => {
       const wrapper = shallow(<NakedMenuItem role="option">test</NakedMenuItem>);
 
diff --git a/tests/__snapshots__/MenuItem.spec.js.snap b/tests/__snapshots__/MenuItem.spec.js.snap
index 597ffc6a..4fed1381 100644
--- a/tests/__snapshots__/MenuItem.spec.js.snap
+++ b/tests/__snapshots__/MenuItem.spec.js.snap
@@ -1,8 +1,18 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`MenuItem overwrite default role should set empty role 1`] = `
+exports[`MenuItem overwrite default role should set role to none if none 1`] = `
 <li
   class="undefined-item"
+  role="none"
+>
+  test
+</li>
+`;
+
+exports[`MenuItem overwrite default role should set role to none if null 1`] = `
+<li
+  class="undefined-item"
+  role="none"
 >
   test
 </li>

From 4551ede9f1a8ff74f33a5f5e71676b5227fe9f16 Mon Sep 17 00:00:00 2001
From: Christopher Deutsch <cd@cdeutsch.com>
Date: Thu, 7 Jun 2018 14:22:05 -0500
Subject: [PATCH 2/2] Add support for specifying any `role` on MenuItem.

Ran into a case where there is a readonly list displayed by a dropdown and the `<li />` role should be "reset" to `listitem`.
---
 src/MenuItem.jsx                          |  2 +-
 tests/MenuItem.spec.js                    |  8 +++++++-
 tests/__snapshots__/MenuItem.spec.js.snap | 11 ++++++++++-
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/src/MenuItem.jsx b/src/MenuItem.jsx
index a6f36b0d..beec9d6f 100644
--- a/src/MenuItem.jsx
+++ b/src/MenuItem.jsx
@@ -150,7 +150,7 @@ export class MenuItem extends React.Component {
       title: props.title,
       className,
       // set to menuitem by default
-      role: 'menuitem',
+      role: props.role || 'menuitem',
       'aria-disabled': props.disabled,
     };
 
diff --git a/tests/MenuItem.spec.js b/tests/MenuItem.spec.js
index 790b2744..d4d1cef1 100644
--- a/tests/MenuItem.spec.js
+++ b/tests/MenuItem.spec.js
@@ -129,7 +129,13 @@ describe('MenuItem', () => {
       expect(wrapper.render()).toMatchSnapshot();
     });
 
-    it('should set specific role', () => {
+    it('should set role to listitem', () => {
+      const wrapper = shallow(<NakedMenuItem role="listitem">test</NakedMenuItem>);
+
+      expect(wrapper.render()).toMatchSnapshot();
+    });
+
+    it('should set role to option', () => {
       const wrapper = shallow(<NakedMenuItem role="option">test</NakedMenuItem>);
 
       expect(wrapper.render()).toMatchSnapshot();
diff --git a/tests/__snapshots__/MenuItem.spec.js.snap b/tests/__snapshots__/MenuItem.spec.js.snap
index 4fed1381..9793955e 100644
--- a/tests/__snapshots__/MenuItem.spec.js.snap
+++ b/tests/__snapshots__/MenuItem.spec.js.snap
@@ -1,5 +1,14 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`MenuItem overwrite default role should set role to listitem 1`] = `
+<li
+  class="undefined-item"
+  role="listitem"
+>
+  test
+</li>
+`;
+
 exports[`MenuItem overwrite default role should set role to none if none 1`] = `
 <li
   class="undefined-item"
@@ -18,7 +27,7 @@ exports[`MenuItem overwrite default role should set role to none if null 1`] = `
 </li>
 `;
 
-exports[`MenuItem overwrite default role should set specific role 1`] = `
+exports[`MenuItem overwrite default role should set role to option 1`] = `
 <li
   class="undefined-item"
   role="option"