ActionList.Item: Check defaultPrevented between onSelect & afterSelect

.changeset/actionlist-check-event-after-onselect.md

@@ -0,0 +1,5 @@
+---
+"@primer/react": minor
+---
+
+ActionMenu: Calling `event.preventDefault` inside `onSelect` of `ActionList.Item` will prevent the default behavior of closing the menu

src/ActionList/Item.tsx

@@ -146,30 +146,29 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
       ...(active ? activeStyles : {}),
     }
 
-    const clickHandler = React.useCallback(
-      (event: React.MouseEvent<HTMLLIElement>) => {
+    const internalOnSelect = React.useCallback(
+      (event: React.MouseEvent<HTMLLIElement> | React.KeyboardEvent<HTMLLIElement>) => {
         if (disabled) return
-        if (!event.defaultPrevented) {
-          if (typeof onSelect === 'function') onSelect(event)
-          // if this Item is inside a Menu, close the Menu
-          if (typeof afterSelect === 'function') afterSelect()
-        }
-      },
-      [onSelect, disabled, afterSelect],
-    )
+        if (event.defaultPrevented) return
 
-    const keyPressHandler = React.useCallback(
-      (event: React.KeyboardEvent<HTMLLIElement>) => {
-        if (disabled) return
-        if (!event.defaultPrevented && [' ', 'Enter'].includes(event.key)) {
-          if (typeof onSelect === 'function') onSelect(event)
-          // if this Item is inside a Menu, close the Menu
-          if (typeof afterSelect === 'function') afterSelect()
-        }
+        if (typeof onSelect === 'function') onSelect(event)
+
+        // typescript does not know onSelect can call event.preventDefault, so this check is valid
+        // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
+        if (event.defaultPrevented) return
+
+        // if this Item is inside a Menu, close the Menu
+        if (typeof afterSelect === 'function') afterSelect()
       },
-      [onSelect, disabled, afterSelect],
+      [disabled, onSelect, afterSelect],
     )
 
+    const clickHandler = internalOnSelect
+
+    const keyPressHandler = (event: React.KeyboardEvent<HTMLLIElement>) => {
+      if ([' ', 'Enter'].includes(event.key)) internalOnSelect(event)
+    }
+
     // use props.id if provided, otherwise generate one.
     const labelId = useId(id)
     const inlineDescriptionId = useId(id && `${id}--inline-description`)

src/ActionMenu/ActionMenu.examples.stories.tsx

@@ -1,5 +1,5 @@
 import React from 'react'
-import {Box, ActionMenu, ActionList, Button, IconButton} from '../'
+import {Box, ActionMenu, ActionList, Button, IconButton, ActionListItemProps} from '../'
 import {
   GearIcon,
   MilestoneIcon,
@@ -11,6 +11,8 @@ import {
   NumberIcon,
   CalendarIcon,
   XIcon,
+  CheckIcon,
+  CopyIcon,
 } from '@primer/octicons-react'
 
 export default {
@@ -282,3 +284,36 @@ export const MultipleSections = () => {
     </ActionMenu>
   )
 }
+
+export const DelayedMenuClose = () => {
+  const [open, setOpen] = React.useState(false)
+  const [copied, setCopied] = React.useState(false)
+  const onSelect: ActionListItemProps['onSelect'] = event => {
+    // prevent default behavior of closing menu
+    event.preventDefault()
+
+    setCopied(true)
+    setTimeout(() => {
+      setOpen(false)
+      setCopied(false)
+    }, 1000)
+  }
+
+  return (
+    <>
+      <h1>Delayed Menu Close</h1>
+
+      <ActionMenu open={open} onOpenChange={setOpen}>
+        <ActionMenu.Button>Anchor</ActionMenu.Button>
+        <ActionMenu.Overlay>
+          <ActionList>
+            <ActionList.Item onSelect={onSelect}>
+              <ActionList.LeadingVisual>{copied ? <CheckIcon /> : <CopyIcon />}</ActionList.LeadingVisual>
+              {copied ? 'Copied!' : 'Copy link'}
+            </ActionList.Item>
+          </ActionList>
+        </ActionMenu.Overlay>
+      </ActionMenu>
+    </>
+  )
+}

src/__tests__/ActionMenu.test.tsx

@@ -22,7 +22,7 @@ function Example(): JSX.Element {
                 <ActionList.Divider />
                 <ActionList.Item>Copy link</ActionList.Item>
                 <ActionList.Item>Edit file</ActionList.Item>
-                <ActionList.Item variant="danger" onClick={event => event.preventDefault()}>
+                <ActionList.Item variant="danger" onSelect={event => event.preventDefault()}>
                   Delete file
                 </ActionList.Item>
                 <ActionList.LinkItem href="//github.com" title="anchor" aria-keyshortcuts="s">