Skip to content

Fix outside click behaviour #19

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 28, 2020
Merged

Fix outside click behaviour #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4940bef
add failing tests to prove the outside-click issue
RobinMalfait Sep 28, 2020
f7478de
fix outside click when we have multiple menu's
RobinMalfait Sep 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions packages/@headlessui-react/src/components/menu/menu.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,24 @@ function getMenuButton(): HTMLElement | null {
return document.querySelector('[role="button"],button')
}

function getMenuButtons(): HTMLElement[] {
// This is just an assumption for our tests. We assume that we only have 1 button. And if we have
// more, than we assume that it is the first one.
return Array.from(document.querySelectorAll('[role="button"],button'))
}

function getMenu(): HTMLElement | null {
// This is just an assumption for our tests. We assume that our menu has this role and that it is
// the first item in the DOM.
return document.querySelector('[role="menu"]')
}

function getMenus(): HTMLElement[] {
// This is just an assumption for our tests. We assume that our menu has this role and that it is
// the first item in the DOM.
return Array.from(document.querySelectorAll('[role="menu"]'))
}

function getMenuItems(): HTMLElement[] {
// This is just an assumption for our tests. We assume that all menu items have this role.
return Array.from(document.querySelectorAll('[role="menuitem"]'))
Expand Down Expand Up @@ -2248,6 +2260,50 @@ describe('Mouse interactions', () => {
})
)

it(
'should be possible to click outside of the menu on another menu button which should close the current menu and open the new menu',
suppressConsoleLogs(async () => {
render(
<div>
<Menu>
<Menu.Button>Trigger</Menu.Button>
<Menu.Items>
<Menu.Item as="a">alice</Menu.Item>
<Menu.Item as="a">bob</Menu.Item>
<Menu.Item as="a">charlie</Menu.Item>
</Menu.Items>
</Menu>

<Menu>
<Menu.Button>Trigger</Menu.Button>
<Menu.Items>
<Menu.Item as="a">alice</Menu.Item>
<Menu.Item as="a">bob</Menu.Item>
<Menu.Item as="a">charlie</Menu.Item>
</Menu.Items>
</Menu>
</div>
)

const [button1, button2] = getMenuButtons()

// Click the first menu button
await click(button1)
expect(getMenus()).toHaveLength(1) // Only 1 menu should be visible

// Ensure the open menu is linked to the first button
assertMenuButtonLinkedWithMenu(button1, getMenu())

// Click the second menu button
await click(button2)

expect(getMenus()).toHaveLength(1) // Only 1 menu should be visible

// Ensure the open menu is linked to the second button
assertMenuButtonLinkedWithMenu(button2, getMenu())
})
)

it(
'should be possible to hover an item and make it active',
suppressConsoleLogs(async () => {
Expand Down
41 changes: 16 additions & 25 deletions packages/@headlessui-react/src/components/menu/menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ type StateDefinition = {
}

enum ActionTypes {
ToggleMenu,
OpenMenu,
CloseMenu,

Expand Down Expand Up @@ -114,7 +113,6 @@ function calculateActiveItemIndex(
}

type Actions =
| { type: ActionTypes.ToggleMenu }
| { type: ActionTypes.CloseMenu }
| { type: ActionTypes.OpenMenu }
| { type: ActionTypes.GoToItem; focus: Focus; id?: string }
Expand All @@ -129,13 +127,6 @@ const reducers: {
action: Extract<Actions, { type: P }>
) => StateDefinition
} = {
[ActionTypes.ToggleMenu]: state => ({
...state,
menuState: match(state.menuState, {
[MenuStates.Open]: MenuStates.Closed,
[MenuStates.Closed]: MenuStates.Open,
}),
}),
[ActionTypes.CloseMenu]: state => ({ ...state, menuState: MenuStates.Closed }),
[ActionTypes.OpenMenu]: state => ({ ...state, menuState: MenuStates.Open }),
[ActionTypes.GoToItem]: (state, action) => {
Expand Down Expand Up @@ -237,17 +228,17 @@ export function Menu<TTag extends React.ElementType = typeof DEFAULT_MENU_TAG>(

React.useEffect(() => {
function handler(event: PointerEvent) {
if (event.defaultPrevented) return
if (menuState !== MenuStates.Open) return
if (buttonRef.current?.contains(event.target as HTMLElement)) return

if (!itemsRef.current?.contains(event.target as HTMLElement)) {
dispatch({ type: ActionTypes.CloseMenu })
d.nextFrame(() => buttonRef.current?.focus())
if (!event.defaultPrevented) buttonRef.current?.focus()
}
}

window.addEventListener('pointerdown', handler)
return () => window.removeEventListener('pointerdown', handler)
window.addEventListener('pointerup', handler)
return () => window.removeEventListener('pointerup', handler)
}, [menuState, itemsRef, buttonRef, d, dispatch])

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

const DEFAULT_BUTTON_TAG = 'button'

Expand Down Expand Up @@ -320,16 +310,18 @@ const Button = forwardRefWithAs(function Button<
[dispatch, state, d]
)

const handlePointerDown = React.useCallback((event: React.PointerEvent<HTMLButtonElement>) => {
// We have a `pointerdown` event listener in the menu for the 'outside click', so we just want
// to prevent going there if we happen to click this button.
event.preventDefault()
}, [])

const handlePointerUp = React.useCallback(() => {
dispatch({ type: ActionTypes.ToggleMenu })
d.nextFrame(() => state.itemsRef.current?.focus())
}, [dispatch, d, state])
const handlePointerUp = React.useCallback(
(event: MouseEvent) => {
if (state.menuState === MenuStates.Open) {
dispatch({ type: ActionTypes.CloseMenu })
} else {
event.preventDefault()
dispatch({ type: ActionTypes.OpenMenu })
d.nextFrame(() => state.itemsRef.current?.focus())
}
},
[dispatch, d, state]
)

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

return render({ ...passthroughProps, ...propsWeControl }, propsBag, DEFAULT_BUTTON_TAG)
Expand Down
56 changes: 56 additions & 0 deletions packages/@headlessui-vue/src/components/menu/menu.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,24 @@ function getMenuButton(): HTMLElement | null {
return document.querySelector('button')
}

function getMenuButtons(): HTMLElement[] {
// This is just an assumption for our tests. We assume that we only have 1 button. And if we have
// more, than we assume that it is the first one.
return Array.from(document.querySelectorAll('[role="button"],button'))
}

function getMenu(): HTMLElement | null {
// This is just an assumption for our tests. We assume that our menu has this role and that it is
// the first item in the DOM.
return document.querySelector('[role="menu"]')
}

function getMenus(): HTMLElement[] {
// This is just an assumption for our tests. We assume that our menu has this role and that it is
// the first item in the DOM.
return Array.from(document.querySelectorAll('[role="menu"]'))
}

function getMenuItems(): HTMLElement[] {
// This is just an assumption for our tests. We assume that all menu items have this role.
return Array.from(document.querySelectorAll('[role="menuitem"]'))
Expand Down Expand Up @@ -2207,6 +2219,50 @@ describe('Mouse interactions', () => {
assertMenu(getMenu(), { state: MenuState.Closed })
})

it(
'should be possible to click outside of the menu on another menu button which should close the current menu and open the new menu',
suppressConsoleLogs(async () => {
renderTemplate(`
<div>
<Menu>
<MenuButton>Trigger</MenuButton>
<MenuItems>
<MenuItem>alice</MenuItem>
<MenuItem>bob</MenuItem>
<MenuItem>charlie</MenuItem>
</MenuItems>
</Menu>

<Menu>
<MenuButton>Trigger</MenuButton>
<MenuItems>
<MenuItem>alice</MenuItem>
<MenuItem>bob</MenuItem>
<MenuItem>charlie</MenuItem>
</MenuItems>
</Menu>
</div>
`)

const [button1, button2] = getMenuButtons()

// Click the first menu button
await click(button1)
expect(getMenus()).toHaveLength(1) // Only 1 menu should be visible

// Ensure the open menu is linked to the first button
assertMenuButtonLinkedWithMenu(button1, getMenu())

// Click the second menu button
await click(button2)

expect(getMenus()).toHaveLength(1) // Only 1 menu should be visible

// Ensure the open menu is linked to the second button
assertMenuButtonLinkedWithMenu(button2, getMenu())
})
)

it('should be possible to hover an item and make it active', async () => {
renderTemplate(`
<Menu>
Expand Down
34 changes: 12 additions & 22 deletions packages/@headlessui-vue/src/components/menu/menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ type StateDefinition = {
activeItemIndex: Ref<number | null>

// State mutators
toggleMenu(): void
closeMenu(): void
openMenu(): void
goToItem(focus: Focus, id?: string): void
Expand Down Expand Up @@ -141,12 +140,6 @@ export const Menu = defineComponent({
items,
searchQuery,
activeItemIndex,
toggleMenu() {
menuState.value = match(menuState.value, {
[MenuStates.Closed]: MenuStates.Open,
[MenuStates.Open]: MenuStates.Closed,
})
},
closeMenu: () => (menuState.value = MenuStates.Closed),
openMenu: () => (menuState.value = MenuStates.Open),
goToItem(focus: Focus, id?: string) {
Expand Down Expand Up @@ -195,17 +188,17 @@ export const Menu = defineComponent({

onMounted(() => {
function handler(event: PointerEvent) {
if (event.defaultPrevented) return
if (menuState.value !== MenuStates.Open) return
if (buttonRef.value?.contains(event.target as HTMLElement)) return

if (!itemsRef.value?.contains(event.target as HTMLElement)) {
api.closeMenu()
nextTick(() => buttonRef.value?.focus())
if (!event.defaultPrevented) buttonRef.value?.focus()
}
}

window.addEventListener('pointerdown', handler)
onUnmounted(() => window.removeEventListener('pointerdown', handler))
window.addEventListener('pointerup', handler)
onUnmounted(() => window.removeEventListener('pointerup', handler))
})

// @ts-expect-error Types of property 'dataRef' are incompatible.
Expand Down Expand Up @@ -234,7 +227,6 @@ export const MenuButton = defineComponent({
onKeyDown: this.handleKeyDown,
onFocus: this.handleFocus,
onPointerUp: this.handlePointerUp,
onPointerDown: this.handlePointerDown,
}

return render({
Expand Down Expand Up @@ -274,15 +266,14 @@ export const MenuButton = defineComponent({
}
}

function handlePointerDown(event: PointerEvent) {
// We have a `pointerdown` event listener in the menu for the 'outside click', so we just want
// to prevent going there if we happen to click this button.
event.preventDefault()
}

function handlePointerUp() {
api.toggleMenu()
nextTick(() => api.itemsRef.value?.focus())
function handlePointerUp(event: MouseEvent) {
if (api.menuState.value === MenuStates.Open) {
api.closeMenu()
} else {
event.preventDefault()
api.openMenu()
nextTick(() => api.itemsRef.value?.focus())
}
}

function handleFocus() {
Expand All @@ -293,7 +284,6 @@ export const MenuButton = defineComponent({
id,
el: api.buttonRef,
handleKeyDown,
handlePointerDown,
handlePointerUp,
handleFocus,
}
Expand Down