From f2c2d3c4e0cf3ea2179366b0ffef90b815209703 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Tue, 26 Jul 2022 15:29:49 +0200 Subject: [PATCH] Fix incorrect scrolling to the bottom when opening a `Dialog` (#1716) * add `?raw` option to playground This will render the component as-is without the wrapper. * delay initial focus and make consistent between React and Vue This will delay the initial focus and makes it consistent between React and Vue. Some explanation from within the code why this is happening: Delaying the focus to the next microtask ensures that a few conditions are true: - The container is rendered - Transitions could be started If we don't do this, then focusing an element will immediately cancel any transitions. This is not ideal because transitions will look broken. There is an additional issue with doing this immediately. The FocusTrap is used inside a Dialog, the Dialog is rendered inside of a Portal and the Portal is rendered at the end of the `document.body`. This means that the moment we call focus, the browser immediately tries to focus the element, which will still be at the bodem resulting in the page to scroll down. Delaying this will prevent the page to scroll down entirely. * update test to reflect initial focus delay Now that we are triggering the initial focus inside a `queueMicroTask` we have to make sure that our tests wait a frame so that the micro task could run, otherwise we will have incorrect results. Also make the implementation similar in React and Vue * update changelog --- packages/@headlessui-react/CHANGELOG.md | 1 + .../src/components/dialog/dialog.test.tsx | 44 ++++++++++++++-- .../components/focus-trap/focus-trap.test.tsx | 52 +++++++++++++++++-- .../src/components/focus-trap/focus-trap.tsx | 50 ++++++++++++------ packages/@headlessui-vue/CHANGELOG.md | 1 + .../src/components/dialog/dialog.test.ts | 26 +++++----- .../src/components/focus-trap/focus-trap.ts | 23 ++++++-- packages/playground-react/pages/_app.tsx | 6 +++ 8 files changed, 164 insertions(+), 39 deletions(-) diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index 8e047e5746..f9c82edb81 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/packages/@headlessui-react/CHANGELOG.md @@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Don't scroll lock when a Transition + Dialog is mounted but hidden ([#1681](https://github.com/tailwindlabs/headlessui/pull/1681)) - Improve outside click on Safari iOS ([#1712](https://github.com/tailwindlabs/headlessui/pull/1712)) - Improve event handler merging ([#1715](https://github.com/tailwindlabs/headlessui/pull/1715)) +- Fix incorrect scrolling to the bottom when opening a `Dialog` ([#1716](https://github.com/tailwindlabs/headlessui/pull/1716)) ## [1.6.6] - 2022-07-07 diff --git a/packages/@headlessui-react/src/components/dialog/dialog.test.tsx b/packages/@headlessui-react/src/components/dialog/dialog.test.tsx index bf339ef86a..985f4050fb 100644 --- a/packages/@headlessui-react/src/components/dialog/dialog.test.tsx +++ b/packages/@headlessui-react/src/components/dialog/dialog.test.tsx @@ -32,8 +32,18 @@ global.IntersectionObserver = class FakeIntersectionObserver { afterAll(() => jest.restoreAllMocks()) -function TabSentinel(props: PropsOf<'div'>) { - return
+function nextFrame() { + return new Promise((resolve) => { + requestAnimationFrame(() => { + requestAnimationFrame(() => { + resolve() + }) + }) + }) +} + +function TabSentinel(props: PropsOf<'button'>) { + return
) } + render() + await nextFrame() + // Verify it is open assertDialog({ state: DialogState.Visible }) @@ -905,8 +931,11 @@ describe('Mouse interactions', () => { ) } + render() + await nextFrame() + // Verify it is open assertDialog({ state: DialogState.Visible }) @@ -936,8 +965,11 @@ describe('Mouse interactions', () => { ) } + render() + await nextFrame() + // Verify it is open assertDialog({ state: DialogState.Visible }) @@ -979,8 +1011,11 @@ describe('Mouse interactions', () => { ) } + render() + await nextFrame() + // Verify it is open assertDialog({ state: DialogState.Visible }) @@ -1023,8 +1058,11 @@ describe('Mouse interactions', () => { ) } + render() + await nextFrame() + // Verify it is open assertDialog({ state: DialogState.Visible }) 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 f342d1f7be..23a5a4e777 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 @@ -6,17 +6,29 @@ import { assertActiveElement } from '../../test-utils/accessibility-assertions' import { suppressConsoleLogs } from '../../test-utils/suppress-console-logs' import { click, press, shift, Keys } from '../../test-utils/interactions' -it('should focus the first focusable element inside the FocusTrap', () => { +function nextFrame() { + return new Promise((resolve) => { + requestAnimationFrame(() => { + requestAnimationFrame(() => { + resolve() + }) + }) + }) +} + +it('should focus the first focusable element inside the FocusTrap', async () => { render( ) + await nextFrame() + assertActiveElement(screen.getByText('Trigger')) }) -it('should focus the autoFocus element inside the FocusTrap if that exists', () => { +it('should focus the autoFocus element inside the FocusTrap if that exists', async () => { render( @@ -25,10 +37,12 @@ it('should focus the autoFocus element inside the FocusTrap if that exists', () ) + await nextFrame() + assertActiveElement(document.getElementById('b')) }) -it('should focus the initialFocus element inside the FocusTrap if that exists', () => { +it('should focus the initialFocus element inside the FocusTrap if that exists', async () => { function Example() { let initialFocusRef = useRef(null) @@ -40,12 +54,15 @@ it('should focus the initialFocus element inside the FocusTrap if that exists', ) } + render() + await nextFrame() + assertActiveElement(document.getElementById('c')) }) -it('should focus the initialFocus element inside the FocusTrap even if another element has autoFocus', () => { +it('should focus the initialFocus element inside the FocusTrap even if another element has autoFocus', async () => { function Example() { let initialFocusRef = useRef(null) @@ -57,12 +74,15 @@ it('should focus the initialFocus element inside the FocusTrap even if another e ) } + render() + await nextFrame() + assertActiveElement(document.getElementById('c')) }) -it('should warn when there is no focusable element inside the FocusTrap', () => { +it('should warn when there is no focusable element inside the FocusTrap', async () => { let spy = jest.spyOn(console, 'warn').mockImplementation(jest.fn()) function Example() { @@ -72,7 +92,11 @@ it('should warn when there is no focusable element inside the FocusTrap', () => ) } + render() + + await nextFrame() + expect(spy.mock.calls[0][0]).toBe('There are no focusable elements inside the ') spy.mockReset() }) @@ -96,6 +120,8 @@ it( render() + await nextFrame() + let [a, b, c, d] = Array.from(document.querySelectorAll('input')) // Ensure that input-b is the active element @@ -163,6 +189,8 @@ it('should restore the previously focused element, before entering the FocusTrap render() + await nextFrame() + // The input should have focus by default because of the autoFocus prop assertActiveElement(document.getElementById('item-1')) @@ -192,6 +220,8 @@ it('should be possible tab to the next focusable element within the focus trap', ) + await nextFrame() + // Item A should be focused because the FocusTrap will focus the first item assertActiveElement(document.getElementById('item-a')) @@ -221,6 +251,8 @@ it('should be possible shift+tab to the previous focusable element within the fo ) + await nextFrame() + // Item A should be focused because the FocusTrap will focus the first item assertActiveElement(document.getElementById('item-a')) @@ -255,6 +287,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')) }) @@ -275,6 +309,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')) @@ -309,6 +345,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')) @@ -357,6 +395,8 @@ it('should try to focus all focusable items (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() @@ -391,6 +431,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(screen.getByText('Item D')) expect(spy).not.toHaveBeenCalled() 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 bbab04b213..2db6dc6679 100644 --- a/packages/@headlessui-react/src/components/focus-trap/focus-trap.tsx +++ b/packages/@headlessui-react/src/components/focus-trap/focus-trap.tsx @@ -185,34 +185,52 @@ function useInitialFocus( ) { let previousActiveElement = useRef(null) + let mounted = useIsMounted() + // Handle initial focus useWatch(() => { if (!enabled) return let containerElement = container.current if (!containerElement) return - let activeElement = ownerDocument?.activeElement as HTMLElement + // Delaying the focus to the next microtask ensures that a few conditions are true: + // - The container is rendered + // - Transitions could be started + // If we don't do this, then focusing an element will immediately cancel any transitions. This + // is not ideal because transitions will look broken. + // There is an additional issue with doing this immediately. The FocusTrap is used inside a + // Dialog, the Dialog is rendered inside of a Portal and the Portal is rendered at the end of + // the `document.body`. This means that the moment we call focus, the browser immediately + // tries to focus the element, which will still be at the bodem resulting in the page to + // scroll down. Delaying this will prevent the page to scroll down entirely. + microTask(() => { + if (!mounted.current) { + return + } - if (initialFocus?.current) { - if (initialFocus?.current === activeElement) { + let activeElement = ownerDocument?.activeElement as HTMLElement + + if (initialFocus?.current) { + if (initialFocus?.current === activeElement) { + previousActiveElement.current = activeElement + return // Initial focus ref is already the active element + } + } else if (containerElement!.contains(activeElement)) { previousActiveElement.current = activeElement - return // Initial focus ref is already the active element + return // Already focused within Dialog } - } else if (containerElement.contains(activeElement)) { - previousActiveElement.current = activeElement - return // Already focused within Dialog - } - // Try to focus the initialFocus ref - if (initialFocus?.current) { - focusElement(initialFocus.current) - } else { - if (focusIn(containerElement, Focus.First) === FocusResult.Error) { - console.warn('There are no focusable elements inside the ') + // Try to focus the initialFocus ref + if (initialFocus?.current) { + focusElement(initialFocus.current) + } else { + if (focusIn(containerElement!, Focus.First) === FocusResult.Error) { + console.warn('There are no focusable elements inside the ') + } } - } - previousActiveElement.current = ownerDocument?.activeElement as HTMLElement + previousActiveElement.current = ownerDocument?.activeElement as HTMLElement + }) }, [enabled]) return previousActiveElement diff --git a/packages/@headlessui-vue/CHANGELOG.md b/packages/@headlessui-vue/CHANGELOG.md index 16e1f9827a..7c50b87d00 100644 --- a/packages/@headlessui-vue/CHANGELOG.md +++ b/packages/@headlessui-vue/CHANGELOG.md @@ -23,6 +23,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Ensure controlled `Tabs` don't change automagically ([#1680](https://github.com/tailwindlabs/headlessui/pull/1680)) - Improve outside click on Safari iOS ([#1712](https://github.com/tailwindlabs/headlessui/pull/1712)) - Improve event handler merging ([#1715](https://github.com/tailwindlabs/headlessui/pull/1715)) +- Fix incorrect scrolling to the bottom when opening a `Dialog` ([#1716](https://github.com/tailwindlabs/headlessui/pull/1716)) ## [1.6.7] - 2022-07-12 diff --git a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts index d2333ca77d..c0d3c668d3 100644 --- a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts +++ b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts @@ -48,7 +48,7 @@ function nextFrame() { let TabSentinel = defineComponent({ name: 'TabSentinel', - template: html`
`, + template: html``, }) jest.mock('../../hooks/use-id') @@ -253,7 +253,7 @@ describe('Rendering', () => { await click(document.getElementById('trigger')) - await new Promise(nextTick) + await nextFrame() assertDialog({ state: DialogState.Visible, attributes: { class: 'relative bg-blue-500' } }) }) @@ -299,7 +299,7 @@ describe('Rendering', () => { }, }) - await new Promise(nextTick) + await nextFrame() // Let's verify that the Dialog is already there expect(getDialog()).not.toBe(null) @@ -329,7 +329,7 @@ describe('Rendering', () => { }, }) - await new Promise(nextTick) + await nextFrame() assertDialog({ state: DialogState.InvisibleHidden }) expect(focusCounter).toHaveBeenCalledTimes(0) @@ -637,7 +637,7 @@ describe('Rendering', () => { ` ) - await new Promise(nextTick) + await nextFrame() assertDialog({ state: DialogState.Visible, @@ -664,7 +664,7 @@ describe('Rendering', () => { ` ) - await new Promise(nextTick) + await nextFrame() assertDialog({ state: DialogState.Visible, @@ -695,7 +695,7 @@ describe('Composition', () => { `, }) - await new Promise(nextTick) + await nextFrame() assertDialog({ state: DialogState.Visible }) assertDialogDescription({ @@ -720,6 +720,8 @@ describe('Composition', () => { `, }) + await nextFrame() + assertDialog({ state: DialogState.InvisibleUnmounted }) }) ) @@ -1214,7 +1216,7 @@ describe('Mouse interactions', () => { }, }) - await new Promise(nextTick) + await nextFrame() // Verify it is open assertDialog({ state: DialogState.Visible }) @@ -1259,7 +1261,7 @@ describe('Mouse interactions', () => { }, }) - await new Promise(nextTick) + await nextFrame() // Verify it is open assertDialog({ state: DialogState.Visible }) @@ -1298,7 +1300,7 @@ describe('Mouse interactions', () => { }, }) - await new Promise(nextTick) + await nextFrame() // Verify it is open assertDialog({ state: DialogState.Visible }) @@ -1344,7 +1346,7 @@ describe('Mouse interactions', () => { }, }) - await new Promise(nextTick) + await nextFrame() // Verify it is open assertDialog({ state: DialogState.Visible }) @@ -1396,7 +1398,7 @@ describe('Mouse interactions', () => { }, }) - await new Promise(nextTick) + await nextFrame() // Verify it is open assertDialog({ state: DialogState.Visible }) 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 1db762823f..8b6b421481 100644 --- a/packages/@headlessui-vue/src/components/focus-trap/focus-trap.ts +++ b/packages/@headlessui-vue/src/components/focus-trap/focus-trap.ts @@ -3,6 +3,7 @@ import { defineComponent, h, onMounted, + onUnmounted, ref, watch, @@ -10,7 +11,6 @@ import { PropType, Fragment, Ref, - onUnmounted, } from 'vue' import { render } from '../../utils/render' import { Hidden, Features as HiddenFeatures } from '../../internal/hidden' @@ -21,7 +21,6 @@ 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. */ @@ -191,6 +190,10 @@ function useInitialFocus( ) { let previousActiveElement = ref(null) + let mounted = ref(false) + onMounted(() => (mounted.value = true)) + onUnmounted(() => (mounted.value = false)) + onMounted(() => { watch( // Handle initial focus @@ -202,7 +205,21 @@ function useInitialFocus( let containerElement = dom(container) if (!containerElement) return - requestAnimationFrame(() => { + // Delaying the focus to the next microtask ensures that a few conditions are true: + // - The container is rendered + // - Transitions could be started + // If we don't do this, then focusing an element will immediately cancel any transitions. This + // is not ideal because transitions will look broken. + // There is an additional issue with doing this immediately. The FocusTrap is used inside a + // Dialog, the Dialog is rendered inside of a Portal and the Portal is rendered at the end of + // the `document.body`. This means that the moment we call focus, the browser immediately + // tries to focus the element, which will still be at the bodem resulting in the page to + // scroll down. Delaying this will prevent the page to scroll down entirely. + microTask(() => { + if (!mounted.value) { + return + } + let initialFocusElement = dom(initialFocus) let activeElement = ownerDocument.value?.activeElement as HTMLElement diff --git a/packages/playground-react/pages/_app.tsx b/packages/playground-react/pages/_app.tsx index c59866797f..bed7351c59 100644 --- a/packages/playground-react/pages/_app.tsx +++ b/packages/playground-react/pages/_app.tsx @@ -3,6 +3,7 @@ import Link from 'next/link' import Head from 'next/head' import 'tailwindcss/tailwind.css' +import { useRouter } from 'next/router' function disposables() { let disposables: Function[] = [] @@ -138,6 +139,11 @@ function KeyCaster() { } function MyApp({ Component, pageProps }) { + let router = useRouter() + if (router.query.raw !== undefined) { + return + } + return ( <>