Correct inactive ActionList.Item behavior in NavList and SelectPanel contexts

.changeset/floppy-peaches-obey.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+Improves how we detect context for inactive item messaging by checking the `role` on `ActionList` instead of relying on parent components, making the logic more robust and consistent. It also fixes incorrect behavior in `NavList` and `SelectPanel`, and improves accessibility by correcting `aria` relationships on tooltip buttons.

packages/react/src/ActionList/ActionList.test.tsx

@@ -100,7 +100,8 @@ describe('ActionList', () => {
     expect(document.activeElement).toHaveTextContent('Option 4')
 
     await userEvent.keyboard('{ArrowDown}')
-    expect(document.activeElement).toHaveAccessibleName('Unavailable due to an outage')
+    expect(document.activeElement).toHaveAccessibleName('Option 5')
+    expect(document.activeElement).toHaveAccessibleDescription('Unavailable due to an outage')
 
     await userEvent.keyboard('{ArrowUp}')
     expect(document.activeElement).toHaveTextContent('Option 4')

packages/react/src/ActionList/Item.test.tsx

@@ -17,6 +17,10 @@ function SimpleActionList(): JSX.Element {
       <ActionList.LinkItem href="//github.com" title="anchor" aria-keyshortcuts="d">
         Link Item
       </ActionList.LinkItem>
+      <ActionList.Item inactiveText="Unavailable due to an outage">Inactive item</ActionList.Item>
+      <ActionList.Item inactiveText="Unavailable due to an outage" loading>
+        Loading and inactive item
+      </ActionList.Item>
     </ActionList>
   )
 }
@@ -171,23 +175,53 @@ describe('ActionList.Item', () => {
     fireEvent.keyPress(option, {key: 'Enter', charCode: 13})
     expect(option).toBeInTheDocument()
   })
-  it('should focus the button around the leading visual when tabbing to an inactive item', async () => {
+  it('should focus the button around the alert icon when tabbing to an inactive item', async () => {
+    const component = HTMLRender(<SimpleActionList />)
+    const inactiveIndicatorButton = await waitFor(() => component.getByRole('button', {name: 'Inactive item'}))
+    await userEvent.tab()
+    await userEvent.tab()
+    await userEvent.tab()
+    await userEvent.tab()
+    await userEvent.tab()
+    await userEvent.tab() // focuses 6th element, which is inactive
+    expect(inactiveIndicatorButton).toHaveFocus()
+    expect(document.activeElement).toHaveAccessibleDescription('Unavailable due to an outage')
+  })
+  it('should focus the option or menu item when moving focus to an inactive item **in a listbox**', async () => {
     const component = HTMLRender(<SingleSelectListStory />)
-    const inactiveOptionButton = await waitFor(() => component.getByRole('button', {name: projects[3].inactiveText}))
+    const inactiveOption = await waitFor(() => component.getByRole('option', {name: projects[3].name}))
     await userEvent.tab() // get focus on first element
     await userEvent.keyboard('{ArrowDown}')
     await userEvent.keyboard('{ArrowDown}')
-    expect(inactiveOptionButton).toHaveFocus()
+    expect(inactiveOption).toHaveFocus()
+    expect(document.activeElement).toHaveAccessibleDescription(projects[3].inactiveText as string)
   })
   it('should behave as inactive if both inactiveText and loading props are passed', async () => {
+    const component = HTMLRender(<SimpleActionList />)
+    const inactiveIndicatorButton = await waitFor(() =>
+      component.getByRole('button', {name: 'Loading and inactive item'}),
+    )
+    await userEvent.tab()
+    await userEvent.tab()
+    await userEvent.tab()
+    await userEvent.tab()
+    await userEvent.tab()
+    await userEvent.tab()
+    await userEvent.tab() // focuses 7th element, which is inactive AND has a loading prop
+    expect(inactiveIndicatorButton).toHaveFocus()
+    expect(document.activeElement).toHaveAccessibleDescription('Unavailable due to an outage')
+  })
+
+  it('should behave as inactive if both inactiveText and loading props are passed **in a listbox**', async () => {
     const component = HTMLRender(<SingleSelectListStory />)
-    const inactiveOptionButton = await waitFor(() => component.getByRole('button', {name: projects[5].inactiveText}))
+    const inactiveOption = await waitFor(() => component.getByRole('option', {name: projects[5].name}))
     await userEvent.tab() // get focus on first element
     await userEvent.keyboard('{ArrowDown}')
     await userEvent.keyboard('{ArrowDown}')
     await userEvent.keyboard('{ArrowDown}')
     await userEvent.keyboard('{ArrowDown}')
-    expect(inactiveOptionButton).toHaveFocus()
+    expect(inactiveOption).toHaveFocus()
+    expect(document.activeElement).toHaveAccessibleDescription(projects[5].inactiveText as string)
   })
   it('should call onClick for a link item', async () => {
     const onClick = jest.fn()

packages/react/src/ActionList/Item.tsx

@@ -112,7 +112,11 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
     } = React.useContext(ListContext)
     const {selectionVariant: groupSelectionVariant} = React.useContext(GroupContext)
     const inactive = Boolean(inactiveText)
-    const showInactiveIndicator = inactive && container === undefined
+    // TODO change `menuContext` check to ```listRole !== undefined && ['menu', 'listbox'].includes(listRole)```
+    // once we have a better way to handle existing usage in dotcom that incorrectly use ActionList.TrailingAction
+    const menuContext = container === 'ActionMenu' || container === 'SelectPanel'
+    // TODO: when we change `menuContext` to check `listRole` instead of `container`
+    const showInactiveIndicator = inactive && !(listRole !== undefined && ['menu', 'listbox'].includes(listRole))
 
     const onSelect = React.useCallback(
       (
@@ -142,10 +146,12 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
     }
 
     const itemRole = role || inferredItemRole
-    const menuContext = container === 'ActionMenu' || container === 'SelectPanel'
 
     if (slots.trailingAction) {
-      invariant(!menuContext, `ActionList.TrailingAction can not be used within a ${container}.`)
+      invariant(
+        !menuContext,
+        `ActionList.TrailingAction can not be used within a list with an ARIA role of "menu" or "listbox".`,
+      )
     }
 
     /** Infer the proper selection attribute based on the item's role */
@@ -405,7 +411,8 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
                     <span id={labelId} className={classes.ItemLabel}>
                       {childrenWithoutSlots}
                       {/* Loading message needs to be in here so it is read with the label */}
-                      {loading === true && <VisuallyHidden>Loading</VisuallyHidden>}
+                      {/* If the item is inactive, we do not simultaneously announce that it is loading */}
+                      {loading === true && !inactive && <VisuallyHidden>Loading</VisuallyHidden>}
                     </span>
                     {slots.description}
                   </ConditionalWrapper>
@@ -422,7 +429,7 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
                   {
                     // If the item is inactive, but it's not in an overlay (e.g. ActionMenu, SelectPanel),
                     // render the inactive warning message directly in the item.
-                    inactive && container ? (
+                    !showInactiveIndicator ? (
                       <span className={classes.InactiveWarning} id={inactiveWarningId}>
                         {inactiveText}
                       </span>
@@ -477,7 +484,8 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
                   <span id={labelId} className={classes.ItemLabel}>
                     {childrenWithoutSlots}
                     {/* Loading message needs to be in here so it is read with the label */}
-                    {loading === true && <VisuallyHidden>Loading</VisuallyHidden>}
+                    {/* If the item is inactive, we do not simultaneously announce that it is loading */}
+                    {loading === true && !inactive && <VisuallyHidden>Loading</VisuallyHidden>}
                   </span>
                   {slots.description}
                 </ConditionalWrapper>
@@ -494,7 +502,7 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
                 {
                   // If the item is inactive, but it's not in an overlay (e.g. ActionMenu, SelectPanel),
                   // render the inactive warning message directly in the item.
-                  inactive && container ? (
+                  !showInactiveIndicator ? (
                     <span className={classes.InactiveWarning} id={inactiveWarningId}>
                       {inactiveText}
                     </span>
@@ -567,7 +575,8 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
                   >
                     {childrenWithoutSlots}
                     {/* Loading message needs to be in here so it is read with the label */}
-                    {loading === true && <VisuallyHidden>Loading</VisuallyHidden>}
+                    {/* If the item is inactive, we do not simultaneously announce that it is loading */}
+                    {loading === true && !inactive && <VisuallyHidden>Loading</VisuallyHidden>}
                   </Box>
                   {slots.inlineDescription}
                 </ConditionalWrapper>
@@ -584,7 +593,7 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
               {
                 // If the item is inactive, but it's not in an overlay (e.g. ActionMenu, SelectPanel),
                 // render the inactive warning message directly in the item.
-                inactive && container ? (
+                !showInactiveIndicator ? (
                   <Box
                     as="span"
                     sx={{

packages/react/src/ActionList/Visuals.tsx

@@ -168,8 +168,8 @@ export const VisualOrIndicator: React.FC<
 
   return inactiveText ? (
     <span className={classes.InactiveButtonWrap}>
-      <Tooltip text={inactiveText} type="label">
-        <button type="button" className={classes.InactiveButtonReset} aria-describedby={labelId}>
+      <Tooltip text={inactiveText} type="description">
+        <button type="button" className={classes.InactiveButtonReset} aria-labelledby={labelId}>
           <VisualComponent>
             <AlertIcon />
           </VisualComponent>