From bbf345df66325f96026559b4f5602d6519412aae Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 4 Sep 2024 12:53:40 +0200 Subject: [PATCH 1/5] add basic polyfills for `CSSTransition` and `Element.prototype.getAnimations` --- packages/@headlessui-react/jest.setup.js | 45 ++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/packages/@headlessui-react/jest.setup.js b/packages/@headlessui-react/jest.setup.js index ef4af5454b..771a30cb96 100644 --- a/packages/@headlessui-react/jest.setup.js +++ b/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)] + } + } +} From eb4017bc571f203e376e22f4a5f5c94f2c8b6721 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 4 Sep 2024 13:00:14 +0200 Subject: [PATCH 2/5] rely on `node.getAnimations()` to know when transitions are done Up until now, we've tried to do this ourselves by listening to the correct events. The new `node.getAnimations()` API is a much simpler API to use. The only requirement is that we call `node.getAnimations()` in a `requestAnimationFrame` so that the browser can flush all the changes. We couldn't do this before, because we needed to setup the event listeners to prevent race conditions. Now there are no race conditions, in fact, if all transitions already complete before we can call the `waitForTransition`, then the `node.getAnimations()` list will be empty and we can call the `done()` function. The `Element.prototype.getAnimations` has been available in browsers since mid 2020, but at the time it was too new to use. Now seems like a safe time to use this. See: https://developer.mozilla.org/en-US/docs/Web/API/Element/getAnimations#browser_compatibility --- .../src/hooks/use-transition.ts | 86 +++++-------------- 1 file changed, 22 insertions(+), 64 deletions(-) diff --git a/packages/@headlessui-react/src/hooks/use-transition.ts b/packages/@headlessui-react/src/hooks/use-transition.ts index ae2602e6fc..8a4251fdef 100644 --- a/packages/@headlessui-react/src/hooks/use-transition.ts +++ b/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 canceled = false + d.add(() => { + canceled = 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 (!canceled) { + done() + } + }) + return d.dispose } From aa9e189c4d6dda6bcf0d7ed698615f3d8f7ade16 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 4 Sep 2024 13:05:36 +0200 Subject: [PATCH 3/5] update failing test The order slightly changed here, but it looks more correct now as well. The `child-2-1` and `child-2-2` have the same duration/delay, so the order that they are rendered in seems to be the correct order. That said, these DOM nodes are on the same level (siblings), so for these callbacks we are testing here the order doesn't really matter. The order that _does_ matter is that the `after*` events of a parent fire _after_ the `after*` events of the children, which it still does. --- .../src/components/transition/transition.test.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/@headlessui-react/src/components/transition/transition.test.tsx b/packages/@headlessui-react/src/components/transition/transition.test.tsx index 5911fbc7d9..137373393e 100644 --- a/packages/@headlessui-react/src/components/transition/transition.test.tsx +++ b/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', ]) From 03da4d510b901069a4b0da905d2fec181350fed4 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 4 Sep 2024 13:57:44 +0200 Subject: [PATCH 4/5] update changelog --- packages/@headlessui-react/CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index be3c86407a..ed81d06702 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/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 From 03d80f496d79804a433ec980570429738cf8a31e Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 4 Sep 2024 14:09:54 +0200 Subject: [PATCH 5/5] use consistent `cancelled` naming --- packages/@headlessui-react/src/hooks/use-transition.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/@headlessui-react/src/hooks/use-transition.ts b/packages/@headlessui-react/src/hooks/use-transition.ts index 8a4251fdef..6c83f466c4 100644 --- a/packages/@headlessui-react/src/hooks/use-transition.ts +++ b/packages/@headlessui-react/src/hooks/use-transition.ts @@ -228,9 +228,9 @@ function waitForTransition(node: HTMLElement | null, done: () => void) { let d = disposables() if (!node) return d.dispose - let canceled = false + let cancelled = false d.add(() => { - canceled = true + cancelled = true }) let transitions = node.getAnimations().filter((animation) => animation instanceof CSSTransition) @@ -242,7 +242,7 @@ function waitForTransition(node: HTMLElement | null, done: () => void) { // Wait for all the transitions to complete. Promise.allSettled(transitions.map((transition) => transition.finished)).then(() => { - if (!canceled) { + if (!cancelled) { done() } })