feat: ensure all menu children are li elements

- wrap non-li children in li tags for consistency
- assign tabIndex for visual focus to children with menuitem role
- add active CSS class to focused MenuItem component
- modify tests

components/menu/src/menu-item/menu-item.js

@@ -53,7 +53,7 @@ const MenuItem = ({
                     destructive,
                     disabled,
                     dense,
-                    active: active || showSubMenu,
+                    active: active || showSubMenu || tabIndex === 0,
                     'with-chevron': children || chevron,
                 })}
                 ref={menuItemRef}

components/menu/src/menu/__tests__/menu.test.js

@@ -139,46 +139,45 @@ describe('Menu Component', () => {
         expect(onClick).toHaveBeenCalledTimes(2)
     })
 
-    it('can handle non MenuItem components', async () => {
+    it('can handle non MenuItem components', () => {
         const onClick = jest.fn()
         const { getByText } = render(
             <Menu dataTest={menuDataTest} dense={false}>
-                <a href="#" role="menuitem">
-                    Link 1
-                </a>
                 <span role="menuitem" onClick={onClick}>
-                    Item 1
+                    Span 1
                 </span>
-                <MenuItem dataTest={menuItemDataTest} label="Menu item 2" />
+                <li tabIndex={-1}>
+                    <a href="#" role="menuitem">
+                        Link 2
+                    </a>
+                </li>
+                <li>
+                    <span onClick={onClick}>Span 2</span>
+                </li>
             </Menu>
         )
 
-        const span = getByText(/Item 1/i)
-        const link = getByText(/link 1/i)
-        const menuItem = getByText(/menu item 2/i)
+        const nonListMenuItem = getByText(/span 1/i)
+        const listMenuItem = getByText(/link 2/i)
+        const plainListItem = getByText(/span 2/i)
+
+        // all children must be list items
+        expect(nonListMenuItem.parentElement.nodeName).toBe('LI')
 
         userEvent.tab()
+        expect(nonListMenuItem.parentElement).toHaveFocus()
+        expect(nonListMenuItem.parentElement.tabIndex).toBe(0)
 
-        expect(link).toHaveFocus()
-        expect(link.tabIndex).toBe(0)
-        expect(span).not.toHaveFocus()
-        expect(menuItem.parentNode.parentNode).not.toHaveFocus()
+        expect(onClick).toHaveBeenCalledTimes(0)
+        userEvent.keyboard('[Space]')
+        expect(onClick).toHaveBeenCalledTimes(1)
 
         userEvent.keyboard('{ArrowDown}')
-        expect(link).not.toHaveFocus()
-        expect(span).toHaveFocus()
-        expect(menuItem.parentNode.parentNode).not.toHaveFocus()
+        expect(listMenuItem.parentElement).toHaveFocus()
 
         userEvent.keyboard('{ArrowDown}')
-        expect(link).not.toHaveFocus()
-        expect(span).not.toHaveFocus()
-        expect(menuItem.parentNode.parentNode).toHaveFocus()
-
-        userEvent.keyboard('{ArrowUp}')
-        expect(span).toHaveFocus()
-
-        expect(onClick).toHaveBeenCalledTimes(0)
-        userEvent.keyboard('[Space]')
-        expect(onClick).toHaveBeenCalledTimes(1)
+        expect(nonListMenuItem.parentElement).toHaveFocus()
+        // non menu items do not receive focus
+        expect(plainListItem.parentElement).not.toHaveFocus()
     })
 })

components/menu/src/menu/helpers.js

@@ -1,23 +1,26 @@
-export const isValidMenuItem = (node) => {
-    const role = node.getAttribute('role')
-
-    const menuItemCheck =
-        role && ['menuitem', 'menuitemcheckbox', 'menuitemradio'].includes(role)
+const isMenuItem = (role) => {
+    return ['menuitem', 'menuitemcheckbox', 'menuitemradio'].includes(role)
+}
 
-    // UI Library MenuItem
-    if (role === 'presentation') {
-        return isValidMenuItem(node.children[0])
-    } else {
-        return role !== 'separator' && menuItemCheck
+const isValidMenuItemNode = (node) => {
+    if (node.nodeName === 'LI' && node.firstElementChild) {
+        return isValidMenuItemNode(node.firstElementChild)
     }
+
+    const role = node.getAttribute('role')
+    return role && isMenuItem(role)
 }
 
-export const filterValidMenuItemsIndices = (children) => {
-    const validIndices = []
-    children.forEach((node, index) => {
-        if (isValidMenuItem(node)) {
-            validIndices.push(index)
+export const getFocusableItemsIndices = (elements) => {
+    const focusableIndices = []
+    elements.forEach((node, index) => {
+        if (isValidMenuItemNode(node)) {
+            focusableIndices.push(index)
         }
     })
-    return validIndices
+    return focusableIndices
+}
+
+export const hasMenuItemRole = (component) => {
+    return isMenuItem(component?.props?.['role'])
 }

components/menu/src/menu/menu.js

@@ -1,9 +1,54 @@
 import PropTypes from 'prop-types'
 import React, { Children, cloneElement, isValidElement } from 'react'
+import { hasMenuItemRole } from './helpers.js'
 import { useMenuNavigation } from './use-menu.js'
 
 const Menu = ({ children, className, dataTest, dense }) => {
     const { menuRef, focusedIndex } = useMenuNavigation(children)
+
+    const childrenToRender = Children.map(children, (child, index) => {
+        if (!isValidElement(child)) {
+            return child
+        }
+        const tabIndex = index === focusedIndex ? 0 : -1
+
+        const childProps = {
+            ...child.props,
+        }
+
+        if (typeof child.type === 'string') {
+            // remove non-native props from native HTML elements
+            delete childProps.hideDivider
+            delete childProps.dense
+            delete childProps.active
+
+            // all ul children must be li elements
+            // add tabindex for focus to those elements that are/contain a menuitem
+            if (child.type === 'li') {
+                return hasMenuItemRole(child.props.children[0])
+                    ? cloneElement(child, { ...childProps, tabIndex })
+                    : cloneElement(child, childProps)
+            } else {
+                return (
+                    <li tabIndex={hasMenuItemRole(child) ? tabIndex : null}>
+                        {cloneElement(child, childProps)}
+                    </li>
+                )
+            }
+        } else {
+            // assign non-native props to custom elements
+            childProps.dense =
+                typeof child.props.dense === 'boolean'
+                    ? child.props.dense
+                    : dense
+            childProps.hideDivider =
+                typeof child.props.hideDivider !== 'boolean' && index === 0
+                    ? true
+                    : child.props.hideDivider
+            return cloneElement(child, { ...childProps, tabIndex })
+        }
+    })
+
     return (
         <ul
             className={className}
@@ -12,37 +57,7 @@ const Menu = ({ children, className, dataTest, dense }) => {
             ref={menuRef}
             tabIndex={0}
         >
-            {Children.map(children, (child, index) => {
-                if (!isValidElement(child)) {
-                    return child
-                }
-                const tabIndex = index === focusedIndex ? 0 : -1
-
-                const childProps = {
-                    tabIndex,
-                    ...child.props,
-                }
-                // remove non native props from native elements
-                if (typeof child.type === 'string') {
-                    delete childProps.hideDivider
-                    delete childProps.dense
-                    delete childProps.active
-                } else {
-                    // assign non native props to custom elements
-                    childProps.active = index === focusedIndex
-                    childProps.dense =
-                        typeof child.props.dense === 'boolean'
-                            ? child.props.dense
-                            : dense
-                    childProps.hideDivider =
-                        typeof child.props.hideDivider !== 'boolean' &&
-                        index === 0
-                            ? true
-                            : child.props.hideDivider
-                }
-                return cloneElement(child, childProps)
-            })}
-
+            {childrenToRender}
             <style jsx>{`
                 ul {
                     display: block;

components/menu/src/menu/use-menu.js

@@ -1,122 +1,88 @@
-import { useCallback, useRef, useState, useEffect } from 'react'
-import { filterValidMenuItemsIndices } from './helpers.js'
+import { useRef, useState, useEffect, useCallback } from 'react'
+import { getFocusableItemsIndices } from './helpers.js'
 
 export const useMenuNavigation = (children) => {
     const menuRef = useRef(null)
-    const menuItemsRef = useRef(null)
     const [focusableItemsIndices, setFocusableItemsIndices] = useState(null)
     const [activeItemIndex, setActiveItemIndex] = useState(-1)
 
-    // Focus the first menu item when the menu receives focus
-    const handleFocus = useCallback(
-        (event) => {
-            if (event.target === menuRef?.current) {
-                const i = focusableItemsIndices?.[0]
-                menuRef.current.children[i].focus()
-                setActiveItemIndex(0)
-            }
-        },
-        [focusableItemsIndices]
-    )
+    // Initializes the indices for focusable items
+    useEffect(() => {
+        if (menuRef) {
+            const menuItems = Array.from(menuRef.current.children)
+            const itemsIndices = getFocusableItemsIndices(menuItems)
+            setFocusableItemsIndices(itemsIndices)
+        }
+    }, [children])
 
-    // Trigger actionable menu item
-    const handleAction = (event) => {
-        switch (event.key) {
-            case 'Enter':
-            case ' ':
-                event.preventDefault()
-                // UI library MenuItem
-                if (event.target.nodeName === 'LI') {
-                    event.target.children[0].click()
-                } else {
-                    event.target.click()
-                }
-                break
-            default:
-                break
+    // Focus the active menu child
+    useEffect(() => {
+        if (menuRef) {
+            if (focusableItemsIndices && activeItemIndex > -1) {
+                const currentIndex = focusableItemsIndices[activeItemIndex]
+                menuRef.current.children[currentIndex].focus()
+            }
         }
-    }
+    }, [activeItemIndex, focusableItemsIndices])
 
     // Navigate through focusable children using arrow keys
-    const handleNavigation = useCallback(
+    // Trigger actionable items
+    const handleKeyDown = useCallback(
         (event) => {
             const totalFocusablePositions = focusableItemsIndices?.length
+            const lastIndex = totalFocusablePositions - 1
             switch (event.key) {
                 case 'ArrowUp':
                     event.preventDefault()
-                    if (activeItemIndex > 0) {
-                        setActiveItemIndex(activeItemIndex - 1)
-                    } else {
-                        setActiveItemIndex(totalFocusablePositions - 1)
-                    }
+                    setActiveItemIndex(
+                        activeItemIndex > 0 ? activeItemIndex - 1 : lastIndex
+                    )
                     break
                 case 'ArrowDown':
                     event.preventDefault()
-                    if (activeItemIndex >= totalFocusablePositions - 1) {
-                        setActiveItemIndex(0)
-                    } else {
-                        setActiveItemIndex(activeItemIndex + 1)
+                    setActiveItemIndex(
+                        activeItemIndex >= lastIndex ? 0 : activeItemIndex + 1
+                    )
+                    break
+                case 'Enter':
+                case ' ':
+                    event.preventDefault()
+                    if (event.target.nodeName === 'LI') {
+                        event.target.children[0].click()
                     }
                     break
                 default:
                     break
             }
         },
-        [activeItemIndex, focusableItemsIndices]
+        [activeItemIndex, focusableItemsIndices?.length]
     )
 
-    // Keydown: handleNavigation and handleAction
-    const handleKeyDown = useCallback(
-        (event) => {
-            handleAction(event)
-            if (menuRef?.current?.children.length > 1) {
-                handleNavigation(event)
-            }
-        },
-        [handleNavigation]
-    )
-
-    // Initializes the indices for focusable items
+    // Event listeners for menu focus and key handling
     useEffect(() => {
         if (!menuRef) {
             return
         }
 
         const menu = menuRef.current
-        menuItemsRef.current = Array.from(menu.children)
-        const itemsIndices = filterValidMenuItemsIndices(
-            Array.from(menu.children)
-        )
-        setFocusableItemsIndices(itemsIndices)
-    }, [children])
 
-    // Focus the active menu child
-    useEffect(() => {
-        if (
-            focusableItemsIndices &&
-            menuItemsRef?.current &&
-            activeItemIndex > -1
-        ) {
-            const currentIndex = focusableItemsIndices[activeItemIndex]
-            menuItemsRef.current[currentIndex].focus()
-        }
-    }, [activeItemIndex, focusableItemsIndices])
-
-    // Event listeners for menu focus and key handling
-    useEffect(() => {
-        if (!menuRef) {
-            return
+        // Focus the first menu item when the menu receives focus
+        const handleFocus = (event) => {
+            if (event.target === menuRef.current) {
+                const firstItemIndex = focusableItemsIndices?.[0]
+                menuRef.current.children[firstItemIndex].focus()
+                setActiveItemIndex(0)
+            }
         }
 
-        const menu = menuRef.current
         menu.addEventListener('focus', handleFocus)
         menu.addEventListener('keydown', handleKeyDown)
 
         return () => {
             menu.removeEventListener('focus', handleFocus)
             menu.removeEventListener('keydown', handleKeyDown)
         }
-    }, [handleFocus, handleKeyDown])
+    }, [activeItemIndex, focusableItemsIndices, handleKeyDown])
 
     return {
         menuRef,

components/menu/types/index.d.ts

@@ -77,6 +77,7 @@ export interface MenuItemProps {
      * When true, nested menu items are shown in a Popper
      */
     showSubMenu?: boolean
+    tabIndex?: number
     /**
      * For using menu item as a link
      */