From 7a6220a29c491b0a079b354d3d15ae784fdbb425 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Tue, 12 Jul 2022 13:57:07 +0200 Subject: [PATCH] Prevent cancelling transitions due to focus trap (#1664) * focus trap after transitions Whenever you focus an element mid transition, the browser will abrublty stop/cancel the transition and it will do anything it can to focus the element. This has a very annoying side effect that this causes very abrubt transitions (instant) in some browsers. To fix this, we used to use `el.focus({ preventScroll: true })` which works in Safari and it used to work in Chrome / Firefox, but there are probably other variables that we have to keep in mind here (didn't figure out _why_ this used to work and not anymore). That said, instead of trying to fight the browser, we will now wait an animation frame before even trying to focus any elements. * update changelog --- packages/@headlessui-vue/CHANGELOG.md | 4 ++ .../src/components/dialog/dialog.test.ts | 14 ++++++- .../components/focus-trap/focus-trap.test.ts | 38 ++++++++++++++---- .../src/components/focus-trap/focus-trap.ts | 39 ++++++++++--------- 4 files changed, 68 insertions(+), 27 deletions(-) diff --git a/packages/@headlessui-vue/CHANGELOG.md b/packages/@headlessui-vue/CHANGELOG.md index 1d78222139..cba47f3a27 100644 --- a/packages/@headlessui-vue/CHANGELOG.md +++ b/packages/@headlessui-vue/CHANGELOG.md @@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add `by` prop for `Listbox`, `Combobox` and `RadioGroup` ([#1482](https://github.com/tailwindlabs/headlessui/pull/1482)) - Add `@headlessui/tailwindcss` plugin ([#1487](https://github.com/tailwindlabs/headlessui/pull/1487)) +### Fixed + +- Prevent cancelling transitions due to focus trap ([#1664](https://github.com/tailwindlabs/headlessui/pull/1664)) + ## [1.6.6] - 2022-07-07 ### Fixed diff --git a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts index 0087fa5116..c5d9b3776a 100644 --- a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts +++ b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts @@ -36,6 +36,16 @@ global.IntersectionObserver = class FakeIntersectionObserver { afterAll(() => jest.restoreAllMocks()) +function nextFrame() { + return new Promise((resolve) => { + requestAnimationFrame(() => { + requestAnimationFrame(() => { + resolve() + }) + }) + }) +} + let TabSentinel = defineComponent({ name: 'TabSentinel', template: html`
`, @@ -243,6 +253,8 @@ describe('Rendering', () => { await click(document.getElementById('trigger')) + await new Promise(nextTick) + assertDialog({ state: DialogState.Visible, attributes: { class: 'relative bg-blue-500' } }) }) @@ -263,7 +275,7 @@ describe('Rendering', () => { }, }) - await new Promise(nextTick) + await nextFrame() // Let's verify that the Dialog is already there expect(getDialog()).not.toBe(null) 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 fe3e4ba647..16ed5f3448 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 @@ -16,6 +16,16 @@ beforeAll(() => { afterAll(() => jest.restoreAllMocks()) +function nextFrame() { + return new Promise((resolve) => { + requestAnimationFrame(() => { + requestAnimationFrame(() => { + resolve() + }) + }) + }) +} + const renderTemplate = createRenderTemplate({ FocusTrap, }) @@ -29,7 +39,7 @@ it('should focus the first focusable element inside the FocusTrap', async () => ` ) - await new Promise(nextTick) + await nextFrame() assertActiveElement(getByText('Trigger')) }) @@ -52,7 +62,7 @@ it('should focus the autoFocus element inside the FocusTrap if that exists', asy }, }) - await new Promise(nextTick) + await nextFrame() assertActiveElement(document.getElementById('b')) }) @@ -72,7 +82,7 @@ it('should focus the initialFocus element inside the FocusTrap if that exists', }, }) - await new Promise(nextTick) + await nextFrame() assertActiveElement(document.getElementById('c')) }) @@ -92,7 +102,7 @@ it('should focus the initialFocus element inside the FocusTrap even if another e }, }) - await new Promise(nextTick) + await nextFrame() assertActiveElement(document.getElementById('c')) }) @@ -109,7 +119,7 @@ it('should warn when there is no focusable element inside the FocusTrap', async ` ) - await new Promise(nextTick) + await nextFrame() expect(spy.mock.calls[0][0]).toBe('There are no focusable elements inside the ') spy.mockReset() @@ -132,7 +142,7 @@ it( `, }) - await new Promise(nextTick) + await nextFrame() let [a, b, c, d] = Array.from(document.querySelectorAll('input')) @@ -199,7 +209,7 @@ it('should restore the previously focused element, before entering the FocusTrap }, }) - await new Promise(nextTick) + await nextFrame() // The input should have focus by default because of the autoFocus prop assertActiveElement(document.getElementById('item-1')) @@ -232,7 +242,7 @@ it('should be possible to tab to the next focusable element within the focus tra ` ) - await new Promise(nextTick) + await nextFrame() // Item A should be focused because the FocusTrap will focus the first item assertActiveElement(document.getElementById('item-a')) @@ -265,6 +275,8 @@ it('should be possible to shift+tab to the previous focusable element within the ` ) + await nextFrame() + // Item A should be focused because the FocusTrap will focus the first item assertActiveElement(document.getElementById('item-a')) @@ -297,6 +309,8 @@ it('should skip the initial "hidden" elements within the focus trap', async () = ` ) + await nextFrame() + // Item C should be focused because the FocusTrap had to skip the first 2 assertActiveElement(document.getElementById('item-c')) }) @@ -317,6 +331,8 @@ it('should be possible skip "hidden" elements within the focus trap', async () = ` ) + await nextFrame() + // Item A should be focused because the FocusTrap will focus the first item assertActiveElement(document.getElementById('item-a')) @@ -351,6 +367,8 @@ it('should be possible skip disabled elements within the focus trap', async () = ` ) + await nextFrame() + // Item A should be focused because the FocusTrap will focus the first item assertActiveElement(document.getElementById('item-a')) @@ -397,6 +415,8 @@ it('should try to focus all focusable items in order (and fail)', async () => { }, }) + await nextFrame() + expect(focusHandler.mock.calls).toEqual([['item-a'], ['item-b'], ['item-c'], ['item-d']]) expect(spy).toHaveBeenCalledWith('There are no focusable elements inside the ') spy.mockReset() @@ -430,6 +450,8 @@ it('should end up at the last focusable element', async () => { }, }) + await nextFrame() + expect(focusHandler.mock.calls).toEqual([['item-a'], ['item-b'], ['item-c']]) assertActiveElement(getByText('Item D')) expect(spy).not.toHaveBeenCalled() 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 d9d5b347e6..b6b1cebbb9 100644 --- a/packages/@headlessui-vue/src/components/focus-trap/focus-trap.ts +++ b/packages/@headlessui-vue/src/components/focus-trap/focus-trap.ts @@ -20,6 +20,7 @@ import { useTabDirection, Direction as TabDirection } from '../../hooks/use-tab- import { getOwnerDocument } from '../../utils/owner' import { useEventListener } from '../../hooks/use-event-listener' import { microTask } from '../../utils/micro-task' +// import { disposables } from '../../utils/disposables' enum Features { /** No features enabled for the focus trap. */ @@ -102,7 +103,7 @@ export let FocusTrap = Object.assign( return () => { let slot = {} - let ourProps = { 'data-hi': 'container', ref: container } + let ourProps = { ref: container } let { features, initialFocus, containers: _containers, ...theirProps } = props return h(Fragment, [ @@ -206,30 +207,32 @@ function useInitialFocus( let containerElement = dom(container) if (!containerElement) return - let initialFocusElement = dom(initialFocus) + requestAnimationFrame(() => { + let initialFocusElement = dom(initialFocus) - let activeElement = ownerDocument.value?.activeElement as HTMLElement + let activeElement = ownerDocument.value?.activeElement as HTMLElement - if (initialFocusElement) { - if (initialFocusElement === activeElement) { + if (initialFocusElement) { + if (initialFocusElement === activeElement) { + previousActiveElement.value = activeElement + return // Initial focus ref is already the active element + } + } else if (containerElement!.contains(activeElement)) { previousActiveElement.value = activeElement - return // Initial focus ref is already the active element + return // Already focused within Dialog } - } else if (containerElement.contains(activeElement)) { - previousActiveElement.value = activeElement - return // Already focused within Dialog - } - // Try to focus the initialFocus ref - if (initialFocusElement) { - focusElement(initialFocusElement) - } else { - if (focusIn(containerElement, Focus.First) === FocusResult.Error) { - console.warn('There are no focusable elements inside the ') + // Try to focus the initialFocus ref + if (initialFocusElement) { + focusElement(initialFocusElement) + } else { + if (focusIn(containerElement!, Focus.First | Focus.NoScroll) === FocusResult.Error) { + console.warn('There are no focusable elements inside the ') + } } - } - previousActiveElement.value = ownerDocument.value?.activeElement as HTMLElement + previousActiveElement.value = ownerDocument.value?.activeElement as HTMLElement + }) }, { immediate: true, flush: 'post' } )