2411 the aria keyshortcuts is wrongly attached for the actionlistlink…

…item components (#2657) * fix(LinkItem): aria-keyshortcuts prop moved to li * fix(LinkItem): key shortcuts functionality * test(LinkItem): verify keyshortcut attached to correct element * test(LinkItem): ActionMenu test and story includes LinkItem * chore(eslint): add trailing commas * chore(test): merge test updates * Add changeset * Merge branch 'main' into '2411-the-aria-keyshortcuts'
primer · Dec 13, 2022 · cc909dc · cc909dc
1 parent d1c2b6b
commit cc909dc
Show file tree

Hide file tree

Showing 9 changed files with 127 additions and 47 deletions.
diff --git a/.changeset/sour-knives-drive.md b/.changeset/sour-knives-drive.md
@@ -0,0 +1,5 @@
+---
+'@primer/react': minor
+---
+
+Assign aria-keyshorcuts and role properties to the correct element in LinkItem.tsx
diff --git a/.gitignore b/.gitignore
@@ -10,3 +10,4 @@ public/
 stats.html
 .env
 storybook-static
+.tool-versions
diff --git a/src/ActionList/ActionList.examples.stories.tsx b/src/ActionList/ActionList.examples.stories.tsx
@@ -57,7 +57,7 @@ export const ListLinkItem = () => (
       </ActionList.LeadingVisual>
       not a link, just an Item for comparison
     </ActionList.Item>
-    <ActionList.LinkItem href="https://github.com/primer">
+    <ActionList.LinkItem href="https://github.com/primer" aria-keyshortcuts="g">
       <ActionList.LeadingVisual>
         <LinkIcon />
       </ActionList.LeadingVisual>

diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx
@@ -176,44 +176,56 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
 
     return (
       <Slots context={{variant, disabled, inlineDescriptionId, blockDescriptionId}}>
-        {slots => (
-          <LiBox
-            ref={forwardedRef}
-            sx={merge<BetterSystemStyleObject>(styles, sxProp)}
-            onClick={clickHandler}
-            onKeyPress={keyPressHandler}
-            aria-disabled={disabled ? true : undefined}
-            tabIndex={disabled || _PrivateItemWrapper ? undefined : 0}
-            aria-labelledby={`${labelId} ${slots.InlineDescription ? inlineDescriptionId : ''}`}
-            aria-describedby={slots.BlockDescription ? blockDescriptionId : undefined}
-            role={role || itemRole}
-            {...(selectionAttribute && {[selectionAttribute]: selected})}
-            {...props}
-          >
-            <ItemWrapper>
-              <Selection selected={selected} />
-              {slots.LeadingVisual}
-              <Box
-                data-component="ActionList.Item--DividerContainer"
-                sx={{display: 'flex', flexDirection: 'column', flexGrow: 1, minWidth: 0}}
-              >
-                <ConditionalBox if={Boolean(slots.TrailingVisual)} sx={{display: 'flex', flexGrow: 1}}>
-                  <ConditionalBox
-                    if={Boolean(slots.InlineDescription)}
-                    sx={{display: 'flex', flexGrow: 1, alignItems: 'baseline', minWidth: 0}}
-                  >
-                    <Box as="span" id={labelId} sx={{flexGrow: slots.InlineDescription ? 0 : 1}}>
-                      {props.children}
-                    </Box>
-                    {slots.InlineDescription}
+        {slots => {
+          const menuItemProps = {
+            onClick: clickHandler,
+            onKeyPress: keyPressHandler,
+            'aria-disabled': disabled ? true : undefined,
+            tabIndex: disabled ? undefined : 0,
+            'aria-labelledby': `${labelId} ${slots.InlineDescription ? inlineDescriptionId : ''}`,
+            'aria-describedby': slots.BlockDescription ? blockDescriptionId : undefined,
+            ...(selectionAttribute && {[selectionAttribute]: selected}),
+            role: role || itemRole,
+          }
+          const containerProps = _PrivateItemWrapper
+            ? {
+                role: role || itemRole ? 'none' : undefined,
+              }
+            : menuItemProps
+          const wrapperProps = _PrivateItemWrapper ? menuItemProps : {}
+
+          return (
+            <LiBox
+              ref={forwardedRef}
+              sx={merge<BetterSystemStyleObject>(styles, sxProp)}
+              {...containerProps}
+              {...props}
+            >
+              <ItemWrapper {...wrapperProps}>
+                <Selection selected={selected} />
+                {slots.LeadingVisual}
+                <Box
+                  data-component="ActionList.Item--DividerContainer"
+                  sx={{display: 'flex', flexDirection: 'column', flexGrow: 1, minWidth: 0}}
+                >
+                  <ConditionalBox if={Boolean(slots.TrailingVisual)} sx={{display: 'flex', flexGrow: 1}}>
+                    <ConditionalBox
+                      if={Boolean(slots.InlineDescription)}
+                      sx={{display: 'flex', flexGrow: 1, alignItems: 'baseline', minWidth: 0}}
+                    >
+                      <Box as="span" id={labelId} sx={{flexGrow: slots.InlineDescription ? 0 : 1}}>
+                        {props.children}
+                      </Box>
+                      {slots.InlineDescription}
+                    </ConditionalBox>
+                    {slots.TrailingVisual}
                   </ConditionalBox>
-                  {slots.TrailingVisual}
-                </ConditionalBox>
-                {slots.BlockDescription}
-              </Box>
-            </ItemWrapper>
-          </LiBox>
-        )}
+                  {slots.BlockDescription}
+                </Box>
+              </ItemWrapper>
+            </LiBox>
+          )
+        }}
       </Slots>
     )
   },

diff --git a/src/ActionList/LinkItem.tsx b/src/ActionList/LinkItem.tsx
@@ -39,8 +39,8 @@ export const LinkItem = React.forwardRef(({sx = {}, active, as: Component, ...pr
     <Item
       active={active}
       sx={{paddingY: 0, paddingX: 0}}
-      _PrivateItemWrapper={({children}) => (
-        <Link as={Component} sx={merge(styles, sx as SxProp)} {...props} ref={forwardedRef}>
+      _PrivateItemWrapper={({children, ...rest}) => (
+        <Link as={Component} sx={merge(styles, sx as SxProp)} {...props} {...rest} ref={forwardedRef}>
           {children}
         </Link>
       )}

diff --git a/src/NavList/__snapshots__/NavList.test.tsx.snap b/src/NavList/__snapshots__/NavList.test.tsx.snap
@@ -305,13 +305,14 @@ exports[`NavList renders a simple list 1`] = `
       class="c0"
     >
       <li
-        aria-labelledby="react-aria-1 "
         class="c1 c2"
       >
         <a
           aria-current="page"
+          aria-labelledby="react-aria-1 "
           class="c3"
           href="/"
+          tabindex="0"
         >
           <div
             class="c4"
@@ -327,12 +328,13 @@ exports[`NavList renders a simple list 1`] = `
         </a>
       </li>
       <li
-        aria-labelledby="react-aria-4 "
         class="c1 c6"
       >
         <a
+          aria-labelledby="react-aria-4 "
           class="c3"
           href="/about"
+          tabindex="0"
         >
           <div
             class="c4"
@@ -348,12 +350,13 @@ exports[`NavList renders a simple list 1`] = `
         </a>
       </li>
       <li
-        aria-labelledby="react-aria-7 "
         class="c1 c6"
       >
         <a
+          aria-labelledby="react-aria-7 "
           class="c3"
           href="/contact"
+          tabindex="0"
         >
           <div
             class="c4"
@@ -735,13 +738,14 @@ exports[`NavList renders with groups 1`] = `
           class="c4"
         >
           <li
-            aria-labelledby="react-aria-2 "
             class="c5 c6"
           >
             <a
               aria-current="page"
+              aria-labelledby="react-aria-2 "
               class="c7"
               href="/getting-started"
+              tabindex="0"
             >
               <div
                 class="c8"
@@ -782,12 +786,13 @@ exports[`NavList renders with groups 1`] = `
           class="c4"
         >
           <li
-            aria-labelledby="react-aria-6 "
             class="c5 c10"
           >
             <a
+              aria-labelledby="react-aria-6 "
               class="c7"
               href="/Avatar"
+              tabindex="0"
             >
               <div
                 class="c8"
@@ -1255,13 +1260,14 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav
             id="react-aria-2"
           >
             <li
-              aria-labelledby="react-aria-3 "
               class="c2 c10"
             >
               <a
                 aria-current="page"
+                aria-labelledby="react-aria-3 "
                 class="c11"
                 href="#"
+                tabindex="0"
               >
                 <div
                   class="c4"
@@ -1742,13 +1748,14 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t
             id="react-aria-2"
           >
             <li
-              aria-labelledby="react-aria-3 "
               class="c2 c10"
             >
               <a
                 aria-current="page"
+                aria-labelledby="react-aria-3 "
                 class="c11"
                 href="#"
+                tabindex="0"
               >
                 <div
                   class="c4"

diff --git a/src/__tests__/ActionList.test.tsx b/src/__tests__/ActionList.test.tsx
@@ -18,6 +18,9 @@ function SimpleActionList(): JSX.Element {
             <ActionList.Item>Copy link</ActionList.Item>
             <ActionList.Item>Edit file</ActionList.Item>
             <ActionList.Item variant="danger">Delete file</ActionList.Item>
+            <ActionList.LinkItem href="//github.com" title="anchor" aria-keyshortcuts="d">
+              Link Item
+            </ActionList.LinkItem>
           </ActionList>
         </BaseStyles>
       </SSRProvider>
@@ -63,6 +66,15 @@ describe('ActionList', () => {
     ActionList,
   })
 
+  it('should have aria-keyshortcuts applied to the correct element', async () => {
+    const {container} = HTMLRender(<SimpleActionList />)
+
+    const linkOptions = await waitFor(() => container.querySelectorAll('a'))
+
+    expect(linkOptions[0]).toHaveAttribute('aria-keyshortcuts', 'd')
+    expect(linkOptions[0].parentElement).not.toHaveAttribute('aria-keyshortcuts', 'd')
+  })
+
   it('should have no axe violations', async () => {
     const {container} = HTMLRender(<SimpleActionList />)
     const results = await axe(container)

diff --git a/src/__tests__/ActionMenu.test.tsx b/src/__tests__/ActionMenu.test.tsx
@@ -24,6 +24,9 @@ function Example(): JSX.Element {
                 <ActionList.Item variant="danger" onClick={event => event.preventDefault()}>
                   Delete file
                 </ActionList.Item>
+                <ActionList.LinkItem href="//github.com" title="anchor" aria-keyshortcuts="s">
+                  Github
+                </ActionList.LinkItem>
               </ActionList>
             </ActionMenu.Overlay>
           </ActionMenu>
@@ -169,6 +172,26 @@ describe('ActionMenu', () => {
 
     expect(component.getAllByRole('menuitem')[0]).toEqual(document.activeElement)
   })
+  it('should be able to select an Item with aria-keyshortcuts after opening Menu with click', async () => {
+    const component = HTMLRender(<Example />)
+    const button = component.getByRole('button')
+
+    const user = userEvent.setup()
+    await user.click(button)
+
+    expect(component.queryByRole('menu')).toBeInTheDocument()
+
+    // linkItem button is the active element at this point
+    await user.keyboard('{ArrowDown}{s}')
+
+    expect(component.getAllByRole('menuitem')[4]).toEqual(document.activeElement)
+    await user.keyboard('{ArrowUp}')
+    expect(component.getAllByRole('menuitem')[3]).toEqual(document.activeElement)
+
+    // assumes mnemonics aria-keyshortcuts are ignored
+    await user.keyboard('{g}')
+    expect(component.getAllByRole('menuitem')[3]).toEqual(document.activeElement)
+  })
 
   it('should select last element when ArrowUp is pressed after opening Menu with click', async () => {
     const component = HTMLRender(<Example />)

diff --git a/src/stories/ActionMenu/fixtures.stories.tsx b/src/stories/ActionMenu/fixtures.stories.tsx
@@ -145,6 +145,7 @@ export function ExternalAnchor(): JSX.Element {
               Delete file
               <ActionList.TrailingVisual>⌘D</ActionList.TrailingVisual>
             </ActionList.Item>
+            <ActionList.Divider />
           </ActionList>
         </ActionMenu.Overlay>
       </ActionMenu>
@@ -759,6 +760,25 @@ export function MnemonicsTest(): JSX.Element {
                 </Box>
               </ActionList.TrailingVisual>
             </ActionList.Item>
+            <ActionList.LinkItem aria-keyshortcuts="d" href="//github.com">
+              User defined Link
+              <ActionList.TrailingVisual>
+                <Box
+                  as="span"
+                  sx={{
+                    backgroundColor: 'canvas.default',
+                    border: '1px solid',
+                    borderColor: 'border.default',
+                    borderRadius: 2,
+                    padding: '2px 6px',
+                    fontSize: 0,
+                  }}
+                >
+                  d
+                </Box>
+              </ActionList.TrailingVisual>
+            </ActionList.LinkItem>
+            <ActionList.LinkItem href="//github.com">Github</ActionList.LinkItem>
             <ActionList.Item disabled>Disabled</ActionList.Item>
           </ActionList>
         </ActionMenu.Overlay>