ActionList: Add more checks for ActionList.Item when using button s…

…emantics (#4876) * Add more checks for `ActionList.Item` and button semantics * Turn on FF * Move events to `li` * Add `role="list"` to role types * Enable flag * disable the flag * Move `ButtonItemWrapper` outside of `Item` * Disable flag again * Remove `itemEventProps` * Enable back FF * Add changeset * Update .changeset/dull-moons-repair.md Co-authored-by: Siddharth Kshetrapal <[email protected]> --------- Co-authored-by: Siddharth Kshetrapal <[email protected]>
primer · Oct 18, 2024 · 9dd95a8 · 9dd95a8
1 parent 65f6437
commit 9dd95a8
Show file tree

Hide file tree

Showing 4 changed files with 69 additions and 15 deletions.
diff --git a/.changeset/dull-moons-repair.md b/.changeset/dull-moons-repair.md
@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+ActionList: Add more guards for `ActionList.Item` before utilizing button semantics (behind feature flag `primer_react_action_list_item_as_button`)
diff --git a/packages/react/src/ActionList/ActionList.features.stories.tsx b/packages/react/src/ActionList/ActionList.features.stories.tsx
@@ -358,6 +358,24 @@ export const ListBoxMultiSelect = () => {
   )
 }
 
+export const WithDynamicContent = () => {
+  const [isTrue, setIsTrue] = React.useState(false)
+
+  return (
+    <FeatureFlags flags={{primer_react_action_list_item_as_button: true}}>
+      <ActionList>
+        <ActionList.Item
+          onSelect={() => {
+            setIsTrue(!isTrue)
+          }}
+        >
+          Activated? {isTrue ? 'Yes' : 'No'}
+        </ActionList.Item>
+      </ActionList>
+    </FeatureFlags>
+  )
+}
+
 export const DisabledSelectedMultiselect = () => (
   <ActionList selectionVariant="multiple" role="menu" aria-label="Project">
     <ActionList.Item role="menuitemcheckbox" selected aria-checked disabled>

diff --git a/packages/react/src/ActionList/ActionList.test.tsx b/packages/react/src/ActionList/ActionList.test.tsx
@@ -586,6 +586,37 @@ describe('ActionList', () => {
     expect(mockOnSelect).toHaveBeenCalledTimes(1)
   })
 
+  it('should not render buttons when feature flag is enabled and is specified role', async () => {
+    const {getByRole} = HTMLRender(
+      <FeatureFlags flags={{primer_react_action_list_item_as_button: true}}>
+        <ActionList>
+          <ActionList.Item role="option">Item 1</ActionList.Item>
+          <ActionList.Item role="menuitem">Item 2</ActionList.Item>
+          <ActionList.Item role="menuitemcheckbox">Item 3</ActionList.Item>
+          <ActionList.Item role="menuitemradio">Item 4</ActionList.Item>
+          <ActionList.Item>Item 5</ActionList.Item>
+        </ActionList>
+      </FeatureFlags>,
+    )
+
+    const option = getByRole('option')
+    expect(option.tagName).toBe('LI')
+    expect(option.textContent).toBe('Item 1')
+
+    const menuItem = getByRole('menuitem')
+    expect(menuItem.tagName).toBe('LI')
+
+    const menuItemCheckbox = getByRole('menuitemcheckbox')
+    expect(menuItemCheckbox.tagName).toBe('LI')
+
+    const menuItemRadio = getByRole('menuitemradio')
+    expect(menuItemRadio.tagName).toBe('LI')
+
+    const button = getByRole('button')
+    expect(button.parentElement?.tagName).toBe('LI')
+    expect(button.textContent).toBe('Item 5')
+  })
+
   it('should be navigatable with arrow keys for certain roles', async () => {
     HTMLRender(
       <ActionList role="listbox" aria-label="Select a project">

diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx
@@ -24,6 +24,14 @@ import VisuallyHidden from '../_VisuallyHidden'
 
 const LiBox = styled.li<SxProp>(sx)
 
+const ButtonItemContainer = React.forwardRef(({as: Component = 'button', children, styles, ...props}, forwardedRef) => {
+  return (
+    <Box as={Component as React.ElementType} ref={forwardedRef} sx={styles} {...props}>
+      {children}
+    </Box>
+  )
+}) as PolymorphicForwardRefComponent<React.ElementType, ActionListItemProps>
+
 export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
   (
     {
@@ -112,7 +120,12 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
 
     const itemSelectionAttribute = selectionAttribute || inferredSelectionAttribute
     // Ensures ActionList.Item retains list item semantics if a valid ARIA role is applied, or if item is inactive
-    const listSemantics = listRole === 'listbox' || listRole === 'menu' || inactive || container === 'NavList'
+    const listItemSemantics =
+      role === 'option' || role === 'menuitem' || role === 'menuitemradio' || role === 'menuitemcheckbox'
+
+    const listRoleTypes = ['listbox', 'menu', 'list']
+    const listSemantics =
+      (listRole && listRoleTypes.includes(listRole)) || inactive || container === 'NavList' || listItemSemantics
     const buttonSemantics = !listSemantics && !_PrivateItemWrapper && buttonSemanticsFeatureFlag
 
     const {theme} = useTheme()
@@ -268,22 +281,9 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
     const trailingVisualId = `${itemId}--trailing-visual`
     const inactiveWarningId = inactive && !showInactiveIndicator ? `${itemId}--warning-message` : undefined
 
-    const ButtonItemWrapper = React.forwardRef(({as: Component = 'button', children, ...props}, forwardedRef) => {
-      return (
-        <Box
-          as={Component as React.ElementType}
-          sx={merge<BetterSystemStyleObject>(styles, sxProp)}
-          ref={forwardedRef}
-          {...props}
-        >
-          {children}
-        </Box>
-      )
-    }) as PolymorphicForwardRefComponent<React.ElementType, ActionListItemProps>
-
     let DefaultItemWrapper = React.Fragment
     if (buttonSemanticsFeatureFlag) {
-      DefaultItemWrapper = listSemantics ? React.Fragment : ButtonItemWrapper
+      DefaultItemWrapper = listSemantics ? React.Fragment : ButtonItemContainer
     }
 
     const ItemWrapper = _PrivateItemWrapper || DefaultItemWrapper