PageHeader: A11y eng review remediations (#2900)

* a11y remediations * Add changeset * Add tests for a11y remediations * add docs for a11y remediations * Update generated/components.json * wrap console.log with __DEV__ and add a todo * fix merge issue * update docs --------- Co-authored-by: broccolinisoup <broccolinisoup@users.noreply.github.com>
primer · Feb 16, 2023 · 3768cd7 · 3768cd7
1 parent 03ebf41
commit 3768cd7
Show file tree

Hide file tree

Showing 7 changed files with 160 additions and 31 deletions.
diff --git a/.changeset/silent-rivers-sell.md b/.changeset/silent-rivers-sell.md
@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+PageHeader: A11y eng review remediations
diff --git a/docs/content/drafts/PageHeader.mdx b/docs/content/drafts/PageHeader.mdx
@@ -121,6 +121,8 @@ import {PageHeader} from '@primer/react/drafts'
 
 ### With navigation slot
 
+#### Utilizing a Navigation component
+
 ```jsx live drafts
 <PageHeader>
   <PageHeader.TitleArea>
@@ -145,6 +147,28 @@ import {PageHeader} from '@primer/react/drafts'
 </PageHeader>
 ```
 
+#### Utilizing a custom navigation
+
+```jsx live drafts
+<PageHeader>
+  <PageHeader.TitleArea>
+    <PageHeader.Title>Page Title</PageHeader.Title>
+  </PageHeader.TitleArea>
+  <PageHeader.Navigation as="nav" aria-label="Item list">
+    <Box as="ul" sx={{display: 'flex', gap: '8px', listStyle: 'none', paddingY: 0, paddingX: 3}} role="list">
+      <li>
+        <Link href="#" aria-current="page">
+          Item 1
+        </Link>
+      </li>
+      <li>
+        <Link href="#">Item 2</Link>
+      </li>
+    </Box>
+  </PageHeader.Navigation>
+</PageHeader>
+```
+
 ### With ContextArea
 
 #### With parent link and actions (only visible on mobile)

diff --git a/generated/components.json b/generated/components.json
@@ -3018,7 +3018,7 @@
           ]
         },
         {
-          "name": "PageHeader.ContextArea Children",
+          "name": "PageHeader.ParentLink",
           "props": [
             {
               "name": "href",
@@ -3039,7 +3039,7 @@
           ]
         },
         {
-          "name": "PageHeader.ContextArea Children",
+          "name": "PageHeader.ContextBar",
           "props": [
             {
               "name": "hidden",
@@ -3054,7 +3054,7 @@
           ]
         },
         {
-          "name": "PageHeader.ContextArea Children",
+          "name": "PageHeader.ContextAreaActions",
           "props": [
             {
               "name": "hidden",
@@ -3090,7 +3090,7 @@
           ]
         },
         {
-          "name": "PageHeader.TitleArea Children",
+          "name": "PageHeader.LeadingAction",
           "props": [
             {
               "name": "hidden",
@@ -3105,7 +3105,7 @@
           ]
         },
         {
-          "name": "PageHeader.TitleArea Children",
+          "name": "PageHeader.LeadingVisual",
           "props": [
             {
               "name": "hidden",
@@ -3120,7 +3120,7 @@
           ]
         },
         {
-          "name": "PageHeader.TitleArea Children",
+          "name": "PageHeader.Title",
           "props": [
             {
               "name": "hidden",
@@ -3140,7 +3140,7 @@
           ]
         },
         {
-          "name": "PageHeader.TitleArea Children",
+          "name": "PageHeader.TrailingVisual",
           "props": [
             {
               "name": "hidden",
@@ -3155,7 +3155,7 @@
           ]
         },
         {
-          "name": "PageHeader.TitleArea Children",
+          "name": "PageHeader.TrailingAction",
           "props": [
             {
               "name": "hidden",
@@ -3170,7 +3170,7 @@
           ]
         },
         {
-          "name": "PageHeader.TitleArea Children",
+          "name": "PageHeader.Actions",
           "props": [
             {
               "name": "hidden",
@@ -3202,6 +3202,22 @@
         {
           "name": "PageHeader.Navigation",
           "props": [
+            {
+              "name": "aria-label",
+              "type": "string",
+              "description": "The aria-label attribute for the navigaton component when it is rendered as a \"nav\" element."
+            },
+            {
+              "name": "aria-labelledby",
+              "type": "string",
+              "description": "The aria-labelledby attribute for the navigaton component when it is rendered as a \"nav\" element."
+            },
+            {
+              "name": "as",
+              "type": "div | nav",
+              "defaultValue": "\"div\"",
+              "description": "The HTML element used to render the navigation."
+            },
             {
               "name": "hidden",
               "type": "| boolean | { narrow?: boolean regular?: boolean wide?: boolean }",

diff --git a/src/PageHeader/PageHeader.docs.json b/src/PageHeader/PageHeader.docs.json
@@ -44,7 +44,7 @@
       ]
     },
     {
-      "name": "PageHeader.ContextArea Children",
+      "name": "PageHeader.ParentLink",
       "props": [
         {
           "name": "href",
@@ -65,7 +65,7 @@
       ]
     },
     {
-      "name": "PageHeader.ContextArea Children",
+      "name": "PageHeader.ContextBar",
       "props": [
         {
           "name": "hidden",
@@ -80,7 +80,7 @@
       ]
     },
     {
-      "name": "PageHeader.ContextArea Children",
+      "name": "PageHeader.ContextAreaActions",
       "props": [
         {
           "name": "hidden",
@@ -116,7 +116,7 @@
       ]
     },
     {
-      "name": "PageHeader.TitleArea Children",
+      "name": "PageHeader.LeadingAction",
       "props": [
         {
           "name": "hidden",
@@ -131,7 +131,7 @@
       ]
     },
     {
-      "name": "PageHeader.TitleArea Children",
+      "name": "PageHeader.LeadingVisual",
       "props": [
         {
           "name": "hidden",
@@ -146,7 +146,7 @@
       ]
     },
     {
-      "name": "PageHeader.TitleArea Children",
+      "name": "PageHeader.Title",
       "props": [
         {
           "name": "hidden",
@@ -166,7 +166,7 @@
       ]
     },
     {
-      "name": "PageHeader.TitleArea Children",
+      "name": "PageHeader.TrailingVisual",
       "props": [
         {
           "name": "hidden",
@@ -181,7 +181,7 @@
       ]
     },
     {
-      "name": "PageHeader.TitleArea Children",
+      "name": "PageHeader.TrailingAction",
       "props": [
         {
           "name": "hidden",
@@ -196,7 +196,7 @@
       ]
     },
     {
-      "name": "PageHeader.TitleArea Children",
+      "name": "PageHeader.Actions",
       "props": [
         {
           "name": "hidden",
@@ -228,6 +228,22 @@
     {
       "name": "PageHeader.Navigation",
       "props": [
+        {
+          "name": "aria-label",
+          "type": "string",
+          "description": "The aria-label attribute for the navigaton component when it is rendered as a \"nav\" element."
+        },
+        {
+          "name": "aria-labelledby",
+          "type": "string",
+          "description": "The aria-labelledby attribute for the navigaton component when it is rendered as a \"nav\" element."
+        },
+        {
+          "name": "as",
+          "type": "div | nav",
+          "defaultValue": "\"div\"",
+          "description": "The HTML element used to render the navigation."
+        },
         {
           "name": "hidden",
           "type": "| boolean | { narrow?: boolean regular?: boolean wide?: boolean }",
@@ -241,4 +257,4 @@
       ]
     }
   ]
-}
+}
diff --git a/src/PageHeader/PageHeader.test.tsx b/src/PageHeader/PageHeader.test.tsx
@@ -178,6 +178,36 @@ describe('PageHeader', () => {
     expect(getByText('Trailing Action')).toHaveStyle('height: 3rem')
     // add actions here
   })
+  it('renders "aria-label" prop when Navigation is rendered as "nav" landmark', () => {
+    const {getByLabelText, getByText} = render(
+      <PageHeader>
+        <PageHeader.Navigation as="nav" aria-label="Custom">
+          Navigation
+        </PageHeader.Navigation>
+      </PageHeader>,
+    )
+    expect(getByLabelText('Custom')).toBeInTheDocument()
+    expect(getByText('Navigation')).toHaveAttribute('aria-label', 'Custom')
+  })
+  it('does not renders "aria-label" prop when Navigation is rendered as "div"', () => {
+    const {getByText} = render(
+      <PageHeader>
+        <PageHeader.Navigation aria-label="Custom">Navigation</PageHeader.Navigation>
+      </PageHeader>,
+    )
+    expect(getByText('Navigation')).not.toHaveAttribute('aria-label')
+  })
+  it('logs a warning when the Navigation component is rendered as "nav" but no "aria-label" or "aria-labelledby" prop is provided', () => {
+    const consoleSpy = jest.spyOn(global.console, 'warn').mockImplementation()
+    render(
+      <PageHeader>
+        <PageHeader.Navigation as="nav">Navigation</PageHeader.Navigation>
+      </PageHeader>,
+    )
+    expect(consoleSpy).toHaveBeenCalled()
+
+    consoleSpy.mockRestore()
+  })
 })
 
 checkStoriesForAxeViolations('examples', '../PageHeader/')
diff --git a/src/PageHeader/PageHeader.tsx b/src/PageHeader/PageHeader.tsx
@@ -101,17 +101,7 @@ type LinkProps = Pick<
 export type ParentLinkProps = React.PropsWithChildren<ChildrenPropTypes & LinkProps>
 
 const ParentLink = React.forwardRef<HTMLAnchorElement, ParentLinkProps>(
-  (
-    {
-      children,
-      sx = {},
-      href,
-      'aria-label': ariaLabel = `Back to ${children}`,
-      as = 'a',
-      hidden = hiddenOnRegularAndWide,
-    },
-    ref,
-  ) => {
+  ({children, sx = {}, href, 'aria-label': ariaLabel, as = 'a', hidden = hiddenOnRegularAndWide}, ref) => {
     return (
       <>
         <Link
@@ -437,10 +427,36 @@ const Description: React.FC<React.PropsWithChildren<ChildrenPropTypes>> = ({chil
   )
 }
 
+export type NavigationProps = {
+  as?: 'nav' | 'div'
+  'aria-label'?: React.AriaAttributes['aria-label']
+  'aria-labelledby'?: React.AriaAttributes['aria-labelledby']
+} & ChildrenPropTypes
+
 // PageHeader.Navigation: The local navigation area of the header. Visible on all viewports
-const Navigation: React.FC<React.PropsWithChildren<ChildrenPropTypes>> = ({children, sx = {}, hidden = false}) => {
+const Navigation: React.FC<React.PropsWithChildren<NavigationProps>> = ({
+  children,
+  sx = {},
+  hidden = false,
+  as,
+  'aria-label': ariaLabel,
+  'aria-labelledby': ariaLabelledBy,
+}) => {
+  // TODO: use warning utility function when it is merged https://github.com/primer/react/pull/2901/
+  if (__DEV__) {
+    if (as === 'nav' && !ariaLabel && !ariaLabelledBy) {
+      // eslint-disable-next-line no-console
+      console.warn(
+        'Use `aria-label` or `aria-labelledby` prop to provide an accessible label to the `nav` landmark for assistive technology',
+      )
+    }
+  }
   return (
     <Box
+      as={as}
+      // Render `aria-label` and `aria-labelledby` only on `nav` elements
+      aria-label={as === 'nav' ? ariaLabel : undefined}
+      aria-labelledby={as === 'nav' ? ariaLabelledBy : undefined}
       sx={merge<BetterSystemStyleObject>(
         {
           display: 'flex',

diff --git a/src/PageHeader/features.stories.tsx b/src/PageHeader/features.stories.tsx
@@ -147,6 +147,28 @@ export const WithNavigationSlot = () => (
   </Box>
 )
 
+export const WithCustomNavigation = () => (
+  <Box sx={{padding: 3}}>
+    <PageHeader>
+      <PageHeader.TitleArea>
+        <PageHeader.Title>Pull request title</PageHeader.Title>
+      </PageHeader.TitleArea>
+      <PageHeader.Navigation as="nav" aria-label="Item list">
+        <Box as="ul" sx={{display: 'flex', gap: '8px', listStyle: 'none', paddingY: 0, paddingX: 3}} role="list">
+          <li>
+            <Link href="#" aria-current="page">
+              Item 1
+            </Link>
+          </li>
+          <li>
+            <Link href="#">Item 2</Link>
+          </li>
+        </Box>
+      </PageHeader.Navigation>
+    </PageHeader>
+  </Box>
+)
+
 export const WithLeadingAndTrailingActions = () => (
   <Box sx={{padding: 3}}>
     <PageHeader>