From ffa3a3daa246962fcd870a44ce76406aaa06865a Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Mon, 5 Oct 2020 17:45:16 +0200 Subject: [PATCH 1/4] make sure the Button is focused when the Menu closes (React) --- .../src/components/menu/menu.test.tsx | 27 +++++++++++++++++++ .../src/components/menu/menu.tsx | 11 ++++---- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/packages/@headlessui-react/src/components/menu/menu.test.tsx b/packages/@headlessui-react/src/components/menu/menu.test.tsx index 9ffa5a891f..97582870b6 100644 --- a/packages/@headlessui-react/src/components/menu/menu.test.tsx +++ b/packages/@headlessui-react/src/components/menu/menu.test.tsx @@ -573,6 +573,9 @@ describe('Keyboard interactions', () => { // Verify it is closed assertMenuButton({ state: MenuState.Closed }) assertMenu({ state: MenuState.Closed }) + + // Verify the button is focused again + assertActiveElement(getMenuButton()) }) ) @@ -616,6 +619,9 @@ describe('Keyboard interactions', () => { assertMenuButton({ state: MenuState.Closed }) assertMenu({ state: MenuState.Closed }) + // Verify the button is focused again + assertActiveElement(getMenuButton()) + // Verify the "click" went through on the `a` tag expect(clickHandler).toHaveBeenCalled() }) @@ -668,6 +674,9 @@ describe('Keyboard interactions', () => { // Verify the button got "clicked" expect(clickHandler).toHaveBeenCalledTimes(1) + // Verify the button is focused again + assertActiveElement(getMenuButton()) + // Click the menu button again await click(getMenuButton()) @@ -679,6 +688,9 @@ describe('Keyboard interactions', () => { // Verify the button got "clicked" expect(clickHandler).toHaveBeenCalledTimes(2) + + // Verify the button is focused again + assertActiveElement(getMenuButton()) }) ) @@ -888,6 +900,9 @@ describe('Keyboard interactions', () => { // Verify it is closed assertMenuButton({ state: MenuState.Closed }) assertMenu({ state: MenuState.Closed }) + + // Verify the button is focused again + assertActiveElement(getMenuButton()) }) ) @@ -933,6 +948,9 @@ describe('Keyboard interactions', () => { // Verify the "click" went through on the `a` tag expect(clickHandler).toHaveBeenCalled() + + // Verify the button is focused again + assertActiveElement(getMenuButton()) }) ) }) @@ -972,6 +990,9 @@ describe('Keyboard interactions', () => { // Verify it is closed assertMenuButton({ state: MenuState.Closed }) assertMenu({ state: MenuState.Closed }) + + // Verify the button is focused again + assertActiveElement(getMenuButton()) }) ) }) @@ -2324,6 +2345,9 @@ describe('Mouse interactions', () => { // Should be closed now assertMenu({ state: MenuState.Closed }) + + // Verify the button is focused again + assertActiveElement(getMenuButton()) }) ) @@ -2350,6 +2374,9 @@ describe('Mouse interactions', () => { // Should be closed now assertMenu({ state: MenuState.Closed }) + + // Verify the button is focused again + assertActiveElement(getMenuButton()) }) ) diff --git a/packages/@headlessui-react/src/components/menu/menu.tsx b/packages/@headlessui-react/src/components/menu/menu.tsx index 3dffd343e7..a3e4bd40f9 100644 --- a/packages/@headlessui-react/src/components/menu/menu.tsx +++ b/packages/@headlessui-react/src/components/menu/menu.tsx @@ -4,6 +4,7 @@ import * as React from 'react' import { Props } from '../../types' import { match } from '../../utils/match' import { forwardRefWithAs, render } from '../../utils/render' +import { disposables } from '../../utils/disposables' import { useDisposables } from '../../hooks/use-disposables' import { useIsoMorphicEffect } from '../../hooks/use-iso-morphic-effect' import { useSyncRefs } from '../../hooks/use-sync-refs' @@ -213,8 +214,8 @@ export function Menu( if (!itemsRef.current?.contains(event.target as HTMLElement)) { dispatch({ type: ActionTypes.CloseMenu }) - if (!event.defaultPrevented) buttonRef.current?.focus() } + if (!event.defaultPrevented) d.nextFrame(() => buttonRef.current?.focus()) } window.addEventListener('click', handler) @@ -294,6 +295,7 @@ const Button = forwardRefWithAs(function Button< (event: MouseEvent) => { if (state.menuState === MenuStates.Open) { dispatch({ type: ActionTypes.CloseMenu }) + d.nextFrame(() => state.buttonRef.current?.focus()) } else { event.preventDefault() dispatch({ type: ActionTypes.OpenMenu }) @@ -357,7 +359,6 @@ const Items = forwardRefWithAs(function Items< const itemsRef = useSyncRefs(state.itemsRef, ref) const id = `headlessui-menu-items-${useId()}` - const d = useDisposables() const searchDisposables = useDisposables() const handleKeyDown = React.useCallback( @@ -380,8 +381,8 @@ const Items = forwardRefWithAs(function Items< if (state.activeItemIndex !== null) { const { id } = state.items[state.activeItemIndex] document.getElementById(id)?.click() - d.nextFrame(() => state.buttonRef.current?.focus()) } + disposables().nextFrame(() => state.buttonRef.current?.focus()) break case Keys.ArrowDown: @@ -405,7 +406,7 @@ const Items = forwardRefWithAs(function Items< case Keys.Escape: event.preventDefault() dispatch({ type: ActionTypes.CloseMenu }) - d.nextFrame(() => state.buttonRef.current?.focus()) + disposables().nextFrame(() => state.buttonRef.current?.focus()) break case Keys.Tab: @@ -419,7 +420,7 @@ const Items = forwardRefWithAs(function Items< break } }, - [d, dispatch, searchDisposables, state] + [dispatch, searchDisposables, state] ) const propsBag = React.useMemo(() => ({ open: state.menuState === MenuStates.Open }), [state]) From 19381b6681235dbd34658d9773fdf80a17f67547 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Mon, 5 Oct 2020 17:45:30 +0200 Subject: [PATCH 2/4] make sure the Button is focused when the Menu closes (Vue) --- .../src/components/menu/menu.test.tsx | 27 +++++++++++++++++++ .../src/components/menu/menu.ts | 5 ++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/packages/@headlessui-vue/src/components/menu/menu.test.tsx b/packages/@headlessui-vue/src/components/menu/menu.test.tsx index cd34c264fc..e702865235 100644 --- a/packages/@headlessui-vue/src/components/menu/menu.test.tsx +++ b/packages/@headlessui-vue/src/components/menu/menu.test.tsx @@ -754,6 +754,9 @@ describe('Keyboard interactions', () => { // Verify it is closed assertMenuButton({ state: MenuState.Closed }) assertMenu({ state: MenuState.Closed }) + + // Verify the button is focused again + assertActiveElement(getMenuButton()) }) it('should be possible to close the menu with Enter and invoke the active menu item', async () => { @@ -795,6 +798,9 @@ describe('Keyboard interactions', () => { assertMenuButton({ state: MenuState.Closed }) assertMenu({ state: MenuState.Closed }) + // Verify the button is focused again + assertActiveElement(getMenuButton()) + // Verify the "click" went through on the `a` tag expect(clickHandler).toHaveBeenCalled() }) @@ -847,6 +853,9 @@ describe('Keyboard interactions', () => { // Verify the button got "clicked" expect(clickHandler).toHaveBeenCalledTimes(1) + // Verify the button is focused again + assertActiveElement(getMenuButton()) + // Click the menu button again await click(getMenuButton()) @@ -858,6 +867,9 @@ describe('Keyboard interactions', () => { // Verify the button got "clicked" expect(clickHandler).toHaveBeenCalledTimes(2) + + // Verify the button is focused again + assertActiveElement(getMenuButton()) }) describe('`Space` key', () => { @@ -1039,6 +1051,9 @@ describe('Keyboard interactions', () => { // Verify it is closed assertMenuButton({ state: MenuState.Closed }) assertMenu({ state: MenuState.Closed }) + + // Verify the button is focused again + assertActiveElement(getMenuButton()) }) ) @@ -1085,6 +1100,9 @@ describe('Keyboard interactions', () => { // Verify the "click" went through on the `a` tag expect(clickHandler).toHaveBeenCalled() + + // Verify the button is focused again + assertActiveElement(getMenuButton()) }) ) }) @@ -1122,6 +1140,9 @@ describe('Keyboard interactions', () => { // Verify it is closed assertMenuButton({ state: MenuState.Closed }) assertMenu({ state: MenuState.Closed }) + + // Verify the button is focused again + assertActiveElement(getMenuButton()) }) }) @@ -2281,6 +2302,9 @@ describe('Mouse interactions', () => { // Should be closed now assertMenu({ state: MenuState.Closed }) + + // Verify the button is focused again + assertActiveElement(getMenuButton()) }) it('should be possible to click outside of the menu which should close the menu (even if we press the menu button)', async () => { @@ -2304,6 +2328,9 @@ describe('Mouse interactions', () => { // Should be closed now assertMenu({ state: MenuState.Closed }) + + // Verify the button is focused again + assertActiveElement(getMenuButton()) }) it( diff --git a/packages/@headlessui-vue/src/components/menu/menu.ts b/packages/@headlessui-vue/src/components/menu/menu.ts index 64c75cc224..9c48fadac7 100644 --- a/packages/@headlessui-vue/src/components/menu/menu.ts +++ b/packages/@headlessui-vue/src/components/menu/menu.ts @@ -174,8 +174,8 @@ export const Menu = defineComponent({ if (!itemsRef.value?.contains(event.target as HTMLElement)) { api.closeMenu() - if (!event.defaultPrevented) buttonRef.value?.focus() } + if (!event.defaultPrevented) nextTick(() => buttonRef.value?.focus()) } window.addEventListener('click', handler) @@ -250,6 +250,7 @@ export const MenuButton = defineComponent({ function handlePointerUp(event: MouseEvent) { if (api.menuState.value === MenuStates.Open) { api.closeMenu() + nextTick(() => api.buttonRef.value?.focus()) } else { event.preventDefault() api.openMenu() @@ -329,8 +330,8 @@ export const MenuItems = defineComponent({ if (api.activeItemIndex.value !== null) { const { id } = api.items.value[api.activeItemIndex.value] document.getElementById(id)?.click() - nextTick(() => api.buttonRef.value?.focus()) } + nextTick(() => api.buttonRef.value?.focus()) break case Keys.ArrowDown: From 1c5e513836cc8342187115e824d4c43a95aa38e7 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Mon, 5 Oct 2020 17:45:49 +0200 Subject: [PATCH 3/4] make sure the Button is focused when the Listbox closes (React) --- .../src/components/listbox/listbox.test.tsx | 21 +++++++++++++++++++ .../src/components/listbox/listbox.tsx | 10 ++++----- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx b/packages/@headlessui-react/src/components/listbox/listbox.test.tsx index eb559bd30d..c3cbe4b79e 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx +++ b/packages/@headlessui-react/src/components/listbox/listbox.test.tsx @@ -743,6 +743,9 @@ describe('Keyboard interactions', () => { // Verify it is closed assertListboxButton({ state: ListboxState.Closed }) assertListbox({ state: ListboxState.Closed }) + + // Verify the button is focused again + assertActiveElement(getListboxButton()) }) ) @@ -801,6 +804,9 @@ describe('Keyboard interactions', () => { expect(handleChange).toHaveBeenCalledTimes(1) expect(handleChange).toHaveBeenCalledWith('a') + // Verify the button is focused again + assertActiveElement(getListboxButton()) + // Open listbox again await click(getListboxButton()) @@ -1086,6 +1092,9 @@ describe('Keyboard interactions', () => { expect(handleChange).toHaveBeenCalledTimes(1) expect(handleChange).toHaveBeenCalledWith('a') + // Verify the button is focused again + assertActiveElement(getListboxButton()) + // Open listbox again await click(getListboxButton()) @@ -1131,6 +1140,9 @@ describe('Keyboard interactions', () => { // Verify it is closed assertListboxButton({ state: ListboxState.Closed }) assertListbox({ state: ListboxState.Closed }) + + // Verify the button is focused again + assertActiveElement(getListboxButton()) }) ) }) @@ -2652,6 +2664,9 @@ describe('Mouse interactions', () => { // Should be closed now assertListbox({ state: ListboxState.Closed }) + + // Verify the button is focused again + assertActiveElement(getListboxButton()) }) ) @@ -2723,6 +2738,9 @@ describe('Mouse interactions', () => { // Should be closed now assertListbox({ state: ListboxState.Closed }) + + // Verify the button is focused again + assertActiveElement(getListboxButton()) }) ) @@ -2979,6 +2997,9 @@ describe('Mouse interactions', () => { expect(handleChange).toHaveBeenCalledTimes(1) expect(handleChange).toHaveBeenCalledWith('bob') + // Verify the button is focused again + assertActiveElement(getListboxButton()) + // Open listbox again await click(getListboxButton()) diff --git a/packages/@headlessui-react/src/components/listbox/listbox.tsx b/packages/@headlessui-react/src/components/listbox/listbox.tsx index fb8b2cb2d7..c975ca55a4 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.tsx +++ b/packages/@headlessui-react/src/components/listbox/listbox.tsx @@ -222,8 +222,8 @@ export function Listbox< if (!optionsRef.current?.contains(event.target as HTMLElement)) { dispatch({ type: ActionTypes.CloseListbox }) - if (!event.defaultPrevented) buttonRef.current?.focus() } + if (!event.defaultPrevented) d.nextFrame(() => buttonRef.current?.focus()) } window.addEventListener('click', handler) @@ -309,6 +309,7 @@ const Button = forwardRefWithAs(function Button< (event: MouseEvent) => { if (state.listboxState === ListboxStates.Open) { dispatch({ type: ActionTypes.CloseListbox }) + d.nextFrame(() => state.buttonRef.current?.focus()) } else { event.preventDefault() dispatch({ type: ActionTypes.OpenListbox }) @@ -441,7 +442,7 @@ const Options = forwardRefWithAs(function Options< const { dataRef } = state.options[state.activeOptionIndex] state.propsRef.current.onChange(dataRef.current.value) } - d.nextFrame(() => state.buttonRef.current?.focus()) + disposables().nextFrame(() => state.buttonRef.current?.focus()) break case Keys.ArrowDown: @@ -535,7 +536,6 @@ function Option state.buttonRef.current?.focus()) + disposables().nextFrame(() => state.buttonRef.current?.focus()) }, - [d, dispatch, state.buttonRef, disabled, select] + [dispatch, state.buttonRef, disabled, select] ) const handleFocus = React.useCallback(() => { From 8d4c5bc325b778b1bfa3cf4717a786940d254953 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Mon, 5 Oct 2020 17:46:00 +0200 Subject: [PATCH 4/4] make sure the Button is focused when the Listbox closes (Vue) --- .../src/components/listbox/listbox.test.tsx | 21 +++++++++++++++++++ .../src/components/listbox/listbox.ts | 5 +++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx b/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx index 0ab7d55f27..33861c5a7b 100644 --- a/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx +++ b/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx @@ -801,6 +801,9 @@ describe('Keyboard interactions', () => { // Verify it is closed assertListboxButton({ state: ListboxState.Closed }) assertListbox({ state: ListboxState.Closed }) + + // Verify the button is focused again + assertActiveElement(getListboxButton()) }) ) @@ -855,6 +858,9 @@ describe('Keyboard interactions', () => { expect(handleChange).toHaveBeenCalledTimes(1) expect(handleChange).toHaveBeenCalledWith('a') + // Verify the button is focused again + assertActiveElement(getListboxButton()) + // Open listbox again await click(getListboxButton()) @@ -1154,6 +1160,9 @@ describe('Keyboard interactions', () => { expect(handleChange).toHaveBeenCalledTimes(1) expect(handleChange).toHaveBeenCalledWith('a') + // Verify the button is focused again + assertActiveElement(getListboxButton()) + // Open listbox again await click(getListboxButton()) @@ -1202,6 +1211,9 @@ describe('Keyboard interactions', () => { // Verify it is closed assertListboxButton({ state: ListboxState.Closed }) assertListbox({ state: ListboxState.Closed }) + + // Verify the button is focused again + assertActiveElement(getListboxButton()) }) ) }) @@ -2846,6 +2858,9 @@ describe('Mouse interactions', () => { // Should be closed now assertListbox({ state: ListboxState.Closed }) + + // Verify the button is focused again + assertActiveElement(getListboxButton()) }) ) @@ -2923,6 +2938,9 @@ describe('Mouse interactions', () => { // Should be closed now assertListbox({ state: ListboxState.Closed }) + + // Verify the button is focused again + assertActiveElement(getListboxButton()) }) ) @@ -3197,6 +3215,9 @@ describe('Mouse interactions', () => { expect(handleChange).toHaveBeenCalledTimes(1) expect(handleChange).toHaveBeenCalledWith('bob') + // Verify the button is focused again + assertActiveElement(getListboxButton()) + // Open listbox again await click(getListboxButton()) diff --git a/packages/@headlessui-vue/src/components/listbox/listbox.ts b/packages/@headlessui-vue/src/components/listbox/listbox.ts index 349eaa88c4..5012cb1aef 100644 --- a/packages/@headlessui-vue/src/components/listbox/listbox.ts +++ b/packages/@headlessui-vue/src/components/listbox/listbox.ts @@ -198,8 +198,8 @@ export const Listbox = defineComponent({ if (!optionsRef.value?.contains(event.target as HTMLElement)) { api.closeListbox() - if (!event.defaultPrevented) buttonRef.value?.focus() } + if (!event.defaultPrevented) nextTick(() => buttonRef.value?.focus()) } window.addEventListener('click', handler) @@ -318,6 +318,7 @@ export const ListboxButton = defineComponent({ function handlePointerUp(event: MouseEvent) { if (api.listboxState.value === ListboxStates.Open) { api.closeListbox() + nextTick(() => api.buttonRef.value?.focus()) } else { event.preventDefault() api.openListbox() @@ -407,8 +408,8 @@ export const ListboxOptions = defineComponent({ if (api.activeOptionIndex.value !== null) { const { dataRef } = api.options.value[api.activeOptionIndex.value] api.select(dataRef.value) - nextTick(() => api.buttonRef.value?.focus()) } + nextTick(() => api.buttonRef.value?.focus()) break case Keys.ArrowDown: