From f07a118db43c5617567c4091a3c2983a6e14ab5c Mon Sep 17 00:00:00 2001 From: Kyle Mercer Date: Wed, 26 Mar 2025 10:27:44 -0400 Subject: [PATCH 1/3] fix: fixes a bug with runAfterTransition getting stuck when elements are suddenly removed without calling transitionend/transitioncancel. --- .../utils/src/runAfterTransition.ts | 14 ++ .../utils/test/runAfterTransition.test.ts | 129 ++++++++++++++++++ 2 files changed, 143 insertions(+) create mode 100644 packages/@react-aria/utils/test/runAfterTransition.test.ts diff --git a/packages/@react-aria/utils/src/runAfterTransition.ts b/packages/@react-aria/utils/src/runAfterTransition.ts index 1656cdf9030..00d206c2e81 100644 --- a/packages/@react-aria/utils/src/runAfterTransition.ts +++ b/packages/@react-aria/utils/src/runAfterTransition.ts @@ -91,9 +91,23 @@ if (typeof document !== 'undefined') { } } +/** + * Cleans up any elements that are no longer in the document. + * This is necessary because we can't rely on transitionend events to fire + * for elements that are removed from the document while transitioning. + */ +function cleanupDetachedElements() { + for (const [element] of transitionsByElement) { + if (element instanceof HTMLElement && !document.contains(element)) { + transitionsByElement.delete(element); + } + } +} + export function runAfterTransition(fn: () => void): void { // Wait one frame to see if an animation starts, e.g. a transition on mount. requestAnimationFrame(() => { + cleanupDetachedElements(); // If no transitions are running, call the function immediately. // Otherwise, add it to a list of callbacks to run at the end of the animation. if (transitionsByElement.size === 0) { diff --git a/packages/@react-aria/utils/test/runAfterTransition.test.ts b/packages/@react-aria/utils/test/runAfterTransition.test.ts new file mode 100644 index 00000000000..216683dd3b9 --- /dev/null +++ b/packages/@react-aria/utils/test/runAfterTransition.test.ts @@ -0,0 +1,129 @@ +import {runAfterTransition} from '../src/runAfterTransition'; + +class MockTransitionEvent extends Event { + propertyName: string; + + constructor(type: string, eventInitDict?: TransitionEventInit) { + super(type, eventInitDict); + this.propertyName = eventInitDict?.propertyName || ''; + } +} + +describe('runAfterTransition', () => { + const originalTransitionEvent = global.TransitionEvent; + const nodes = new Set(); + + beforeAll(() => { + global.TransitionEvent = MockTransitionEvent as any; + }); + + afterAll(() => { + global.TransitionEvent = originalTransitionEvent; + }); + + beforeEach(() => { + jest + .spyOn(window, 'requestAnimationFrame') + // @ts-expect-error -- mock + .mockImplementation((cb) => cb()); + }); + + afterEach(() => { + jest.restoreAllMocks(); + cleanupElements(); + }); + + function appendElement(element: HTMLElement) { + nodes.add(element); + document.body.appendChild(element); + return element; + } + + function cleanupElements() { + for (const node of nodes) { + document.body.removeChild(node); + nodes.delete(node); + } + } + + it('calls callback immediately when no transition is running', () => { + const callback = jest.fn(); + runAfterTransition(callback); + expect(callback).toHaveBeenCalled(); + }); + + it('defers callback until transition end when a transition is running', () => { + const element = appendElement(document.createElement('div')); + + const callback = jest.fn(); + + element.dispatchEvent( + new TransitionEvent('transitionrun', { + propertyName: 'opacity', + bubbles: true + }) + ); + + runAfterTransition(callback); + // Callback should not be called immediately since a transition is active. + expect(callback).not.toHaveBeenCalled(); + + // Dispatch a transitionend event to finish the transition. + element.dispatchEvent( + new TransitionEvent('transitionend', { + propertyName: 'opacity', + bubbles: true + }) + ); + expect(callback).toHaveBeenCalled(); + }); + + it('calls multiple queued callbacks after transition ends', () => { + const element = appendElement(document.createElement('div')); + const callback1 = jest.fn(); + const callback2 = jest.fn(); + + element.dispatchEvent( + new TransitionEvent('transitionrun', { + propertyName: 'width', + bubbles: true + }) + ); + + runAfterTransition(callback1); + runAfterTransition(callback2); + // Callbacks should not be called during transition. + expect(callback1).not.toHaveBeenCalled(); + expect(callback2).not.toHaveBeenCalled(); + + element.dispatchEvent( + new TransitionEvent('transitionend', { + propertyName: 'width', + bubbles: true + }) + ); + + expect(callback1).toHaveBeenCalled(); + expect(callback2).toHaveBeenCalled(); + }); + + it('clears out detached elements from transitionsByElement', () => { + const element = document.createElement('div'); + element.id = 'test-element'; + appendElement(element); + const callback = jest.fn(); + + element.dispatchEvent( + new TransitionEvent('transitionrun', { + propertyName: 'width', + bubbles: true + }) + ); + + cleanupElements(); + + runAfterTransition(callback); + + expect(callback).toHaveBeenCalled(); + }); +}); From 680cbeecba0cadd338ec887b14b76ae565985c3c Mon Sep 17 00:00:00 2001 From: Kyle Mercer Date: Mon, 31 Mar 2025 10:15:04 -0400 Subject: [PATCH 2/3] converts to isConnected and uses non-mock RAF for tests --- .../@react-aria/utils/src/runAfterTransition.ts | 2 +- .../utils/test/runAfterTransition.test.ts | 16 +++++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/packages/@react-aria/utils/src/runAfterTransition.ts b/packages/@react-aria/utils/src/runAfterTransition.ts index 00d206c2e81..d03233d1507 100644 --- a/packages/@react-aria/utils/src/runAfterTransition.ts +++ b/packages/@react-aria/utils/src/runAfterTransition.ts @@ -98,7 +98,7 @@ if (typeof document !== 'undefined') { */ function cleanupDetachedElements() { for (const [element] of transitionsByElement) { - if (element instanceof HTMLElement && !document.contains(element)) { + if (element instanceof HTMLElement && !element.isConnected) { transitionsByElement.delete(element); } } diff --git a/packages/@react-aria/utils/test/runAfterTransition.test.ts b/packages/@react-aria/utils/test/runAfterTransition.test.ts index 216683dd3b9..c0e67b5c260 100644 --- a/packages/@react-aria/utils/test/runAfterTransition.test.ts +++ b/packages/@react-aria/utils/test/runAfterTransition.test.ts @@ -1,3 +1,4 @@ +import {act} from '@testing-library/react'; import {runAfterTransition} from '../src/runAfterTransition'; class MockTransitionEvent extends Event { @@ -20,15 +21,13 @@ describe('runAfterTransition', () => { afterAll(() => { global.TransitionEvent = originalTransitionEvent; }); - + beforeEach(() => { - jest - .spyOn(window, 'requestAnimationFrame') - // @ts-expect-error -- mock - .mockImplementation((cb) => cb()); + jest.useFakeTimers(); }); afterEach(() => { + jest.useRealTimers(); jest.restoreAllMocks(); cleanupElements(); }); @@ -49,6 +48,7 @@ describe('runAfterTransition', () => { it('calls callback immediately when no transition is running', () => { const callback = jest.fn(); runAfterTransition(callback); + act(() => {jest.runOnlyPendingTimers();}); expect(callback).toHaveBeenCalled(); }); @@ -64,7 +64,10 @@ describe('runAfterTransition', () => { }) ); + runAfterTransition(callback); + act(() => {jest.runOnlyPendingTimers();}); + // Callback should not be called immediately since a transition is active. expect(callback).not.toHaveBeenCalled(); @@ -91,7 +94,9 @@ describe('runAfterTransition', () => { ); runAfterTransition(callback1); + act(() => {jest.runOnlyPendingTimers();}); runAfterTransition(callback2); + act(() => {jest.runOnlyPendingTimers();}); // Callbacks should not be called during transition. expect(callback1).not.toHaveBeenCalled(); expect(callback2).not.toHaveBeenCalled(); @@ -123,6 +128,7 @@ describe('runAfterTransition', () => { cleanupElements(); runAfterTransition(callback); + act(() => {jest.runOnlyPendingTimers();}); expect(callback).toHaveBeenCalled(); }); From 260f5f3479c35fe72d59e8eb60934a6a288d976e Mon Sep 17 00:00:00 2001 From: Kyle Mercer Date: Wed, 2 Apr 2025 12:38:04 -0400 Subject: [PATCH 3/3] update check --- packages/@react-aria/utils/src/runAfterTransition.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/@react-aria/utils/src/runAfterTransition.ts b/packages/@react-aria/utils/src/runAfterTransition.ts index d03233d1507..3004d2313df 100644 --- a/packages/@react-aria/utils/src/runAfterTransition.ts +++ b/packages/@react-aria/utils/src/runAfterTransition.ts @@ -97,9 +97,11 @@ if (typeof document !== 'undefined') { * for elements that are removed from the document while transitioning. */ function cleanupDetachedElements() { - for (const [element] of transitionsByElement) { - if (element instanceof HTMLElement && !element.isConnected) { - transitionsByElement.delete(element); + for (const [eventTarget] of transitionsByElement) { + // Similar to `eventTarget instanceof Element && !eventTarget.isConnected`, but avoids + // the explicit instanceof check, since it may be different in different contexts. + if ('isConnected' in eventTarget && !eventTarget.isConnected) { + transitionsByElement.delete(eventTarget); } } }