From 4b2aacfdf2f8ff5adffeca7531ba6ba4a7a6ac49 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Tue, 29 Sep 2020 12:49:30 +0200 Subject: [PATCH] simplify the click / pointer up handling on Menu Item Thank you @MartinMa for pointing this complexity out! --- .../src/components/menu/menu.tsx | 21 +++++----------- .../src/components/menu/menu.test.tsx | 9 +------ .../src/components/menu/menu.ts | 24 ++++--------------- 3 files changed, 12 insertions(+), 42 deletions(-) diff --git a/packages/@headlessui-react/src/components/menu/menu.tsx b/packages/@headlessui-react/src/components/menu/menu.tsx index 88037c4..d7e12d2 100644 --- a/packages/@headlessui-react/src/components/menu/menu.tsx +++ b/packages/@headlessui-react/src/components/menu/menu.tsx @@ -227,7 +227,7 @@ export function Menu( const [{ menuState, itemsRef, buttonRef }, dispatch] = reducerBag React.useEffect(() => { - function handler(event: PointerEvent) { + function handler(event: MouseEvent) { if (menuState !== MenuStates.Open) return if (buttonRef.current?.contains(event.target as HTMLElement)) return @@ -237,8 +237,8 @@ export function Menu( } } - window.addEventListener('pointerup', handler) - return () => window.removeEventListener('pointerup', handler) + window.addEventListener('click', handler) + return () => window.removeEventListener('click', handler) }, [menuState, itemsRef, buttonRef, d, dispatch]) const propsBag = React.useMemo(() => ({ open: menuState === MenuStates.Open }), [menuState]) @@ -498,7 +498,6 @@ type MenuItemPropsWeControl = | 'aria-disabled' | 'onPointerEnter' | 'onPointerLeave' - | 'onPointerUp' | 'onFocus' const DEFAULT_ITEM_TAG = React.Fragment @@ -557,21 +556,14 @@ function Item( dispatch({ type: ActionTypes.GoToItem, focus: Focus.SpecificItem, id }) }, [disabled, active, id, dispatch]) - const handlePointerUp = React.useCallback( - (event: React.PointerEvent) => { - if (disabled) return event.preventDefault() - dispatch({ type: ActionTypes.CloseMenu }) - d.nextFrame(() => state.buttonRef.current?.focus()) - }, - [dispatch, disabled, d, state.buttonRef] - ) - const handleClick = React.useCallback( (event: { preventDefault: Function }) => { if (disabled) return event.preventDefault() + dispatch({ type: ActionTypes.CloseMenu }) + d.nextFrame(() => state.buttonRef.current?.focus()) if (onClick) return onClick(event) }, - [disabled, onClick] + [d, dispatch, state.buttonRef, disabled, onClick] ) const propsBag = React.useMemo(() => ({ active, disabled }), [active, disabled]) @@ -586,7 +578,6 @@ function Item( onMouseMove: handleMouseMove, onPointerEnter: handlePointerEnter, onPointerLeave: handlePointerLeave, - onPointerUp: handlePointerUp, } return render( diff --git a/packages/@headlessui-vue/src/components/menu/menu.test.tsx b/packages/@headlessui-vue/src/components/menu/menu.test.tsx index 1aa7783..c68ae69 100644 --- a/packages/@headlessui-vue/src/components/menu/menu.test.tsx +++ b/packages/@headlessui-vue/src/components/menu/menu.test.tsx @@ -1,4 +1,4 @@ -import { defineComponent, h, nextTick } from 'vue' +import { defineComponent, h } from 'vue' import { render } from '../../test-utils/vue-testing-library' import { Menu, MenuButton, MenuItems, MenuItem } from './menu' import { suppressConsoleLogs } from '../../test-utils/suppress-console-logs' @@ -73,13 +73,6 @@ function getMenuItems(): HTMLElement[] { return Array.from(document.querySelectorAll('[role="menuitem"]')) } -beforeAll(() => { - jest.spyOn(window, 'requestAnimationFrame').mockImplementation(nextTick as any) - jest.spyOn(window, 'cancelAnimationFrame').mockImplementation(jest.fn()) -}) - -afterAll(() => jest.restoreAllMocks()) - describe('Safe guards', () => { it.each([ ['MenuButton', MenuButton], diff --git a/packages/@headlessui-vue/src/components/menu/menu.ts b/packages/@headlessui-vue/src/components/menu/menu.ts index 14f817e..e259fce 100644 --- a/packages/@headlessui-vue/src/components/menu/menu.ts +++ b/packages/@headlessui-vue/src/components/menu/menu.ts @@ -187,7 +187,7 @@ export const Menu = defineComponent({ } onMounted(() => { - function handler(event: PointerEvent) { + function handler(event: MouseEvent) { if (menuState.value !== MenuStates.Open) return if (buttonRef.value?.contains(event.target as HTMLElement)) return @@ -197,8 +197,8 @@ export const Menu = defineComponent({ } } - window.addEventListener('pointerup', handler) - onUnmounted(() => window.removeEventListener('pointerup', handler)) + window.addEventListener('click', handler) + onUnmounted(() => window.removeEventListener('click', handler)) }) // @ts-expect-error Types of property 'dataRef' are incompatible. @@ -440,23 +440,10 @@ export const MenuItem = defineComponent({ api.goToItem(Focus.SpecificItem, id) } - function handlePointerUp(event: PointerEvent) { - if (disabled) return event.preventDefault() - - // Turns out that we can't use nextTick here. Even if we do, the `handleClick` would *not* be - // called because the closeMenu() update is *too fast* and the tree gets unmounted before it - // bubbles up. So instead of nextTick, we use the good old double requestAnimationFrame to - // wait for a "nextFrame". - requestAnimationFrame(() => { - requestAnimationFrame(() => { - api.closeMenu() - api.buttonRef.value?.focus() - }) - }) - } - function handleClick(event: MouseEvent) { if (disabled) return event.preventDefault() + api.closeMenu() + nextTick(() => api.buttonRef.value?.focus()) } return () => { @@ -472,7 +459,6 @@ export const MenuItem = defineComponent({ onMouseMove: handleMouseMove, onPointerEnter: handlePointerEnter, onPointerLeave: handlePointerLeave, - onPointerUp: handlePointerUp, } return render({