From 3c467efc794d47326613796149f422c24a9b3cb9 Mon Sep 17 00:00:00 2001
From: Tyler Jones <tylerjdev@github.com>
Date: Tue, 18 Jun 2024 16:58:40 -0400
Subject: [PATCH] Fixes for `ActionList` semantics (#4272)

* Add fixes for ActionList

* More type fixes

* temp type fixes

* Add condition for inactive items

* add yet another condition

* chore(deps): bump changesets/action from 1.4.5 to 1.4.6 (#4282)

Bumps [changesets/action](https://github.com/changesets/action) from 1.4.5 to 1.4.6.
- [Release notes](https://github.com/changesets/action/releases)
- [Changelog](https://github.com/changesets/action/blob/main/CHANGELOG.md)
- [Commits](https://github.com/changesets/action/compare/f13b1baaa620fde937751f5d2c3572b9da32af23...e2f8e964d080ae97c874b19e27b12e0a8620fb6c)

---
updated-dependencies:
- dependency-name: changesets/action
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* test(e2e): add e2e test for SelectPanel2 default story (#4279)

* fix(SelectPanel2): add aria-labelledby to listbox

* test(e2e): add e2e test for SelectPanel2 default story

* chore: add changeset

* test(vrt): update snapshots

---------

Co-authored-by: Josh Black <joshblack@users.noreply.github.com>

* Address a few v8 color bugs (#4278)

* small bug fixes with v8

* Create khaki-schools-lay.md

* test(vrt): update snapshots

* snippy snaps

* test(vrt): update snapshots

* test: update snapshots

* test: update snapshots

* try commenting flakey tests

* test: comment out flakey snapshot test

---------

Co-authored-by: langermank <langermank@users.noreply.github.com>
Co-authored-by: Josh Black <joshblack@users.noreply.github.com>

* chore(deps-dev): bump ip from 2.0.0 to 2.0.1 (#4291)

Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1.
- [Commits](https://github.com/indutny/node-ip/compare/v2.0.0...v2.0.1)

---
updated-dependencies:
- dependency-name: ip
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Change how `ref` is handled

* Change types

* Update storybook example types

* Update types on component

* Add another type

* Update type in `SegmentedControl`

* Add back `li`-only type

* Add onto `onSelect` type

* Update more types

* Type fixes for `LinkItem`

* Changes from feedback

* Change types

* Replace `role="list"` with context

* Add feature flag to `ActionList.Item

* Add back forwardedRef in cases where valid role is true

* Update FF name

* Add lint disable

* Update FF name

* Add changeset

* Remove `list` condition

* Rename FF

* Address feedback

* Add feature flag story

* Add new test

* Add feature flag to all stories

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Josh Black <joshblack@github.com>
Co-authored-by: Josh Black <joshblack@users.noreply.github.com>
Co-authored-by: Katie Langerman <18661030+langermank@users.noreply.github.com>
Co-authored-by: langermank <langermank@users.noreply.github.com>
---
 .changeset/hot-mayflies-pump.md               |  5 ++
 packages/react/.storybook/preview.js          |  5 +-
 .../ActionList.features.stories.tsx           | 24 +++++++
 .../react/src/ActionList/ActionList.test.tsx  | 67 +++++++++++++++++
 packages/react/src/ActionList/Item.tsx        | 71 ++++++++++++++++---
 packages/react/src/ActionList/LinkItem.tsx    |  2 +-
 packages/react/src/ActionList/shared.ts       |  6 +-
 .../ActionMenu.examples.stories.tsx           |  2 +-
 packages/react/src/NavList/NavList.tsx        |  9 ++-
 .../src/SegmentedControl/SegmentedControl.tsx |  2 +-
 .../_AutocompleteSuggestions.tsx              |  2 +-
 11 files changed, 177 insertions(+), 18 deletions(-)
 create mode 100644 .changeset/hot-mayflies-pump.md

diff --git a/.changeset/hot-mayflies-pump.md b/.changeset/hot-mayflies-pump.md
new file mode 100644
index 00000000000..a03fcc22499
--- /dev/null
+++ b/.changeset/hot-mayflies-pump.md
@@ -0,0 +1,5 @@
+---
+'@primer/react': minor
+---
+
+(Behind feature flag) ActionList: Utilizes `<button>` inside of `<li>` for interactive items.
\ No newline at end of file
diff --git a/packages/react/.storybook/preview.js b/packages/react/.storybook/preview.js
index ccbef115ed2..26e04c221c5 100644
--- a/packages/react/.storybook/preview.js
+++ b/packages/react/.storybook/preview.js
@@ -1,6 +1,7 @@
 import {PrimerBreakpoints} from '../src/utils/layout'
 import React, {useEffect} from 'react'
 import {ThemeProvider, BaseStyles, theme} from '../src'
+import {FeatureFlags} from '../src/FeatureFlags'
 import clsx from 'clsx'
 
 import './storybook.css'
@@ -222,7 +223,9 @@ export const decorators = [
         <div className={clsx('story-wrap')}>
           <BaseStyles>
             {showSurroundingElements ? <a href="/primer/react">Primer documentation</a> : ''}
-            <Story {...context} />
+            <FeatureFlags flags={{primer_react_action_list_item_as_button: true}}>
+              <Story {...context} />
+            </FeatureFlags>
             {showSurroundingElements ? <a href="/primer/react">Primer documentation</a> : ''}
           </BaseStyles>
         </div>
diff --git a/packages/react/src/ActionList/ActionList.features.stories.tsx b/packages/react/src/ActionList/ActionList.features.stories.tsx
index a8cf67067d1..dbc1d16fd74 100644
--- a/packages/react/src/ActionList/ActionList.features.stories.tsx
+++ b/packages/react/src/ActionList/ActionList.features.stories.tsx
@@ -25,7 +25,9 @@ import {
   PeopleIcon,
   FileDirectoryIcon,
   PlusCircleIcon,
+  LinkExternalIcon,
 } from '@primer/octicons-react'
+import {FeatureFlags} from '../FeatureFlags'
 
 export default {
   title: 'Components/ActionList/Features',
@@ -710,3 +712,25 @@ export const GroupWithFilledTitle = () => {
     </ActionList>
   )
 }
+
+export const ActionListWithButtonSemantics = () => {
+  return (
+    <FeatureFlags flags={{primer_react_action_list_item_as_button: true}}>
+      <ActionList>
+        <ActionList.Item>Copy link</ActionList.Item>
+        <ActionList.Item inactiveText="Nothing to quote">Quote reply</ActionList.Item>
+        <ActionList.Item disabled>Edit comment</ActionList.Item>
+        <ActionList.Divider />
+        <ActionList.Item variant="danger">Delete file</ActionList.Item>
+        <ActionList.LinkItem href="/primer/react#readme">
+          Support
+          <ActionList.TrailingVisual>
+            <LinkExternalIcon />
+          </ActionList.TrailingVisual>
+        </ActionList.LinkItem>
+      </ActionList>
+    </FeatureFlags>
+  )
+}
+
+ActionListWithButtonSemantics.storyName = 'With Button Semantics (Behind feature flag)'
diff --git a/packages/react/src/ActionList/ActionList.test.tsx b/packages/react/src/ActionList/ActionList.test.tsx
index d6d0d48bd51..68b1d8794a2 100644
--- a/packages/react/src/ActionList/ActionList.test.tsx
+++ b/packages/react/src/ActionList/ActionList.test.tsx
@@ -6,6 +6,7 @@ import theme from '../theme'
 import {ActionList} from '.'
 import {behavesAsComponent, checkExports} from '../utils/testing'
 import {BaseStyles, ThemeProvider, SSRProvider, ActionMenu} from '..'
+import {FeatureFlags} from '../FeatureFlags'
 
 function SimpleActionList(): JSX.Element {
   return (
@@ -378,4 +379,70 @@ describe('ActionList', () => {
     const heading = getByText('Group Heading')
     expect(list).toHaveAttribute('aria-label', heading.textContent)
   })
+
+  it('should render ActionList.Item as button when feature flag is enabled', async () => {
+    const featureFlag = {
+      primer_react_action_list_item_as_button: true,
+    }
+
+    const {container} = HTMLRender(
+      <FeatureFlags flags={featureFlag}>
+        <ActionList>
+          <ActionList.Item disabled={true}>Item 1</ActionList.Item>
+          <ActionList.Item>Item 2</ActionList.Item>
+        </ActionList>
+      </FeatureFlags>,
+    )
+
+    const button = container.querySelector('button')
+    expect(button).toHaveTextContent('Item 1')
+
+    // Ensure passed prop "disabled" is applied to the button
+    expect(button).toHaveAttribute('aria-disabled', 'true')
+
+    const listItems = container.querySelectorAll('li')
+    expect(listItems.length).toBe(2)
+  })
+
+  it('should render ActionList.Item as li when feature flag is disabled', async () => {
+    const {container} = HTMLRender(
+      <FeatureFlags flags={{primer_react_action_list_item_as_button: false}}>
+        <ActionList>
+          <ActionList.Item>Item 1</ActionList.Item>
+          <ActionList.Item>Item 2</ActionList.Item>
+        </ActionList>
+      </FeatureFlags>,
+    )
+
+    const listitem = container.querySelector('li')
+    const button = container.querySelector('button')
+
+    expect(listitem).toHaveTextContent('Item 1')
+    expect(listitem).toHaveAttribute('tabindex', '0')
+    expect(button).toBeNull()
+
+    const listItems = container.querySelectorAll('li')
+    expect(listItems.length).toBe(2)
+  })
+
+  it('should render ActionList.Item as li when feature flag is enabled and has proper aria role', async () => {
+    const {container} = HTMLRender(
+      <FeatureFlags flags={{primer_react_action_list_item_as_button: false}}>
+        <ActionList role="listbox">
+          <ActionList.Item role="option">Item 1</ActionList.Item>
+          <ActionList.Item role="option">Item 2</ActionList.Item>
+        </ActionList>
+      </FeatureFlags>,
+    )
+
+    const listitem = container.querySelector('li')
+    const button = container.querySelector('button')
+
+    expect(listitem).toHaveTextContent('Item 1')
+    expect(listitem).toHaveAttribute('tabindex', '0')
+    expect(button).toBeNull()
+
+    const listItems = container.querySelectorAll('li')
+    expect(listItems.length).toBe(2)
+  })
 })
diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx
index 517f188b0ba..720343000a3 100644
--- a/packages/react/src/ActionList/Item.tsx
+++ b/packages/react/src/ActionList/Item.tsx
@@ -21,6 +21,7 @@ import {getVariantStyles, ItemContext, TEXT_ROW_HEIGHT, ListContext} from './sha
 import type {VisualProps} from './Visuals'
 import {LeadingVisual, TrailingVisual} from './Visuals'
 import {ConditionalWrapper} from '../internal/components/ConditionalWrapper'
+import {useFeatureFlag} from '../FeatureFlags'
 
 const LiBox = styled.li<SxProp>(sx)
 
@@ -77,6 +78,8 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
     const {container, afterSelect, selectionAttribute, defaultTrailingVisual} =
       React.useContext(ActionListContainerContext)
 
+    const buttonSemantics = useFeatureFlag('primer_react_action_list_item_as_button')
+
     // Be sure to avoid rendering the container unless there is a default
     const wrappedDefaultTrailingVisual = defaultTrailingVisual ? (
       <TrailingVisual>{defaultTrailingVisual}</TrailingVisual>
@@ -95,7 +98,7 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
 
     const onSelect = React.useCallback(
       (
-        event: React.MouseEvent<HTMLLIElement> | React.KeyboardEvent<HTMLLIElement>,
+        event: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>,
         // eslint-disable-next-line @typescript-eslint/ban-types
         afterSelect?: Function,
       ) => {
@@ -146,6 +149,12 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
       },
     }
 
+    const listItemStyles = {
+      display: 'flex',
+      // show between 2 items
+      ':not(:first-of-type)': {'--divider-color': theme?.colors.actionListItem.inlineDivider},
+    }
+
     const styles = {
       position: 'relative',
       display: 'flex',
@@ -231,7 +240,7 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
     }
 
     const clickHandler = React.useCallback(
-      (event: React.MouseEvent<HTMLLIElement>) => {
+      (event: React.MouseEvent<HTMLElement>) => {
         if (disabled || inactive) return
         onSelect(event, afterSelect)
       },
@@ -239,7 +248,7 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
     )
 
     const keyPressHandler = React.useCallback(
-      (event: React.KeyboardEvent<HTMLLIElement>) => {
+      (event: React.KeyboardEvent<HTMLElement>) => {
         if (disabled || inactive) return
         if ([' ', 'Enter'].includes(event.key)) {
           if (event.key === ' ') {
@@ -259,8 +268,28 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
     const inlineDescriptionId = `${itemId}--inline-description`
     const blockDescriptionId = `${itemId}--block-description`
     const inactiveWarningId = inactive && !showInactiveIndicator ? `${itemId}--warning-message` : undefined
+    // 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 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 (buttonSemantics) {
+      DefaultItemWrapper = listSemantics ? React.Fragment : ButtonItemWrapper
+    }
 
-    const ItemWrapper = _PrivateItemWrapper || React.Fragment
+    const ItemWrapper = _PrivateItemWrapper || DefaultItemWrapper
 
     // only apply aria-selected and aria-checked to selectable items
     const selectableRoles = ['menuitemradio', 'menuitemcheckbox', 'option']
@@ -281,20 +310,44 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
       id: itemId,
     }
 
-    const containerProps = _PrivateItemWrapper ? {role: itemRole ? 'none' : undefined} : menuItemProps
+    let containerProps
+    let wrapperProps
 
-    const wrapperProps = _PrivateItemWrapper ? menuItemProps : {}
+    if (buttonSemantics) {
+      containerProps = _PrivateItemWrapper
+        ? {role: itemRole ? 'none' : undefined, ...props}
+        : // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
+          (listSemantics && {...menuItemProps, ...props, ref: forwardedRef}) || {}
+
+      wrapperProps = _PrivateItemWrapper
+        ? menuItemProps
+        : !listSemantics && {
+            ...menuItemProps,
+            ...props,
+            styles: merge<BetterSystemStyleObject>(styles, sxProp),
+            ref: forwardedRef,
+          }
+    } else {
+      containerProps = _PrivateItemWrapper ? {role: itemRole ? 'none' : undefined} : {...menuItemProps, ...props}
+      wrapperProps = _PrivateItemWrapper ? menuItemProps : {}
+    }
 
     return (
       <ItemContext.Provider
         value={{variant, disabled, inactive: Boolean(inactiveText), inlineDescriptionId, blockDescriptionId}}
       >
         <LiBox
-          ref={forwardedRef}
-          sx={merge<BetterSystemStyleObject>(styles, sxProp)}
+          ref={buttonSemantics || listSemantics ? forwardedRef : null}
+          sx={
+            buttonSemantics
+              ? merge<BetterSystemStyleObject>(
+                  listSemantics || _PrivateItemWrapper ? styles : listItemStyles,
+                  listSemantics || _PrivateItemWrapper ? sxProp : {},
+                )
+              : merge<BetterSystemStyleObject>(styles, sxProp)
+          }
           data-variant={variant === 'danger' ? variant : undefined}
           {...containerProps}
-          {...props}
         >
           <ItemWrapper {...wrapperProps}>
             <Selection selected={selected} />
diff --git a/packages/react/src/ActionList/LinkItem.tsx b/packages/react/src/ActionList/LinkItem.tsx
index 93db8e6e725..215cbaac74a 100644
--- a/packages/react/src/ActionList/LinkItem.tsx
+++ b/packages/react/src/ActionList/LinkItem.tsx
@@ -45,7 +45,7 @@ export const LinkItem = React.forwardRef(({sx = {}, active, inactiveText, as: Co
       inactiveText={inactiveText}
       data-inactive={inactiveText ? true : undefined}
       _PrivateItemWrapper={({children, onClick, ...rest}) => {
-        const clickHandler = (event: React.MouseEvent) => {
+        const clickHandler = (event: React.MouseEvent<HTMLElement>) => {
           onClick && onClick(event)
           props.onClick && props.onClick(event as React.MouseEvent<HTMLAnchorElement>)
         }
diff --git a/packages/react/src/ActionList/shared.ts b/packages/react/src/ActionList/shared.ts
index 85d291bbb36..44fe91b3a84 100644
--- a/packages/react/src/ActionList/shared.ts
+++ b/packages/react/src/ActionList/shared.ts
@@ -11,7 +11,7 @@ export type ActionListItemProps = {
    * Callback that will trigger both on click selection and keyboard selection.
    * This is not called for disabled or inactive items.
    */
-  onSelect?: (event: React.MouseEvent<HTMLLIElement> | React.KeyboardEvent<HTMLLIElement>) => void
+  onSelect?: (event: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>) => void
   /**
    * Is the `Item` is currently selected?
    */
@@ -51,8 +51,8 @@ export type ActionListItemProps = {
 } & SxProp
 
 type MenuItemProps = {
-  onClick?: (event: React.MouseEvent) => void
-  onKeyPress?: (event: React.KeyboardEvent) => void
+  onClick?: (event: React.MouseEvent<HTMLElement>) => void
+  onKeyPress?: (event: React.KeyboardEvent<HTMLElement>) => void
   'aria-disabled'?: boolean
   tabIndex?: number
   'aria-labelledby'?: string
diff --git a/packages/react/src/ActionMenu/ActionMenu.examples.stories.tsx b/packages/react/src/ActionMenu/ActionMenu.examples.stories.tsx
index 1a4048f6017..a5a77a18190 100644
--- a/packages/react/src/ActionMenu/ActionMenu.examples.stories.tsx
+++ b/packages/react/src/ActionMenu/ActionMenu.examples.stories.tsx
@@ -391,7 +391,7 @@ export const MultipleSections = () => {
 export const DelayedMenuClose = () => {
   const [open, setOpen] = React.useState(false)
   const [copyLinkSuccess, setCopyLinkSuccess] = React.useState(false)
-  const onSelect = (event: React.MouseEvent<HTMLLIElement> | React.KeyboardEvent<HTMLLIElement>) => {
+  const onSelect = (event: React.MouseEvent | React.KeyboardEvent) => {
     event.preventDefault()
 
     setCopyLinkSuccess(true)
diff --git a/packages/react/src/NavList/NavList.tsx b/packages/react/src/NavList/NavList.tsx
index 91a35b467e4..908322fb475 100644
--- a/packages/react/src/NavList/NavList.tsx
+++ b/packages/react/src/NavList/NavList.tsx
@@ -4,6 +4,7 @@ import React, {isValidElement} from 'react'
 import styled from 'styled-components'
 import type {ActionListDividerProps, ActionListLeadingVisualProps, ActionListTrailingVisualProps} from '../ActionList'
 import {ActionList} from '../ActionList'
+import {ActionListContainerContext} from '../ActionList/ActionListContainerContext'
 import Box from '../Box'
 import Octicon from '../Octicon'
 import type {SxProp} from '../sx'
@@ -33,7 +34,13 @@ const NavBox = styled.nav<SxProp>(sx)
 const Root = React.forwardRef<HTMLElement, NavListProps>(({children, ...props}, ref) => {
   return (
     <NavBox {...props} ref={ref}>
-      <ActionList>{children}</ActionList>
+      <ActionListContainerContext.Provider
+        value={{
+          container: 'NavList',
+        }}
+      >
+        <ActionList>{children}</ActionList>
+      </ActionListContainerContext.Provider>
     </NavBox>
   )
 })
diff --git a/packages/react/src/SegmentedControl/SegmentedControl.tsx b/packages/react/src/SegmentedControl/SegmentedControl.tsx
index 548cc73e65d..51d00cf267f 100644
--- a/packages/react/src/SegmentedControl/SegmentedControl.tsx
+++ b/packages/react/src/SegmentedControl/SegmentedControl.tsx
@@ -129,7 +129,7 @@ const Root: React.FC<React.PropsWithChildren<SegmentedControlProps>> = ({
                 <ActionList.Item
                   key={`segmented-control-action-btn-${index}`}
                   selected={index === selectedIndex}
-                  onSelect={(event: React.MouseEvent<HTMLLIElement> | React.KeyboardEvent<HTMLLIElement>) => {
+                  onSelect={(event: React.MouseEvent | React.KeyboardEvent) => {
                     isUncontrolled && setSelectedIndexInternalState(index)
                     onChange && onChange(index)
                     child.props.onClick && child.props.onClick(event as React.MouseEvent<HTMLLIElement>)
diff --git a/packages/react/src/drafts/InlineAutocomplete/_AutocompleteSuggestions.tsx b/packages/react/src/drafts/InlineAutocomplete/_AutocompleteSuggestions.tsx
index 9f335e83db1..7cd38a1552f 100644
--- a/packages/react/src/drafts/InlineAutocomplete/_AutocompleteSuggestions.tsx
+++ b/packages/react/src/drafts/InlineAutocomplete/_AutocompleteSuggestions.tsx
@@ -133,7 +133,7 @@ const AutocompleteSuggestions = ({
       left={triggerCharCoords.left}
       ref={overlayRef}
     >
-      <ActionList ref={setList}>
+      <ActionList ref={setList} role="listbox">
         {suggestions === 'loading' ? (
           <LoadingIndicator />
         ) : (