Skip to content

Commit 78c03fa

Browse files
siddharthkpTylerJDev
siddharthkp
authored and
TylerJDev
committed
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
1 parent 08ed6ca commit 78c03fa

File tree

3 files changed

+224
-9
lines changed

3 files changed

+224
-9
lines changed

‎.changeset/green-schools-smell.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@primer/react": patch
3+
---
4+
5+
ActionMenu: Make sure event handlers on ActionMenu.Button and ActionMenu.Anchor are called

‎packages/react/src/ActionMenu/ActionMenu.tsx‎

Lines changed: 51 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,31 @@ export type ActionMenuProps = {
4343
onOpenChange?: (s: boolean) => void
4444
} & Pick<AnchoredOverlayProps, 'anchorRef'>
4545

46+
// anchorProps adds onClick and onKeyDown, so we need to merge them with buttonProps
47+
const mergeAnchorHandlers = (anchorProps: React.HTMLAttributes<HTMLElement>, buttonProps: ButtonProps) => {
48+
const mergedAnchorProps = {...anchorProps}
49+
50+
if (typeof buttonProps.onClick === 'function') {
51+
const anchorOnClick = anchorProps.onClick
52+
const mergedOnClick = (event: React.MouseEvent<HTMLButtonElement>) => {
53+
buttonProps.onClick?.(event)
54+
anchorOnClick?.(event)
55+
}
56+
mergedAnchorProps.onClick = mergedOnClick
57+
}
58+
59+
if (typeof buttonProps.onKeyDown === 'function') {
60+
const anchorOnKeyDown = anchorProps.onKeyDown
61+
const mergedOnAnchorKeyDown = (event: React.KeyboardEvent<HTMLButtonElement>) => {
62+
buttonProps.onKeyDown?.(event)
63+
anchorOnKeyDown?.(event)
64+
}
65+
mergedAnchorProps.onKeyDown = mergedOnAnchorKeyDown
66+
}
67+
68+
return mergedAnchorProps
69+
}
70+
4671
const Menu: React.FC<React.PropsWithChildren<ActionMenuProps>> = ({
4772
anchorRef: externalAnchorRef,
4873
open,
@@ -87,7 +112,10 @@ const Menu: React.FC<React.PropsWithChildren<ActionMenuProps>> = ({
87112
if (anchorChildren.type === MenuButton) {
88113
renderAnchor = anchorProps => {
89114
// We need to attach the anchor props to the tooltip trigger (ActionMenu.Button's grandchild) not the tooltip itself.
90-
const triggerButton = React.cloneElement(anchorChildren, {...anchorProps})
115+
const triggerButton = React.cloneElement(
116+
anchorChildren,
117+
mergeAnchorHandlers({...anchorProps}, anchorChildren.props),
118+
)
91119
return React.cloneElement(child, {children: triggerButton, ref: anchorRef})
92120
}
93121
}
@@ -101,7 +129,10 @@ const Menu: React.FC<React.PropsWithChildren<ActionMenuProps>> = ({
101129
// ActionMenu.Anchor's children can be wrapped with Tooltip. If this is the case, our anchor is the tooltip's trigger
102130
const tooltipTrigger = anchorChildren.props.children
103131
// We need to attach the anchor props to the tooltip trigger not the tooltip itself.
104-
const tooltipTriggerEl = React.cloneElement(tooltipTrigger, {...anchorProps})
132+
const tooltipTriggerEl = React.cloneElement(
133+
tooltipTrigger,
134+
mergeAnchorHandlers({...anchorProps}, tooltipTrigger.props),
135+
)
105136
const tooltip = React.cloneElement(anchorChildren, {children: tooltipTriggerEl})
106137
return React.cloneElement(child, {children: tooltip, ref: anchorRef})
107138
}
@@ -111,7 +142,7 @@ const Menu: React.FC<React.PropsWithChildren<ActionMenuProps>> = ({
111142
}
112143
return null
113144
} else if (child.type === MenuButton) {
114-
renderAnchor = anchorProps => React.cloneElement(child, anchorProps)
145+
renderAnchor = anchorProps => React.cloneElement(child, mergeAnchorHandlers(anchorProps, child.props))
115146
return null
116147
} else {
117148
return child
@@ -136,18 +167,28 @@ const Menu: React.FC<React.PropsWithChildren<ActionMenuProps>> = ({
136167
)
137168
}
138169

139-
export type ActionMenuAnchorProps = {children: React.ReactElement; id?: string}
140-
const Anchor = React.forwardRef<HTMLElement, ActionMenuAnchorProps>(({children, ...anchorProps}, anchorRef) => {
170+
export type ActionMenuAnchorProps = {children: React.ReactElement; id?: string} & React.HTMLAttributes<HTMLElement>
171+
const Anchor = React.forwardRef<HTMLElement, ActionMenuAnchorProps>(({children: child, ...anchorProps}, anchorRef) => {
141172
const {onOpen, isSubmenu} = React.useContext(MenuContext)
142173

143174
const openSubmenuOnRightArrow: React.KeyboardEventHandler<HTMLElement> = useCallback(
144175
event => {
145-
children.props.onKeyDown?.(event)
146176
if (isSubmenu && event.key === 'ArrowRight' && !event.defaultPrevented) onOpen?.('anchor-key-press')
147177
},
148-
[children, isSubmenu, onOpen],
178+
[isSubmenu, onOpen],
149179
)
150180

181+
const onButtonClick = (event: React.MouseEvent<HTMLElement>) => {
182+
child.props.onClick?.(event)
183+
anchorProps.onClick?.(event) // onClick is passed from AnchoredOverlay
184+
}
185+
186+
const onButtonKeyDown = (event: React.KeyboardEvent<HTMLElement>) => {
187+
child.props.onKeyDown?.(event)
188+
openSubmenuOnRightArrow(event)
189+
anchorProps.onKeyDown?.(event) // onKeyDown is passed from AnchoredOverlay
190+
}
191+
151192
// Add right chevron icon to submenu anchors rendered using `ActionList.Item`
152193
const parentActionListContext = useContext(ActionListContainerContext)
153194
const thisActionListContext = useMemo(
@@ -165,10 +206,11 @@ const Anchor = React.forwardRef<HTMLElement, ActionMenuAnchorProps>(({children,
165206

166207
return (
167208
<ActionListContainerContext.Provider value={thisActionListContext}>
168-
{React.cloneElement(children, {
209+
{React.cloneElement(child, {
169210
...anchorProps,
170211
ref: anchorRef,
171-
onKeyDown: openSubmenuOnRightArrow,
212+
onClick: onButtonClick,
213+
onKeyDown: onButtonKeyDown,
172214
})}
173215
</ActionListContainerContext.Provider>
174216
)

‎packages/react/src/__tests__/ActionMenu.test.tsx‎

Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -641,4 +641,172 @@ describe('ActionMenu', () => {
641641
expect(baseAnchor).not.toHaveAttribute('aria-expanded', 'true')
642642
})
643643
})
644+
645+
describe('calls event handlers on trigger', () => {
646+
it('should call onClick and onKeyDown passed to ActionMenu.Button', async () => {
647+
const mockOnClick = jest.fn()
648+
const mockOnKeyDown = jest.fn()
649+
650+
const component = HTMLRender(
651+
<ThemeProvider theme={theme}>
652+
<ActionMenu>
653+
<ActionMenu.Button onClick={mockOnClick} onKeyDown={mockOnKeyDown}>
654+
Open menu
655+
</ActionMenu.Button>
656+
<ActionMenu.Overlay>
657+
<ActionList>
658+
<ActionList.Item>New file</ActionList.Item>
659+
<ActionList.Item>Copy link</ActionList.Item>
660+
</ActionList>
661+
</ActionMenu.Overlay>
662+
</ActionMenu>
663+
</ThemeProvider>,
664+
)
665+
666+
const user = userEvent.setup()
667+
const button = component.getByRole('button')
668+
await user.click(button)
669+
670+
expect(component.getByRole('menu')).toBeInTheDocument()
671+
expect(mockOnClick).toHaveBeenCalledTimes(1)
672+
673+
// select and close menu
674+
const menuItems = component.getAllByRole('menuitem')
675+
await user.click(menuItems[0])
676+
expect(component.queryByRole('menu')).toBeNull()
677+
678+
expect(button).toEqual(document.activeElement)
679+
await user.keyboard('{Enter}')
680+
expect(component.queryByRole('menu')).toBeInTheDocument()
681+
expect(mockOnKeyDown).toHaveBeenCalledTimes(1)
682+
})
683+
684+
it('should call onClick and onKeyDown passed to IconButton inside ActionMenu.Anchor', async () => {
685+
const mockOnClick = jest.fn()
686+
const mockOnKeyDown = jest.fn()
687+
688+
const component = HTMLRender(
689+
<ThemeProvider theme={theme}>
690+
<ActionMenu>
691+
<ActionMenu.Anchor>
692+
<IconButton
693+
icon={KebabHorizontalIcon}
694+
aria-label="Open menu"
695+
onClick={mockOnClick}
696+
onKeyDown={mockOnKeyDown}
697+
/>
698+
</ActionMenu.Anchor>
699+
<ActionMenu.Overlay>
700+
<ActionList>
701+
<ActionList.Item>New file</ActionList.Item>
702+
<ActionList.Item>Copy link</ActionList.Item>
703+
</ActionList>
704+
</ActionMenu.Overlay>
705+
</ActionMenu>
706+
</ThemeProvider>,
707+
)
708+
709+
const user = userEvent.setup()
710+
const button = component.getByRole('button')
711+
await user.click(button)
712+
713+
expect(component.getByRole('menu')).toBeInTheDocument()
714+
expect(mockOnClick).toHaveBeenCalledTimes(1)
715+
716+
// select and close menu
717+
const menuItems = component.getAllByRole('menuitem')
718+
await user.click(menuItems[0])
719+
expect(component.queryByRole('menu')).toBeNull()
720+
721+
expect(button).toEqual(document.activeElement)
722+
await user.keyboard('{Enter}')
723+
expect(component.queryByRole('menu')).toBeInTheDocument()
724+
expect(mockOnKeyDown).toHaveBeenCalledTimes(1)
725+
})
726+
727+
it('should call onClick and onKeyDown passed to ActionMenu.Button with Tooltip', async () => {
728+
const mockOnClick = jest.fn()
729+
const mockOnKeyDown = jest.fn()
730+
731+
const component = HTMLRender(
732+
<ThemeProvider theme={theme}>
733+
<ActionMenu>
734+
<TooltipV2 text="Additional context about the menu button" direction="s">
735+
<ActionMenu.Button onClick={mockOnClick} onKeyDown={mockOnKeyDown}>
736+
Open menu
737+
</ActionMenu.Button>
738+
</TooltipV2>
739+
<ActionMenu.Overlay>
740+
<ActionList>
741+
<ActionList.Item>New file</ActionList.Item>
742+
<ActionList.Item>Copy link</ActionList.Item>
743+
</ActionList>
744+
</ActionMenu.Overlay>
745+
</ActionMenu>
746+
</ThemeProvider>,
747+
)
748+
749+
const user = userEvent.setup()
750+
const button = component.getByRole('button')
751+
await user.click(button)
752+
753+
expect(component.getByRole('menu')).toBeInTheDocument()
754+
expect(mockOnClick).toHaveBeenCalledTimes(1)
755+
756+
// select and close menu
757+
const menuItems = component.getAllByRole('menuitem')
758+
await user.click(menuItems[0])
759+
expect(component.queryByRole('menu')).toBeNull()
760+
761+
expect(button).toEqual(document.activeElement)
762+
await user.keyboard('{Enter}')
763+
expect(component.queryByRole('menu')).toBeInTheDocument()
764+
expect(mockOnKeyDown).toHaveBeenCalledTimes(1)
765+
})
766+
767+
it('should call onClick and onKeyDown passed to IconButton inside ActionMenu.Anchor with Tooltip', async () => {
768+
const mockOnClick = jest.fn()
769+
const mockOnKeyDown = jest.fn()
770+
771+
const component = HTMLRender(
772+
<ThemeProvider theme={theme}>
773+
<ActionMenu>
774+
<ActionMenu.Anchor>
775+
<TooltipV2 text="Additional context about the menu button" direction="s">
776+
<IconButton
777+
icon={KebabHorizontalIcon}
778+
aria-label="Open menu"
779+
onClick={mockOnClick}
780+
onKeyDown={mockOnKeyDown}
781+
/>
782+
</TooltipV2>
783+
</ActionMenu.Anchor>
784+
<ActionMenu.Overlay>
785+
<ActionList>
786+
<ActionList.Item>New file</ActionList.Item>
787+
<ActionList.Item>Copy link</ActionList.Item>
788+
</ActionList>
789+
</ActionMenu.Overlay>
790+
</ActionMenu>
791+
</ThemeProvider>,
792+
)
793+
794+
const user = userEvent.setup()
795+
const button = component.getByRole('button')
796+
await user.click(button)
797+
798+
expect(component.getByRole('menu')).toBeInTheDocument()
799+
expect(mockOnClick).toHaveBeenCalledTimes(1)
800+
801+
// select and close menu
802+
const menuItems = component.getAllByRole('menuitem')
803+
await user.click(menuItems[0])
804+
expect(component.queryByRole('menu')).toBeNull()
805+
806+
expect(button).toEqual(document.activeElement)
807+
await user.keyboard('{Enter}')
808+
expect(component.queryByRole('menu')).toBeInTheDocument()
809+
expect(mockOnKeyDown).toHaveBeenCalledTimes(1)
810+
})
811+
})
644812
})

0 commit comments

Comments
 (0)