fix(ui-menu): screenreaders should read the correct number of menu items

Closes: INSTUI-4285

Tested on VoiceOver, NVDA, JAWS. Also simplified DOM structure quite a bit
TEST PLAN:
Open the Menu example and go trough it with NVDA, JAWS, VoiceOver. You should be able to navigate
and they should read the correct menu item number (e.g. 3 of 7)

Author: matyasf

packages/ui-menu/src/Menu/MenuItem/index.tsx

@@ -187,8 +187,6 @@ class MenuItem extends Component<MenuItemProps, MenuItemState> {
         return 'menuitemcheckbox'
       case 'radio':
         return 'menuitemradio'
-      case 'flyout':
-        return 'button'
       default:
         return 'menuitem'
     }

packages/ui-menu/src/Menu/MenuItemGroup/__new-tests__/MenuItemGroup.test.tsx

@@ -40,44 +40,11 @@ describe('<MenuItemGroup />', () => {
         <MenuItemSeparator />
       </MenuItemGroup>
     )
-    const group = container.querySelector("[id*='MenuItemGroup_']")
-    const groupMenu = screen.getByRole('menu')
-
+    const group = container.querySelector("[class*='menuItemGroup']")
     expect(group).toBeInTheDocument()
     expect(group).toHaveTextContent('Menu Label')
-
-    expect(groupMenu).toBeInTheDocument()
-    expect(groupMenu).toHaveTextContent('Item Text 1')
-    expect(groupMenu).toHaveTextContent('Item Text 2')
-  })
-
-  it('should set the role to "menu"', () => {
-    const { container } = render(
-      <MenuItemGroup label="Select one">
-        <MenuItem>Foo</MenuItem>
-        <MenuItem>Bar</MenuItem>
-        <MenuItemSeparator />
-      </MenuItemGroup>
-    )
-    const menuItemGroup = container.querySelector(
-      "[class*='menuItemGroup__items']"
-    )
-
-    expect(menuItemGroup).toHaveAttribute('role', 'menu')
-  })
-
-  it('should set the list item role to "none"', () => {
-    render(
-      <MenuItemGroup label="Select one">
-        <MenuItem>Food</MenuItem>
-        <MenuItem>Bar</MenuItem>
-      </MenuItemGroup>
-    )
-    const menu = screen.getByRole('menu')
-    const menuListItem = menu.firstChild as HTMLElement
-
-    expect(menuListItem.tagName).toBe('LI')
-    expect(menuListItem).toHaveAttribute('role', 'none')
+    expect(group).toHaveTextContent('Item Text 1')
+    expect(group).toHaveTextContent('Item Text 2')
   })
 
   it('should default to children with type "radio"', () => {
@@ -114,10 +81,8 @@ describe('<MenuItemGroup />', () => {
         <MenuItemSeparator />
       </MenuItemGroup>
     )
-    const menu = screen.getByRole('menu')
     const menuItems = screen.getAllByRole('menuitemradio')
 
-    expect(menu).toHaveAttribute('aria-disabled', 'true')
     expect(menuItems).toHaveLength(2)
     expect(menuItems[0]).toHaveAttribute('aria-disabled', 'true')
     expect(menuItems[1]).toHaveAttribute('aria-disabled', 'true')

packages/ui-menu/src/Menu/MenuItemGroup/index.tsx

@@ -82,11 +82,8 @@ class MenuItemGroup extends Component<MenuGroupProps, MenuGroupState> {
         selected: this.selectedFromChildren(props) || props.defaultSelected!
       }
     }
-
-    this._labelId = props.deterministicId!('MenuItemGroup')
   }
 
-  private _labelId: string
   ref: Element | null = null
 
   handleRef = (el: Element | null) => {
@@ -210,23 +207,18 @@ class MenuItemGroup extends Component<MenuGroupProps, MenuGroupState> {
         ++index
         const value = child.props.value || index
 
-        return (
-          <li role="none">
-            {' '}
-            {safeCloneElement(child, {
-              tabIndex: isTabbable && index === 0 ? 0 : -1,
-              controls,
-              value,
-              children: child.props.children,
-              type: allowMultiple ? 'checkbox' : 'radio',
-              ref: this.props.itemRef,
-              disabled: disabled || child.props.disabled,
-              selected: this.selected.indexOf(value) > -1,
-              onSelect: this.handleSelect,
-              onMouseOver
-            })}{' '}
-          </li>
-        )
+        return safeCloneElement(child, {
+          tabIndex: isTabbable && index === 0 ? 0 : -1,
+          controls,
+          value,
+          children: child.props.children,
+          type: allowMultiple ? 'checkbox' : 'radio',
+          ref: this.props.itemRef,
+          disabled: disabled || child.props.disabled,
+          selected: this.selected.indexOf(value) > -1,
+          onSelect: this.handleSelect,
+          onMouseOver
+        })
       } else {
         return child
       }
@@ -239,18 +231,15 @@ class MenuItemGroup extends Component<MenuGroupProps, MenuGroupState> {
       <span
         {...props}
         css={this.props.styles?.menuItemGroup}
-        role="presentation"
         ref={this.handleRef}
       >
-        <span id={this._labelId}>{this.renderLabel()}</span>
-        <ul
-          role="menu"
+        {this.renderLabel()}
+        <div
           css={this.props.styles?.items}
           aria-disabled={this.props.disabled ? 'true' : undefined}
-          aria-labelledby={this._labelId}
         >
           {this.renderChildren()}
-        </ul>
+        </div>
       </span>
     )
   }

packages/ui-menu/src/Menu/MenuItemSeparator/index.tsx

@@ -66,6 +66,8 @@ class MenuItemSeparator extends Component<MenuSeparatorProps> {
 
   render() {
     const props = omitProps(this.props, MenuItemSeparator.allowedProps)
+    // role="separator" would fit better here, but it causes NVDA to stop the
+    // MenuItem count after it
     return (
       <div
         {...props}

packages/ui-menu/src/Menu/index.tsx

@@ -95,15 +95,15 @@ class Menu extends Component<MenuProps> {
   _popover: Popover | null = null
   _trigger: MenuItem | (React.ReactInstance & { focus?: () => void }) | null =
     null
-  _menu: HTMLUListElement | null = null
+  _menu: HTMLElement | null = null
   _labelId = this.props.deterministicId!('Menu__label')
 
   _activeSubMenu?: Menu | null
   _id: string
 
   ref: Element | null = null
 
-  handleRef = (el: HTMLUListElement | null) => {
+  handleRef = (el: HTMLElement | null) => {
     const { menuRef } = this.props
     this._menu = el
     if (typeof menuRef === 'function') {
@@ -169,7 +169,7 @@ class Menu extends Component<MenuProps> {
     }
   }
 
-  handleMenuKeyDown = (event: React.KeyboardEvent<HTMLUListElement>) => {
+  handleMenuKeyDown = (event: React.KeyboardEvent<HTMLElement>) => {
     const key = event && event.keyCode
     const { down, up, tab, left } = keycode.codes
     const pgdn = keycode.codes['page down']
@@ -338,7 +338,7 @@ class Menu extends Component<MenuProps> {
         if (
           matchComponentTypes<MenuSeparatorChild>(child, ['MenuItemSeparator'])
         ) {
-          return <li role="none">{child}</li>
+          return child
         }
 
         const menuItemChild = child
@@ -350,71 +350,59 @@ class Menu extends Component<MenuProps> {
           this.props.controls
 
         if (matchComponentTypes<MenuItemChild>(child, ['MenuItem'])) {
-          return (
-            <li role="none">
-              {safeCloneElement(child, {
-                controls,
-                children: child.props.children,
-                disabled: disabled || child.props.disabled,
-                onFocus: this.handleMenuItemFocus,
-                onBlur: this.handleMenuItemBlur,
-                onSelect: this.handleMenuItemSelect,
-                onMouseOver: this.handleMenuItemMouseOver,
-                tabIndex: isTabbable ? 0 : -1
-              })}
-            </li>
-          )
+          return safeCloneElement(child, {
+            controls,
+            children: child.props.children,
+            disabled: disabled || child.props.disabled,
+            onFocus: this.handleMenuItemFocus,
+            onBlur: this.handleMenuItemBlur,
+            onSelect: this.handleMenuItemSelect,
+            onMouseOver: this.handleMenuItemMouseOver,
+            tabIndex: isTabbable ? 0 : -1
+          })
         }
 
         if (matchComponentTypes<MenuGroupChild>(child, ['MenuItemGroup'])) {
-          return (
-            <li role="none">
-              {safeCloneElement(child, {
-                label: child.props.label,
-                controls,
-                disabled: disabled || child.props.disabled,
-                onFocus: this.handleMenuItemFocus,
-                onBlur: this.handleMenuItemBlur,
-                onSelect: this.handleMenuItemSelect,
-                onMouseOver: this.handleMenuItemMouseOver,
-                isTabbable
-              })}
-            </li>
-          )
+          return safeCloneElement(child, {
+            label: child.props.label,
+            controls,
+            disabled: disabled || child.props.disabled,
+            onFocus: this.handleMenuItemFocus,
+            onBlur: this.handleMenuItemBlur,
+            onSelect: this.handleMenuItemSelect,
+            onMouseOver: this.handleMenuItemMouseOver,
+            isTabbable
+          })
         }
 
         if (matchComponentTypes(child, ['Menu'])) {
           const submenuDisabled = disabled || child.props.disabled
 
-          return (
-            <li role="none">
-              {safeCloneElement(child, {
-                type: 'flyout',
-                controls,
-                disabled: submenuDisabled,
-                onSelect: this.handleMenuItemSelect,
-                placement: 'end top',
-                offsetX: -5,
-                offsetY: 5,
-                withArrow: false,
-                onToggle: this.handleSubMenuToggle,
-                onDismiss: this.handleSubMenuDismiss,
-                trigger: (
-                  <MenuItem
-                    onMouseOver={this.handleMenuItemMouseOver}
-                    onFocus={this.handleMenuItemFocus}
-                    onBlur={this.handleMenuItemBlur}
-                    tabIndex={isTabbable ? 0 : -1}
-                    type="flyout"
-                    disabled={submenuDisabled}
-                    renderLabelInfo={child.props.renderLabelInfo}
-                  >
-                    {child.props.title || child.props.label}
-                  </MenuItem>
-                )
-              })}
-            </li>
-          )
+          return safeCloneElement(child, {
+            type: 'flyout',
+            controls,
+            disabled: submenuDisabled,
+            onSelect: this.handleMenuItemSelect,
+            placement: 'end top',
+            offsetX: -5,
+            offsetY: 5,
+            withArrow: false,
+            onToggle: this.handleSubMenuToggle,
+            onDismiss: this.handleSubMenuDismiss,
+            trigger: (
+              <MenuItem
+                onMouseOver={this.handleMenuItemMouseOver}
+                onFocus={this.handleMenuItemFocus}
+                onBlur={this.handleMenuItemBlur}
+                tabIndex={isTabbable ? 0 : -1}
+                type="flyout"
+                disabled={submenuDisabled}
+                renderLabelInfo={child.props.renderLabelInfo}
+              >
+                {child.props.title || child.props.label}
+              </MenuItem>
+            )
+          })
         }
         return
       }
@@ -434,7 +422,7 @@ class Menu extends Component<MenuProps> {
           registerMenuItem: this.registerMenuItem
         }}
       >
-        <ul
+        <div
           role="menu"
           aria-label={label}
           tabIndex={0}
@@ -447,7 +435,7 @@ class Menu extends Component<MenuProps> {
           ref={this.handleRef}
         >
           {this.renderChildren()}
-        </ul>
+        </div>
       </MenuContext.Provider>
     )
   }

packages/ui-menu/src/Menu/props.ts

@@ -111,15 +111,15 @@ type MenuOwnProps = {
   /**
    * Callback fired on the onKeyDown of the `<Menu />`
    */
-  onKeyDown?: (event: React.KeyboardEvent<HTMLUListElement>) => void
+  onKeyDown?: (event: React.KeyboardEvent<HTMLElement>) => void
   /**
    * Callback fired on the onKeyUp of the `<Menu />`
    */
-  onKeyUp?: (event: React.KeyboardEvent<HTMLUListElement>) => void
+  onKeyUp?: (event: React.KeyboardEvent<HTMLElement>) => void
   /**
    * A function that returns a reference to the `<Menu />`
    */
-  menuRef?: (el: HTMLUListElement | null) => void
+  menuRef?: (el: HTMLElement | null) => void
   /**
    * A function that returns a reference to the `<Popover />`
    */