From c6931d20ea37888f0416429d068cc495d6cb804d Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Fri, 13 Sep 2024 11:52:37 +0200 Subject: [PATCH] ActionMenu: Make sure event handlers on trigger are called (#4648) * wip: add mergeAnchorHandlers * merged onClick and onKeyDown in ActionMenu.Anchor * improve types * Create green-schools-smell.md * cover cases with Tooltip --- .changeset/green-schools-smell.md | 5 + packages/react/src/ActionMenu/ActionMenu.tsx | 60 ++++++- .../react/src/__tests__/ActionMenu.test.tsx | 168 ++++++++++++++++++ 3 files changed, 224 insertions(+), 9 deletions(-) create mode 100644 .changeset/green-schools-smell.md diff --git a/.changeset/green-schools-smell.md b/.changeset/green-schools-smell.md new file mode 100644 index 00000000000..aa283a70445 --- /dev/null +++ b/.changeset/green-schools-smell.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +ActionMenu: Make sure event handlers on ActionMenu.Button and ActionMenu.Anchor are called diff --git a/packages/react/src/ActionMenu/ActionMenu.tsx b/packages/react/src/ActionMenu/ActionMenu.tsx index a410257614b..376dce0ac1a 100644 --- a/packages/react/src/ActionMenu/ActionMenu.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.tsx @@ -43,6 +43,31 @@ export type ActionMenuProps = { onOpenChange?: (s: boolean) => void } & Pick +// anchorProps adds onClick and onKeyDown, so we need to merge them with buttonProps +const mergeAnchorHandlers = (anchorProps: React.HTMLAttributes, buttonProps: ButtonProps) => { + const mergedAnchorProps = {...anchorProps} + + if (typeof buttonProps.onClick === 'function') { + const anchorOnClick = anchorProps.onClick + const mergedOnClick = (event: React.MouseEvent) => { + buttonProps.onClick?.(event) + anchorOnClick?.(event) + } + mergedAnchorProps.onClick = mergedOnClick + } + + if (typeof buttonProps.onKeyDown === 'function') { + const anchorOnKeyDown = anchorProps.onKeyDown + const mergedOnAnchorKeyDown = (event: React.KeyboardEvent) => { + buttonProps.onKeyDown?.(event) + anchorOnKeyDown?.(event) + } + mergedAnchorProps.onKeyDown = mergedOnAnchorKeyDown + } + + return mergedAnchorProps +} + const Menu: React.FC> = ({ anchorRef: externalAnchorRef, open, @@ -87,7 +112,10 @@ const Menu: React.FC> = ({ if (anchorChildren.type === MenuButton) { renderAnchor = anchorProps => { // We need to attach the anchor props to the tooltip trigger (ActionMenu.Button's grandchild) not the tooltip itself. - const triggerButton = React.cloneElement(anchorChildren, {...anchorProps}) + const triggerButton = React.cloneElement( + anchorChildren, + mergeAnchorHandlers({...anchorProps}, anchorChildren.props), + ) return React.cloneElement(child, {children: triggerButton, ref: anchorRef}) } } @@ -101,7 +129,10 @@ const Menu: React.FC> = ({ // ActionMenu.Anchor's children can be wrapped with Tooltip. If this is the case, our anchor is the tooltip's trigger const tooltipTrigger = anchorChildren.props.children // We need to attach the anchor props to the tooltip trigger not the tooltip itself. - const tooltipTriggerEl = React.cloneElement(tooltipTrigger, {...anchorProps}) + const tooltipTriggerEl = React.cloneElement( + tooltipTrigger, + mergeAnchorHandlers({...anchorProps}, tooltipTrigger.props), + ) const tooltip = React.cloneElement(anchorChildren, {children: tooltipTriggerEl}) return React.cloneElement(child, {children: tooltip, ref: anchorRef}) } @@ -111,7 +142,7 @@ const Menu: React.FC> = ({ } return null } else if (child.type === MenuButton) { - renderAnchor = anchorProps => React.cloneElement(child, anchorProps) + renderAnchor = anchorProps => React.cloneElement(child, mergeAnchorHandlers(anchorProps, child.props)) return null } else { return child @@ -136,18 +167,28 @@ const Menu: React.FC> = ({ ) } -export type ActionMenuAnchorProps = {children: React.ReactElement; id?: string} -const Anchor = React.forwardRef(({children, ...anchorProps}, anchorRef) => { +export type ActionMenuAnchorProps = {children: React.ReactElement; id?: string} & React.HTMLAttributes +const Anchor = React.forwardRef(({children: child, ...anchorProps}, anchorRef) => { const {onOpen, isSubmenu} = React.useContext(MenuContext) const openSubmenuOnRightArrow: React.KeyboardEventHandler = useCallback( event => { - children.props.onKeyDown?.(event) if (isSubmenu && event.key === 'ArrowRight' && !event.defaultPrevented) onOpen?.('anchor-key-press') }, - [children, isSubmenu, onOpen], + [isSubmenu, onOpen], ) + const onButtonClick = (event: React.MouseEvent) => { + child.props.onClick?.(event) + anchorProps.onClick?.(event) // onClick is passed from AnchoredOverlay + } + + const onButtonKeyDown = (event: React.KeyboardEvent) => { + child.props.onKeyDown?.(event) + openSubmenuOnRightArrow(event) + anchorProps.onKeyDown?.(event) // onKeyDown is passed from AnchoredOverlay + } + // Add right chevron icon to submenu anchors rendered using `ActionList.Item` const parentActionListContext = useContext(ActionListContainerContext) const thisActionListContext = useMemo( @@ -165,10 +206,11 @@ const Anchor = React.forwardRef(({children, return ( - {React.cloneElement(children, { + {React.cloneElement(child, { ...anchorProps, ref: anchorRef, - onKeyDown: openSubmenuOnRightArrow, + onClick: onButtonClick, + onKeyDown: onButtonKeyDown, })} ) diff --git a/packages/react/src/__tests__/ActionMenu.test.tsx b/packages/react/src/__tests__/ActionMenu.test.tsx index 05585b74a10..4b067fda8be 100644 --- a/packages/react/src/__tests__/ActionMenu.test.tsx +++ b/packages/react/src/__tests__/ActionMenu.test.tsx @@ -641,4 +641,172 @@ describe('ActionMenu', () => { expect(baseAnchor).not.toHaveAttribute('aria-expanded', 'true') }) }) + + describe('calls event handlers on trigger', () => { + it('should call onClick and onKeyDown passed to ActionMenu.Button', async () => { + const mockOnClick = jest.fn() + const mockOnKeyDown = jest.fn() + + const component = HTMLRender( + + + + Open menu + + + + New file + Copy link + + + + , + ) + + const user = userEvent.setup() + const button = component.getByRole('button') + await user.click(button) + + expect(component.getByRole('menu')).toBeInTheDocument() + expect(mockOnClick).toHaveBeenCalledTimes(1) + + // select and close menu + const menuItems = component.getAllByRole('menuitem') + await user.click(menuItems[0]) + expect(component.queryByRole('menu')).toBeNull() + + expect(button).toEqual(document.activeElement) + await user.keyboard('{Enter}') + expect(component.queryByRole('menu')).toBeInTheDocument() + expect(mockOnKeyDown).toHaveBeenCalledTimes(1) + }) + + it('should call onClick and onKeyDown passed to IconButton inside ActionMenu.Anchor', async () => { + const mockOnClick = jest.fn() + const mockOnKeyDown = jest.fn() + + const component = HTMLRender( + + + + + + + + New file + Copy link + + + + , + ) + + const user = userEvent.setup() + const button = component.getByRole('button') + await user.click(button) + + expect(component.getByRole('menu')).toBeInTheDocument() + expect(mockOnClick).toHaveBeenCalledTimes(1) + + // select and close menu + const menuItems = component.getAllByRole('menuitem') + await user.click(menuItems[0]) + expect(component.queryByRole('menu')).toBeNull() + + expect(button).toEqual(document.activeElement) + await user.keyboard('{Enter}') + expect(component.queryByRole('menu')).toBeInTheDocument() + expect(mockOnKeyDown).toHaveBeenCalledTimes(1) + }) + + it('should call onClick and onKeyDown passed to ActionMenu.Button with Tooltip', async () => { + const mockOnClick = jest.fn() + const mockOnKeyDown = jest.fn() + + const component = HTMLRender( + + + + + Open menu + + + + + New file + Copy link + + + + , + ) + + const user = userEvent.setup() + const button = component.getByRole('button') + await user.click(button) + + expect(component.getByRole('menu')).toBeInTheDocument() + expect(mockOnClick).toHaveBeenCalledTimes(1) + + // select and close menu + const menuItems = component.getAllByRole('menuitem') + await user.click(menuItems[0]) + expect(component.queryByRole('menu')).toBeNull() + + expect(button).toEqual(document.activeElement) + await user.keyboard('{Enter}') + expect(component.queryByRole('menu')).toBeInTheDocument() + expect(mockOnKeyDown).toHaveBeenCalledTimes(1) + }) + + it('should call onClick and onKeyDown passed to IconButton inside ActionMenu.Anchor with Tooltip', async () => { + const mockOnClick = jest.fn() + const mockOnKeyDown = jest.fn() + + const component = HTMLRender( + + + + + + + + + + New file + Copy link + + + + , + ) + + const user = userEvent.setup() + const button = component.getByRole('button') + await user.click(button) + + expect(component.getByRole('menu')).toBeInTheDocument() + expect(mockOnClick).toHaveBeenCalledTimes(1) + + // select and close menu + const menuItems = component.getAllByRole('menuitem') + await user.click(menuItems[0]) + expect(component.queryByRole('menu')).toBeNull() + + expect(button).toEqual(document.activeElement) + await user.keyboard('{Enter}') + expect(component.queryByRole('menu')).toBeInTheDocument() + expect(mockOnKeyDown).toHaveBeenCalledTimes(1) + }) + }) })