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](changesets/action@f13b1ba...e2f8e96) --- 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](indutny/node-ip@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>
primer · Jun 18, 2024 · 3c467ef · 3c467ef
1 parent e2f35e2
commit 3c467ef
Show file tree

Hide file tree

Showing 11 changed files with 177 additions and 18 deletions.
diff --git a/.changeset/hot-mayflies-pump.md 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.
diff --git 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="https://github.com/primer/react">Primer documentation</a> : ''}
-            <Story {...context} />
+            <FeatureFlags flags={{primer_react_action_list_item_as_button: true}}>
+              <Story {...context} />
+            </FeatureFlags>
             {showSurroundingElements ? <a href="https://github.com/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
@@ -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="https://github.com/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
@@ -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
@@ -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,15 +240,15 @@ 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)
       },
       [onSelect, disabled, inactive, afterSelect],
     )
 
     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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -133,7 +133,7 @@ const AutocompleteSuggestions = ({
       left={triggerCharCoords.left}
       ref={overlayRef}
     >
-      <ActionList ref={setList}>
+      <ActionList ref={setList} role="listbox">
         {suggestions === 'loading' ? (
           <LoadingIndicator />
         ) : (