From a6dea8af4b773b171a0b8f73481ce502bff789b4 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Tue, 6 Dec 2022 17:22:27 +0100 Subject: [PATCH] Fix `Dialog` unmounting problem due to incorrect `transitioncancel` event in the `Transition` component on Android (#2071) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * remove `transitioncancel` logic On Desktop Sarai, Chrome, and on mobile iOS Safari the `transitioncancel` is never called on outside click of the Dialog. However, on mobile Android Chrome it _is_ called, and the `transitionend` is never triggered for _some_ reason. According to the MDN docs: > If the transitioncancel event is fired, the transitionend event will not fire. > > — https://developer.mozilla.org/en-US/docs/Web/API/Element/transitioncancel_event When testing this, I never got into the `transitionend` when I got into the `transitioncancel` first. But, once I removed the `transitioncancel` logic, the `transitionend` code _was_ being called. The code is now both simpler, and works again. The nice part is that we never did anything with the `cancel` event. We marked it as done using the `Reason.Cancelled` and that's about it. * cleanup transition completion `Reason` * update changelog --- packages/@headlessui-react/CHANGELOG.md | 1 + .../transitions/utils/transition.test.ts | 10 ++- .../transitions/utils/transition.ts | 67 +++++-------------- .../src/hooks/use-transition.ts | 13 +--- 4 files changed, 25 insertions(+), 66 deletions(-) diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index 209b4e00e0..d9e8b5041e 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/packages/@headlessui-react/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix crash when using `multiple` mode without `value` prop (uncontrolled) for `Listbox` and `Combobox` components ([#2058](https://github.com/tailwindlabs/headlessui/pull/2058)) - Apply `enter` and `enterFrom` classes in SSR for `Transition` component ([#2059](https://github.com/tailwindlabs/headlessui/pull/2059)) - Allow passing in your own `id` prop ([#2060](https://github.com/tailwindlabs/headlessui/pull/2060)) +- Fix `Dialog` unmounting problem due to incorrect `transitioncancel` event in the `Transition` component on Android ([#2071](https://github.com/tailwindlabs/headlessui/pull/2071)) ## [1.7.4] - 2022-11-03 diff --git a/packages/@headlessui-react/src/components/transitions/utils/transition.test.ts b/packages/@headlessui-react/src/components/transitions/utils/transition.test.ts index 65230b0621..67fe9c8d95 100644 --- a/packages/@headlessui-react/src/components/transitions/utils/transition.test.ts +++ b/packages/@headlessui-react/src/components/transitions/utils/transition.test.ts @@ -1,4 +1,4 @@ -import { Reason, transition } from './transition' +import { transition } from './transition' import { reportChanges } from '../../../test-utils/report-dom-node-changes' import { disposables } from '../../../utils/disposables' @@ -26,7 +26,7 @@ it('should be possible to transition', async () => { ) ) - await new Promise((resolve) => { + await new Promise((resolve) => { transition( element, { @@ -83,7 +83,7 @@ it('should wait the correct amount of time to finish a transition', async () => ) ) - let reason = await new Promise((resolve) => { + await new Promise((resolve) => { transition( element, { @@ -101,7 +101,6 @@ it('should wait the correct amount of time to finish a transition', async () => }) await new Promise((resolve) => d.nextFrame(resolve)) - expect(reason).toBe(Reason.Ended) // Initial render: expect(snapshots[0].content).toEqual(`
`) @@ -153,7 +152,7 @@ it('should keep the delay time into account', async () => { ) ) - let reason = await new Promise((resolve) => { + await new Promise((resolve) => { transition( element, { @@ -171,7 +170,6 @@ it('should keep the delay time into account', async () => { }) await new Promise((resolve) => d.nextFrame(resolve)) - expect(reason).toBe(Reason.Ended) let estimatedDuration = Number( (snapshots[snapshots.length - 1].recordedAt - snapshots[snapshots.length - 2].recordedAt) / diff --git a/packages/@headlessui-react/src/components/transitions/utils/transition.ts b/packages/@headlessui-react/src/components/transitions/utils/transition.ts index d37f4b42a2..108a0ac15d 100644 --- a/packages/@headlessui-react/src/components/transitions/utils/transition.ts +++ b/packages/@headlessui-react/src/components/transitions/utils/transition.ts @@ -10,15 +10,7 @@ function removeClasses(node: HTMLElement, ...classes: string[]) { node && classes.length > 0 && node.classList.remove(...classes) } -export enum Reason { - // Transition succesfully ended - Ended = 'ended', - - // Transition was cancelled - Cancelled = 'cancelled', -} - -function waitForTransition(node: HTMLElement, done: (reason: Reason) => void) { +function waitForTransition(node: HTMLElement, done: () => void) { let d = disposables() if (!node) return d.dispose @@ -41,49 +33,26 @@ function waitForTransition(node: HTMLElement, done: (reason: Reason) => void) { let totalDuration = durationMs + delayMs if (totalDuration !== 0) { - let listeners: (() => void)[] = [] - if (process.env.NODE_ENV === 'test') { - listeners.push( - d.setTimeout(() => { - done(Reason.Ended) - listeners.splice(0).forEach((dispose) => dispose()) - }, totalDuration) - ) + let dispose = d.setTimeout(() => { + done() + dispose() + }, totalDuration) } else { - listeners.push( - d.addEventListener(node, 'transitionrun', (event) => { - if (event.target !== event.currentTarget) return - - // Cleanup "old" listeners - listeners.splice(0).forEach((dispose) => dispose()) - - // Register new listeners - listeners.push( - d.addEventListener(node, 'transitionend', (event) => { - if (event.target !== event.currentTarget) return - - done(Reason.Ended) - listeners.splice(0).forEach((dispose) => dispose()) - }), - d.addEventListener(node, 'transitioncancel', (event) => { - if (event.target !== event.currentTarget) return - - done(Reason.Cancelled) - listeners.splice(0).forEach((dispose) => dispose()) - }) - ) - }) - ) + let dispose = d.addEventListener(node, 'transitionend', (event) => { + if (event.target !== event.currentTarget) return + done() + dispose() + }) } } else { // No transition is happening, so we should cleanup already. Otherwise we have to wait until we // get disposed. - done(Reason.Ended) + done() } // If we get disposed before the transition finishes, we should cleanup anyway. - d.add(() => done(Reason.Cancelled)) + d.add(() => done()) return d.dispose } @@ -100,7 +69,7 @@ export function transition( entered: string[] }, show: boolean, - done?: (reason: Reason) => void + done?: () => void ) { let direction = show ? 'enter' : 'leave' let d = disposables() @@ -144,13 +113,11 @@ export function transition( removeClasses(node, ...from) addClasses(node, ...to) - waitForTransition(node, (reason) => { - if (reason === Reason.Ended) { - removeClasses(node, ...base) - addClasses(node, ...classes.entered) - } + waitForTransition(node, () => { + removeClasses(node, ...base) + addClasses(node, ...classes.entered) - return _done(reason) + return _done() }) }) diff --git a/packages/@headlessui-react/src/hooks/use-transition.ts b/packages/@headlessui-react/src/hooks/use-transition.ts index a171e7660e..23405f8a9b 100644 --- a/packages/@headlessui-react/src/hooks/use-transition.ts +++ b/packages/@headlessui-react/src/hooks/use-transition.ts @@ -1,8 +1,7 @@ import { MutableRefObject } from 'react' -import { Reason, transition } from '../components/transitions/utils/transition' +import { transition } from '../components/transitions/utils/transition' import { disposables } from '../utils/disposables' -import { match } from '../utils/match' import { useDisposables } from './use-disposables' import { useIsMounted } from './use-is-mounted' @@ -47,15 +46,9 @@ export function useTransition({ container, direction, classes, onStart, onStop } onStart.current(latestDirection.current) dd.add( - transition(node, classes.current, latestDirection.current === 'enter', (reason) => { + transition(node, classes.current, latestDirection.current === 'enter', () => { dd.dispose() - - match(reason, { - [Reason.Ended]() { - onStop.current(latestDirection.current) - }, - [Reason.Cancelled]: () => {}, - }) + onStop.current(latestDirection.current) }) )