From 9dd95a86274fc0358966aded932526f88c9d505a Mon Sep 17 00:00:00 2001
From: Tyler Jones <tylerjdev@github.com>
Date: Fri, 18 Oct 2024 14:31:25 -0400
Subject: [PATCH] ActionList: Add more checks for `ActionList.Item` when using
 button semantics (#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 <siddharthkp@github.com>

---------

Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com>
---
 .changeset/dull-moons-repair.md               |  5 +++
 .../ActionList.features.stories.tsx           | 18 +++++++++++
 .../react/src/ActionList/ActionList.test.tsx  | 31 +++++++++++++++++++
 packages/react/src/ActionList/Item.tsx        | 30 +++++++++---------
 4 files changed, 69 insertions(+), 15 deletions(-)
 create mode 100644 .changeset/dull-moons-repair.md

diff --git a/.changeset/dull-moons-repair.md b/.changeset/dull-moons-repair.md
new file mode 100644
index 00000000000..25257d13a72
--- /dev/null
+++ 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
index 23e126409dd..59155c9c0f4 100644
--- 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
index fb847677cdc..79b50978b4c 100644
--- 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
index 53dae15b6a3..5db1765bfb5 100644
--- 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