diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index 8e047e5..f9c82ed 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 bf339ef..985f405 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 f342d1f..23a5a4e 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 bbab04b..2db6dc6 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 16e1f98..7c50b87 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 d2333ca..c0d3c66 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 1db7628..8b6b421 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 c598667..bed7351 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 ( <>