From dbcfb23bc33a24fc7e4cbb56d82c1b9a933446ad Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 25 Jan 2023 13:18:51 +0100 Subject: [PATCH] Fix `FocusTrap` in `Dialog` when there is only 1 focusable element (#2172) * add tests to guarantee `FocusTrap` with a single element works as expected * it should keep the focus in the Dialog Even if there is only 1 element. We were skipping the current active element so the container didn't have any elements anymore and just continued to the next focusable element in line. This will prevent that and ensure that we can only skip elements if there are multiple ones. * update changelog --- packages/@headlessui-react/CHANGELOG.md | 1 + .../src/components/dialog/dialog.test.tsx | 102 ++++++++++++++- .../components/focus-trap/focus-trap.test.tsx | 94 ++++++++++++++ .../src/components/focus-trap/focus-trap.tsx | 10 +- .../src/utils/focus-management.ts | 2 +- packages/@headlessui-vue/CHANGELOG.md | 1 + .../src/components/dialog/dialog.test.ts | 122 +++++++++++++++++- .../components/focus-trap/focus-trap.test.ts | 54 ++++++++ .../src/components/focus-trap/focus-trap.ts | 10 +- .../src/utils/focus-management.ts | 2 +- 10 files changed, 386 insertions(+), 12 deletions(-) diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index 1da0b7a..b77c788 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/packages/@headlessui-react/CHANGELOG.md @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Don’t overwrite classes during SSR when rendering fragments ([#2173](https://github.com/tailwindlabs/headlessui/pull/2173)) - Improve `Combobox` accessibility ([#2153](https://github.com/tailwindlabs/headlessui/pull/2153)) - Fix crash when reading `headlessuiFocusGuard` of `relatedTarget` in the `FocusTrap` component ([#2203](https://github.com/tailwindlabs/headlessui/pull/2203)) +- Fix `FocusTrap` in `Dialog` when there is only 1 focusable element ([#2172](https://github.com/tailwindlabs/headlessui/pull/2172)) ## [1.7.7] - 2022-12-16 diff --git a/packages/@headlessui-react/src/components/dialog/dialog.test.tsx b/packages/@headlessui-react/src/components/dialog/dialog.test.tsx index 1f3efc9..f632115 100644 --- a/packages/@headlessui-react/src/components/dialog/dialog.test.tsx +++ b/packages/@headlessui-react/src/components/dialog/dialog.test.tsx @@ -21,7 +21,7 @@ import { getDialogs, getDialogOverlays, } from '../../test-utils/accessibility-assertions' -import { click, mouseDrag, press, Keys } from '../../test-utils/interactions' +import { click, mouseDrag, press, Keys, shift } from '../../test-utils/interactions' import { PropsOf } from '../../types' import { Transition } from '../transitions/transition' import { createPortal } from 'react-dom' @@ -797,6 +797,106 @@ describe('Keyboard interactions', () => { assertActiveElement(document.getElementById('a')) }) ) + + it( + 'should not escape the FocusTrap when there is only 1 focusable element (going forwards)', + suppressConsoleLogs(async () => { + function Example() { + let [isOpen, setIsOpen] = useState(false) + return ( + <> + + + + + + + + + + ) + } + render() + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + // Open dialog + await click(document.getElementById('trigger')) + + // Verify it is open + assertDialog({ + state: DialogState.Visible, + attributes: { id: 'headlessui-dialog-1' }, + }) + + // Verify that the input field is focused + assertActiveElement(document.getElementById('a')) + + // Verify that we stay within the Dialog + await press(Keys.Tab) + assertActiveElement(document.getElementById('a')) + + // Verify that we stay within the Dialog + await press(Keys.Tab) + assertActiveElement(document.getElementById('a')) + + // Verify that we stay within the Dialog + await press(Keys.Tab) + assertActiveElement(document.getElementById('a')) + }) + ) + + it( + 'should not escape the FocusTrap when there is only 1 focusable element (going backwards)', + suppressConsoleLogs(async () => { + function Example() { + let [isOpen, setIsOpen] = useState(false) + return ( + <> + + + + + + + + + + ) + } + render() + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + // Open dialog + await click(document.getElementById('trigger')) + + // Verify it is open + assertDialog({ + state: DialogState.Visible, + attributes: { id: 'headlessui-dialog-1' }, + }) + + // Verify that the input field is focused + assertActiveElement(document.getElementById('a')) + + // Verify that we stay within the Dialog + await press(shift(Keys.Tab)) + assertActiveElement(document.getElementById('a')) + + // Verify that we stay within the Dialog + await press(shift(Keys.Tab)) + assertActiveElement(document.getElementById('a')) + + // Verify that we stay within the Dialog + await press(shift(Keys.Tab)) + assertActiveElement(document.getElementById('a')) + }) + ) }) }) diff --git a/packages/@headlessui-react/src/components/focus-trap/focus-trap.test.tsx b/packages/@headlessui-react/src/components/focus-trap/focus-trap.test.tsx index 7f43d5c..054ed6d 100644 --- a/packages/@headlessui-react/src/components/focus-trap/focus-trap.test.tsx +++ b/packages/@headlessui-react/src/components/focus-trap/focus-trap.test.tsx @@ -108,6 +108,50 @@ it('should warn when there is no focusable element inside the FocusTrap', async spy.mockReset() }) +it( + 'should not be possible to programmatically escape the focus trap (if there is only 1 focusable element)', + suppressConsoleLogs(async () => { + function Example() { + return ( + <> + + + + + + + ) + } + + render() + + await nextFrame() + + let [a, b] = Array.from(document.querySelectorAll('input')) + + // Ensure that input-b is the active element + assertActiveElement(b) + + // Tab to the next item + await press(Keys.Tab) + + // Ensure that input-b is still the active element + assertActiveElement(b) + + // Try to move focus + a?.focus() + + // Ensure that input-b is still the active element + assertActiveElement(b) + + // Click on an element within the FocusTrap + await click(b) + + // Ensure that input-b is the active element + assertActiveElement(b) + }) +) + it( 'should not be possible to programmatically escape the focus trap', suppressConsoleLogs(async () => { @@ -214,6 +258,56 @@ it('should restore the previously focused element, before entering the FocusTrap assertActiveElement(document.getElementById('item-2')) }) +it('should stay in the FocusTrap when using `tab`, if there is only 1 focusable element', async () => { + render( + <> + + + + + + + ) + + await nextFrame() + + // Item A should be focused because the FocusTrap will focus the first item + assertActiveElement(document.getElementById('item-a')) + + // Next + await press(Keys.Tab) + assertActiveElement(document.getElementById('item-a')) + + // Next + await press(Keys.Tab) + assertActiveElement(document.getElementById('item-a')) +}) + +it('should stay in the FocusTrap when using `shift+tab`, if there is only 1 focusable element', async () => { + render( + <> + + + + + + + ) + + await nextFrame() + + // Item A should be focused because the FocusTrap will focus the first item + assertActiveElement(document.getElementById('item-a')) + + // Previous (loop around!) + await press(shift(Keys.Tab)) + assertActiveElement(document.getElementById('item-a')) + + // Previous + await press(shift(Keys.Tab)) + assertActiveElement(document.getElementById('item-a')) +}) + it('should be possible tab to the next focusable element within the focus trap', async () => { render( <> diff --git a/packages/@headlessui-react/src/components/focus-trap/focus-trap.tsx b/packages/@headlessui-react/src/components/focus-trap/focus-trap.tsx index 606d267..df13b43 100644 --- a/packages/@headlessui-react/src/components/focus-trap/focus-trap.tsx +++ b/packages/@headlessui-react/src/components/focus-trap/focus-trap.tsx @@ -85,10 +85,12 @@ export let FocusTrap = Object.assign( let wrapper = process.env.NODE_ENV === 'test' ? microTask : (cb: Function) => cb() wrapper(() => { match(direction.current, { - [TabDirection.Forwards]: () => - focusIn(el, Focus.First, { skipElements: [e.relatedTarget as HTMLElement] }), - [TabDirection.Backwards]: () => - focusIn(el, Focus.Last, { skipElements: [e.relatedTarget as HTMLElement] }), + [TabDirection.Forwards]: () => { + focusIn(el, Focus.First, { skipElements: [e.relatedTarget as HTMLElement] }) + }, + [TabDirection.Backwards]: () => { + focusIn(el, Focus.Last, { skipElements: [e.relatedTarget as HTMLElement] }) + }, }) }) }) diff --git a/packages/@headlessui-react/src/utils/focus-management.ts b/packages/@headlessui-react/src/utils/focus-management.ts index ab8971a..3301664 100644 --- a/packages/@headlessui-react/src/utils/focus-management.ts +++ b/packages/@headlessui-react/src/utils/focus-management.ts @@ -171,7 +171,7 @@ export function focusIn( : container : getFocusableElements(container) - if (skipElements.length > 0) { + if (skipElements.length > 0 && elements.length > 1) { elements = elements.filter((x) => !skipElements.includes(x)) } diff --git a/packages/@headlessui-vue/CHANGELOG.md b/packages/@headlessui-vue/CHANGELOG.md index 35069d5..cf7fb2a 100644 --- a/packages/@headlessui-vue/CHANGELOG.md +++ b/packages/@headlessui-vue/CHANGELOG.md @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Don’t overwrite classes during SSR when rendering fragments ([#2173](https://github.com/tailwindlabs/headlessui/pull/2173)) - Improve `Combobox` accessibility ([#2153](https://github.com/tailwindlabs/headlessui/pull/2153)) - Fix crash when reading `headlessuiFocusGuard` of `relatedTarget` in the `FocusTrap` component ([#2203](https://github.com/tailwindlabs/headlessui/pull/2203)) +- Fix `FocusTrap` in `Dialog` when there is only 1 focusable element ([#2172](https://github.com/tailwindlabs/headlessui/pull/2172)) ## [1.7.7] - 2022-12-16 diff --git a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts index 9d4bac9..3a9f600 100644 --- a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts +++ b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts @@ -30,7 +30,7 @@ import { getDialogs, getDialogOverlays, } from '../../test-utils/accessibility-assertions' -import { click, mouseDrag, press, Keys } from '../../test-utils/interactions' +import { click, mouseDrag, press, Keys, shift } from '../../test-utils/interactions' import { html } from '../../test-utils/html' // @ts-expect-error @@ -1074,6 +1074,126 @@ describe('Keyboard interactions', () => { assertActiveElement(document.getElementById('a')) }) ) + + it( + 'should not escape the FocusTrap when there is only 1 focusable element (going forwards)', + suppressConsoleLogs(async () => { + renderTemplate({ + template: ` +
+ + + + + + +
+ `, + setup() { + let isOpen = ref(false) + let initialFocusRef = ref(null) + return { + isOpen, + initialFocusRef, + setIsOpen(value: boolean) { + isOpen.value = value + }, + toggleOpen() { + isOpen.value = !isOpen.value + }, + } + }, + }) + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + // Open dialog + await click(document.getElementById('trigger')) + + // Verify it is open + assertDialog({ + state: DialogState.Visible, + attributes: { id: 'headlessui-dialog-1' }, + }) + + // Verify that the input field is focused + assertActiveElement(document.getElementById('a')) + + // Verify that we stay within the Dialog + await press(Keys.Tab) + assertActiveElement(document.getElementById('a')) + + // Verify that we stay within the Dialog + await press(Keys.Tab) + assertActiveElement(document.getElementById('a')) + + // Verify that we stay within the Dialog + await press(Keys.Tab) + assertActiveElement(document.getElementById('a')) + }) + ) + + it( + 'should not escape the FocusTrap when there is only 1 focusable element (going backwards)', + suppressConsoleLogs(async () => { + renderTemplate({ + template: ` +
+ + + + + + +
+ `, + setup() { + let isOpen = ref(false) + let initialFocusRef = ref(null) + return { + isOpen, + initialFocusRef, + setIsOpen(value: boolean) { + isOpen.value = value + }, + toggleOpen() { + isOpen.value = !isOpen.value + }, + } + }, + }) + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + // Open dialog + await click(document.getElementById('trigger')) + + // Verify it is open + assertDialog({ + state: DialogState.Visible, + attributes: { id: 'headlessui-dialog-1' }, + }) + + // Verify that the input field is focused + assertActiveElement(document.getElementById('a')) + + // Verify that we stay within the Dialog + await press(shift(Keys.Tab)) + assertActiveElement(document.getElementById('a')) + + // Verify that we stay within the Dialog + await press(shift(Keys.Tab)) + assertActiveElement(document.getElementById('a')) + + // Verify that we stay within the Dialog + await press(shift(Keys.Tab)) + assertActiveElement(document.getElementById('a')) + }) + ) }) }) diff --git a/packages/@headlessui-vue/src/components/focus-trap/focus-trap.test.ts b/packages/@headlessui-vue/src/components/focus-trap/focus-trap.test.ts index c7996c7..f3c4c58 100644 --- a/packages/@headlessui-vue/src/components/focus-trap/focus-trap.test.ts +++ b/packages/@headlessui-vue/src/components/focus-trap/focus-trap.test.ts @@ -227,6 +227,60 @@ it('should restore the previously focused element, before entering the FocusTrap assertActiveElement(document.getElementById('item-2')) }) +it('should stay in the FocusTrap when using `tab`, if there is only 1 focusable element', async () => { + renderTemplate({ + template: html` +
+ + + + + +
+ `, + }) + + await nextFrame() + + // Item A should be focused because the FocusTrap will focus the first item + assertActiveElement(document.getElementById('item-a')) + + // Next + await press(Keys.Tab) + assertActiveElement(document.getElementById('item-a')) + + // Next + await press(Keys.Tab) + assertActiveElement(document.getElementById('item-a')) +}) + +it('should stay in the FocusTrap when using `shift+tab`, if there is only 1 focusable element', async () => { + renderTemplate({ + template: html` +
+ + + + + +
+ `, + }) + + await nextFrame() + + // Item A should be focused because the FocusTrap will focus the first item + assertActiveElement(document.getElementById('item-a')) + + // Previous (loop around!) + await press(shift(Keys.Tab)) + assertActiveElement(document.getElementById('item-a')) + + // Previous + await press(shift(Keys.Tab)) + assertActiveElement(document.getElementById('item-a')) +}) + it('should be possible to tab to the next focusable element within the focus trap', async () => { renderTemplate( html` diff --git a/packages/@headlessui-vue/src/components/focus-trap/focus-trap.ts b/packages/@headlessui-vue/src/components/focus-trap/focus-trap.ts index 4cd3790..119ec91 100644 --- a/packages/@headlessui-vue/src/components/focus-trap/focus-trap.ts +++ b/packages/@headlessui-vue/src/components/focus-trap/focus-trap.ts @@ -89,10 +89,12 @@ export let FocusTrap = Object.assign( let wrapper = process.env.NODE_ENV === 'test' ? microTask : (cb: Function) => cb() wrapper(() => { match(direction.value, { - [TabDirection.Forwards]: () => - focusIn(el, Focus.First, { skipElements: [e.relatedTarget as HTMLElement] }), - [TabDirection.Backwards]: () => - focusIn(el, Focus.Last, { skipElements: [e.relatedTarget as HTMLElement] }), + [TabDirection.Forwards]: () => { + focusIn(el, Focus.First, { skipElements: [e.relatedTarget as HTMLElement] }) + }, + [TabDirection.Backwards]: () => { + focusIn(el, Focus.Last, { skipElements: [e.relatedTarget as HTMLElement] }) + }, }) }) } diff --git a/packages/@headlessui-vue/src/utils/focus-management.ts b/packages/@headlessui-vue/src/utils/focus-management.ts index 9ec2cd5..846f666 100644 --- a/packages/@headlessui-vue/src/utils/focus-management.ts +++ b/packages/@headlessui-vue/src/utils/focus-management.ts @@ -165,7 +165,7 @@ export function focusIn( : container : getFocusableElements(container) - if (skipElements.length > 0) { + if (skipElements.length > 0 && elements.length > 1) { elements = elements.filter((x) => !skipElements.includes(x)) }