Fix ActionList group roles (#1876)

* Bring in changes from #1851 * wasn't supposed to commit that * Fix ActionMenu stories * Fix ActionList/Groups story * Narrow role for listRole to AriaRole
primer · Feb 23, 2022 · 6cc9260 · 6cc9260
1 parent 2abd7b7
commit 6cc9260
Show file tree

Hide file tree

Showing 10 changed files with 48 additions and 82 deletions.
diff --git a/.changeset/drafts-actionlist-group-roles.md b/.changeset/drafts-actionlist-group-roles.md
@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+Better aria roles for ActionList group
diff --git a/src/ActionList2/ActionListContainerContext.tsx b/src/ActionList2/ActionListContainerContext.tsx
@@ -1,10 +1,11 @@
 /** This context can be used by components that compose ActionList inside a Menu */
 
 import React from 'react'
+import {AriaRole} from '../utils/types'
 
 type ContextProps = {
   container?: string
-  listRole?: string
+  listRole?: AriaRole
   selectionVariant?: 'single' | 'multiple' // TODO: Remove after DropdownMenu2 deprecation
   selectionAttribute?: 'aria-selected' | 'aria-checked'
   listLabelledBy?: string

diff --git a/src/ActionList2/Group.tsx b/src/ActionList2/Group.tsx
@@ -45,10 +45,12 @@ export const Group: React.FC<GroupProps> = ({
   ...props
 }) => {
   const labelId = useSSRSafeId()
+  const {role: listRole} = React.useContext(ListContext)
 
   return (
     <Box
       as="li"
+      role="none"
       sx={{
         '&:not(:first-child)': {marginTop: 2},
         listStyle: 'none', // hide the ::marker inserted by browser's stylesheet
@@ -58,7 +60,12 @@ export const Group: React.FC<GroupProps> = ({
     >
       {title && <Header title={title} variant={variant} auxiliaryText={auxiliaryText} labelId={labelId} />}
       <GroupContext.Provider value={{selectionVariant}}>
-        <Box as="ul" sx={{paddingInlineStart: 0}} aria-labelledby={title ? labelId : undefined} role={role}>
+        <Box
+          as="ul"
+          sx={{paddingInlineStart: 0}}
+          aria-labelledby={title ? labelId : undefined}
+          role={role || (listRole && 'group')}
+        >
           {props.children}
         </Box>
       </GroupContext.Provider>

diff --git a/src/ActionList2/Item.tsx b/src/ActionList2/Item.tsx
@@ -104,7 +104,7 @@ export const Item = React.forwardRef<HTMLLIElement, ItemProps>(
   ): JSX.Element => {
     const {variant: listVariant, showDividers, selectionVariant: listSelectionVariant} = React.useContext(ListContext)
     const {selectionVariant: groupSelectionVariant} = React.useContext(GroupContext)
-    const {container, afterSelect, selectionAttribute = 'aria-selected'} = React.useContext(ActionListContainerContext)
+    const {container, afterSelect, selectionAttribute} = React.useContext(ActionListContainerContext)
 
     let selectionVariant: ListProps['selectionVariant'] | GroupProps['selectionVariant']
     if (typeof groupSelectionVariant !== 'undefined') selectionVariant = groupSelectionVariant
@@ -227,7 +227,7 @@ export const Item = React.forwardRef<HTMLLIElement, ItemProps>(
             aria-labelledby={`${labelId} ${slots.InlineDescription ? inlineDescriptionId : ''}`}
             aria-describedby={slots.BlockDescription ? blockDescriptionId : undefined}
             role={role || itemRole}
-            {...{[selectionAttribute]: selected}}
+            {...(selectionAttribute && {[selectionAttribute]: selected})}
             {...props}
           >
             <ItemWrapper>

diff --git a/src/ActionList2/List.tsx b/src/ActionList2/List.tsx
@@ -24,7 +24,7 @@ export type ListProps = {
   role?: AriaRole
 } & SxProp
 
-type ContextProps = Pick<ListProps, 'variant' | 'selectionVariant' | 'showDividers'>
+type ContextProps = Pick<ListProps, 'variant' | 'selectionVariant' | 'showDividers' | 'role'>
 export const ListContext = React.createContext<ContextProps>({})
 
 const ListBox = styled.ul<SxProp>(sx)
@@ -56,7 +56,12 @@ export const List = React.forwardRef<HTMLUListElement, ListProps>(
         ref={forwardedRef}
       >
         <ListContext.Provider
-          value={{variant, selectionVariant: selectionVariant || containerSelectionVariant, showDividers}}
+          value={{
+            variant,
+            selectionVariant: selectionVariant || containerSelectionVariant,
+            showDividers,
+            role: role || listRole
+          }}
         >
           {props.children}
         </ListContext.Provider>

diff --git a/src/__tests__/ActionList2.test.tsx b/src/__tests__/ActionList2.test.tsx
@@ -42,6 +42,7 @@ function SingleSelectListStory(): JSX.Element {
           key={index}
           role="option"
           selected={index === selectedIndex}
+          aria-selected={index === selectedIndex}
           onSelect={() => setSelectedIndex(index)}
           disabled={project.disabled}
         >

diff --git a/src/stories/ActionList2/examples.stories.tsx b/src/stories/ActionList2/examples.stories.tsx
@@ -190,13 +190,14 @@ export function Groups(): JSX.Element {
 
       <p>Grouped content with labels and description. This patterns appears in pull requests to pick a reviewer.</p>
 
-      <ActionList selectionVariant="multiple" showDividers aria-label="Select reviewers">
-        <ActionList.Group title="Suggestions" role="listbox">
+      <ActionList selectionVariant="multiple" role="menu" showDividers aria-label="Select reviewers">
+        <ActionList.Group title="Suggestions">
           {users.slice(0, 2).map(user => (
             <ActionList.Item
               key={user.login}
-              role="option"
+              role="menuitemcheckbox"
               selected={Boolean(assignees.find(assignee => assignee.login === user.login))}
+              aria-checked={Boolean(assignees.find(assignee => assignee.login === user.login))}
               onSelect={() => toggleAssignee(user)}
             >
               <ActionList.LeadingVisual>
@@ -208,12 +209,13 @@ export function Groups(): JSX.Element {
             </ActionList.Item>
           ))}
         </ActionList.Group>
-        <ActionList.Group title="Everyone" role="listbox">
+        <ActionList.Group title="Everyone">
           {users.slice(2).map(user => (
             <ActionList.Item
-              role="option"
+              role="menuitemcheckbox"
               key={user.login}
               selected={Boolean(assignees.find(assignee => assignee.login === user.login))}
+              aria-checked={Boolean(assignees.find(assignee => assignee.login === user.login))}
               onSelect={() => toggleAssignee(user)}
             >
               <ActionList.LeadingVisual>

diff --git a/src/stories/ActionList2/fixtures.stories.tsx b/src/stories/ActionList2/fixtures.stories.tsx
@@ -213,12 +213,13 @@ export function DisabledStory(): JSX.Element {
     <>
       <h1>Disabled Items</h1>
       <ErsatzOverlay>
-        <ActionList selectionVariant="single" showDividers role="listbox" aria-label="Select a project">
+        <ActionList selectionVariant="single" showDividers role="menu" aria-label="Select a project">
           {projects.map((project, index) => (
             <ActionList.Item
               key={index}
-              role="option"
+              role="menuitemradio"
               selected={index === selectedIndex}
+              aria-checked={index === selectedIndex}
               onSelect={() => setSelectedIndex(index)}
               disabled={index === 1}
             >
@@ -236,61 +237,6 @@ export function DisabledStory(): JSX.Element {
 }
 DisabledStory.storyName = 'Disabled Items'
 
-export function GroupsStory(): JSX.Element {
-  const [assignees, setAssignees] = React.useState(users.slice(0, 1))
-
-  const toggleAssignee = (assignee: typeof users[number]) => {
-    const assigneeIndex = assignees.findIndex(a => a.login === assignee.login)
-
-    if (assigneeIndex === -1) setAssignees([...assignees, assignee])
-    else setAssignees(assignees.filter((_, index) => index !== assigneeIndex))
-  }
-
-  return (
-    <>
-      <h1>Groups</h1>
-      <ErsatzOverlay>
-        <ActionList selectionVariant="multiple" showDividers aria-label="Select reviewers">
-          <ActionList.Group title="Suggestions" variant="filled" role="listbox">
-            {users.slice(0, 2).map(user => (
-              <ActionList.Item
-                key={user.login}
-                role="option"
-                selected={Boolean(assignees.find(assignee => assignee.login === user.login))}
-                onSelect={() => toggleAssignee(user)}
-              >
-                <ActionList.LeadingVisual>
-                  <Avatar src={`https://github.com/${user.login}.png`} />
-                </ActionList.LeadingVisual>
-                {user.login}
-                <ActionList.Description>{user.name}</ActionList.Description>
-                <ActionList.Description variant="block">Recently edited these files</ActionList.Description>
-              </ActionList.Item>
-            ))}
-          </ActionList.Group>
-          <ActionList.Group title="Everyone" variant="filled" role="listbox">
-            {users.slice(2).map(user => (
-              <ActionList.Item
-                role="option"
-                key={user.login}
-                selected={Boolean(assignees.find(assignee => assignee.login === user.login))}
-                onSelect={() => toggleAssignee(user)}
-              >
-                <ActionList.LeadingVisual>
-                  <Avatar src={`https://github.com/${user.login}.png`} />
-                </ActionList.LeadingVisual>
-                {user.login}
-                <ActionList.Description>{user.name}</ActionList.Description>
-              </ActionList.Item>
-            ))}
-          </ActionList.Group>
-        </ActionList>
-      </ErsatzOverlay>
-    </>
-  )
-}
-GroupsStory.storyName = 'Groups'
-
 export function ActionsStory(): JSX.Element {
   return (
     <>
@@ -1007,20 +953,19 @@ export function MemexSortable(): JSX.Element {
       <h1>Memex Sortable List</h1>
       <ErsatzOverlay>
         <DndProvider backend={HTML5Backend}>
-          <ActionList selectionVariant="multiple">
-            <ActionList.Group title="Visible fields (can be reordered)" role="listbox">
+          <ActionList selectionVariant="multiple" role="menu">
+            <ActionList.Group title="Visible fields (can be reordered)">
               {visibleOptions.map(option => (
                 <SortableItem
                   key={option.text}
-                  role="option"
+                  role="menuitemcheckbox"
                   option={option}
                   onSelect={() => toggle(option.text)}
                   reorder={reorder}
                 />
               ))}
             </ActionList.Group>
             <ActionList.Group
-              role="listbox"
               title="Hidden fields"
               selectionVariant={
                 /** selectionVariant override on Group: disable selection if there are no options */
@@ -1030,7 +975,7 @@ export function MemexSortable(): JSX.Element {
               {hiddenOptions.map((option, index) => (
                 <ActionList.Item
                   key={index}
-                  role="option"
+                  role="menuitemcheckbox"
                   selected={option.selected}
                   onSelect={() => toggle(option.text)}
                 >

diff --git a/src/stories/ActionMenu2/examples.stories.tsx b/src/stories/ActionMenu2/examples.stories.tsx
@@ -171,8 +171,8 @@ export function GroupsAndDescription(): JSX.Element {
             Milestone
           </ActionMenu.Button>
           <ActionMenu.Overlay width="medium">
-            <ActionList selectionVariant="single" showDividers role="none">
-              <ActionList.Group title="Open" role="menu">
+            <ActionList selectionVariant="single" showDividers>
+              <ActionList.Group title="Open">
                 {milestones
                   .filter(milestone => !milestone.name.includes('21'))
                   .map((milestone, index) => (
@@ -189,7 +189,7 @@ export function GroupsAndDescription(): JSX.Element {
                     </ActionList.Item>
                   ))}
               </ActionList.Group>
-              <ActionList.Group title="Closed" role="menu">
+              <ActionList.Group title="Closed">
                 {milestones
                   .filter(milestone => milestone.name.includes('21'))
                   .map((milestone, index) => (
@@ -270,7 +270,6 @@ export function MultipleSelection(): JSX.Element {
             <ActionList selectionVariant="multiple" showDividers>
               {users.map(user => (
                 <ActionList.Item
-                  role="option"
                   key={user.login}
                   selected={Boolean(assignees.find(assignee => assignee.login === user.login))}
                   onSelect={() => toggleAssignee(user)}
@@ -321,8 +320,8 @@ export function MixedSelection(): JSX.Element {
           {selectedOption ? `Group by ${selectedOption.text}` : 'Group items by'}
         </ActionMenu.Button>
         <ActionMenu.Overlay width="medium">
-          <ActionList role="none">
-            <ActionList.Group selectionVariant="single" title="Group by" role="menu">
+          <ActionList>
+            <ActionList.Group selectionVariant="single" title="Group by">
               {options.map((option, index) => (
                 <ActionList.Item
                   key={index}
@@ -337,9 +336,9 @@ export function MixedSelection(): JSX.Element {
               ))}
             </ActionList.Group>
             {typeof selectedIndex === 'number' && (
-              <ActionList.Group role="menu">
+              <ActionList.Group>
                 <ActionList.Divider />
-                <ActionList.Item onSelect={() => setSelectedIndex(null)} role="menuitem">
+                <ActionList.Item onSelect={() => setSelectedIndex(null)}>
                   <ActionList.LeadingVisual>
                     <XIcon />
                   </ActionList.LeadingVisual>

diff --git a/src/utils/testing.tsx b/src/utils/testing.tsx
@@ -254,7 +254,8 @@ export function checkStoriesForAxeViolations(name: string) {
   Object.values(Stories).map(Story => {
     if (typeof Story !== 'function') return
 
-    it(`story ${(Story as StoryType).storyName} should have no axe violations`, async () => {
+    const {storyName, name: StoryFunctionName} = Story as StoryType
+    it(`story ${storyName || StoryFunctionName} should have no axe violations`, async () => {
       const {container} = HTMLRender(<Story />)
       const results = await axe(container)
       expect(results).toHaveNoViolations()