From cc909dc46edaf8515f80b6d718e356661679beb3 Mon Sep 17 00:00:00 2001 From: Amanda Brown Date: Tue, 13 Dec 2022 10:22:31 -0500 Subject: [PATCH] 2411 the aria keyshortcuts is wrongly attached for the actionlistlinkitem 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' --- .changeset/sour-knives-drive.md | 5 ++ .gitignore | 1 + .../ActionList.examples.stories.tsx | 2 +- src/ActionList/Item.tsx | 86 +++++++++++-------- src/ActionList/LinkItem.tsx | 4 +- .../__snapshots__/NavList.test.tsx.snap | 21 +++-- src/__tests__/ActionList.test.tsx | 12 +++ src/__tests__/ActionMenu.test.tsx | 23 +++++ src/stories/ActionMenu/fixtures.stories.tsx | 20 +++++ 9 files changed, 127 insertions(+), 47 deletions(-) create mode 100644 .changeset/sour-knives-drive.md diff --git a/.changeset/sour-knives-drive.md b/.changeset/sour-knives-drive.md new file mode 100644 index 00000000000..d85f5853ea5 --- /dev/null +++ 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 index 0812cbae758..a044e6566fa 100644 --- 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 index d1875ef8cd6..8b2bdef698f 100644 --- a/src/ActionList/ActionList.examples.stories.tsx +++ b/src/ActionList/ActionList.examples.stories.tsx @@ -57,7 +57,7 @@ export const ListLinkItem = () => ( not a link, just an Item for comparison - + diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index ca97e3a727a..29733f354de 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -176,44 +176,56 @@ export const Item = React.forwardRef( return ( - {slots => ( - (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} - > - - - {slots.LeadingVisual} - - - - - {props.children} - - {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 ( + (styles, sxProp)} + {...containerProps} + {...props} + > + + + {slots.LeadingVisual} + + + + + {props.children} + + {slots.InlineDescription} + + {slots.TrailingVisual} - {slots.TrailingVisual} - - {slots.BlockDescription} - - - - )} + {slots.BlockDescription} + + + + ) + }} ) }, diff --git a/src/ActionList/LinkItem.tsx b/src/ActionList/LinkItem.tsx index fb1f17a8d01..daea8a98ddc 100644 --- a/src/ActionList/LinkItem.tsx +++ b/src/ActionList/LinkItem.tsx @@ -39,8 +39,8 @@ export const LinkItem = React.forwardRef(({sx = {}, active, as: Component, ...pr ( - + _PrivateItemWrapper={({children, ...rest}) => ( + {children} )} diff --git a/src/NavList/__snapshots__/NavList.test.tsx.snap b/src/NavList/__snapshots__/NavList.test.tsx.snap index 6f060de1e02..49de269f85a 100644 --- 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" >
  • Copy link Edit file Delete file + + Link Item + @@ -63,6 +66,15 @@ describe('ActionList', () => { ActionList, }) + it('should have aria-keyshortcuts applied to the correct element', async () => { + const {container} = HTMLRender() + + 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() const results = await axe(container) diff --git a/src/__tests__/ActionMenu.test.tsx b/src/__tests__/ActionMenu.test.tsx index 3321c28936e..6412b03bb5c 100644 --- a/src/__tests__/ActionMenu.test.tsx +++ b/src/__tests__/ActionMenu.test.tsx @@ -24,6 +24,9 @@ function Example(): JSX.Element { event.preventDefault()}> Delete file + + Github + @@ -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() + 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() diff --git a/src/stories/ActionMenu/fixtures.stories.tsx b/src/stories/ActionMenu/fixtures.stories.tsx index 000e285b6d4..6619cacce55 100644 --- 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 ⌘D + @@ -759,6 +760,25 @@ export function MnemonicsTest(): JSX.Element { + + User defined Link + + + d + + + + Github Disabled