From e9fd05e06da6bf81d0aa5657011630a62561ec51 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Mon, 28 Sep 2020 11:47:49 +0200 Subject: [PATCH] ensure that anchor links are still clickable This is an issue in the Vue version (it just works in the React version) but I added tests for them anyway. While this solution "works" I am not 100% happy with it. Let me explain what's happening here and why I am not that happy about it: - For starters, the Vue `nextTick` is apparently too fast. So what we do is when we get the pointer up event, we will close the menu and re-focus the button. We ran this code in a `nextTick` so that we can ensure that we close the menu *after* all the click events are finished. However because this is too fast, the menu is already closed and the anchor link is already unmounted and thus not clickable anymore. So instead we use a double requestAnimationFrame (to mimick a `nextFrame` as seen in the `disposables` from the React code). This works, but a bit messy, oh well. - The next reason why I am not that happy is because I can't reproduce it in JSDOM (Jest tests). When you *click* a link in JSDOM it doesn't update the `window.location.hash` or `window.location.href`. To mimick that behaviour I put a `@click` event on the anchor to verify that we actually clicked it. However this already works, even before the "fix". So I left a TODO in there so that we can hopefully fix the test, so that we _can_ reproduce this behaviour. Fixes: #14 --- .../src/components/menu/menu.test.tsx | 14 ++++- .../src/components/menu/menu.tsx | 3 +- .../src/components/menu/menu.test.tsx | 61 ++++++++++++------- .../src/components/menu/menu.ts | 16 +++-- 4 files changed, 65 insertions(+), 29 deletions(-) diff --git a/packages/@headlessui-react/src/components/menu/menu.test.tsx b/packages/@headlessui-react/src/components/menu/menu.test.tsx index 11bdf3d..92418c2 100644 --- a/packages/@headlessui-react/src/components/menu/menu.test.tsx +++ b/packages/@headlessui-react/src/components/menu/menu.test.tsx @@ -593,11 +593,14 @@ describe('Keyboard interactions', () => { it( 'should be possible to close the menu with Enter and invoke the active menu item', suppressConsoleLogs(async () => { + const clickHandler = jest.fn() render( Trigger - Item A + + Item A + Item B Item C @@ -626,6 +629,9 @@ describe('Keyboard interactions', () => { // Verify it is closed assertMenuButton(getMenuButton(), { state: MenuButtonState.Closed }) assertMenu(getMenu(), { state: MenuState.Closed }) + + // Verify the "click" went through on the `a` tag + expect(clickHandler).toHaveBeenCalled() }) ) }) @@ -2458,12 +2464,15 @@ describe('Mouse interactions', () => { it( 'should be possible to click a menu item, which closes the menu', suppressConsoleLogs(async () => { + const clickHandler = jest.fn() render( Trigger alice - bob + + bob + charlie @@ -2478,6 +2487,7 @@ describe('Mouse interactions', () => { // We should be able to click the first item await click(items[1]) assertMenu(getMenu(), { state: MenuState.Closed }) + expect(clickHandler).toHaveBeenCalled() }) ) diff --git a/packages/@headlessui-react/src/components/menu/menu.tsx b/packages/@headlessui-react/src/components/menu/menu.tsx index fb45529..7530c50 100644 --- a/packages/@headlessui-react/src/components/menu/menu.tsx +++ b/packages/@headlessui-react/src/components/menu/menu.tsx @@ -562,8 +562,7 @@ function Item( const handlePointerUp = React.useCallback( (event: React.PointerEvent) => { - if (disabled) return - event.preventDefault() + if (disabled) return event.preventDefault() dispatch({ type: ActionTypes.CloseMenu }) d.nextFrame(() => state.buttonRef.current?.focus()) }, diff --git a/packages/@headlessui-vue/src/components/menu/menu.test.tsx b/packages/@headlessui-vue/src/components/menu/menu.test.tsx index f80d03a..a3cd55d 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 } from 'vue' +import { defineComponent, h, nextTick } 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' @@ -61,6 +61,13 @@ 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], @@ -771,16 +778,20 @@ describe('Keyboard interactions', () => { }) it('should be possible to close the menu with Enter and invoke the active menu item', async () => { - renderTemplate(` - - Trigger - - Item A - Item B - Item C - - - `) + const clickHandler = jest.fn() + renderTemplate({ + template: ` + + Trigger + + Item A + Item B + Item C + + + `, + setup: () => ({ clickHandler }), + }) assertMenuButton(getMenuButton(), { state: MenuButtonState.Closed, @@ -804,6 +815,9 @@ describe('Keyboard interactions', () => { // Verify it is closed assertMenuButton(getMenuButton(), { state: MenuButtonState.Closed }) assertMenu(getMenu(), { state: MenuState.Closed }) + + // Verify the "click" went through on the `a` tag + expect(clickHandler).toHaveBeenCalled() }) }) @@ -2380,16 +2394,20 @@ describe('Mouse interactions', () => { }) it('should be possible to click a menu item, which closes the menu', async () => { - renderTemplate(` - - Trigger - - alice - bob - charlie - - - `) + const clickHandler = jest.fn() + renderTemplate({ + template: ` + + Trigger + + alice + bob + charlie + + + `, + setup: () => ({ clickHandler }), + }) // Open menu await click(getMenuButton()) @@ -2400,6 +2418,7 @@ describe('Mouse interactions', () => { // We should be able to click the first item await click(items[1]) assertMenu(getMenu(), { state: MenuState.Closed }) + expect(clickHandler).toHaveBeenCalled() }) it('should be possible to click a menu item, which closes the menu and invokes the @click handler', async () => { diff --git a/packages/@headlessui-vue/src/components/menu/menu.ts b/packages/@headlessui-vue/src/components/menu/menu.ts index 416d324..beb94ef 100644 --- a/packages/@headlessui-vue/src/components/menu/menu.ts +++ b/packages/@headlessui-vue/src/components/menu/menu.ts @@ -445,10 +445,18 @@ export const MenuItem = defineComponent({ } function handlePointerUp(event: PointerEvent) { - if (disabled) return - event.preventDefault() - api.closeMenu() - nextTick(() => api.buttonRef.value?.focus()) + 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) {