From 33d49e3ca8c330bfe3e74573e5725cee82e8814b Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Tue, 23 Feb 2021 13:26:01 +0100 Subject: [PATCH] Fix refocus button bug (#256) * fix outside click on span inside button works as expected We have `outside click` behaviour implemented. Whenever the target element is focusable we make sure that the newly clicked/focused element stays focused. If it is not a focusable element we will make sure that the Menu/Listbox button is re-focused so that screenreader users don't get confused. This is all fine, but it turns out that when you have a button with a span, and you click on the span, then the event.target will be that span. The span itself is not focusable of course, but the button will get the focus. This results in the Menu/Listbox button being re-focused which is incorrect. For this we will introduce a FocusableMode on the `isFocusableElement`, we will have a `Strict` mode, which means the actual element should be focusable. And a `Loose` mode, which means that the actual element can be inside a focusable element. E.g.: A span within a button. * rename menu to listbox Copy paste can be fun sometimes * update changelog --- CHANGELOG.md | 4 +- .../src/components/listbox/listbox.test.tsx | 56 ++++++++++++++++--- .../src/components/listbox/listbox.tsx | 4 +- .../src/components/menu/menu.test.tsx | 42 ++++++++++++++ .../src/components/menu/menu.tsx | 4 +- .../src/test-utils/interactions.ts | 11 +++- .../src/utils/focus-management.ts | 45 ++++++++++++--- 7 files changed, 142 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d77bca5c72..42d432c9f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,8 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixes -- Fixed `outside click` not re-focusing the `Menu.Button` ([#220](https://github.com/tailwindlabs/headlessui/pull/220)) -- Fixed `outside click` not re-focusing the `Listbox.Button` ([#220](https://github.com/tailwindlabs/headlessui/pull/220)) +- Fixed `outside click` not re-focusing the `Menu.Button` ([#220](https://github.com/tailwindlabs/headlessui/pull/220), [#256](https://github.com/tailwindlabs/headlessui/pull/256)) +- Fixed `outside click` not re-focusing the `Listbox.Button` ([#220](https://github.com/tailwindlabs/headlessui/pull/220), [#256](https://github.com/tailwindlabs/headlessui/pull/256)) - Fix incorrect type error `unique symbol` ([#248](https://github.com/tailwindlabs/headlessui/pull/248), [#240](https://github.com/tailwindlabs/headlessui/issues/240)) ### Added diff --git a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx b/packages/@headlessui-react/src/components/listbox/listbox.test.tsx index fc6691a5b0..2f7fac76f3 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx +++ b/packages/@headlessui-react/src/components/listbox/listbox.test.tsx @@ -34,6 +34,7 @@ import { getListboxOptions, getListboxLabel, ListboxState, + getByText, } from '../../test-utils/accessibility-assertions' jest.mock('../../hooks/use-id') @@ -2878,7 +2879,7 @@ describe('Mouse interactions', () => { }) assertListbox({ state: ListboxState.InvisibleUnmounted }) - // Try to open the menu + // Try to open the listbox await click(getListboxButton(), MouseButton.Right) // Verify it is still closed @@ -3071,19 +3072,19 @@ describe('Mouse interactions', () => { let [button1, button2] = getListboxButtons() - // Click the first menu button + // Click the first listbox button await click(button1) - expect(getListboxes()).toHaveLength(1) // Only 1 menu should be visible + expect(getListboxes()).toHaveLength(1) // Only 1 listbox should be visible - // Ensure the open menu is linked to the first button + // Ensure the open listbox is linked to the first button assertListboxButtonLinkedWithListbox(button1, getListbox()) - // Click the second menu button + // Click the second listbox button await click(button2) - expect(getListboxes()).toHaveLength(1) // Only 1 menu should be visible + expect(getListboxes()).toHaveLength(1) // Only 1 listbox should be visible - // Ensure the open menu is linked to the second button + // Ensure the open listbox is linked to the second button assertListboxButtonLinkedWithListbox(button2, getListbox()) }) ) @@ -3118,6 +3119,47 @@ describe('Mouse interactions', () => { }) ) + it( + 'should be possible to click outside of the listbox, on an element which is within a focusable element, which closes the listbox', + suppressConsoleLogs(async () => { + let focusFn = jest.fn() + render( +
+ + Trigger + + alice + bob + charlie + + + + +
+ ) + + // Click the listbox button + await click(getListboxButton()) + + // Ensure the listbox is open + assertListbox({ state: ListboxState.Visible }) + + // Click the span inside the button + await click(getByText('Next')) + + // Ensure the listbox is closed + assertListbox({ state: ListboxState.InvisibleUnmounted }) + + // Ensure the outside button is focused + assertActiveElement(document.getElementById('btn')) + + // Ensure that the focus button only got focus once (first click) + expect(focusFn).toHaveBeenCalledTimes(1) + }) + ) + it( 'should be possible to hover an option and make it active', suppressConsoleLogs(async () => { diff --git a/packages/@headlessui-react/src/components/listbox/listbox.tsx b/packages/@headlessui-react/src/components/listbox/listbox.tsx index 3e799cdff7..3ac55fa31d 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.tsx +++ b/packages/@headlessui-react/src/components/listbox/listbox.tsx @@ -31,7 +31,7 @@ import { Keys } from '../keyboard' import { Focus, calculateActiveIndex } from '../../utils/calculate-active-index' import { resolvePropValue } from '../../utils/resolve-prop-value' import { isDisabledReactIssue7711 } from '../../utils/bugs' -import { isFocusableElement } from '../../utils/focus-management' +import { isFocusableElement, FocusableMode } from '../../utils/focus-management' enum ListboxStates { Open, @@ -229,7 +229,7 @@ export function Listbox { }) ) + it( + 'should be possible to click outside of the menu, on an element which is within a focusable element, which closes the menu', + suppressConsoleLogs(async () => { + let focusFn = jest.fn() + render( +
+ + Trigger + + alice + bob + charlie + + + + +
+ ) + + // Click the menu button + await click(getMenuButton()) + + // Ensure the menu is open + assertMenu({ state: MenuState.Visible }) + + // Click the span inside the button + await click(getByText('Next')) + + // Ensure the menu is closed + assertMenu({ state: MenuState.InvisibleUnmounted }) + + // Ensure the outside button is focused + assertActiveElement(document.getElementById('btn')) + + // Ensure that the focus button only got focus once (first click) + expect(focusFn).toHaveBeenCalledTimes(1) + }) + ) + it( 'should be possible to hover an item and make it active', suppressConsoleLogs(async () => { diff --git a/packages/@headlessui-react/src/components/menu/menu.tsx b/packages/@headlessui-react/src/components/menu/menu.tsx index 25e8f7dc17..97132154fb 100644 --- a/packages/@headlessui-react/src/components/menu/menu.tsx +++ b/packages/@headlessui-react/src/components/menu/menu.tsx @@ -31,7 +31,7 @@ import { Keys } from '../keyboard' import { Focus, calculateActiveIndex } from '../../utils/calculate-active-index' import { resolvePropValue } from '../../utils/resolve-prop-value' import { isDisabledReactIssue7711 } from '../../utils/bugs' -import { isFocusableElement } from '../../utils/focus-management' +import { isFocusableElement, FocusableMode } from '../../utils/focus-management' enum MenuStates { Open, @@ -185,7 +185,7 @@ export function Menu( dispatch({ type: ActionTypes.CloseMenu }) - if (!isFocusableElement(target)) { + if (!isFocusableElement(target, FocusableMode.Loose)) { event.preventDefault() buttonRef.current?.focus() } diff --git a/packages/@headlessui-react/src/test-utils/interactions.ts b/packages/@headlessui-react/src/test-utils/interactions.ts index b7a2c6e332..2d839ce19a 100644 --- a/packages/@headlessui-react/src/test-utils/interactions.ts +++ b/packages/@headlessui-react/src/test-utils/interactions.ts @@ -191,9 +191,14 @@ export async function click( fireEvent.mouseDown(element, options) } - // Ensure to trigger a `focus` event if the element is focusable - if ((element as HTMLElement)?.matches(focusableSelector)) { - ;(element as HTMLElement).focus() + // Ensure to trigger a `focus` event if the element is focusable, or within a focusable element + let next: HTMLElement | null = element as HTMLElement | null + while (next !== null) { + if (next.matches(focusableSelector)) { + next.focus() + break + } + next = next.parentElement } fireEvent.pointerUp(element, options) diff --git a/packages/@headlessui-react/src/utils/focus-management.ts b/packages/@headlessui-react/src/utils/focus-management.ts index d95d20b579..311db48f9b 100644 --- a/packages/@headlessui-react/src/utils/focus-management.ts +++ b/packages/@headlessui-react/src/utils/focus-management.ts @@ -1,3 +1,5 @@ +import { match } from './match' + // Credit: // - https://stackoverflow.com/a/30753870 let focusableSelector = [ @@ -22,22 +24,22 @@ let focusableSelector = [ .join(',') export enum Focus { - /* Focus the first non-disabled element */ + /** Focus the first non-disabled element */ First = 1 << 0, - /* Focus the previous non-disabled element */ + /** Focus the previous non-disabled element */ Previous = 1 << 1, - /* Focus the next non-disabled element */ + /** Focus the next non-disabled element */ Next = 1 << 2, - /* Focus the last non-disabled element */ + /** Focus the last non-disabled element */ Last = 1 << 3, - /* Wrap tab around */ + /** Wrap tab around */ WrapAround = 1 << 4, - /* Prevent scrolling the focusable elements into view */ + /** Prevent scrolling the focusable elements into view */ NoScroll = 1 << 5, } @@ -58,8 +60,35 @@ export function getFocusableElements(container: HTMLElement | null = document.bo return Array.from(container.querySelectorAll(focusableSelector)) } -export function isFocusableElement(element: HTMLElement) { - return element.matches(focusableSelector) +export enum FocusableMode { + /** The element itself must be focusable. */ + Strict, + + /** The element should be inside of a focusable element. */ + Loose, +} + +export function isFocusableElement( + element: HTMLElement, + mode: FocusableMode = FocusableMode.Strict +) { + if (element === document.body) return false + + return match(mode, { + [FocusableMode.Strict]() { + return element.matches(focusableSelector) + }, + [FocusableMode.Loose]() { + let next: HTMLElement | null = element + + while (next !== null) { + if (next.matches(focusableSelector)) return true + next = next.parentElement + } + + return false + }, + }) } export function focusElement(element: HTMLElement | null) {