[UnderlineNav] Accessibility Remediations (#2406)

* UnderlineNav accessibility remediations

* update tests

* lint and interaction story update

* ariaLabel optional and add a comment

* remove conditional tabindex and find a better way to manage focus

* restyle arrow btns and fix focus issue

* add aria-label type

* aria-label improvements

* add changeset

* arrow button slide in and out behaviour

* bump octicons_react version up

* update snapshots

* update docs

* add docs name

Author: broccolinisoup

.changeset/wet-glasses-kneel.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+UnderlineNav2: Address accessibility feedback and re-style arrow buttons for scroll behaviour

docs/content/drafts/UnderlineNav2.mdx

@@ -16,7 +16,7 @@ import {UnderlineNav} from '@primer/react/drafts'
 ### Simple
 
 ```jsx live drafts
-<UnderlineNav>
+<UnderlineNav aria-label="Repository">
   <UnderlineNav.Item selected>Item 1</UnderlineNav.Item>
   <UnderlineNav.Item>Item 2</UnderlineNav.Item>
   <UnderlineNav.Item>Item 3</UnderlineNav.Item>
@@ -26,7 +26,7 @@ import {UnderlineNav} from '@primer/react/drafts'
 ### With Icons
 
 ```jsx live drafts
-<UnderlineNav>
+<UnderlineNav aria-label="Repository">
   <UnderlineNav.Item selected icon={CodeIcon}>
     Code
   </UnderlineNav.Item>
@@ -53,7 +53,7 @@ When overflow occurs, the component first hides icons if present to optimize for
 #### Items Without Icons
 
 ```jsx live drafts
-<UnderlineNav>
+<UnderlineNav aria-label="Repository">
   <UnderlineNav.Item selected icon={CodeIcon}>
     Code
   </UnderlineNav.Item>
@@ -83,7 +83,7 @@ When overflow occurs, the component first hides icons if present to optimize for
 If there is still overflow, the component will behave depending on the pointer.
 
 ```jsx live drafts
-<UnderlineNav>
+<UnderlineNav aria-label="Repository">
   <UnderlineNav.Item selected icon={CodeIcon}>
     Code
   </UnderlineNav.Item>
@@ -114,7 +114,7 @@ If there is still overflow, the component will behave depending on the pointer.
 ### Loading state for counters
 
 ```jsx live drafts
-<UnderlineNav loadingCounters={true}>
+<UnderlineNav aria-label="Repository" loadingCounters={true}>
   <UnderlineNav.Item counter={4} selected>
     Item 1
   </UnderlineNav.Item>
@@ -128,9 +128,11 @@ If there is still overflow, the component will behave depending on the pointer.
 ### UnderlineNav
 
 <PropsTable>
-  <PropsTableRow name="aria-label" type="string" />
-  <PropsTableRow name="aria-labelledby" type="string" />
-  <PropsTableRow name="aria-describedby" type="string" />
+  <PropsTableRow
+    name="aria-label"
+    type="string"
+    description="A unique name for the rendered 'nav' landmark. It will also be used to label the arrow buttons that control the scroll behaviour on coarse pointer devices. (I.e. 'Scroll ${aria-label} left/right')"
+  />
   <PropsTableRow
     name="loadingCounters"
     type="boolean"

docs/package-lock.json

@@ -14,7 +14,7 @@
   },
   "dependencies": {
     "@primer/gatsby-theme-doctocat": "^4.2.0",
-    "@primer/octicons-react": "^17.5.0",
+    "@primer/octicons-react": "^17.7.0",
     "@primer/primitives": "4.1.0",
     "@styled-system/prop-types": "^5.1.0",
     "@styled-system/theme-get": "^5.1.0",

docs/package.json

@@ -82,7 +82,7 @@
     "@github/markdown-toolbar-element": "^2.1.0",
     "@github/paste-markdown": "^1.4.0",
     "@primer/behaviors": "^1.1.1",
-    "@primer/octicons-react": "^17.3.0",
+    "@primer/octicons-react": "^17.7.0",
     "@primer/primitives": "7.9.0",
     "@react-aria/ssr": "^3.1.0",
     "@styled-system/css": "^5.1.5",

package-lock.json

@@ -1220,6 +1220,7 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav
                   aria-hidden="true"
                   class="c8"
                   fill="currentColor"
+                  focusable="false"
                   height="16"
                   role="img"
                   style="display: inline-block; user-select: none; vertical-align: text-bottom; overflow: visible;"
@@ -1702,6 +1703,7 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t
                   aria-hidden="true"
                   class="c8"
                   fill="currentColor"
+                  focusable="false"
                   height="16"
                   role="img"
                   style="display: inline-block; user-select: none; vertical-align: text-bottom; overflow: visible;"

package.json

@@ -54,7 +54,7 @@ const ResponsiveUnderlineNav = ({
     {navigation: 'Security', icon: ShieldLockIcon}
   ]
   return (
-    <UnderlineNav label="Repository" loadingCounters={loadingCounters}>
+    <UnderlineNav aria-label="Repository" loadingCounters={loadingCounters}>
       {items.map(item => (
         <UnderlineNav.Item
           key={item.navigation}
@@ -91,7 +91,7 @@ describe('UnderlineNav', () => {
   it('fires onSelect on click and keypress', async () => {
     const onSelect = jest.fn()
     const {getByText} = render(
-      <UnderlineNav label="Test nav">
+      <UnderlineNav aria-label="Test Navigation">
         <UnderlineNav.Item onSelect={onSelect}>Item 1</UnderlineNav.Item>
         <UnderlineNav.Item onSelect={onSelect}>Item 2</UnderlineNav.Item>
         <UnderlineNav.Item onSelect={onSelect}>Item 3</UnderlineNav.Item>

src/NavList/__snapshots__/NavList.test.tsx.snap

@@ -21,12 +21,12 @@ import {
   moreMenuStyles,
   menuItemStyles
 } from './styles'
-import {LeftArrowButton, RightArrowButton} from './UnderlineNavArrowButton'
+import {ArrowButton} from './UnderlineNavArrowButton'
 import styled from 'styled-components'
 import {LoadingCounter} from './LoadingCounter'
 
 export type UnderlineNavProps = {
-  label: string
+  'aria-label'?: React.AriaAttributes['aria-label']
   as?: React.ElementType
   align?: 'right'
   sx?: SxProp
@@ -68,6 +68,7 @@ const handleArrowBtnsVisibility = (
   const scrollOffsets = {scrollLeft, scrollRight}
   callback(scrollOffsets)
 }
+
 const overflowEffect = (
   navWidth: number,
   moreMenuWidth: number,
@@ -157,7 +158,7 @@ export const UnderlineNav = forwardRef(
     {
       as = 'nav',
       align,
-      label,
+      'aria-label': ariaLabel,
       sx: sxProp = {},
       afterSelect,
       variant = 'default',
@@ -225,6 +226,8 @@ export const UnderlineNav = forwardRef(
 
     const [selectedLink, setSelectedLink] = useState<RefObject<HTMLElement> | undefined>(undefined)
 
+    const [focusedLink, setFocusedLink] = useState<RefObject<HTMLElement> | null>(null)
+
     // selectedLinkText is needed to be able set the selected menu item as selectedLink.
     // This is needed because setSelectedLink only accepts ref but at the time of setting selected menu item as selectedLink, its ref as a list item is not available
     const [selectedLinkText, setSelectedLinkText] = useState<string>('')
@@ -311,14 +314,28 @@ export const UnderlineNav = forwardRef(
       return () => listEl?.removeEventListener('scroll', scrollOnList)
     }, [scrollOnList])
 
-    useEffect(() => {
-      // scroll the selected link into the view (coarse pointer behaviour)
-      if (isCoarsePointer && selectedLink?.current && listRef.current) {
-        scrollIntoView(selectedLink.current, listRef.current, underlineNavScrollMargins)
+    function scrollLinkIntoView(link: RefObject<HTMLElement>) {
+      if (link.current && listRef.current) {
+        scrollIntoView(link.current, listRef.current, underlineNavScrollMargins)
         return
       }
+    }
+
+    useEffect(() => {
+      // scroll the selected link into the view (coarse pointer behaviour)
+      selectedLink?.current && isCoarsePointer && scrollLinkIntoView(selectedLink)
     }, [selectedLink, isCoarsePointer])
 
+    useEffect(() => {
+      // scroll the focused link into the view (coarse pointer behaviour)
+      focusedLink?.current && isCoarsePointer && scrollLinkIntoView(focusedLink)
+    }, [focusedLink, isCoarsePointer])
+
+    if (!ariaLabel) {
+      // eslint-disable-next-line no-console
+      console.warn('Use the `aria-label` prop to provide an accessible label for assistive technology')
+    }
+
     return (
       <UnderlineNavContext.Provider
         value={{
@@ -329,6 +346,7 @@ export const UnderlineNav = forwardRef(
           setSelectedLink,
           selectedLinkText,
           setSelectedLinkText,
+          setFocusedLink,
           selectEvent,
           afterSelect: afterSelectHandler,
           variant,
@@ -339,11 +357,17 @@ export const UnderlineNav = forwardRef(
         <Box
           as={as}
           sx={merge<BetterSystemStyleObject>(getNavStyles(theme, {align}), sxProp)}
-          aria-label={label}
+          aria-label={ariaLabel}
           ref={navRef}
         >
           {isCoarsePointer && (
-            <LeftArrowButton show={scrollValues.scrollLeft > 0} onScrollWithButton={onScrollWithButton} />
+            <ArrowButton
+              scrollValue={scrollValues.scrollLeft}
+              type="left"
+              show={scrollValues.scrollLeft > 0}
+              onScrollWithButton={onScrollWithButton}
+              aria-label={ariaLabel}
+            />
           )}
 
           <NavigationList sx={merge<BetterSystemStyleObject>(responsiveProps.overflowStyles, ulStyles)} ref={listRef}>
@@ -390,7 +414,13 @@ export const UnderlineNav = forwardRef(
           </NavigationList>
 
           {isCoarsePointer && (
-            <RightArrowButton show={scrollValues.scrollRight > 0} onScrollWithButton={onScrollWithButton} />
+            <ArrowButton
+              scrollValue={scrollValues.scrollRight}
+              type="right"
+              show={scrollValues.scrollRight > 0}
+              onScrollWithButton={onScrollWithButton}
+              aria-label={ariaLabel}
+            />
           )}
         </Box>
       </UnderlineNavContext.Provider>

src/UnderlineNav2/UnderlineNav.test.tsx

@@ -1,44 +1,66 @@
 import React, {useContext} from 'react'
 import {IconButton} from '../Button/IconButton'
 import {ChevronLeftIcon, ChevronRightIcon} from '@primer/octicons-react'
-import {getLeftArrowHiddenBtn, getRightArrowHiddenBtn, getLeftArrowVisibleBtn, getRightArrowVisibleBtn} from './styles'
+import {btnWrapperStyles, getArrowBtnStyles} from './styles'
 import {OnScrollWithButtonEventType} from './types'
 import {UnderlineNavContext} from './UnderlineNavContext'
+import Box from '../Box'
 
-const LeftArrowButton = ({
+const ArrowButton = ({
+  scrollValue,
+  type,
   show,
-  onScrollWithButton
+  onScrollWithButton,
+  'aria-label': ariaLabel
 }: {
+  scrollValue: number
+  type: 'left' | 'right'
   show: boolean
   onScrollWithButton: OnScrollWithButtonEventType
+  'aria-label'?: React.AriaAttributes['aria-label']
 }) => {
+  const leftBtnRef = React.useRef<HTMLButtonElement>(null)
+  const rightBtnRef = React.useRef<HTMLButtonElement>(null)
   const {theme} = useContext(UnderlineNavContext)
-  return (
-    <IconButton
-      aria-label="Scroll Left"
-      onClick={(e: React.MouseEvent<HTMLButtonElement, MouseEvent>) => onScrollWithButton(e, -1)}
-      icon={ChevronLeftIcon}
-      sx={show ? getLeftArrowVisibleBtn(theme) : getLeftArrowHiddenBtn(theme)}
-    />
-  )
-}
+  const direction = type === 'left' ? -1 : 1
+  const ARROW_BTN_WIDTH = 44 // Min touch target size is 44px
+
+  // re-trigger focus on the button with aria-disabled=true when it becomes hidden to communicate to screen readers that the button is no longer available
+  React.useEffect(() => {
+    const currentBtn = type === 'left' ? leftBtnRef.current : rightBtnRef.current
+    if (currentBtn?.getAttribute('aria-disabled') === 'true') {
+      currentBtn.focus()
+    } else {
+      // eslint-disable-next-line github/no-blur
+      currentBtn?.blur()
+    }
+  }, [show, type])
+
+  let translateX = 0
+  let display = 'flex'
+
+  // Determine the translateX value to transform for the slide in/out effect
+  if (scrollValue === 0) {
+    // If the scrollValue is 0, the buttons should be hidden
+    translateX = ARROW_BTN_WIDTH * direction
+    // This is mainly needed for the right arrow button. Because hiding translateX value for it is positive (44px) and this positive value was causing button to be visibly overflowed rathan than hiding.
+    display = 'none'
+  } else if (scrollValue <= ARROW_BTN_WIDTH) translateX = (ARROW_BTN_WIDTH - scrollValue) * direction
+  else translateX = 0
 
-const RightArrowButton = ({
-  show,
-  onScrollWithButton
-}: {
-  show: boolean
-  onScrollWithButton: OnScrollWithButtonEventType
-}) => {
-  const {theme} = useContext(UnderlineNavContext)
   return (
-    <IconButton
-      aria-label="Scroll Right"
-      onClick={(e: React.MouseEvent<HTMLButtonElement, MouseEvent>) => onScrollWithButton(e, 1)}
-      icon={ChevronRightIcon}
-      sx={show ? getRightArrowVisibleBtn(theme) : getRightArrowHiddenBtn(theme)}
-    />
+    <Box sx={btnWrapperStyles(theme, type, show, translateX, display)}>
+      <IconButton
+        tabIndex={show ? 0 : -1}
+        ref={type === 'left' ? leftBtnRef : rightBtnRef}
+        aria-label={`Scroll ${ariaLabel} navigation ${type}`}
+        onClick={(e: React.MouseEvent<HTMLButtonElement, MouseEvent>) => onScrollWithButton(e, direction)}
+        icon={type === 'left' ? ChevronLeftIcon : ChevronRightIcon}
+        sx={getArrowBtnStyles(theme, type)}
+        aria-disabled={!show}
+      />
+    </Box>
   )
 }
 
-export {LeftArrowButton, RightArrowButton}
+export {ArrowButton}