Prevent cancelling transitions due to focus trap (#1664)

* focus trap after transitions Whenever you focus an element mid transition, the browser will abrublty stop/cancel the transition and it will do anything it can to focus the element. This has a very annoying side effect that this causes very abrubt transitions (instant) in some browsers. To fix this, we used to use `el.focus({ preventScroll: true })` which works in Safari and it used to work in Chrome / Firefox, but there are probably other variables that we have to keep in mind here (didn't figure out _why_ this used to work and not anymore). That said, instead of trying to fight the browser, we will now wait an animation frame before even trying to focus any elements. * update changelog
tailwindlabs · Jul 12, 2022 · 7a6220a · 7a6220a
1 parent fc7def3
commit 7a6220a
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 27 deletions.
diff --git a/packages/@headlessui-vue/CHANGELOG.md b/packages/@headlessui-vue/CHANGELOG.md
@@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Add `by` prop for `Listbox`, `Combobox` and `RadioGroup` ([#1482](https://github.com/tailwindlabs/headlessui/pull/1482))
 - Add `@headlessui/tailwindcss` plugin ([#1487](https://github.com/tailwindlabs/headlessui/pull/1487))
 
+### Fixed
+
+- Prevent cancelling transitions due to focus trap ([#1664](https://github.com/tailwindlabs/headlessui/pull/1664))
+
 ## [1.6.6] - 2022-07-07
 
 ### Fixed

diff --git a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts
@@ -36,6 +36,16 @@ global.IntersectionObserver = class FakeIntersectionObserver {
 
 afterAll(() => jest.restoreAllMocks())
 
+function nextFrame() {
+  return new Promise<void>((resolve) => {
+    requestAnimationFrame(() => {
+      requestAnimationFrame(() => {
+        resolve()
+      })
+    })
+  })
+}
+
 let TabSentinel = defineComponent({
   name: 'TabSentinel',
   template: html`<div :tabindex="0"></div>`,
@@ -243,6 +253,8 @@ describe('Rendering', () => {
 
       await click(document.getElementById('trigger'))
 
+      await new Promise<void>(nextTick)
+
       assertDialog({ state: DialogState.Visible, attributes: { class: 'relative bg-blue-500' } })
     })
 
@@ -263,7 +275,7 @@ describe('Rendering', () => {
         },
       })
 
-      await new Promise<void>(nextTick)
+      await nextFrame()
 
       // Let's verify that the Dialog is already there
       expect(getDialog()).not.toBe(null)

diff --git a/packages/@headlessui-vue/src/components/focus-trap/focus-trap.test.ts b/packages/@headlessui-vue/src/components/focus-trap/focus-trap.test.ts
@@ -16,6 +16,16 @@ beforeAll(() => {
 
 afterAll(() => jest.restoreAllMocks())
 
+function nextFrame() {
+  return new Promise<void>((resolve) => {
+    requestAnimationFrame(() => {
+      requestAnimationFrame(() => {
+        resolve()
+      })
+    })
+  })
+}
+
 const renderTemplate = createRenderTemplate({
   FocusTrap,
 })
@@ -29,7 +39,7 @@ it('should focus the first focusable element inside the FocusTrap', async () =>
     `
   )
 
-  await new Promise<void>(nextTick)
+  await nextFrame()
 
   assertActiveElement(getByText('Trigger'))
 })
@@ -52,7 +62,7 @@ it('should focus the autoFocus element inside the FocusTrap if that exists', asy
     },
   })
 
-  await new Promise<void>(nextTick)
+  await nextFrame()
 
   assertActiveElement(document.getElementById('b'))
 })
@@ -72,7 +82,7 @@ it('should focus the initialFocus element inside the FocusTrap if that exists',
     },
   })
 
-  await new Promise<void>(nextTick)
+  await nextFrame()
 
   assertActiveElement(document.getElementById('c'))
 })
@@ -92,7 +102,7 @@ it('should focus the initialFocus element inside the FocusTrap even if another e
     },
   })
 
-  await new Promise<void>(nextTick)
+  await nextFrame()
 
   assertActiveElement(document.getElementById('c'))
 })
@@ -109,7 +119,7 @@ it('should warn when there is no focusable element inside the FocusTrap', async
     `
   )
 
-  await new Promise<void>(nextTick)
+  await nextFrame()
 
   expect(spy.mock.calls[0][0]).toBe('There are no focusable elements inside the <FocusTrap />')
   spy.mockReset()
@@ -132,7 +142,7 @@ it(
       `,
     })
 
-    await new Promise<void>(nextTick)
+    await nextFrame()
 
     let [a, b, c, d] = Array.from(document.querySelectorAll('input'))
 
@@ -199,7 +209,7 @@ it('should restore the previously focused element, before entering the FocusTrap
     },
   })
 
-  await new Promise<void>(nextTick)
+  await nextFrame()
 
   // The input should have focus by default because of the autoFocus prop
   assertActiveElement(document.getElementById('item-1'))
@@ -232,7 +242,7 @@ it('should be possible to tab to the next focusable element within the focus tra
     `
   )
 
-  await new Promise<void>(nextTick)
+  await nextFrame()
 
   // Item A should be focused because the FocusTrap will focus the first item
   assertActiveElement(document.getElementById('item-a'))
@@ -265,6 +275,8 @@ it('should be possible to shift+tab to the previous focusable element within the
     `
   )
 
+  await nextFrame()
+
   // Item A should be focused because the FocusTrap will focus the first item
   assertActiveElement(document.getElementById('item-a'))
 
@@ -297,6 +309,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'))
 })
@@ -317,6 +331,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'))
 
@@ -351,6 +367,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'))
 
@@ -397,6 +415,8 @@ it('should try to focus all focusable items in order (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()
@@ -430,6 +450,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(getByText('Item D'))
   expect(spy).not.toHaveBeenCalled()

diff --git a/packages/@headlessui-vue/src/components/focus-trap/focus-trap.ts b/packages/@headlessui-vue/src/components/focus-trap/focus-trap.ts
@@ -20,6 +20,7 @@ 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. */
@@ -102,7 +103,7 @@ export let FocusTrap = Object.assign(
 
       return () => {
         let slot = {}
-        let ourProps = { 'data-hi': 'container', ref: container }
+        let ourProps = { ref: container }
         let { features, initialFocus, containers: _containers, ...theirProps } = props
 
         return h(Fragment, [
@@ -206,30 +207,32 @@ function useInitialFocus(
         let containerElement = dom(container)
         if (!containerElement) return
 
-        let initialFocusElement = dom(initialFocus)
+        requestAnimationFrame(() => {
+          let initialFocusElement = dom(initialFocus)
 
-        let activeElement = ownerDocument.value?.activeElement as HTMLElement
+          let activeElement = ownerDocument.value?.activeElement as HTMLElement
 
-        if (initialFocusElement) {
-          if (initialFocusElement === activeElement) {
+          if (initialFocusElement) {
+            if (initialFocusElement === activeElement) {
+              previousActiveElement.value = activeElement
+              return // Initial focus ref is already the active element
+            }
+          } else if (containerElement!.contains(activeElement)) {
             previousActiveElement.value = activeElement
-            return // Initial focus ref is already the active element
+            return // Already focused within Dialog
           }
-        } else if (containerElement.contains(activeElement)) {
-          previousActiveElement.value = activeElement
-          return // Already focused within Dialog
-        }
 
-        // Try to focus the initialFocus ref
-        if (initialFocusElement) {
-          focusElement(initialFocusElement)
-        } 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 (initialFocusElement) {
+            focusElement(initialFocusElement)
+          } else {
+            if (focusIn(containerElement!, Focus.First | Focus.NoScroll) === FocusResult.Error) {
+              console.warn('There are no focusable elements inside the <FocusTrap />')
+            }
           }
-        }
 
-        previousActiveElement.value = ownerDocument.value?.activeElement as HTMLElement
+          previousActiveElement.value = ownerDocument.value?.activeElement as HTMLElement
+        })
       },
       { immediate: true, flush: 'post' }
     )