Merge pull request #382 from youluna/test/menu

test(Menu): pass a11y test
alibaba-fusion · Feb 27, 2019 · ecc6bb9 · ecc6bb9
2 parents cb819ca + 9b35b8f
commit ecc6bb9
Show file tree

Hide file tree

Showing 7 changed files with 48 additions and 22 deletions.
diff --git a/src/menu/view/item.jsx b/src/menu/view/item.jsx
@@ -187,6 +187,7 @@ export default class Item extends Component {
               });
         if (disabled) {
             others['aria-disabled'] = true;
+            others['aria-hidden'] = true;
         }
 
         others.tabIndex = root.tabbableKey === _key ? '0' : '-1';
@@ -206,9 +207,14 @@ export default class Item extends Component {
         }
         const TagName = component;
 
+        let role = 'menuitem';
+        if ('selectMode' in root.props) {
+            role = 'listitem';
+        }
+
         return (
             <TagName
-                role="menuitem"
+                role={role}
                 title={this.getTitle(children)}
                 {...others}
                 className={newClassName}

diff --git a/src/menu/view/menu.jsx b/src/menu/view/menu.jsx
@@ -729,7 +729,13 @@ export default class Menu extends Component {
             [className]: !!className,
         });
 
-        const role = direction === 'hoz' ? 'menubar' : 'menu';
+        let role = direction === 'hoz' ? 'menubar' : 'menu';
+        let ariaMultiselectable;
+        if ('selectMode' in this.props) {
+            role = 'listbox';
+            ariaMultiselectable = !!(selectMode === 'multiple');
+        }
+
         const headerElement = header ? (
             <li className={`${prefix}menu-header`}>{header}</li>
         ) : null;
@@ -754,7 +760,7 @@ export default class Menu extends Component {
                 onBlur={this.onBlur}
                 className={newClassName}
                 onKeyDown={this.handleEnter}
-                aria-multiselectable={selectMode === 'multiple'}
+                aria-multiselectable={ariaMultiselectable}
                 {...others}
             >
                 {headerElement}

diff --git a/src/menu/view/selectable-item.jsx b/src/menu/view/selectable-item.jsx
@@ -129,14 +129,21 @@ export default class SelectableItem extends Component {
             onKeyDown: this.handleKeyDown,
             onClick: !disabled ? this.handleClick : this.props.onClick,
             needIndent,
-            'aria-selected': selected,
             ...others,
         };
 
+        const textProps = {};
+
+        if ('selectMode' in root.props) {
+            textProps['aria-selected'] = selected;
+        }
+
         return (
             <Item {...newProps}>
                 {this.renderSelectedIcon(selected)}
-                <span className={`${prefix}menu-item-text`}>{children}</span>
+                <span className={`${prefix}menu-item-text`} {...textProps}>
+                    {children}
+                </span>
                 {helper ? (
                     <div className={`${prefix}menu-item-helper`}>{helper}</div>
                 ) : null}

diff --git a/src/menu/view/sub-menu.jsx b/src/menu/view/sub-menu.jsx
@@ -200,9 +200,16 @@ export default class SubMenu extends Component {
             [subMenuContentClassName]: !!subMenuContentClassName,
         });
 
+        let roleMenu = 'menu',
+            roleItem = 'menuitem';
+        if ('selectMode' in root.props) {
+            roleMenu = 'listbox';
+            roleItem = 'listitem';
+        }
+
         const subMenu = open ? (
             <ul
-                role="menu"
+                role={roleMenu}
                 dir={rtl ? 'rtl' : undefined}
                 ref="subMenu"
                 className={newSubMenuContentClassName}
@@ -212,7 +219,7 @@ export default class SubMenu extends Component {
         ) : null;
 
         return (
-            <li {...others} {...liProps}>
+            <li role={roleItem} {...others} {...liProps}>
                 <NewItem {...itemProps}>
                     <span className={`${prefix}menu-item-text`}>{label}</span>
                     <Icon {...arrorProps} />

diff --git a/test/menu-button/index-spec.js b/test/menu-button/index-spec.js
@@ -29,7 +29,7 @@ describe('MenuButton', () => {
         it('should controlled selectedKeys', () => {
             const wrapper = mount(<MenuButton label="hello world" visible selectedKeys={['a']} selectMode="single">{menu}</MenuButton>);
             wrapper.setProps({ selectedKeys: ['b'] });
-            assert(wrapper.find('li[title="b"][role="menuitem"]').hasClass('next-selected'));
+            assert(wrapper.find('li[title="b"][role="listitem"]').hasClass('next-selected'));
         });
 
         it('should controlled popup visible', () => {
@@ -51,14 +51,14 @@ describe('MenuButton', () => {
 
         it('should select in uncontrolled mode', () => {
             const wrapper = mount(<MenuButton label="hello world" visible selectMode="single">{menu}</MenuButton>);
-            wrapper.find('li[title="b"][role="menuitem"]').simulate('click');
-            assert(wrapper.find('li[title="b"][role="menuitem"]').hasClass('next-selected'));
+            wrapper.find('li[title="b"][role="listitem"]').simulate('click');
+            assert(wrapper.find('li[title="b"][role="listitem"]').hasClass('next-selected'));
         });
 
         it('should select in controlled mode', () => {
             const wrapper = mount(<MenuButton label="hello world" visible selectedKeys={['a']} selectMode="single">{menu}</MenuButton>);
-            wrapper.find('li[title="b"][role="menuitem"]').simulate('click');
-            assert(wrapper.find('li[title="a"][role="menuitem"]').hasClass('next-selected'));
+            wrapper.find('li[title="b"][role="listitem"]').simulate('click');
+            assert(wrapper.find('li[title="a"][role="listitem"]').hasClass('next-selected'));
         });
     });
 });
diff --git a/test/menu/a11y-spec.js b/test/menu/a11y-spec.js
@@ -20,7 +20,7 @@ describe.skip('Menu A11y', () => {
         unmount();
     });
 
-    // TODO: Fix `aria-allowed-attr` due to `aria-multiselectable=\"false\"`, `aria-selected=\"false\"`
+    // Fix `aria-allowed-attr` due to `aria-multiselectable=\"false\"`, `aria-selected=\"false\"`
     it('should not have any violations for Item', async () => {
         wrapper = await mountReact(
             <Menu>
@@ -32,7 +32,7 @@ describe.skip('Menu A11y', () => {
         return test('#item');
     });
 
-    // TODO: Fix issue with Item
+    // Fix issue with Item
     it('should not have any violations for simple example', async () => {
         wrapper = await testReact(
             <Menu>
@@ -46,7 +46,7 @@ describe.skip('Menu A11y', () => {
         return wrapper;
     });
 
-    // TODO: Fix issue with Item
+    // Fix issue with Item
     it('should not have any violations for Group', async () => {
         wrapper = await testReact(
             <Menu>
@@ -59,9 +59,9 @@ describe.skip('Menu A11y', () => {
         return wrapper;
     });
 
-    // TODO: This throws a false error for `li` nested inside role="menu". This is a bug in axe-core. Issue was created (https://github.com/dequelabs/axe-core/issues/1365)
+    // This throws a false error for `li` nested inside role="menu". This is a bug in axe-core. Issue was created (https://github.com/dequelabs/axe-core/issues/1365)
     //       Follow up to resolve this bug.
-    it.skip('should not have any violations for SubMenu', async () => {
+    it('should not have any violations for SubMenu', async () => {
         wrapper = await testReact(
             <Menu className="my-menu" defaultOpenKeys="sub-menu">
                 <SubMenu id="submenu" key="sub-menu" label="Sub menu">

diff --git a/test/split-button/index-spec.js b/test/split-button/index-spec.js
@@ -29,7 +29,7 @@ describe('SplitButton', () => {
         it('should controlled selectedkeys', () => {
             const wrapper = mount(<SplitButton label="hello world" visible selectedKeys={['a']} selectMode="single">{menu}</SplitButton>);
             wrapper.setProps({ selectedKeys: ['b'] });
-            assert(wrapper.find('li[title="b"][role="menuitem"]').hasClass('next-selected'));
+            assert(wrapper.find('li[title="b"][role="listitem"]').hasClass('next-selected'));
         });
 
         it('should controlled popup visible', () => {
@@ -51,14 +51,14 @@ describe('SplitButton', () => {
 
         it('should select in uncontrolled mode', () => {
             const wrapper = mount(<SplitButton label="hello world" visible selectMode="single">{menu}</SplitButton>);
-            wrapper.find('li[title="b"][role="menuitem"]').simulate('click');
-            assert(wrapper.find('li[title="b"][role="menuitem"]').hasClass('next-selected'));
+            wrapper.find('li[title="b"][role="listitem"]').simulate('click');
+            assert(wrapper.find('li[title="b"][role="listitem"]').hasClass('next-selected'));
         });
 
         it('should select in controlled mode', () => {
             const wrapper = mount(<SplitButton label="hello world" visible selectedKeys={['a']} selectMode="single">{menu}</SplitButton>);
-            wrapper.find('li[title="b"][role="menuitem"]').simulate('click');
-            assert(wrapper.find('li[title="a"][role="menuitem"]').hasClass('next-selected'));
+            wrapper.find('li[title="b"][role="listitem"]').simulate('click');
+            assert(wrapper.find('li[title="a"][role="listitem"]').hasClass('next-selected'));
         });
 
     });