Fix enter transitions for the Transition component (#3074)

* 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... * simplify check * updating playgrounds to improve transitions * update changelog * track in-flight transitions * document `disposables()` and `useDisposables()` functions * ensure we alway return a cleanup function * move comment * apply `enterTo` or `leaveTo` classes once we are done transitioning * cleanup & simplify logic * update comment * fix another bug + update tests * add failing test as playground * simplify event callbacks Instead of always ensuring that there is an event, let's use the `?.` operator and conditionally call the callbacks instead. * use existing `useOnDisappear` hook * remove repro * only unmount if we are not transitioning ourselves * `show` is already guaranteed to be a boolean In a previous commit we checked for `undefined` and threw an error. Which means that this part is unreachable if `show` is undefined. * cleanup temporary test changes * Update packages/@headlessui-react/src/components/transition/utils/transition.ts Co-authored-by: Jonathan Reinink <jonathan@reinink.ca> * Update packages/@headlessui-react/src/components/transition/utils/transition.ts Co-authored-by: Jonathan Reinink <jonathan@reinink.ca> * Update packages/@headlessui-react/src/components/transition/utils/transition.ts Co-authored-by: Jonathan Reinink <jonathan@reinink.ca> * Update packages/@headlessui-react/src/components/transition/utils/transition.ts Co-authored-by: Jonathan Reinink <jonathan@reinink.ca> * Update packages/@headlessui-react/src/utils/disposables.ts Co-authored-by: Jonathan Reinink <jonathan@reinink.ca> * Update packages/@headlessui-react/src/components/transition/transition.tsx Co-authored-by: Jonathan Reinink <jonathan@reinink.ca> * Update packages/@headlessui-react/src/components/transition/transition.tsx Co-authored-by: Jonathan Reinink <jonathan@reinink.ca> * run prettier --------- Co-authored-by: Jonathan Reinink <jonathan@reinink.ca>
tailwindlabs · Apr 5, 2024 · c8037fc · c8037fc
1 parent c1d3b7e
commit c8037fc
Show file tree

Hide file tree

Showing 11 changed files with 234 additions and 166 deletions.
diff --git 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 `<ComboboxInput />` 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
 

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
@@ -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`] = `
 <div
   class="enter enter-from"
+  style=""
 >
   Children
 </div>

diff --git 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', () => {
           <div
             class="foo1
         foo2 foo1 foo2 leave"
+            style=""
           >
             Children
           </div>

diff --git 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,
@@ -18,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'
@@ -259,26 +259,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<keyof typeof events, () => 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 +329,7 @@ function TransitionChildFn<TTag extends ElementType = typeof DEFAULT_TRANSITION_
     leaveTo: splitClasses(leaveTo),
   })
 
-  let events = useEvents({
+  let events = useLatestValue({
     beforeEnter,
     afterEnter,
     beforeLeave,
@@ -369,6 +349,7 @@ function TransitionChildFn<TTag extends ElementType = typeof DEFAULT_TRANSITION_
   let immediate = appear && show && initial
 
   let transitionDirection = (() => {
+    if (immediate) return 'enter'
     if (!ready) return 'idle'
     if (skip) return 'idle'
     return show ? 'enter' : 'leave'
@@ -380,11 +361,11 @@ function TransitionChildFn<TTag extends ElementType = typeof DEFAULT_TRANSITION_
     return match(direction, {
       enter: () => {
         transitionStateFlags.addFlag(State.Opening)
-        events.current.beforeEnter()
+        events.current.beforeEnter?.()
       },
       leave: () => {
         transitionStateFlags.addFlag(State.Closing)
-        events.current.beforeLeave()
+        events.current.beforeLeave?.()
       },
       idle: () => {},
     })
@@ -394,26 +375,29 @@ function TransitionChildFn<TTag extends ElementType = typeof DEFAULT_TRANSITION_
     return match(direction, {
       enter: () => {
         transitionStateFlags.removeFlag(State.Opening)
-        events.current.afterEnter()
+        events.current.afterEnter?.()
       },
       leave: () => {
         transitionStateFlags.removeFlag(State.Closing)
-        events.current.afterLeave()
+        events.current.afterLeave?.()
       },
       idle: () => {},
     })
   })
 
+  let isTransitioning = useRef(false)
+
   let nesting = useNesting(() => {
-    // When all children have been unmounted we can only hide ourselves if and only if we are not
-    // transitioning ourselves. Otherwise we would unmount before the transitions are finished.
+    // When all children have been unmounted we can only hide ourselves if and
+    // only if we are not transitioning ourselves. Otherwise we would unmount
+    // before the transitions are finished.
+    if (isTransitioning.current) return
+
     setState(TreeStates.Hidden)
     unregister(container)
   }, parentNesting)
 
-  let isTransitioning = useRef(false)
   useTransition({
-    immediate,
     container,
     classes,
     direction: transitionDirection,
@@ -426,8 +410,8 @@ function TransitionChildFn<TTag extends ElementType = typeof DEFAULT_TRANSITION_
       nesting.onStop(container, direction, afterEvent)
 
       if (direction === 'leave' && !hasChildren(nesting)) {
-        // When we don't have children anymore we can safely unregister from the parent and hide
-        // ourselves.
+        // When we don't have children anymore we can safely unregister from the
+        // parent and hide ourselves.
         setState(TreeStates.Hidden)
         unregister(container)
       }
@@ -437,10 +421,10 @@ function TransitionChildFn<TTag extends ElementType = typeof DEFAULT_TRANSITION_
   let theirProps = rest
   let ourProps = { ref: transitionRef }
 
+  // Already apply the `enter` and `enterFrom` on the server if required
   if (immediate) {
     theirProps = {
       ...theirProps,
-      // Already apply the `enter` and `enterFrom` on the server if required
       className: classNames(rest.className, ...classes.current.enter, ...classes.current.enterFrom),
     }
   }
@@ -457,6 +441,21 @@ function TransitionChildFn<TTag extends ElementType = typeof DEFAULT_TRANSITION_
     if (theirProps.className === '') delete theirProps.className
   }
 
+  // If we were never transitioning, or we're not transitioning anymore, then
+  // apply the `enterTo` and `leaveTo` classes as the final state.
+  else {
+    theirProps.className = classNames(
+      rest.className,
+      container.current?.className,
+      ...match(transitionDirection, {
+        enter: [...classes.current.enterTo, ...classes.current.entered],
+        leave: classes.current.leaveTo,
+        idle: [],
+      })
+    )
+    if (theirProps.className === '') delete theirProps.className
+  }
+
   return (
     <NestingContext.Provider value={nesting}>
       <OpenClosedProvider
@@ -504,7 +503,7 @@ function TransitionRootFn<TTag extends ElementType = typeof DEFAULT_TRANSITION_C
     show = (usesOpenClosedState & State.Open) === State.Open
   }
 
-  if (![true, false].includes(show as unknown as boolean)) {
+  if (show === undefined) {
     throw new Error('A <Transition /> is used but it is missing a `show={true | false}` prop.')
   }
 
@@ -532,27 +531,18 @@ function TransitionRootFn<TTag extends ElementType = typeof DEFAULT_TRANSITION_C
   }, [changes, show])
 
   let transitionBag = useMemo<TransitionContextValues>(
-    () => ({ show: show as boolean, appear, initial }),
+    () => ({ show, appear, initial }),
     [show, appear, initial]
   )
 
+  // Ensure we change the tree state to hidden once the transition becomes hidden
+  useOnDisappear(internalTransitionRef, () => setState(TreeStates.Hidden))
+
   useIsoMorphicEffect(() => {
     if (show) {
       setState(TreeStates.Visible)
     } else if (!hasChildren(nestingBag)) {
       setState(TreeStates.Hidden)
-    } else if (
-      process.env.NODE_ENV !==
-      'test' /* TODO: Remove this once we have real tests! JSDOM doesn't "render", therefore getBoundingClientRect() will always result in `0`. */
-    ) {
-      let node = internalTransitionRef.current
-      if (!node) return
-      let rect = node.getBoundingClientRect()
-
-      if (rect.x === 0 && rect.y === 0 && rect.width === 0 && rect.height === 0) {
-        // The node is completely hidden, let's hide it
-        setState(TreeStates.Hidden)
-      }
     }
   }, [show, nestingBag])
 

diff --git 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<void>((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))
@@ -49,13 +48,13 @@ it('should be possible to transition', async () => {
   expect(snapshots[0].content).toEqual('<div></div>')
 
   // Start of transition
-  expect(snapshots[1].content).toEqual('<div class="enter enterFrom"></div>')
+  expect(snapshots[1].content).toEqual('<div style="" class="enter enterFrom"></div>')
 
   // 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('<div class="enterTo entered"></div>')
+  expect(snapshots[2].content).toEqual('<div style="" class="entered enterTo enter"></div>')
 
   d.dispose()
 })
@@ -84,9 +83,9 @@ it('should wait the correct amount of time to finish a transition', async () =>
   )
 
   await new Promise<void>((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<void>((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))