Make SubNav SubMenus accessible using keyboard

.changeset/wild-olives-sin.md

@@ -0,0 +1,5 @@
+---
+'@primer/react-brand': patch
+---
+
+Fixed an issue where `SubNav` submenus were not accessible through keyboard navigation

packages/react/src/SubNav/SubNav.module.css

@@ -42,6 +42,10 @@
   display: none !important;
 }
 
+.SubNav__sub-menu-children {
+  display: contents;
+}
+
 /*
  * Narrow breakpoint
  */
@@ -289,17 +293,28 @@
     width: var(--brand-SubNav-width-subMenu);
   }
 
-  .SubNav__link--has-sub-menu:hover .SubNav__sub-menu,
-  .SubNav__link--has-sub-menu:focus-visible .SubNav__sub-menu {
+  .SubNav__link--expanded .SubNav__sub-menu {
     visibility: visible;
     opacity: 1;
     transform: scale(1) translateY(0);
     box-shadow: var(--brand-SubNav-shadow);
   }
 
+  .SubNav__sub-menu-toggle {
+    border: none;
+    padding: 0;
+    margin: 0;
+    background: none;
+    display: inline-flex;
+    align-items: center;
+    justify-content: center;
+    cursor: pointer;
+  }
+
   .SubNav__sub-menu .SubNav__link {
     display: block;
   }
+
   .SubNav__sub-menu .SubNav__link-label {
     color: var(--brand-color-text-default);
     font-weight: var(--brand-text-weight-100);

packages/react/src/SubNav/SubNav.module.css.d.ts

@@ -6,6 +6,7 @@ declare const styles: {
   readonly "SubNav--full-width": string;
   readonly "SubNav__action-container": string;
   readonly "SubNav__sub-menu-icon": string;
+  readonly "SubNav__sub-menu-children": string;
   readonly "SubNav--open": string;
   readonly "fade-in": string;
   readonly "SubNav__links-overlay": string;
@@ -17,6 +18,8 @@ declare const styles: {
   readonly "SubNav__overlay-toggle-icon": string;
   readonly "SubNav__overlay-toggle-content": string;
   readonly "SubNav__link-label": string;
+  readonly "SubNav__link--expanded": string;
+  readonly "SubNav__sub-menu-toggle": string;
   readonly "SubNav__link--has-sub-menu": string;
   readonly "fade-in-down": string;
 };

packages/react/src/SubNav/SubNav.test.tsx

@@ -1,10 +1,11 @@
 import {HTMLAttributes} from 'react'
-import React, {render, cleanup, fireEvent} from '@testing-library/react'
+import React, {render, cleanup, fireEvent, within} from '@testing-library/react'
 import '@testing-library/jest-dom'
 import {axe, toHaveNoViolations} from 'jest-axe'
 
 import {SubNav} from './SubNav'
 import '../test-utils/mocks/match-media-mock'
+import userEvent from '@testing-library/user-event'
 
 expect.extend(toHaveNoViolations)
 
@@ -106,4 +107,51 @@ describe('SubNav', () => {
 
     expect(results).toHaveNoViolations()
   })
+
+  it('shows subitems when the submenu toggle is activated', async () => {
+    const {getByRole, getAllByTestId} = render(
+      <SubNav fullWidth>
+        <SubNav.Link href="#" aria-current="page">
+          Copilot
+          <SubNav.SubMenu>
+            <SubNav.Link href="#">Copilot feature page one</SubNav.Link>
+            <SubNav.Link href="#">Copilot feature page two</SubNav.Link>
+            <SubNav.Link href="#">Copilot feature page three</SubNav.Link>
+          </SubNav.SubMenu>
+        </SubNav.Link>
+        <SubNav.Link href="#">Code review</SubNav.Link>
+        <SubNav.Link href="#">Search</SubNav.Link>
+        <SubNav.Action href="#">Call to action</SubNav.Action>
+      </SubNav>,
+    )
+
+    userEvent.tab()
+    expect(getByRole('link', {name: 'Copilot'})).toHaveFocus()
+
+    const toggleSubmenuButton = getByRole('button', {name: 'Open submenu'})
+    expect(toggleSubmenuButton).toHaveAttribute('aria-expanded', 'false')
+
+    userEvent.tab()
+    expect(toggleSubmenuButton).toHaveFocus()
+
+    userEvent.keyboard('{enter}')
+    expect(toggleSubmenuButton).toHaveFocus()
+    expect(toggleSubmenuButton).toHaveAttribute('aria-expanded', 'true')
+
+    const expanded = getAllByTestId('SubNav-root-link')[0]
+
+    userEvent.tab()
+    expect(within(expanded).getByRole('link', {name: 'Copilot feature page one'})).toHaveFocus()
+
+    userEvent.tab()
+    expect(within(expanded).getByRole('link', {name: 'Copilot feature page two'})).toHaveFocus()
+
+    userEvent.tab()
+    expect(within(expanded).getByRole('link', {name: 'Copilot feature page three'})).toHaveFocus()
+
+    userEvent.tab()
+    expect(getByRole('link', {name: 'Code review'})).toHaveFocus()
+
+    expect(toggleSubmenuButton).toHaveAttribute('aria-expanded', 'false')
+  })
 })

packages/react/src/SubNav/SubNav.tsx

@@ -1,12 +1,14 @@
 import React, {
   Children,
+  forwardRef,
   isValidElement,
-  useState,
-  PropsWithChildren,
   memo,
-  ReactElement,
-  ReactNode,
   useCallback,
+  useState,
+  type PropsWithChildren,
+  type ReactElement,
+  type ReactNode,
+  type RefObject,
 } from 'react'
 import {Button, ButtonSizes, ButtonVariants, Text} from '..'
 
@@ -16,6 +18,8 @@ import {useId} from '@reach/auto-id'
 import {useKeyboardEscape} from '../hooks/useKeyboardEscape'
 import {useFocusTrap} from '../hooks/useFocusTrap'
 import {useOnClickOutside} from '../hooks/useOnClickOutside'
+import {useProvidedRefOrCreate} from '../hooks/useRef'
+import {useContainsFocus} from './useContainsFocus'
 
 import type {BaseProps} from '../component-helpers'
 
@@ -176,58 +180,100 @@ type SubNavLinkProps = {
 } & PropsWithChildren<React.HTMLProps<HTMLAnchorElement>> &
   BaseProps<HTMLAnchorElement>
 
-const SubNavLink = ({
-  children,
-  href,
-  'aria-current': ariaCurrent,
-  'data-testid': testId,
-  className,
-  ...props
-}: SubNavLinkProps) => {
-  const hasSubMenu = Children.toArray(children).some(child => {
-    if (isValidElement(child)) {
-      return child.type === _SubMenu
-    }
-  })
+const SubNavLinkWithSubmenu = forwardRef<HTMLDivElement, SubNavLinkProps>(
+  ({children, href, 'aria-current': ariaCurrent, 'data-testid': testId, className, ...props}, forwardedRef) => {
+    const submenuId = useId()
 
-  const [label, SubMenuChildren] = children as ReactNode[]
+    const [isExpanded, setIsExpanded] = useState(false)
+    const ref = useProvidedRefOrCreate(forwardedRef as RefObject<HTMLDivElement>)
 
-  return (
-    <>
-      {hasSubMenu ? (
-        <div
-          className={clsx(styles['SubNav__link'], styles['SubNav__link--has-sub-menu'])}
-          data-testid={testId || testIds.link}
-        >
-          <a
-            href={href}
-            className={clsx(styles['SubNav__link'], ariaCurrent && styles['SubNav__link--active'], className)}
-            aria-current={ariaCurrent}
-            {...props}
-          >
-            <Text as="span" size="200" className={styles['SubNav__link-label']}>
-              {label}
-            </Text>
-          </a>
-          <>{SubMenuChildren}</>
-          {<ChevronDownIcon className={styles['SubNav__sub-menu-icon']} size={16} />}
-        </div>
-      ) : (
+    useContainsFocus(ref, (containsFocus: boolean) => {
+      if (!containsFocus) {
+        setIsExpanded(false)
+      }
+    })
+
+    const [label, SubMenuChildren] = children as ReactNode[]
+
+    const onKeyDown = useCallback((e: React.KeyboardEvent<HTMLButtonElement>) => {
+      if (['Enter', ' '].includes(e.key)) {
+        setIsExpanded(prev => !prev)
+      }
+    }, [])
+
+    return (
+      <div
+        className={clsx(
+          styles['SubNav__link'],
+          styles['SubNav__link--has-sub-menu'],
+          isExpanded && styles['SubNav__link--expanded'],
+        )}
+        data-testid={testId || testIds.link}
+        ref={ref}
+        onMouseOver={() => setIsExpanded(true)}
+        onMouseOut={() => setIsExpanded(false)}
+        /**
+         * onFocus and onBlur need to be defined to keep the jsx-a11y/mouse-events-have-key-events
+         * eslint rule happy. The focus/blur behaviour is handled by useContainsFocus
+         */
+        onFocus={() => null}
+        onBlur={() => null}
+      >
         <a
           href={href}
           className={clsx(styles['SubNav__link'], ariaCurrent && styles['SubNav__link--active'], className)}
           aria-current={ariaCurrent}
-          data-testid={testId || testIds.link}
           {...props}
         >
           <Text as="span" size="200" className={styles['SubNav__link-label']}>
-            {children}
+            {label}
           </Text>
         </a>
-      )}
-    </>
+        <button
+          className={styles['SubNav__sub-menu-toggle']}
+          onKeyDown={onKeyDown}
+          aria-expanded={isExpanded ? 'true' : 'false'}
+          aria-controls={submenuId}
+          aria-label={`${isExpanded ? 'Close' : 'Open'} submenu`}
+        >
+          <ChevronDownIcon className={styles['SubNav__sub-menu-icon']} size={16} />
+        </button>
+        <div id={submenuId} className={styles['SubNav__sub-menu-children']}>
+          {SubMenuChildren}
+        </div>
+      </div>
+    )
+  },
+)
+
+const SubNavLink = forwardRef<HTMLAnchorElement | HTMLDivElement, SubNavLinkProps>((props, ref) => {
+  const hasSubMenu = Children.toArray(props.children).some(child => {
+    if (isValidElement(child)) {
+      return child.type === _SubMenu
+    }
+  })
+
+  if (hasSubMenu) {
+    return <SubNavLinkWithSubmenu {...props} ref={ref as RefObject<HTMLDivElement>} />
+  }
+
+  const {children, href, 'aria-current': ariaCurrent, 'data-testid': testId, className, ...rest} = props
+
+  return (
+    <a
+      href={href}
+      className={clsx(styles['SubNav__link'], ariaCurrent && styles['SubNav__link--active'], className)}
+      aria-current={ariaCurrent}
+      data-testid={testId || testIds.link}
+      ref={ref as RefObject<HTMLAnchorElement>}
+      {...rest}
+    >
+      <Text as="span" size="200" className={styles['SubNav__link-label']}>
+        {children}
+      </Text>
+    </a>
   )
-}
+})
 
 function _SubMenu({children, className, ...props}: PropsWithChildren<BaseProps<HTMLDivElement>>) {
   return (

packages/react/src/SubNav/useContainsFocus.ts

@@ -0,0 +1,53 @@
+import {useCallback, useEffect, useState, type RefObject} from 'react'
+
+/**
+ * Determine if a child element of the provided ref is currently focussed.
+ * @param containerRef The ref to the container element.
+ * @param onFocusChange The callback to be called when the focus state changes.
+ * @type T The type of the container element.
+ * @returns The ref to be applied to the container element.
+ */
+export const useContainsFocus = <T extends HTMLElement>(
+  containerRef?: RefObject<T>,
+  onFocusChange?: (isFocussed: boolean) => void,
+) => {
+  const [isChildFocused, setIsChildFocused] = useState(false)
+
+  const updateState = useCallback(
+    (isFocused: boolean) => {
+      if (isFocused !== isChildFocused) {
+        setIsChildFocused(isFocused)
+        onFocusChange?.(isFocused)
+      }
+    },
+    [isChildFocused, onFocusChange],
+  )
+
+  useEffect(() => {
+    if (!containerRef) {
+      return
+    }
+
+    const handleFocusIn = () => {
+      updateState(true)
+    }
+
+    const handleFocusOut = (event: FocusEvent) => {
+      if (containerRef.current && !containerRef.current.contains(event.relatedTarget as Node)) {
+        updateState(false)
+      }
+    }
+
+    const container = containerRef.current
+
+    if (container) {
+      container.addEventListener('focusin', handleFocusIn, true)
+      container.addEventListener('focusout', handleFocusOut, true)
+
+      return () => {
+        container.removeEventListener('focusin', handleFocusIn, true)
+        container.removeEventListener('focusout', handleFocusOut, true)
+      }
+    }
+  }, [updateState, containerRef])
+}