Skip to content

Commit 93e8b8f

Browse files
RobinMalfaitRobinMalfait
authored
fix: outside click behaviour (#19)
* add failing tests to prove the outside-click issue * fix outside click when we have multiple menu's - We removed the `toggleMenu` since we only used it in a single spot, and had to do some side effect logic (focus & event.preventDefault). Wanted to make this consistent between React and Vue. - If, in the "outside click" logic we detect that we clicked on the button, we also ignore it. - If, we click on the button we will toggle the menu. Fixes: #18
1 parent e9fd05e commit 93e8b8f

File tree

4 files changed

+140
-47
lines changed

4 files changed

+140
-47
lines changed

‎packages/@headlessui-react/src/components/menu/menu.test.tsx‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,24 @@ function getMenuButton(): HTMLElement | null {
3535
return document.querySelector('[role="button"],button')
3636
}
3737

38+
function getMenuButtons(): HTMLElement[] {
39+
// This is just an assumption for our tests. We assume that we only have 1 button. And if we have
40+
// more, than we assume that it is the first one.
41+
return Array.from(document.querySelectorAll('[role="button"],button'))
42+
}
43+
3844
function getMenu(): HTMLElement | null {
3945
// This is just an assumption for our tests. We assume that our menu has this role and that it is
4046
// the first item in the DOM.
4147
return document.querySelector('[role="menu"]')
4248
}
4349

50+
function getMenus(): HTMLElement[] {
51+
// This is just an assumption for our tests. We assume that our menu has this role and that it is
52+
// the first item in the DOM.
53+
return Array.from(document.querySelectorAll('[role="menu"]'))
54+
}
55+
4456
function getMenuItems(): HTMLElement[] {
4557
// This is just an assumption for our tests. We assume that all menu items have this role.
4658
return Array.from(document.querySelectorAll('[role="menuitem"]'))
@@ -2248,6 +2260,50 @@ describe('Mouse interactions', () => {
22482260
})
22492261
)
22502262

2263+
it(
2264+
'should be possible to click outside of the menu on another menu button which should close the current menu and open the new menu',
2265+
suppressConsoleLogs(async () => {
2266+
render(
2267+
<div>
2268+
<Menu>
2269+
<Menu.Button>Trigger</Menu.Button>
2270+
<Menu.Items>
2271+
<Menu.Item as="a">alice</Menu.Item>
2272+
<Menu.Item as="a">bob</Menu.Item>
2273+
<Menu.Item as="a">charlie</Menu.Item>
2274+
</Menu.Items>
2275+
</Menu>
2276+
2277+
<Menu>
2278+
<Menu.Button>Trigger</Menu.Button>
2279+
<Menu.Items>
2280+
<Menu.Item as="a">alice</Menu.Item>
2281+
<Menu.Item as="a">bob</Menu.Item>
2282+
<Menu.Item as="a">charlie</Menu.Item>
2283+
</Menu.Items>
2284+
</Menu>
2285+
</div>
2286+
)
2287+
2288+
const [button1, button2] = getMenuButtons()
2289+
2290+
// Click the first menu button
2291+
await click(button1)
2292+
expect(getMenus()).toHaveLength(1) // Only 1 menu should be visible
2293+
2294+
// Ensure the open menu is linked to the first button
2295+
assertMenuButtonLinkedWithMenu(button1, getMenu())
2296+
2297+
// Click the second menu button
2298+
await click(button2)
2299+
2300+
expect(getMenus()).toHaveLength(1) // Only 1 menu should be visible
2301+
2302+
// Ensure the open menu is linked to the second button
2303+
assertMenuButtonLinkedWithMenu(button2, getMenu())
2304+
})
2305+
)
2306+
22512307
it(
22522308
'should be possible to hover an item and make it active',
22532309
suppressConsoleLogs(async () => {

‎packages/@headlessui-react/src/components/menu/menu.tsx‎

Lines changed: 16 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@ type StateDefinition = {
4747
}
4848

4949
enum ActionTypes {
50-
ToggleMenu,
5150
OpenMenu,
5251
CloseMenu,
5352

@@ -114,7 +113,6 @@ function calculateActiveItemIndex(
114113
}
115114

116115
type Actions =
117-
| { type: ActionTypes.ToggleMenu }
118116
| { type: ActionTypes.CloseMenu }
119117
| { type: ActionTypes.OpenMenu }
120118
| { type: ActionTypes.GoToItem; focus: Focus; id?: string }
@@ -129,13 +127,6 @@ const reducers: {
129127
action: Extract<Actions, { type: P }>
130128
) => StateDefinition
131129
} = {
132-
[ActionTypes.ToggleMenu]: state => ({
133-
...state,
134-
menuState: match(state.menuState, {
135-
[MenuStates.Open]: MenuStates.Closed,
136-
[MenuStates.Closed]: MenuStates.Open,
137-
}),
138-
}),
139130
[ActionTypes.CloseMenu]: state => ({ ...state, menuState: MenuStates.Closed }),
140131
[ActionTypes.OpenMenu]: state => ({ ...state, menuState: MenuStates.Open }),
141132
[ActionTypes.GoToItem]: (state, action) => {
@@ -237,17 +228,17 @@ export function Menu<TTag extends React.ElementType = typeof DEFAULT_MENU_TAG>(
237228

238229
React.useEffect(() => {
239230
function handler(event: PointerEvent) {
240-
if (event.defaultPrevented) return
241231
if (menuState !== MenuStates.Open) return
232+
if (buttonRef.current?.contains(event.target as HTMLElement)) return
242233

243234
if (!itemsRef.current?.contains(event.target as HTMLElement)) {
244235
dispatch({ type: ActionTypes.CloseMenu })
245-
d.nextFrame(() => buttonRef.current?.focus())
236+
if (!event.defaultPrevented) buttonRef.current?.focus()
246237
}
247238
}
248239

249-
window.addEventListener('pointerdown', handler)
250-
return () => window.removeEventListener('pointerdown', handler)
240+
window.addEventListener('pointerup', handler)
241+
return () => window.removeEventListener('pointerup', handler)
251242
}, [menuState, itemsRef, buttonRef, d, dispatch])
252243

253244
const propsBag = React.useMemo(() => ({ open: menuState === MenuStates.Open }), [menuState])
@@ -272,7 +263,6 @@ type ButtonPropsWeControl =
272263
| 'onFocus'
273264
| 'onBlur'
274265
| 'onPointerUp'
275-
| 'onPointerDown'
276266

277267
const DEFAULT_BUTTON_TAG = 'button'
278268

@@ -320,16 +310,18 @@ const Button = forwardRefWithAs(function Button<
320310
[dispatch, state, d]
321311
)
322312

323-
const handlePointerDown = React.useCallback((event: React.PointerEvent<HTMLButtonElement>) => {
324-
// We have a `pointerdown` event listener in the menu for the 'outside click', so we just want
325-
// to prevent going there if we happen to click this button.
326-
event.preventDefault()
327-
}, [])
328-
329-
const handlePointerUp = React.useCallback(() => {
330-
dispatch({ type: ActionTypes.ToggleMenu })
331-
d.nextFrame(() => state.itemsRef.current?.focus())
332-
}, [dispatch, d, state])
313+
const handlePointerUp = React.useCallback(
314+
(event: MouseEvent) => {
315+
if (state.menuState === MenuStates.Open) {
316+
dispatch({ type: ActionTypes.CloseMenu })
317+
} else {
318+
event.preventDefault()
319+
dispatch({ type: ActionTypes.OpenMenu })
320+
d.nextFrame(() => state.itemsRef.current?.focus())
321+
}
322+
},
323+
[dispatch, d, state]
324+
)
333325

334326
const handleFocus = React.useCallback(() => {
335327
if (state.menuState === MenuStates.Open) state.itemsRef.current?.focus()
@@ -354,7 +346,6 @@ const Button = forwardRefWithAs(function Button<
354346
onFocus: handleFocus,
355347
onBlur: handleBlur,
356348
onPointerUp: handlePointerUp,
357-
onPointerDown: handlePointerDown,
358349
}
359350

360351
return render({ ...passthroughProps, ...propsWeControl }, propsBag, DEFAULT_BUTTON_TAG)

‎packages/@headlessui-vue/src/components/menu/menu.test.tsx‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,12 +50,24 @@ function getMenuButton(): HTMLElement | null {
5050
return document.querySelector('button')
5151
}
5252

53+
function getMenuButtons(): HTMLElement[] {
54+
// This is just an assumption for our tests. We assume that we only have 1 button. And if we have
55+
// more, than we assume that it is the first one.
56+
return Array.from(document.querySelectorAll('[role="button"],button'))
57+
}
58+
5359
function getMenu(): HTMLElement | null {
5460
// This is just an assumption for our tests. We assume that our menu has this role and that it is
5561
// the first item in the DOM.
5662
return document.querySelector('[role="menu"]')
5763
}
5864

65+
function getMenus(): HTMLElement[] {
66+
// This is just an assumption for our tests. We assume that our menu has this role and that it is
67+
// the first item in the DOM.
68+
return Array.from(document.querySelectorAll('[role="menu"]'))
69+
}
70+
5971
function getMenuItems(): HTMLElement[] {
6072
// This is just an assumption for our tests. We assume that all menu items have this role.
6173
return Array.from(document.querySelectorAll('[role="menuitem"]'))
@@ -2207,6 +2219,50 @@ describe('Mouse interactions', () => {
22072219
assertMenu(getMenu(), { state: MenuState.Closed })
22082220
})
22092221

2222+
it(
2223+
'should be possible to click outside of the menu on another menu button which should close the current menu and open the new menu',
2224+
suppressConsoleLogs(async () => {
2225+
renderTemplate(`
2226+
<div>
2227+
<Menu>
2228+
<MenuButton>Trigger</MenuButton>
2229+
<MenuItems>
2230+
<MenuItem>alice</MenuItem>
2231+
<MenuItem>bob</MenuItem>
2232+
<MenuItem>charlie</MenuItem>
2233+
</MenuItems>
2234+
</Menu>
2235+
2236+
<Menu>
2237+
<MenuButton>Trigger</MenuButton>
2238+
<MenuItems>
2239+
<MenuItem>alice</MenuItem>
2240+
<MenuItem>bob</MenuItem>
2241+
<MenuItem>charlie</MenuItem>
2242+
</MenuItems>
2243+
</Menu>
2244+
</div>
2245+
`)
2246+
2247+
const [button1, button2] = getMenuButtons()
2248+
2249+
// Click the first menu button
2250+
await click(button1)
2251+
expect(getMenus()).toHaveLength(1) // Only 1 menu should be visible
2252+
2253+
// Ensure the open menu is linked to the first button
2254+
assertMenuButtonLinkedWithMenu(button1, getMenu())
2255+
2256+
// Click the second menu button
2257+
await click(button2)
2258+
2259+
expect(getMenus()).toHaveLength(1) // Only 1 menu should be visible
2260+
2261+
// Ensure the open menu is linked to the second button
2262+
assertMenuButtonLinkedWithMenu(button2, getMenu())
2263+
})
2264+
)
2265+
22102266
it('should be possible to hover an item and make it active', async () => {
22112267
renderTemplate(`
22122268
<Menu>

‎packages/@headlessui-vue/src/components/menu/menu.ts‎

Lines changed: 12 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,6 @@ type StateDefinition = {
5959
activeItemIndex: Ref<number | null>
6060

6161
// State mutators
62-
toggleMenu(): void
6362
closeMenu(): void
6463
openMenu(): void
6564
goToItem(focus: Focus, id?: string): void
@@ -141,12 +140,6 @@ export const Menu = defineComponent({
141140
items,
142141
searchQuery,
143142
activeItemIndex,
144-
toggleMenu() {
145-
menuState.value = match(menuState.value, {
146-
[MenuStates.Closed]: MenuStates.Open,
147-
[MenuStates.Open]: MenuStates.Closed,
148-
})
149-
},
150143
closeMenu: () => (menuState.value = MenuStates.Closed),
151144
openMenu: () => (menuState.value = MenuStates.Open),
152145
goToItem(focus: Focus, id?: string) {
@@ -195,17 +188,17 @@ export const Menu = defineComponent({
195188

196189
onMounted(() => {
197190
function handler(event: PointerEvent) {
198-
if (event.defaultPrevented) return
199191
if (menuState.value !== MenuStates.Open) return
192+
if (buttonRef.value?.contains(event.target as HTMLElement)) return
200193

201194
if (!itemsRef.value?.contains(event.target as HTMLElement)) {
202195
api.closeMenu()
203-
nextTick(() => buttonRef.value?.focus())
196+
if (!event.defaultPrevented) buttonRef.value?.focus()
204197
}
205198
}
206199

207-
window.addEventListener('pointerdown', handler)
208-
onUnmounted(() => window.removeEventListener('pointerdown', handler))
200+
window.addEventListener('pointerup', handler)
201+
onUnmounted(() => window.removeEventListener('pointerup', handler))
209202
})
210203

211204
// @ts-expect-error Types of property 'dataRef' are incompatible.
@@ -234,7 +227,6 @@ export const MenuButton = defineComponent({
234227
onKeyDown: this.handleKeyDown,
235228
onFocus: this.handleFocus,
236229
onPointerUp: this.handlePointerUp,
237-
onPointerDown: this.handlePointerDown,
238230
}
239231

240232
return render({
@@ -274,15 +266,14 @@ export const MenuButton = defineComponent({
274266
}
275267
}
276268

277-
function handlePointerDown(event: PointerEvent) {
278-
// We have a `pointerdown` event listener in the menu for the 'outside click', so we just want
279-
// to prevent going there if we happen to click this button.
280-
event.preventDefault()
281-
}
282-
283-
function handlePointerUp() {
284-
api.toggleMenu()
285-
nextTick(() => api.itemsRef.value?.focus())
269+
function handlePointerUp(event: MouseEvent) {
270+
if (api.menuState.value === MenuStates.Open) {
271+
api.closeMenu()
272+
} else {
273+
event.preventDefault()
274+
api.openMenu()
275+
nextTick(() => api.itemsRef.value?.focus())
276+
}
286277
}
287278

288279
function handleFocus() {
@@ -293,7 +284,6 @@ export const MenuButton = defineComponent({
293284
id,
294285
el: api.buttonRef,
295286
handleKeyDown,
296-
handlePointerDown,
297287
handlePointerUp,
298288
handleFocus,
299289
}

0 commit comments

Comments
 (0)