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

Author: RobinMalfait

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

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)

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

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' }
     )