Refactor(ActionList): ActionList.Item should render content as a button if parent is not interactive. (#3284)

* Using button as the underlying tag for ActionList.Item content.

* Fix subnav layout.

* Changing ActionList content to button if the top-level is not a button. ActionMenu is busted.

* Fix issue where ActionList.Item content would render as a button inside ActionMenu, breaking keyboard navigation.

* Create purple-crabs-matter.md

* test(vrt): update snapshots

* Fix bug with MenuContext not being exported properly.

* Fix color violation in ActionList.Item.

* Formatting, update snapshots.

* test(vrt): update snapshots

* Fix a11y violation in ActionList story.

* Formatting.

* Fix ts-ignore comment.

* Adjust line-height when button is rendered.

* Revert "test(vrt): update snapshots"

This reverts commit ba6d863.

* Revert "test(vrt): update snapshots"

This reverts commit 31d5358.

* Update snapshots.

* Remove flexGrow from ActionList.Item to remove extra 1px vertical height.

* Set padding to 0 and put flexGrow back to fix padding issue for ActionList buttons.

* Fix ActionList text wrap story.

* Fix underlinenav story by checking ActionList.Item if as prop is a.

* Update snapshots and formatting.

* Update snapshots.

* If ActionList.Item content is rendered as a button, remove tabIndex from li and fix padding.

* Fix various layout edge cases.

* ts-ignore event handlers on ItemWrapper.

* Update snapshots.

* Revert fontWeight config that differed from production docs.

* Pass selectionVariant prop illegally in UnderlineNav story to fix issue with Selection spans showing up.

* fix: prevent closing menu when `event.preventDefault()` is called on `ActionList.Item`'s `onSelect` handler by @gr2m (#3346)

* fix: prevent closing menu when `event.preventDefault()` is called on `ActionList.Item`'s `onSelect` handler (#3163)

* test: prevent closing menu when `event.preventDefault()` is called on `ActionList.Item`'s `onSelect` handler

Failing test for #3162

* fix: prevent closing menu when `event.preventDefault()` is called on `ActionList.Item`ߴs `onSelect` handler

* add storybook example: Delayed Menu Close

* update docs

* docs: changeset

* Update changelog

---------

Co-authored-by: Siddharth Kshetrapal <[email protected]>

* Update generated/components.json

---------

Co-authored-by: Gregor Martynus <[email protected]>
Co-authored-by: siddharthkp <[email protected]>

* Check for tabIndex value in isTopLevelInteractive.

* Reference styles in updated menuProps.

* Updated snapshots.

* Fix padding setting instead of using values from styles, which are inverted.

* Update snapshots.

---------

Co-authored-by: radglob <[email protected]>
Co-authored-by: Gregor Martynus <[email protected]>
Co-authored-by: siddharthkp <[email protected]>

Author: 3 people

.changeset/purple-crabs-matter.md

@@ -0,0 +1,5 @@
+---
+"@primer/react": patch
+---
+
+Refactor(ActionList): ActionList.Item should render content as a button if parent is not interactive.

src/ActionList/Description.tsx

@@ -44,7 +44,6 @@ export const Description: React.FC<React.PropsWithChildren<ActionListDescription
       sx={merge({...styles, color: disabled ? 'fg.disabled' : 'fg.muted'}, sx as SxProp)}
       title={props.children as string}
       inline={true}
-      maxWidth="100%"
     >
       {props.children}
     </Truncate>

src/ActionList/Item.tsx

@@ -9,10 +9,11 @@ import {defaultSxProp} from '../utils/defaultSxProp'
 import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic'
 import {ActionListContainerContext} from './ActionListContainerContext'
 import {Description} from './Description'
-import {ActionListProps, ListContext} from './List'
+import {ListContext} from './List'
 import {Selection} from './Selection'
 import {ActionListItemProps, getVariantStyles, ItemContext, TEXT_ROW_HEIGHT} from './shared'
 import {LeadingVisual, TrailingVisual} from './Visuals'
+import {MenuContext} from '../ActionMenu/ActionMenu'
 import {GroupContext} from './Group'
 
 const LiBox = styled.li<SxProp>(sx)
@@ -29,6 +30,8 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
       id,
       role,
       _PrivateItemWrapper,
+      // @ts-ignore tabIndex is sometimes passed as a prop in dotcom.
+      tabIndex,
       ...props
     },
     forwardedRef,
@@ -38,10 +41,13 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
       trailingVisual: TrailingVisual,
       description: Description,
     })
+
     const {variant: listVariant, showDividers, selectionVariant: listSelectionVariant} = React.useContext(ListContext)
     const {container, afterSelect, selectionAttribute} = React.useContext(ActionListContainerContext)
+    const menuContext = React.useContext(MenuContext)
     const {selectionVariant: groupSelectionVariant} = React.useContext(GroupContext)
 
+    const selectionVariant = groupSelectionVariant ?? listSelectionVariant
     const onSelect = React.useCallback(
       (
         event: React.MouseEvent<HTMLLIElement> | React.KeyboardEvent<HTMLLIElement>,
@@ -55,10 +61,6 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
       [onSelectUser],
     )
 
-    const selectionVariant: ActionListProps['selectionVariant'] = groupSelectionVariant
-      ? groupSelectionVariant
-      : listSelectionVariant
-
     /** Infer item role based on the container */
     let itemRole: ActionListItemProps['role']
     if (container === 'ActionMenu' || container === 'DropdownMenu') {
@@ -84,12 +86,22 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
       },
     }
 
+    const isTopLevelInteractive = () =>
+      _PrivateItemWrapper !== undefined ||
+      // @ts-ignore props.as may be defined, may not.
+      props.as === 'button' ||
+      // @ts-ignore props.as may be defined, may not.
+      props.as === 'a' ||
+      menuContext.anchorId !== undefined ||
+      role?.match(/menuitem/) ||
+      tabIndex !== undefined
+
     const styles = {
       position: 'relative',
       display: 'flex',
-      paddingX: 2,
+      paddingX: isTopLevelInteractive() ? 2 : 0,
       fontSize: 1,
-      paddingY: '6px', // custom value off the scale
+      paddingY: isTopLevelInteractive() ? '6px' : 0, // custom value off the scale
       lineHeight: TEXT_ROW_HEIGHT,
       minHeight: 5,
       marginX: listVariant === 'inset' ? 2 : 0,
@@ -145,6 +157,10 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
         borderTopWidth: showDividers ? `1px` : '0',
         borderColor: 'var(--divider-color, transparent)',
       },
+      'button[data-component="ActionList.Item--DividerContainer"]': {
+        textAlign: 'left',
+        padding: 0,
+      },
       // show between 2 items
       ':not(:first-of-type)': {'--divider-color': theme?.colors.actionListItem.inlineDivider},
       // hide divider after dividers & group header, with higher importance!
@@ -182,13 +198,13 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
     const inlineDescriptionId = useId(id && `${id}--inline-description`)
     const blockDescriptionId = useId(id && `${id}--block-description`)
 
-    const ItemWrapper = _PrivateItemWrapper || React.Fragment
+    const ItemWrapper = _PrivateItemWrapper || Box
 
     const menuItemProps = {
       onClick: clickHandler,
       onKeyPress: keyPressHandler,
       'aria-disabled': disabled ? true : undefined,
-      tabIndex: disabled ? undefined : 0,
+      tabIndex: disabled || !isTopLevelInteractive() ? undefined : 0,
       'aria-labelledby': `${labelId} ${
         slots.description && slots.description.props.variant !== 'block' ? inlineDescriptionId : ''
       }`,
@@ -199,17 +215,41 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
 
     const containerProps = _PrivateItemWrapper ? {role: role || itemRole ? 'none' : undefined} : menuItemProps
 
-    const wrapperProps = _PrivateItemWrapper ? menuItemProps : {}
+    const wrapperProps = _PrivateItemWrapper
+      ? menuItemProps
+      : {
+          sx: {
+            display: 'flex',
+            paddingX: isTopLevelInteractive() ? 0 : 2,
+            paddingY: isTopLevelInteractive() ? 0 : '6px', // custom value off the scale
+            flexGrow: 1,
+          },
+        }
 
     return (
       <ItemContext.Provider value={{variant, disabled, inlineDescriptionId, blockDescriptionId}}>
         <LiBox ref={forwardedRef} sx={merge<BetterSystemStyleObject>(styles, sxProp)} {...containerProps} {...props}>
+          {/* @ts-ignore onClick prop is only passed when _PrivateItemWrapper is set by ActionList.LinkItem. */}
           <ItemWrapper {...wrapperProps}>
             <Selection selected={selected} />
             {slots.leadingVisual}
             <Box
               data-component="ActionList.Item--DividerContainer"
-              sx={{display: 'flex', flexDirection: 'column', flexGrow: 1, minWidth: 0}}
+              sx={{
+                display: 'flex',
+                flexDirection: 'column',
+                flexGrow: 1,
+                minWidth: 0,
+                borderStyle: 'none',
+                backgroundColor: 'transparent',
+                cursor: 'inherit',
+                fontSize: 'inherit',
+                color: getVariantStyles(variant, disabled).color,
+                lineHeight: '20px',
+              }}
+              // @ts-ignore `as` prop may be passed to ActionList.Item, even if it isn't defined in ActionListItemProps.
+              // If this item is inside an ActionMenu, don't render an interactive button.
+              as={isTopLevelInteractive() ? 'div' : 'button'}
             >
               <ConditionalBox if={Boolean(slots.trailingVisual)} sx={{display: 'flex', flexGrow: 1}}>
                 <ConditionalBox

src/ActionMenu.tsx

@@ -3,7 +3,7 @@ import userEvent from '@testing-library/user-event'
 import {axe, toHaveNoViolations} from 'jest-axe'
 import React from 'react'
 import theme from '../theme'
-import {ActionMenu, ActionList, BaseStyles, ThemeProvider, SSRProvider} from '..'
+import {ActionMenu, MenuContext, ActionList, BaseStyles, ThemeProvider, SSRProvider} from '..'
 import {behavesAsComponent, checkExports} from '../utils/testing'
 import {SingleSelect} from '../ActionMenu/ActionMenu.features.stories'
 import {MixedSelection} from '../ActionMenu/ActionMenu.examples.stories'
@@ -47,6 +47,7 @@ describe('ActionMenu', () => {
   checkExports('ActionMenu', {
     default: undefined,
     ActionMenu,
+    MenuContext,
   })
 
   it('should open Menu on MenuButton click', async () => {

src/__tests__/ActionMenu.test.tsx renamed to src/ActionMenu/ActionMenu.test.tsx

@@ -16,7 +16,8 @@ export type MenuContextProps = Pick<
 > & {
   onClose?: (gesture: 'anchor-click' | 'click-outside' | 'escape' | 'tab') => void
 }
-const MenuContext = React.createContext<MenuContextProps>({renderAnchor: null, open: false})
+
+export const MenuContext = React.createContext<MenuContextProps>({renderAnchor: null, open: false})
 
 export type ActionMenuProps = {
   /**

src/ActionMenu/ActionMenu.tsx

@@ -211,7 +211,7 @@ describe('NavList.Item with NavList.SubNav', () => {
       </ThemeProvider>,
     )
 
-    const button = getByRole('button')
+    const button = getByRole('button', {name: 'Item'})
 
     // Starts open
     expect(queryByRole('list', {name: 'Item'})).toBeVisible()