Skip to content

Commit e9fd05e

Browse files
committed
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
1 parent 93f315b commit e9fd05e

File tree

4 files changed

+65
-29
lines changed

4 files changed

+65
-29
lines changed

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -593,11 +593,14 @@ describe('Keyboard interactions', () => {
593593
it(
594594
'should be possible to close the menu with Enter and invoke the active menu item',
595595
suppressConsoleLogs(async () => {
596+
const clickHandler = jest.fn()
596597
render(
597598
<Menu>
598599
<Menu.Button>Trigger</Menu.Button>
599600
<Menu.Items>
600-
<Menu.Item as="a">Item A</Menu.Item>
601+
<Menu.Item as="a" onClick={clickHandler}>
602+
Item A
603+
</Menu.Item>
601604
<Menu.Item as="a">Item B</Menu.Item>
602605
<Menu.Item as="a">Item C</Menu.Item>
603606
</Menu.Items>
@@ -626,6 +629,9 @@ describe('Keyboard interactions', () => {
626629
// Verify it is closed
627630
assertMenuButton(getMenuButton(), { state: MenuButtonState.Closed })
628631
assertMenu(getMenu(), { state: MenuState.Closed })
632+
633+
// Verify the "click" went through on the `a` tag
634+
expect(clickHandler).toHaveBeenCalled()
629635
})
630636
)
631637
})
@@ -2458,12 +2464,15 @@ describe('Mouse interactions', () => {
24582464
it(
24592465
'should be possible to click a menu item, which closes the menu',
24602466
suppressConsoleLogs(async () => {
2467+
const clickHandler = jest.fn()
24612468
render(
24622469
<Menu>
24632470
<Menu.Button>Trigger</Menu.Button>
24642471
<Menu.Items>
24652472
<Menu.Item as="a">alice</Menu.Item>
2466-
<Menu.Item as="a">bob</Menu.Item>
2473+
<Menu.Item as="a" onClick={clickHandler}>
2474+
bob
2475+
</Menu.Item>
24672476
<Menu.Item as="a">charlie</Menu.Item>
24682477
</Menu.Items>
24692478
</Menu>
@@ -2478,6 +2487,7 @@ describe('Mouse interactions', () => {
24782487
// We should be able to click the first item
24792488
await click(items[1])
24802489
assertMenu(getMenu(), { state: MenuState.Closed })
2490+
expect(clickHandler).toHaveBeenCalled()
24812491
})
24822492
)
24832493

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -562,8 +562,7 @@ function Item<TTag extends React.ElementType = typeof DEFAULT_ITEM_TAG>(
562562

563563
const handlePointerUp = React.useCallback(
564564
(event: React.PointerEvent<HTMLElement>) => {
565-
if (disabled) return
566-
event.preventDefault()
565+
if (disabled) return event.preventDefault()
567566
dispatch({ type: ActionTypes.CloseMenu })
568567
d.nextFrame(() => state.buttonRef.current?.focus())
569568
},

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

Lines changed: 40 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { defineComponent, h } from 'vue'
1+
import { defineComponent, h, nextTick } from 'vue'
22
import { render } from '../../test-utils/vue-testing-library'
33
import { Menu, MenuButton, MenuItems, MenuItem } from './menu'
44
import { suppressConsoleLogs } from '../../test-utils/suppress-console-logs'
@@ -61,6 +61,13 @@ function getMenuItems(): HTMLElement[] {
6161
return Array.from(document.querySelectorAll('[role="menuitem"]'))
6262
}
6363

64+
beforeAll(() => {
65+
jest.spyOn(window, 'requestAnimationFrame').mockImplementation(nextTick as any)
66+
jest.spyOn(window, 'cancelAnimationFrame').mockImplementation(jest.fn())
67+
})
68+
69+
afterAll(() => jest.restoreAllMocks())
70+
6471
describe('Safe guards', () => {
6572
it.each([
6673
['MenuButton', MenuButton],
@@ -771,16 +778,20 @@ describe('Keyboard interactions', () => {
771778
})
772779

773780
it('should be possible to close the menu with Enter and invoke the active menu item', async () => {
774-
renderTemplate(`
775-
<Menu>
776-
<MenuButton>Trigger</MenuButton>
777-
<MenuItems>
778-
<MenuItem as="a">Item A</MenuItem>
779-
<MenuItem as="a">Item B</MenuItem>
780-
<MenuItem as="a">Item C</MenuItem>
781-
</MenuItems>
782-
</Menu>
783-
`)
781+
const clickHandler = jest.fn()
782+
renderTemplate({
783+
template: `
784+
<Menu>
785+
<MenuButton>Trigger</MenuButton>
786+
<MenuItems>
787+
<MenuItem as="a" @click="clickHandler">Item A</MenuItem>
788+
<MenuItem as="a">Item B</MenuItem>
789+
<MenuItem as="a">Item C</MenuItem>
790+
</MenuItems>
791+
</Menu>
792+
`,
793+
setup: () => ({ clickHandler }),
794+
})
784795

785796
assertMenuButton(getMenuButton(), {
786797
state: MenuButtonState.Closed,
@@ -804,6 +815,9 @@ describe('Keyboard interactions', () => {
804815
// Verify it is closed
805816
assertMenuButton(getMenuButton(), { state: MenuButtonState.Closed })
806817
assertMenu(getMenu(), { state: MenuState.Closed })
818+
819+
// Verify the "click" went through on the `a` tag
820+
expect(clickHandler).toHaveBeenCalled()
807821
})
808822
})
809823

@@ -2380,16 +2394,20 @@ describe('Mouse interactions', () => {
23802394
})
23812395

23822396
it('should be possible to click a menu item, which closes the menu', async () => {
2383-
renderTemplate(`
2384-
<Menu>
2385-
<MenuButton>Trigger</MenuButton>
2386-
<MenuItems>
2387-
<MenuItem as="a">alice</MenuItem>
2388-
<MenuItem as="a">bob</MenuItem>
2389-
<MenuItem as="a">charlie</MenuItem>
2390-
</MenuItems>
2391-
</Menu>
2392-
`)
2397+
const clickHandler = jest.fn()
2398+
renderTemplate({
2399+
template: `
2400+
<Menu>
2401+
<MenuButton>Trigger</MenuButton>
2402+
<MenuItems>
2403+
<MenuItem as="a">alice</MenuItem>
2404+
<MenuItem as="a" @click="clickHandler">bob</MenuItem>
2405+
<MenuItem as="a">charlie</MenuItem>
2406+
</MenuItems>
2407+
</Menu>
2408+
`,
2409+
setup: () => ({ clickHandler }),
2410+
})
23932411

23942412
// Open menu
23952413
await click(getMenuButton())
@@ -2400,6 +2418,7 @@ describe('Mouse interactions', () => {
24002418
// We should be able to click the first item
24012419
await click(items[1])
24022420
assertMenu(getMenu(), { state: MenuState.Closed })
2421+
expect(clickHandler).toHaveBeenCalled()
24032422
})
24042423

24052424
it('should be possible to click a menu item, which closes the menu and invokes the @click handler', async () => {

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

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -445,10 +445,18 @@ export const MenuItem = defineComponent({
445445
}
446446

447447
function handlePointerUp(event: PointerEvent) {
448-
if (disabled) return
449-
event.preventDefault()
450-
api.closeMenu()
451-
nextTick(() => api.buttonRef.value?.focus())
448+
if (disabled) return event.preventDefault()
449+
450+
// Turns out that we can't use nextTick here. Even if we do, the `handleClick` would *not* be
451+
// called because the closeMenu() update is *too fast* and the tree gets unmounted before it
452+
// bubbles up. So instead of nextTick, we use the good old double requestAnimationFrame to
453+
// wait for a "nextFrame".
454+
requestAnimationFrame(() => {
455+
requestAnimationFrame(() => {
456+
api.closeMenu()
457+
api.buttonRef.value?.focus()
458+
})
459+
})
452460
}
453461

454462
function handleClick(event: MouseEvent) {

0 commit comments

Comments
 (0)