From f27367da77b52fd9bf8539bd74d5ac67f1832c4b Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Sat, 30 Mar 2024 14:45:03 +0100 Subject: [PATCH 01/27] improve `transition` logic I spend a lot of time trying to debug this, and I'm not 100% sure that I'm correct yet. However, what we used to do is apply the "before" set of classes, wait a frame and then apply the "after" set of classes which should trigger a transition. However, for some reason, applying the "before" classes already start a transition. My current thinking is that our component is doing a lot of work and therfore prematurely applying the classes and actually triggering the transition. For example, if we want to go from `opacity-0` to `opacity-100`, it looks like setting `opacity-0` is already transitioning (from 100% because that's the default value). Then, we set `opacity-100`, but our component was just going from 100 -> 0 and we were only at let's say 98%. It looks like we cancelled the transition and only went from 98% -> 100%. I can't fully explain it purely because I don't 100% get what's going on. However, this commit fixes it in a way where we first prepare the transition by explicitly cancelling all in-flight transitions. Once that is done, we can apply the "before" set of classes, then we can apply the "after" set of classes. This seems to work consistently (even though we have failing tests, might be a JSDOM issue). This tells me that at least parts of my initial thoughts are correct where some transition is already happening (for some reason). I'm not sure what the reason is exactly. Are we doing too much work and blocking the main thread? That would be my guess... --- .../components/transition/utils/transition.ts | 68 ++++++++++++------- 1 file changed, 45 insertions(+), 23 deletions(-) diff --git a/packages/@headlessui-react/src/components/transition/utils/transition.ts b/packages/@headlessui-react/src/components/transition/utils/transition.ts index 659050c30b..3906798879 100644 --- a/packages/@headlessui-react/src/components/transition/utils/transition.ts +++ b/packages/@headlessui-react/src/components/transition/utils/transition.ts @@ -121,30 +121,52 @@ export function transition( leave: () => classes.leaveFrom, }) - removeClasses( - node, - ...classes.base, - ...classes.enter, - ...classes.enterTo, - ...classes.enterFrom, - ...classes.leave, - ...classes.leaveFrom, - ...classes.leaveTo, - ...classes.entered - ) - addClasses(node, ...classes.base, ...base, ...from) - - d.nextFrame(() => { - removeClasses(node, ...classes.base, ...base, ...from) - addClasses(node, ...classes.base, ...base, ...to) - - waitForTransition(node, () => { - removeClasses(node, ...classes.base, ...base) - addClasses(node, ...classes.base, ...classes.entered) - - return _done() - }) + // Prepare the transitions by ensuring that all the "before" classes are + // applied and flushed to the DOM. + prepareTransition(node, () => { + removeClasses( + node, + ...classes.base, + ...classes.enter, + ...classes.enterTo, + ...classes.enterFrom, + ...classes.leave, + ...classes.leaveFrom, + ...classes.leaveTo, + ...classes.entered + ) + addClasses(node, ...classes.base, ...base, ...from) }) + // 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. + waitForTransition(node, () => { + removeClasses(node, ...classes.base, ...base) + addClasses(node, ...classes.base, ...classes.entered) + + return _done() + }) + + // Initiate the transition by applying the new classes. + removeClasses(node, ...classes.base, ...base, ...from) + addClasses(node, ...classes.base, ...base, ...to) + return d.dispose } + +function prepareTransition(node: HTMLElement, prepare: () => void) { + let previous = node.style.transition + + // Force cancel current transition + node.style.transition = 'none' + + prepare() + + // Trigger a reflow, flushing the CSS changes + node.offsetHeight + + // Reset the transition to what it was before + node.style.transition = previous +} From 123fd32b93de3fd48c5ea2e67d4ffdc980d1c3fb Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Sat, 30 Mar 2024 14:57:27 +0100 Subject: [PATCH 02/27] simplify check --- .../@headlessui-react/src/components/transition/transition.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@headlessui-react/src/components/transition/transition.tsx b/packages/@headlessui-react/src/components/transition/transition.tsx index f5b3f1e406..dc856052d2 100644 --- a/packages/@headlessui-react/src/components/transition/transition.tsx +++ b/packages/@headlessui-react/src/components/transition/transition.tsx @@ -504,7 +504,7 @@ function TransitionRootFn is used but it is missing a `show={true | false}` prop.') } From 3513a706f9747f8e7bb96e16ec96ab7334237e2b Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Sat, 30 Mar 2024 15:46:33 +0100 Subject: [PATCH 03/27] updating playgrounds to improve transitions --- .../listbox/listbox-with-pure-tailwind.tsx | 62 +++++++++++-------- .../react/pages/menu/menu-with-transition.tsx | 11 +++- 2 files changed, 44 insertions(+), 29 deletions(-) diff --git a/playgrounds/react/pages/listbox/listbox-with-pure-tailwind.tsx b/playgrounds/react/pages/listbox/listbox-with-pure-tailwind.tsx index d6f017837d..5264d09b84 100644 --- a/playgrounds/react/pages/listbox/listbox-with-pure-tailwind.tsx +++ b/playgrounds/react/pages/listbox/listbox-with-pure-tailwind.tsx @@ -1,5 +1,5 @@ -import { Listbox } from '@headlessui/react' -import { useEffect, useState } from 'react' +import { Listbox, Transition } from '@headlessui/react' +import { Fragment, useEffect, useState } from 'react' let people = [ 'Wade Cooper', @@ -59,30 +59,40 @@ export default function Home() { -
- - {people.map((name) => ( - - - {name} - - - - - - - - ))} - -
+ +
+ + {people.map((name) => ( + + + {name} + + + + + + + + ))} + +
+ diff --git a/playgrounds/react/pages/menu/menu-with-transition.tsx b/playgrounds/react/pages/menu/menu-with-transition.tsx index 89191e626a..bef7e0497d 100644 --- a/playgrounds/react/pages/menu/menu-with-transition.tsx +++ b/playgrounds/react/pages/menu/menu-with-transition.tsx @@ -1,4 +1,5 @@ import { Menu, Transition } from '@headlessui/react' +import { Fragment } from 'react' import { Button } from '../../components/button' import { classNames } from '../../utils/class-names' @@ -29,10 +30,11 @@ export default function Home() { console.log('Before enter')} @@ -40,7 +42,10 @@ export default function Home() { beforeLeave={() => console.log('Before leave')} afterLeave={() => console.log('After leave')} > - +

Signed in as

From d0e4787644d22b7dc1609df81a74141ab40b4975 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Tue, 2 Apr 2024 12:47:09 +0200 Subject: [PATCH 04/27] update changelog --- packages/@headlessui-react/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index 56148639ea..9ffdbb63c4 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/packages/@headlessui-react/CHANGELOG.md @@ -23,6 +23,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Expose missing `data-disabled` and `data-focus` attributes on the `TabsPanel`, `MenuButton`, `PopoverButton` and `DisclosureButton` components ([#3061](https://github.com/tailwindlabs/headlessui/pull/3061)) - Fix cursor position when re-focusing the `ComboboxInput` component ([#3065](https://github.com/tailwindlabs/headlessui/pull/3065)) - Keep focus inside of the `` component ([#3073](https://github.com/tailwindlabs/headlessui/pull/3073)) +- Fix enter transitions for the `Transition` component ([#3074](https://github.com/tailwindlabs/headlessui/pull/3074)) ### Changed From 025fd7d3cef9b98c95e527594e6dfd508c7b5afb Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Tue, 2 Apr 2024 16:44:44 +0200 Subject: [PATCH 05/27] track in-flight transitions --- .../transition/utils/transition.test.ts | 33 ++++--- .../components/transition/utils/transition.ts | 87 ++++++++++++------- .../src/hooks/use-transition.ts | 19 +++- 3 files changed, 88 insertions(+), 51 deletions(-) diff --git a/packages/@headlessui-react/src/components/transition/utils/transition.test.ts b/packages/@headlessui-react/src/components/transition/utils/transition.test.ts index 8f89ae76f2..7d0aff5bde 100644 --- a/packages/@headlessui-react/src/components/transition/utils/transition.test.ts +++ b/packages/@headlessui-react/src/components/transition/utils/transition.test.ts @@ -26,9 +26,9 @@ it('should be possible to transition', async () => { ) await new Promise((resolve) => { - transition( - element, - { + transition(element, { + direction: 'enter', // Show + classes: { base: [], enter: ['enter'], enterFrom: ['enterFrom'], @@ -38,9 +38,8 @@ it('should be possible to transition', async () => { leaveTo: [], entered: ['entered'], }, - true, // Show - resolve - ) + done: resolve, + }) }) await new Promise((resolve) => d.nextFrame(resolve)) @@ -84,9 +83,9 @@ it('should wait the correct amount of time to finish a transition', async () => ) await new Promise((resolve) => { - transition( - element, - { + transition(element, { + direction: 'enter', // Show + classes: { base: [], enter: ['enter'], enterFrom: ['enterFrom'], @@ -96,9 +95,8 @@ it('should wait the correct amount of time to finish a transition', async () => leaveTo: [], entered: ['entered'], }, - true, // Show - resolve - ) + done: resolve, + }) }) await new Promise((resolve) => d.nextFrame(resolve)) @@ -154,9 +152,9 @@ it('should keep the delay time into account', async () => { ) await new Promise((resolve) => { - transition( - element, - { + transition(element, { + direction: 'enter', // Show + classes: { base: [], enter: ['enter'], enterFrom: ['enterFrom'], @@ -166,9 +164,8 @@ it('should keep the delay time into account', async () => { leaveTo: [], entered: ['entered'], }, - true, // Show - resolve - ) + done: resolve, + }) }) await new Promise((resolve) => d.nextFrame(resolve)) diff --git a/packages/@headlessui-react/src/components/transition/utils/transition.ts b/packages/@headlessui-react/src/components/transition/utils/transition.ts index 3906798879..0d34dc1cd4 100644 --- a/packages/@headlessui-react/src/components/transition/utils/transition.ts +++ b/packages/@headlessui-react/src/components/transition/utils/transition.ts @@ -1,3 +1,4 @@ +import type { MutableRefObject } from 'react' import { disposables } from '../../../utils/disposables' import { match } from '../../../utils/match' import { once } from '../../../utils/once' @@ -10,7 +11,8 @@ function removeClasses(node: HTMLElement, ...classes: string[]) { node && classes.length > 0 && node.classList.remove(...classes) } -function waitForTransition(node: HTMLElement, done: () => void) { +function waitForTransition(node: HTMLElement, _done: () => void) { + let done = once(_done) let d = disposables() if (!node) return d.dispose @@ -82,20 +84,27 @@ function waitForTransition(node: HTMLElement, done: () => void) { export function transition( node: HTMLElement, - classes: { - base: string[] - enter: string[] - enterFrom: string[] - enterTo: string[] - leave: string[] - leaveFrom: string[] - leaveTo: string[] - entered: string[] - }, - show: boolean, - done?: () => void + { + direction, + done, + classes, + inFlight, + }: { + direction: 'enter' | 'leave' + done?: () => void + classes: { + base: string[] + enter: string[] + enterFrom: string[] + enterTo: string[] + leave: string[] + leaveFrom: string[] + leaveTo: string[] + entered: string[] + } + inFlight?: MutableRefObject + } ) { - let direction = show ? 'enter' : 'leave' let d = disposables() let _done = done !== undefined ? once(done) : () => {} @@ -123,28 +132,37 @@ export function transition( // Prepare the transitions by ensuring that all the "before" classes are // applied and flushed to the DOM. - prepareTransition(node, () => { - removeClasses( - node, - ...classes.base, - ...classes.enter, - ...classes.enterTo, - ...classes.enterFrom, - ...classes.leave, - ...classes.leaveFrom, - ...classes.leaveTo, - ...classes.entered - ) - addClasses(node, ...classes.base, ...base, ...from) + prepareTransition(node, { + prepare() { + removeClasses( + node, + ...classes.base, + ...classes.enter, + ...classes.enterTo, + ...classes.enterFrom, + ...classes.leave, + ...classes.leaveFrom, + ...classes.leaveTo, + ...classes.entered + ) + addClasses(node, ...classes.base, ...base, ...from) + }, + inFlight, }) + // Mark the transition as in-flight. + if (inFlight) inFlight.current = true + // 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. waitForTransition(node, () => { removeClasses(node, ...classes.base, ...base) - addClasses(node, ...classes.base, ...classes.entered) + addClasses(node, ...classes.base, ...classes.entered, ...to) + + // Mark the transition as done. + if (inFlight) inFlight.current = false return _done() }) @@ -156,7 +174,18 @@ export function transition( return d.dispose } -function prepareTransition(node: HTMLElement, prepare: () => void) { +function prepareTransition( + node: HTMLElement, + { inFlight, prepare }: { inFlight?: MutableRefObject; prepare: () => void } +) { + // If we are already in a transition, then we can skip the preparation of + // force cancelling the current transition. This improves the cancellation of + // existing transitions instead of a hard cut-off. + if (inFlight?.current) { + prepare() + return + } + let previous = node.style.transition // Force cancel current transition diff --git a/packages/@headlessui-react/src/hooks/use-transition.ts b/packages/@headlessui-react/src/hooks/use-transition.ts index e9daf42111..8eb8945e6b 100644 --- a/packages/@headlessui-react/src/hooks/use-transition.ts +++ b/packages/@headlessui-react/src/hooks/use-transition.ts @@ -1,4 +1,4 @@ -import type { MutableRefObject } from 'react' +import { useRef, type MutableRefObject } from 'react' import { transition } from '../components/transition/utils/transition' import { disposables } from '../utils/disposables' import { useDisposables } from './use-disposables' @@ -40,6 +40,12 @@ export function useTransition({ let latestDirection = useLatestValue(direction) + // Track whether the transition is in flight or not. This will help us for + // cancelling mid-transition because in that case we don't have to force + // clearing existing transitions. See: `prepareTransition` in the `transition` + // file. + let inFlight = useRef(false) + useIsoMorphicEffect(() => { if (!immediate) return @@ -60,9 +66,14 @@ export function useTransition({ onStart.current(latestDirection.current) dd.add( - transition(node, classes.current, latestDirection.current === 'enter', () => { - dd.dispose() - onStop.current(latestDirection.current) + transition(node, { + direction: latestDirection.current, + classes: classes.current, + inFlight, + done() { + dd.dispose() + onStop.current(latestDirection.current) + }, }) ) From 5c2d990883976d991bf8d8056c318f39557ebf89 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 3 Apr 2024 16:00:05 +0200 Subject: [PATCH 06/27] document `disposables()` and `useDisposables()` functions --- packages/@headlessui-react/src/hooks/use-disposables.ts | 4 ++++ packages/@headlessui-react/src/utils/disposables.ts | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/packages/@headlessui-react/src/hooks/use-disposables.ts b/packages/@headlessui-react/src/hooks/use-disposables.ts index 0e90188137..c60a20c0aa 100644 --- a/packages/@headlessui-react/src/hooks/use-disposables.ts +++ b/packages/@headlessui-react/src/hooks/use-disposables.ts @@ -1,6 +1,10 @@ import { useEffect, useState } from 'react' import { disposables } from '../utils/disposables' +/** + * The `useDisposables` hook returns a `disposables` object that is disposed + * when the component is unmounted. + */ export function useDisposables() { // Using useState instead of useRef so that we can use the initializer function. let [d] = useState(disposables) diff --git a/packages/@headlessui-react/src/utils/disposables.ts b/packages/@headlessui-react/src/utils/disposables.ts index 55ca27b6b5..737f018d83 100644 --- a/packages/@headlessui-react/src/utils/disposables.ts +++ b/packages/@headlessui-react/src/utils/disposables.ts @@ -2,6 +2,14 @@ import { microTask } from './micro-task' export type Disposables = ReturnType +/** + * Disposables are a way to manage resources that need to be cleaned up when + * they are no longer needed. + * + * Each function returns a dispose function that can be called to clean up the + * resource. This also returns a `dispose` function that will clean up all the + * pending resources that have been added. + */ export function disposables() { let _disposables: Function[] = [] From fb59c52a7bfdf565ad26515b0e954b03f44434a0 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 3 Apr 2024 16:00:25 +0200 Subject: [PATCH 07/27] ensure we alway return a cleanup function --- packages/@headlessui-react/src/utils/disposables.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/@headlessui-react/src/utils/disposables.ts b/packages/@headlessui-react/src/utils/disposables.ts index 737f018d83..dc5eb1d227 100644 --- a/packages/@headlessui-react/src/utils/disposables.ts +++ b/packages/@headlessui-react/src/utils/disposables.ts @@ -67,11 +67,11 @@ export function disposables() { }, add(cb: () => void) { - if (_disposables.includes(cb)) { - return + // Ensure we don't add the same callback twice + if (!_disposables.includes(cb)) { + _disposables.push(cb) } - _disposables.push(cb) return () => { let idx = _disposables.indexOf(cb) if (idx >= 0) { From 3204ea4eab8809a191208eb2455d8113935427e4 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 3 Apr 2024 16:00:40 +0200 Subject: [PATCH 08/27] move comment --- .../@headlessui-react/src/components/transition/transition.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@headlessui-react/src/components/transition/transition.tsx b/packages/@headlessui-react/src/components/transition/transition.tsx index dc856052d2..fdbfcee51b 100644 --- a/packages/@headlessui-react/src/components/transition/transition.tsx +++ b/packages/@headlessui-react/src/components/transition/transition.tsx @@ -437,10 +437,10 @@ function TransitionChildFn Date: Wed, 3 Apr 2024 16:00:52 +0200 Subject: [PATCH 09/27] apply `enterTo` or `leaveTo` classes once we are done transitioning --- .../src/components/transition/transition.tsx | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/@headlessui-react/src/components/transition/transition.tsx b/packages/@headlessui-react/src/components/transition/transition.tsx index fdbfcee51b..b5f992a306 100644 --- a/packages/@headlessui-react/src/components/transition/transition.tsx +++ b/packages/@headlessui-react/src/components/transition/transition.tsx @@ -457,6 +457,21 @@ function TransitionChildFn Date: Wed, 3 Apr 2024 16:49:38 +0200 Subject: [PATCH 10/27] cleanup & simplify logic --- .../components/transition/utils/transition.ts | 27 +++++++++---------- .../src/hooks/use-transition.ts | 12 +++------ 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/packages/@headlessui-react/src/components/transition/utils/transition.ts b/packages/@headlessui-react/src/components/transition/utils/transition.ts index 0d34dc1cd4..cbce26fe23 100644 --- a/packages/@headlessui-react/src/components/transition/utils/transition.ts +++ b/packages/@headlessui-react/src/components/transition/utils/transition.ts @@ -76,9 +76,6 @@ function waitForTransition(node: HTMLElement, _done: () => void) { done() } - // If we get disposed before the transition finishes, we should cleanup anyway. - d.add(() => done()) - return d.dispose } @@ -155,17 +152,19 @@ export function transition( // 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. - waitForTransition(node, () => { - removeClasses(node, ...classes.base, ...base) - addClasses(node, ...classes.base, ...classes.entered, ...to) - - // Mark the transition as done. - if (inFlight) inFlight.current = false - - return _done() - }) + // happen that the transition is already done before we start waiting for + // the actual event. + d.add( + waitForTransition(node, () => { + removeClasses(node, ...classes.base, ...base) + addClasses(node, ...classes.base, ...classes.entered, ...to) + + // Mark the transition as done. + if (inFlight) inFlight.current = false + + return _done() + }) + ) // Initiate the transition by applying the new classes. removeClasses(node, ...classes.base, ...base, ...from) diff --git a/packages/@headlessui-react/src/hooks/use-transition.ts b/packages/@headlessui-react/src/hooks/use-transition.ts index 8eb8945e6b..f3479e1fa0 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, type MutableRefObject } from 'react' import { transition } from '../components/transition/utils/transition' -import { disposables } from '../utils/disposables' import { useDisposables } from './use-disposables' import { useIsMounted } from './use-is-mounted' import { useIsoMorphicEffect } from './use-iso-morphic-effect' @@ -53,30 +52,25 @@ export function useTransition({ }, [immediate]) useIsoMorphicEffect(() => { - let dd = disposables() - d.add(dd.dispose) - let node = container.current if (!node) return // We don't have a DOM node (yet) if (latestDirection.current === 'idle') return // We don't need to transition if (!mounted.current) return - dd.dispose() - onStart.current(latestDirection.current) - dd.add( + d.add( transition(node, { direction: latestDirection.current, classes: classes.current, inFlight, done() { - dd.dispose() + d.dispose() onStop.current(latestDirection.current) }, }) ) - return dd.dispose + return d.dispose }, [direction]) } From 5fcd736fc045519625e9725d89918e3253a9e56c Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 3 Apr 2024 16:49:50 +0200 Subject: [PATCH 11/27] update comment --- .../src/components/transition/utils/transition.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/@headlessui-react/src/components/transition/utils/transition.ts b/packages/@headlessui-react/src/components/transition/utils/transition.ts index cbce26fe23..7b7933af38 100644 --- a/packages/@headlessui-react/src/components/transition/utils/transition.ts +++ b/packages/@headlessui-react/src/components/transition/utils/transition.ts @@ -177,9 +177,8 @@ function prepareTransition( node: HTMLElement, { inFlight, prepare }: { inFlight?: MutableRefObject; prepare: () => void } ) { - // If we are already in a transition, then we can skip the preparation of - // force cancelling the current transition. This improves the cancellation of - // existing transitions instead of a hard cut-off. + // If we are already transitioning, then we don't need to force cancel the + // current transition (by triggering a reflow). if (inFlight?.current) { prepare() return From 5b8627400fea2a572a5297a3ae62220db0947d1f Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 3 Apr 2024 20:05:07 +0200 Subject: [PATCH 12/27] fix another bug + update tests --- .../__snapshots__/transition.test.tsx.snap | 1 + .../components/transition/transition.test.tsx | 1 + .../src/components/transition/transition.tsx | 2 +- .../transition/utils/transition.test.ts | 4 +- .../components/transition/utils/transition.ts | 49 ++++++++++++------- .../src/hooks/use-transition.ts | 28 ++--------- .../components/transitions/transition.test.ts | 6 +++ 7 files changed, 46 insertions(+), 45 deletions(-) diff --git a/packages/@headlessui-react/src/components/transition/__snapshots__/transition.test.tsx.snap b/packages/@headlessui-react/src/components/transition/__snapshots__/transition.test.tsx.snap index 38465fb1ef..1555d0302d 100644 --- a/packages/@headlessui-react/src/components/transition/__snapshots__/transition.test.tsx.snap +++ b/packages/@headlessui-react/src/components/transition/__snapshots__/transition.test.tsx.snap @@ -136,6 +136,7 @@ exports[`Setup API transition classes should be possible to passthrough the tran exports[`Setup API transition classes should be possible to passthrough the transition classes and immediately apply the enter transitions when appear is set to true 1`] = `

Children
diff --git a/packages/@headlessui-react/src/components/transition/transition.test.tsx b/packages/@headlessui-react/src/components/transition/transition.test.tsx index 1b33ed6b57..2b1b10e50d 100644 --- a/packages/@headlessui-react/src/components/transition/transition.test.tsx +++ b/packages/@headlessui-react/src/components/transition/transition.test.tsx @@ -343,6 +343,7 @@ describe('Setup API', () => {
Children
diff --git a/packages/@headlessui-react/src/components/transition/transition.tsx b/packages/@headlessui-react/src/components/transition/transition.tsx index b5f992a306..6b682f3471 100644 --- a/packages/@headlessui-react/src/components/transition/transition.tsx +++ b/packages/@headlessui-react/src/components/transition/transition.tsx @@ -369,6 +369,7 @@ function TransitionChildFn { + if (immediate) return 'enter' if (!ready) return 'idle' if (skip) return 'idle' return show ? 'enter' : 'leave' @@ -413,7 +414,6 @@ function TransitionChildFn { expect(snapshots[0].content).toEqual('
') // Start of transition - expect(snapshots[1].content).toEqual('
') + expect(snapshots[1].content).toEqual('
') // NOTE: There is no `enter enterTo`, because we didn't define a duration. Therefore it is not // necessary to put the classes on the element and immediately remove them. // Cleanup phase - expect(snapshots[2].content).toEqual('
') + expect(snapshots[2].content).toEqual('
') d.dispose() }) diff --git a/packages/@headlessui-react/src/components/transition/utils/transition.ts b/packages/@headlessui-react/src/components/transition/utils/transition.ts index 7b7933af38..92e44af0df 100644 --- a/packages/@headlessui-react/src/components/transition/utils/transition.ts +++ b/packages/@headlessui-react/src/components/transition/utils/transition.ts @@ -150,25 +150,36 @@ export function transition( // Mark the transition as in-flight. if (inFlight) inFlight.current = true - // 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, () => { - removeClasses(node, ...classes.base, ...base) - addClasses(node, ...classes.base, ...classes.entered, ...to) - - // Mark the transition as done. - if (inFlight) inFlight.current = false - - return _done() - }) - ) - - // Initiate the transition by applying the new classes. - removeClasses(node, ...classes.base, ...base, ...from) - addClasses(node, ...classes.base, ...base, ...to) + // BUG: This is a workaround for a bug in all major browsers. + // + // 1. When an element is just mounted + // 2. And you apply a transition to it (e.g.: via a class) + // 3. When you use `getComputedStyle` and read any returned value + // 4. Then the `transition` immediately jumps to the end state + // + // This means that no transition happen at all. To fix this, we delay the + // actual transition by one frame. This fixes the bug. + 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, () => { + removeClasses(node, ...classes.base, ...base) + addClasses(node, ...classes.base, ...classes.entered, ...to) + + // Mark the transition as done. + if (inFlight) inFlight.current = false + + return _done() + }) + ) + + // Initiate the transition by applying the new classes. + removeClasses(node, ...classes.base, ...base, ...from) + addClasses(node, ...classes.base, ...base, ...to) + }) return d.dispose } diff --git a/packages/@headlessui-react/src/hooks/use-transition.ts b/packages/@headlessui-react/src/hooks/use-transition.ts index f3479e1fa0..2594bf85b1 100644 --- a/packages/@headlessui-react/src/hooks/use-transition.ts +++ b/packages/@headlessui-react/src/hooks/use-transition.ts @@ -3,10 +3,8 @@ import { transition } from '../components/transition/utils/transition' import { useDisposables } from './use-disposables' import { useIsMounted } from './use-is-mounted' import { useIsoMorphicEffect } from './use-iso-morphic-effect' -import { useLatestValue } from './use-latest-value' interface TransitionArgs { - immediate: boolean container: MutableRefObject classes: MutableRefObject<{ base: string[] @@ -26,47 +24,31 @@ interface TransitionArgs { onStop: MutableRefObject<(direction: TransitionArgs['direction']) => void> } -export function useTransition({ - immediate, - container, - direction, - classes, - onStart, - onStop, -}: TransitionArgs) { +export function useTransition({ container, direction, classes, onStart, onStop }: TransitionArgs) { let mounted = useIsMounted() let d = useDisposables() - let latestDirection = useLatestValue(direction) - // Track whether the transition is in flight or not. This will help us for // cancelling mid-transition because in that case we don't have to force // clearing existing transitions. See: `prepareTransition` in the `transition` // file. let inFlight = useRef(false) - useIsoMorphicEffect(() => { - if (!immediate) return - - latestDirection.current = 'enter' - }, [immediate]) - useIsoMorphicEffect(() => { let node = container.current if (!node) return // We don't have a DOM node (yet) - if (latestDirection.current === 'idle') return // We don't need to transition + if (direction === 'idle') return // We don't need to transition if (!mounted.current) return - onStart.current(latestDirection.current) + onStart.current(direction) d.add( transition(node, { - direction: latestDirection.current, + direction, classes: classes.current, inFlight, done() { - d.dispose() - onStop.current(latestDirection.current) + onStop.current(direction) }, }) ) diff --git a/packages/@headlessui-vue/src/components/transitions/transition.test.ts b/packages/@headlessui-vue/src/components/transitions/transition.test.ts index 933db08629..12cb56f04b 100644 --- a/packages/@headlessui-vue/src/components/transitions/transition.test.ts +++ b/packages/@headlessui-vue/src/components/transitions/transition.test.ts @@ -864,6 +864,12 @@ describe('Events', () => { let leaveDuration = 75 withStyles(` + .enter-1, .enter-2, .leave-1, .leave-2 { + transition-property: opacity; + transition-timing-function: cubic-bezier(0.4, 0, 0.2, 1); + transition-duration: 150ms; + } + .enter-1 { transition-duration: ${enterDuration * 1}ms; } .enter-2 { transition-duration: ${enterDuration * 2}ms; } .enter-from { opacity: 0%; } From b28c2a5fab3e674462a6d99d31f45830ed17cae7 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 3 Apr 2024 20:15:56 +0200 Subject: [PATCH 13/27] add failing test as playground --- playgrounds/react/pages/dialog/repro.tsx | 66 ++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 playgrounds/react/pages/dialog/repro.tsx diff --git a/playgrounds/react/pages/dialog/repro.tsx b/playgrounds/react/pages/dialog/repro.tsx new file mode 100644 index 0000000000..ba88267787 --- /dev/null +++ b/playgrounds/react/pages/dialog/repro.tsx @@ -0,0 +1,66 @@ +import { Transition } from '@headlessui/react' +import { useEffect, useState } from 'react' + +let enterDuration = 50 +let leaveDuration = 75 + +export default function Example() { + let [show, setShow] = useState(false) + let [start, setStart] = useState(Date.now()) + + useEffect(() => setStart(Date.now()), []) + + return ( + <> + + console.log('beforeEnter', Date.now() - start)} + afterEnter={() => console.log('afterEnter', Date.now() - start)} + beforeLeave={() => console.log('beforeLeave', Date.now() - start)} + afterLeave={() => console.log('afterLeave', Date.now() - start)} + enter="enter-2" + enterFrom="enter-from" + enterTo="enter-to" + leave="leave-2" + leaveFrom="leave-from" + leaveTo="leave-to" + > + + + + + + + + ) +} From 7b2fb84b1a8ec59ac3393a23637b60a4afda978a Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 3 Apr 2024 23:47:51 +0200 Subject: [PATCH 14/27] simplify event callbacks Instead of always ensuring that there is an event, let's use the `?.` operator and conditionally call the callbacks instead. --- .../src/components/transition/transition.tsx | 31 +++---------------- 1 file changed, 5 insertions(+), 26 deletions(-) diff --git a/packages/@headlessui-react/src/components/transition/transition.tsx b/packages/@headlessui-react/src/components/transition/transition.tsx index 6b682f3471..1cee2bf062 100644 --- a/packages/@headlessui-react/src/components/transition/transition.tsx +++ b/packages/@headlessui-react/src/components/transition/transition.tsx @@ -4,7 +4,6 @@ import React, { Fragment, createContext, useContext, - useEffect, useMemo, useRef, useState, @@ -259,26 +258,6 @@ function useNesting(done?: () => void, parent?: NestingContextValues) { ) } -function noop() {} -let eventNames = ['beforeEnter', 'afterEnter', 'beforeLeave', 'afterLeave'] as const -function ensureEventHooksExist(events: TransitionEvents) { - let result = {} as Record void> - for (let name of eventNames) { - result[name] = events[name] ?? noop - } - return result -} - -function useEvents(events: TransitionEvents) { - let eventsRef = useRef(ensureEventHooksExist(events)) - - useEffect(() => { - eventsRef.current = ensureEventHooksExist(events) - }, [events]) - - return eventsRef -} - // --- let DEFAULT_TRANSITION_CHILD_TAG = 'div' as const @@ -349,7 +328,7 @@ function TransitionChildFn { transitionStateFlags.addFlag(State.Opening) - events.current.beforeEnter() + events.current.beforeEnter?.() }, leave: () => { transitionStateFlags.addFlag(State.Closing) - events.current.beforeLeave() + events.current.beforeLeave?.() }, idle: () => {}, }) @@ -395,11 +374,11 @@ function TransitionChildFn { transitionStateFlags.removeFlag(State.Opening) - events.current.afterEnter() + events.current.afterEnter?.() }, leave: () => { transitionStateFlags.removeFlag(State.Closing) - events.current.afterLeave() + events.current.afterLeave?.() }, idle: () => {}, }) From ce083ca5344035a57573363040200ef83f07a2ab Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Wed, 3 Apr 2024 23:50:12 +0200 Subject: [PATCH 15/27] use existing `useOnDisappear` hook --- .../src/components/transition/transition.tsx | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/packages/@headlessui-react/src/components/transition/transition.tsx b/packages/@headlessui-react/src/components/transition/transition.tsx index 1cee2bf062..4d74bdeea9 100644 --- a/packages/@headlessui-react/src/components/transition/transition.tsx +++ b/packages/@headlessui-react/src/components/transition/transition.tsx @@ -17,6 +17,7 @@ import { useFlags } from '../../hooks/use-flags' import { useIsMounted } from '../../hooks/use-is-mounted' import { useIsoMorphicEffect } from '../../hooks/use-iso-morphic-effect' import { useLatestValue } from '../../hooks/use-latest-value' +import { useOnDisappear } from '../../hooks/use-on-disappear' import { useServerHandoffComplete } from '../../hooks/use-server-handoff-complete' import { useSyncRefs } from '../../hooks/use-sync-refs' import { useTransition } from '../../hooks/use-transition' @@ -530,23 +531,14 @@ function TransitionRootFn