Fix transition bug on Firefox, triggered by clicking the `PopoverButt…

…on` in rapid succession (#3452)

We recently landed a fix for `Popover`s not closing correctly when using
the `transition` prop (#3448). Once this fix was published, some users
still ran into issues using Firefox on Windows (see:
tailwindlabs/tailwindui-issues#1625).

One fun thing I discovered is that transitions somehow behave
differently based on where they are triggered from (?). What I mean by
this is that holding down the <kbd>space</kbd> key on the button does
properly open/close the `Popover`. But if you rapidly click the button,
the `Popover` will eventually get stuck.

> Note: when testing this, I made sure that the handling of the `space`
key (in a `keydown` handler) and the clicking of the mouse (handled in a
`click` handler) called the exact same code. It still happened.

The debugging continues…

One thing I noticed is that when the `Popover` gets stuck, it meant that
a transition didn't properly complete.

The current implementation of the internal `useTransition(…)` hook has
to wait for all the transitions to finish. This is done using a
`waitForTransition(…)` helper. This helper sets up some event listeners
(`transitionstart`, `transitionend`, …) and waits for them to fire.

This seems to be unreliable on Firefox for some unknown reason.

I knew the code for waiting for transitions wasn't ideal, so I wanted to
see if using the native `node.getAnimations()` simplifies this and makes
it work in general.

Lo and behold, it did! 🎉

This now has multiple benefits:

1. It works as expected on Firefox
2. The code is much much simpler
3. Uses native features

The `getAnimations(…)` function is supported in all modern browsers
(since 2020). At the time it was too early to rely on it, but right now
it should be safe to use.

Fixes: tailwindlabs/tailwindui-issues#1625

packages/@headlessui-react/CHANGELOG.md

@@ -7,7 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
-- Nothing yet!
+### Fixed
+
+- Fix transition bug on Firefox, triggered by clicking the `PopoverButton` in rapid succession ([#3452](https://github.com/tailwindlabs/headlessui/pull/3452))
 
 ## [2.1.4] - 2024-09-03
 

packages/@headlessui-react/jest.setup.js

@@ -1 +1,46 @@
 globalThis.IS_REACT_ACT_ENVIRONMENT = true
+
+// These are not 1:1 perfect polyfills, but they implement the parts we need for
+// testing. The implementation of the `getAnimations` uses the `setTimeout`
+// approach we used in the past.
+//
+// This is only necessary because JSDOM does not implement `getAnimations` or
+// `CSSTransition` yet. This is a temporary solution until JSDOM implements
+// these features. Or, until we use proper browser tests using Puppeteer or
+// Playwright.
+{
+  if (typeof CSSTransition === 'undefined') {
+    globalThis.CSSTransition = class CSSTransition {
+      constructor(duration) {
+        this.duration = duration
+      }
+
+      finished = new Promise((resolve) => {
+        setTimeout(resolve, this.duration)
+      })
+    }
+  }
+
+  if (typeof Element.prototype.getAnimations !== 'function') {
+    Element.prototype.getAnimations = function () {
+      let { transitionDuration, transitionDelay } = getComputedStyle(this)
+
+      let [durationMs, delayMs] = [transitionDuration, transitionDelay].map((value) => {
+        let [resolvedValue = 0] = value
+          .split(',')
+          // Remove falsy we can't work with
+          .filter(Boolean)
+          // Values are returned as `0.3s` or `75ms`
+          .map((v) => (v.includes('ms') ? parseFloat(v) : parseFloat(v) * 1000))
+          .sort((a, z) => z - a)
+
+        return resolvedValue
+      })
+
+      let totalDuration = durationMs + delayMs
+      if (totalDuration === 0) return []
+
+      return [new CSSTransition(totalDuration)]
+    }
+  }
+}

packages/@headlessui-react/src/components/transition/transition.test.tsx

@@ -1050,8 +1050,8 @@ describe('Events', () => {
         'child-2-2: beforeEnter',
 
         'child-1: afterEnter',
-        'child-2-2: afterEnter',
         'child-2-1: afterEnter',
+        'child-2-2: afterEnter',
         'child-2: afterEnter',
         'root: afterEnter',
 
@@ -1064,8 +1064,8 @@ describe('Events', () => {
         'root: beforeLeave',
 
         'child-1: afterLeave',
-        'child-2-2: afterLeave',
         'child-2-1: afterLeave',
+        'child-2-2: afterLeave',
         'child-2: afterLeave',
         'root: afterLeave',
 
@@ -1078,8 +1078,8 @@ describe('Events', () => {
         'child-2-2: beforeEnter',
 
         'child-1: afterEnter',
-        'child-2-2: afterEnter',
         'child-2-1: afterEnter',
+        'child-2-2: afterEnter',
         'child-2: afterEnter',
         'root: afterEnter',
 
@@ -1092,8 +1092,8 @@ describe('Events', () => {
         'root: beforeLeave',
 
         'child-1: afterLeave',
-        'child-2-2: afterLeave',
         'child-2-1: afterLeave',
+        'child-2-2: afterLeave',
         'child-2: afterLeave',
         'root: afterLeave',
       ])

packages/@headlessui-react/src/hooks/use-transition.ts

@@ -1,6 +1,5 @@
 import { useRef, useState, type MutableRefObject } from 'react'
 import { disposables } from '../utils/disposables'
-import { once } from '../utils/once'
 import { useDisposables } from './use-disposables'
 import { useFlags } from './use-flags'
 import { useIsoMorphicEffect } from './use-iso-morphic-effect'
@@ -211,84 +210,43 @@ function transition(
   // This means that no transition happens at all. To fix this, we delay the
   // actual transition by one frame.
   d.nextFrame(() => {
-    // Wait for the transition, once the transition is complete we can cleanup.
-    // This is registered first to prevent race conditions, otherwise it could
-    // happen that the transition is already done before we start waiting for
-    // the actual event.
-    d.add(waitForTransition(node, done))
-
     // Initiate the transition by applying the new classes.
     run()
+
+    // Wait for the transition, once the transition is complete we can cleanup.
+    // We wait for a frame such that the browser has time to flush the changes
+    // to the DOM.
+    d.requestAnimationFrame(() => {
+      d.add(waitForTransition(node, done))
+    })
   })
 
   return d.dispose
 }
 
-function waitForTransition(node: HTMLElement, _done: () => void) {
-  let done = once(_done)
+function waitForTransition(node: HTMLElement | null, done: () => void) {
   let d = disposables()
-
   if (!node) return d.dispose
 
-  // Safari returns a comma separated list of values, so let's sort them and take the highest value.
-  let { transitionDuration, transitionDelay } = getComputedStyle(node)
-
-  let [durationMs, delayMs] = [transitionDuration, transitionDelay].map((value) => {
-    let [resolvedValue = 0] = value
-      .split(',')
-      // Remove falsy we can't work with
-      .filter(Boolean)
-      // Values are returned as `0.3s` or `75ms`
-      .map((v) => (v.includes('ms') ? parseFloat(v) : parseFloat(v) * 1000))
-      .sort((a, z) => z - a)
-
-    return resolvedValue
+  let cancelled = false
+  d.add(() => {
+    cancelled = true
   })
 
-  let totalDuration = durationMs + delayMs
-
-  if (totalDuration !== 0) {
-    if (process.env.NODE_ENV === 'test') {
-      let dispose = d.setTimeout(() => {
-        done()
-        dispose()
-      }, totalDuration)
-    } else {
-      let disposeGroup = d.group((d) => {
-        // Mark the transition as done when the timeout is reached. This is a fallback in case the
-        // transitionrun event is not fired.
-        let cancelTimeout = d.setTimeout(() => {
-          done()
-          d.dispose()
-        }, totalDuration)
-
-        // The moment the transitionrun event fires, we should cleanup the timeout fallback, because
-        // then we know that we can use the native transition events because something is
-        // transitioning.
-        d.addEventListener(node, 'transitionrun', (event) => {
-          if (event.target !== event.currentTarget) return
-          cancelTimeout()
-
-          d.addEventListener(node, 'transitioncancel', (event) => {
-            if (event.target !== event.currentTarget) return
-            done()
-            disposeGroup()
-          })
-        })
-      })
-
-      d.addEventListener(node, 'transitionend', (event) => {
-        if (event.target !== event.currentTarget) return
-        done()
-        d.dispose()
-      })
-    }
-  } else {
-    // No transition is happening, so we should cleanup already. Otherwise we have to wait until we
-    // get disposed.
+  let transitions = node.getAnimations().filter((animation) => animation instanceof CSSTransition)
+  // If there are no transitions, we can stop early.
+  if (transitions.length === 0) {
     done()
+    return d.dispose
   }
 
+  // Wait for all the transitions to complete.
+  Promise.allSettled(transitions.map((transition) => transition.finished)).then(() => {
+    if (!cancelled) {
+      done()
+    }
+  })
+
   return d.dispose
 }