Fix incorrect scrolling to the bottom when opening a Dialog

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