SubNav v2 a11y fixes (#854)

.changeset/curly-gorillas-happen.md

@@ -0,0 +1,9 @@
+---
+'@primer/react-brand': patch
+---
+
+`SubNav` accessibility improvements:
+
+- Removed focus-trapping in the menu overlay on narrow viewports
+- Added hover state to the `SubNav.Heading`
+- Added `aria-current` visual indicator parity on narrow viewports

.github/workflows/visual_test_storybook.yml

@@ -7,7 +7,7 @@ jobs:
   ci:
     if: ${{ github.repository == 'primer/brand' }}
     name: Storybook
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-24.04
     env:
       NODE_ENV: test
     steps:
@@ -17,7 +17,7 @@ jobs:
       - name: Set up Node
         uses: actions/setup-node@v2
         with:
-          node-version: 18
+          node-version: 20
 
       - name: Cache dependencies
         uses: actions/cache@v4
@@ -43,19 +43,13 @@ jobs:
         run: NODE_ENV=test npx start-server-and-test 'npx http-server ./apps/storybook/storybook-static -p 6006' 6006 'cd packages/e2e && npx playwright test'
         continue-on-error: true
 
-      - name: Upload error screenshots
-        uses: actions/upload-artifact@v3
-        if: steps.playwright-step.outcome != 'success'
-        with:
-          name: playwright-test-results
-          path: playwright-test-results
-
       - name: Upload test results
-        if: always()
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
+        if: ${{ !cancelled() }}
         with:
           name: playwright-report
-          path: playwright-report
+          path: playwright-report/
+          retention-days: 30
 
       - name: Comment on the PR about no visual differences
         if: steps.playwright-step.outcome != 'success'

packages/e2e/scripts/playwright/run-visual-tests

@@ -1,2 +1,2 @@
 cd ../../apps/storybook && npx storybook build && cd ../../packages/e2e
-docker run --rm --ipc=host -v ${PWD%/*/*}:/tests mcr.microsoft.com/playwright:v1.24.0-focal /bin/bash -c "cd tests && npm install && npx playwright install chrome && npx playwright install && npx start-server-and-test 'npx http-server ./apps/storybook/storybook-static -p 6006' 6006 'cd packages/e2e && npx playwright test'"
+docker run --rm --ipc=host -v ${PWD%/*/*}:/tests mcr.microsoft.com/playwright:v1.24.0-focal /bin/bash -c "cd tests && npm install && npx playwright install chromium && npx playwright install && npx start-server-and-test 'npx http-server ./apps/storybook/storybook-static -p 6006' 6006 'cd packages/e2e && npx playwright test'"

packages/e2e/scripts/playwright/update-visual-snapshots

@@ -1,2 +1,2 @@
 cd ../../apps/storybook && npx storybook build && cd ../../packages/e2e
-docker run --rm --ipc=host -v ${PWD%/*/*}:/tests mcr.microsoft.com/playwright:v1.37.0-jammy /bin/bash -c "cd tests && npm install && npx playwright install && npx start-server-and-test 'npx http-server ./apps/storybook/storybook-static -p 6006' 6006 'cd packages/e2e && npx playwright test --update-snapshots'"
+docker run --rm --ipc=host -v ${PWD%/*/*}:/tests mcr.microsoft.com/playwright:v1.46.0-noble /bin/bash -c "cd tests && npm install && npx playwright install && npx start-server-and-test 'npx http-server ./apps/storybook/storybook-static -p 6006' 6006 'cd packages/e2e && npx playwright test --update-snapshots'"

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

@@ -24,7 +24,7 @@
 }
 
 .SubNav__heading:hover {
-  color: var(--brand-SubNav-color-link-active);
+  color: var(--brand-color-text-muted);
   text-decoration: none !important; /* dotcom override */
 }
 
@@ -333,6 +333,10 @@
     text-decoration: none;
   }
 
+  .SubNav__links-overlay--open .SubNav__link-label {
+    position: relative;
+  }
+
   .SubNav__links-overlay--open .SubNav__link[aria-current]:not([aria-current='false']) * {
     color: var(--brand-SubNav-color-link-active);
   }
@@ -394,6 +398,7 @@
   .SubNav__link-label {
     font-size: var(--brand-text-size-200);
     font-weight: var(--base-text-weight-semibold);
+    padding-block-end: var(--base-size-6);
   }
 
   .SubNav__sub-menu--anchor {
@@ -418,9 +423,7 @@
     position: relative;
   }
 
-  .SubNav .SubNav__link:hover .SubNav__link-label::after,
-  .SubNav .SubNav__link:focus-visible .SubNav__link-label::after,
-  .SubNav .SubNav__link[aria-current]:not([aria-current='false']) .SubNav__link-label::after {
+  .SubNav .SubNav__link--has-sub-menu:hover .SubNav__sub-menu-children .SubNav__link-label::after {
     display: none;
   }
 }

packages/react/src/SubNav/SubNav.tsx

@@ -21,7 +21,6 @@ import {default as clsx} from 'clsx'
 import {ChevronDownIcon, ChevronUpIcon} from '@primer/octicons-react'
 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'
@@ -154,7 +153,6 @@ const _SubNavRoot = memo(({id, children, className, 'data-testid': testId, fullW
 
   useOnClickOutside(rootRef, closeMenuCallback)
   useKeyboardEscape(closeMenuCallback)
-  useFocusTrap({containerRef: overlayRef, restoreFocusOnCleanUp: true, disabled: !isOpenAtNarrow})
 
   useEffect(() => {
     if (isOpenAtNarrow && !isLarge) {
@@ -241,7 +239,7 @@ const _SubNavRoot = memo(({id, children, className, 'data-testid': testId, fullW
           <div ref={rootRef} className={styles['SubNav--header-container-outer']}>
             <div className={styles['SubNav__header-container']}>
               {HeadingChild && <div className={styles['SubNav__heading-container']}>{HeadingChild}</div>}
-              {activeLinklabel && (
+              {(activeLinklabel || isLarge) && (
                 <span role="separator" className={styles['SubNav__heading-separator']} aria-hidden>
                   <svg
                     xmlns="http://www.w3.org/2000/svg"