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
tailwindlabs · Jul 26, 2022 · f2c2d3c · f2c2d3c
1 parent 42db5b0
commit f2c2d3c
Show file tree

Hide file tree

Showing 8 changed files with 164 additions and 39 deletions.
diff --git 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
@@ -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 })
 

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

diff --git 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<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