Bring in changes from #1851

primer · Feb 21, 2022 · a920142 · a920142
1 parent 96a151a
commit a920142
Show file tree

Hide file tree

Showing 9 changed files with 46 additions and 23 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/.storybook/main.js b/.storybook/main.js
@@ -4,7 +4,7 @@ module.exports = {
     '@storybook/addon-a11y',
     '@storybook/addon-links',
     '@storybook/addon-essentials',
-    'storybook-addon-performance/register',
-    '@whitespace/storybook-addon-html'
+    'storybook-addon-performance/register'
+    // '@whitespace/storybook-addon-html'
   ]
 }
diff --git a/src/ActionList2/Group.tsx b/src/ActionList2/Group.tsx
@@ -45,10 +45,14 @@ export const Group: React.FC<GroupProps> = ({
   ...props
 }) => {
   const labelId = useSSRSafeId()
+  const {role: listRole} = React.useContext(ListContext)
+
+  console.log({listRole})
 
   return (
     <Box
       as="li"
+      role="none"
       sx={{
         '&:not(:first-child)': {marginTop: 2},
         listStyle: 'none', // hide the ::marker inserted by browser's stylesheet
@@ -58,7 +62,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,8 +190,8 @@ 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="listbox" showDividers aria-label="Select reviewers">
+        <ActionList.Group title="Suggestions">
           {users.slice(0, 2).map(user => (
             <ActionList.Item
               key={user.login}
@@ -208,7 +208,7 @@ 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"

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}
             >
@@ -250,13 +251,14 @@ export function GroupsStory(): JSX.Element {
     <>
       <h1>Groups</h1>
       <ErsatzOverlay>
-        <ActionList selectionVariant="multiple" showDividers aria-label="Select reviewers">
-          <ActionList.Group title="Suggestions" variant="filled" role="listbox">
+        <ActionList role="menu" selectionVariant="multiple" showDividers aria-label="Select reviewers">
+          <ActionList.Group title="Suggestions" variant="filled">
             {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)) || undefined}
                 onSelect={() => toggleAssignee(user)}
               >
                 <ActionList.LeadingVisual>
@@ -268,12 +270,13 @@ export function GroupsStory(): JSX.Element {
               </ActionList.Item>
             ))}
           </ActionList.Group>
-          <ActionList.Group title="Everyone" variant="filled" role="listbox">
+          <ActionList.Group title="Everyone" variant="filled">
             {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)) || undefined}
                 onSelect={() => toggleAssignee(user)}
               >
                 <ActionList.LeadingVisual>
@@ -1007,20 +1010,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 +1032,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/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()