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

@@ -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
 

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 <div tabIndex={0} {...props} />
+function nextFrame() {
+  return new Promise<void>((resolve) => {
+    requestAnimationFrame(() => {
+      requestAnimationFrame(() => {
+        resolve()
+      })
+    })
+  })
+}
+
+function TabSentinel(props: PropsOf<'button'>) {
+  return <button {...props} />
 }
 
 describe('Safe guards', () => {
@@ -167,7 +177,7 @@ describe('Rendering', () => {
       })
     )
 
-    it('should be possible to always render the Dialog if we provide it a `static` prop (and enable focus trapping based on `open`)', () => {
+    it('should be possible to always render the Dialog if we provide it a `static` prop (and enable focus trapping based on `open`)', async () => {
       let focusCounter = jest.fn()
       render(
         <>
@@ -179,6 +189,8 @@ describe('Rendering', () => {
         </>
       )
 
+      await nextFrame()
+
       // Let's verify that the Dialog is already there
       expect(getDialog()).not.toBe(null)
       expect(focusCounter).toHaveBeenCalledTimes(1)
@@ -217,8 +229,11 @@ describe('Rendering', () => {
           </>
         )
       }
+
       render(<Example />)
 
+      await nextFrame()
+
       assertDialog({ state: DialogState.InvisibleHidden })
       expect(focusCounter).toHaveBeenCalledTimes(0)
 
@@ -463,6 +478,8 @@ describe('Rendering', () => {
           </Dialog>
         )
 
+        await nextFrame()
+
         assertDialog({
           state: DialogState.Visible,
           attributes: { id: 'headlessui-dialog-1' },
@@ -486,6 +503,8 @@ describe('Rendering', () => {
           </Dialog>
         )
 
+        await nextFrame()
+
         assertDialog({
           state: DialogState.Visible,
           attributes: { id: 'headlessui-dialog-1' },
@@ -512,6 +531,8 @@ describe('Composition', () => {
         </Transition>
       )
 
+      await nextFrame()
+
       assertDialog({ state: DialogState.Visible })
       assertDialogDescription({
         state: DialogState.Visible,
@@ -532,6 +553,8 @@ describe('Composition', () => {
         </Transition>
       )
 
+      await nextFrame()
+
       assertDialog({ state: DialogState.InvisibleUnmounted })
     })
   )
@@ -870,8 +893,11 @@ describe('Mouse interactions', () => {
           </div>
         )
       }
+
       render(<Example />)
 
+      await nextFrame()
+
       // Verify it is open
       assertDialog({ state: DialogState.Visible })
 
@@ -905,8 +931,11 @@ describe('Mouse interactions', () => {
           </Dialog>
         )
       }
+
       render(<Example />)
 
+      await nextFrame()
+
       // Verify it is open
       assertDialog({ state: DialogState.Visible })
 
@@ -936,8 +965,11 @@ describe('Mouse interactions', () => {
           </div>
         )
       }
+
       render(<Example />)
 
+      await nextFrame()
+
       // Verify it is open
       assertDialog({ state: DialogState.Visible })
 
@@ -979,8 +1011,11 @@ describe('Mouse interactions', () => {
           </Dialog>
         )
       }
+
       render(<Example />)
 
+      await nextFrame()
+
       // Verify it is open
       assertDialog({ state: DialogState.Visible })
 
@@ -1023,8 +1058,11 @@ describe('Mouse interactions', () => {
           </div>
         )
       }
+
       render(<Example />)
 
+      await nextFrame()
+
       // Verify it is open
       assertDialog({ state: DialogState.Visible })
 

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<void>((resolve) => {
+    requestAnimationFrame(() => {
+      requestAnimationFrame(() => {
+        resolve()
+      })
+    })
+  })
+}
+
+it('should focus the first focusable element inside the FocusTrap', async () => {
   render(
     <FocusTrap>
       <button>Trigger</button>
     </FocusTrap>
   )
 
+  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(
     <FocusTrap>
       <input id="a" type="text" />
@@ -25,10 +37,12 @@ it('should focus the autoFocus element inside the FocusTrap if that exists', ()
     </FocusTrap>
   )
 
+  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<HTMLInputElement | null>(null)
 
@@ -40,12 +54,15 @@ it('should focus the initialFocus element inside the FocusTrap if that exists',
       </FocusTrap>
     )
   }
+
   render(<Example />)
 
+  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<HTMLInputElement | null>(null)
 
@@ -57,12 +74,15 @@ it('should focus the initialFocus element inside the FocusTrap even if another e
       </FocusTrap>
     )
   }
+
   render(<Example />)
 
+  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', () =>
       </FocusTrap>
     )
   }
+
   render(<Example />)
+
+  await nextFrame()
+
   expect(spy.mock.calls[0][0]).toBe('There are no focusable elements inside the <FocusTrap />')
   spy.mockReset()
 })
@@ -96,6 +120,8 @@ it(
 
     render(<Example />)
 
+    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(<Example />)
 
+  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 <FocusTrap />')
   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()

packages/@headlessui-react/src/components/focus-trap/focus-trap.tsx

@@ -185,34 +185,52 @@ function useInitialFocus(
 ) {
   let previousActiveElement = useRef<HTMLElement | null>(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 <FocusTrap />')
+      // 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 <FocusTrap />')
+        }
       }
-    }
 
-    previousActiveElement.current = ownerDocument?.activeElement as HTMLElement
+      previousActiveElement.current = ownerDocument?.activeElement as HTMLElement
+    })
   }, [enabled])
 
   return previousActiveElement

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
 

packages/@headlessui-vue/src/components/dialog/dialog.test.ts

@@ -48,7 +48,7 @@ function nextFrame() {
 
 let TabSentinel = defineComponent({
   name: 'TabSentinel',
-  template: html`<div :tabindex="0"></div>`,
+  template: html`<button></button>`,
 })
 
 jest.mock('../../hooks/use-id')
@@ -253,7 +253,7 @@ describe('Rendering', () => {
 
       await click(document.getElementById('trigger'))
 
-      await new Promise<void>(nextTick)
+      await nextFrame()
 
       assertDialog({ state: DialogState.Visible, attributes: { class: 'relative bg-blue-500' } })
     })
@@ -299,7 +299,7 @@ describe('Rendering', () => {
         },
       })
 
-      await new Promise<void>(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<void>(nextTick)
+      await nextFrame()
 
       assertDialog({ state: DialogState.InvisibleHidden })
       expect(focusCounter).toHaveBeenCalledTimes(0)
@@ -637,7 +637,7 @@ describe('Rendering', () => {
           `
         )
 
-        await new Promise<void>(nextTick)
+        await nextFrame()
 
         assertDialog({
           state: DialogState.Visible,
@@ -664,7 +664,7 @@ describe('Rendering', () => {
           `
         )
 
-        await new Promise<void>(nextTick)
+        await nextFrame()
 
         assertDialog({
           state: DialogState.Visible,
@@ -695,7 +695,7 @@ describe('Composition', () => {
         `,
       })
 
-      await new Promise<void>(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<void>(nextTick)
+      await nextFrame()
 
       // Verify it is open
       assertDialog({ state: DialogState.Visible })
@@ -1259,7 +1261,7 @@ describe('Mouse interactions', () => {
         },
       })
 
-      await new Promise<void>(nextTick)
+      await nextFrame()
 
       // Verify it is open
       assertDialog({ state: DialogState.Visible })
@@ -1298,7 +1300,7 @@ describe('Mouse interactions', () => {
         },
       })
 
-      await new Promise<void>(nextTick)
+      await nextFrame()
 
       // Verify it is open
       assertDialog({ state: DialogState.Visible })
@@ -1344,7 +1346,7 @@ describe('Mouse interactions', () => {
         },
       })
 
-      await new Promise<void>(nextTick)
+      await nextFrame()
 
       // Verify it is open
       assertDialog({ state: DialogState.Visible })
@@ -1396,7 +1398,7 @@ describe('Mouse interactions', () => {
         },
       })
 
-      await new Promise<void>(nextTick)
+      await nextFrame()
 
       // Verify it is open
       assertDialog({ state: DialogState.Visible })

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<HTMLElement | null>(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

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 <Component {...pageProps} />
+  }
+
   return (
     <>
       <div className="flex h-screen flex-col overflow-hidden bg-gray-700 font-sans text-gray-900 antialiased">