Skip to content

Commit 49b4a5b

Browse files
sampottscursoragent
andcommitted
fix(core): resolve transition Bugbot findings on #1582
Read the current element when open animations settle so lazy-mounted popups pick up setElement during the render cycle, and defer scheduleTransitionSettle to the menu viewport PR where it is used. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 4f2f64e commit 49b4a5b

2 files changed

Lines changed: 27 additions & 55 deletions

File tree

packages/core/src/dom/ui/tests/transition.test.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,31 @@ describe('createTransition', () => {
5353

5454
expect(handler.state.current.transitioning).toBe(false);
5555
});
56+
57+
it('waits for animations on an element registered after open', async () => {
58+
const handler = createTransition();
59+
let resolveAnimation!: () => void;
60+
const animation = new Promise<void>((resolve) => {
61+
resolveAnimation = resolve;
62+
});
63+
64+
const promise = handler.open();
65+
66+
const el = document.createElement('div');
67+
el.getAnimations = vi.fn(() => [{ finished: animation } as unknown as Animation]);
68+
handler.setElement(el);
69+
70+
await vi.waitFor(() => {
71+
expect(handler.state.current.status).toBe('idle');
72+
});
73+
74+
expect(handler.state.current.transitioning).toBe(true);
75+
76+
resolveAnimation();
77+
await promise;
78+
79+
expect(handler.state.current.transitioning).toBe(false);
80+
});
5681
});
5782

5883
describe('close', () => {

packages/core/src/dom/ui/transition.ts

Lines changed: 2 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { createState, type State } from '@videojs/store';
2-
import { afterDoubleAnimationFrame, getMaxCSSTransitionTime } from '@videojs/utils/dom';
2+
import { getMaxCSSTransitionTime } from '@videojs/utils/dom';
33
import { noop } from '@videojs/utils/function';
44
import type { TransitionState } from '../../core/ui/transition';
55

@@ -16,57 +16,6 @@ export interface WaitForAnimationsOptions {
1616
includeCSSTransitions?: boolean;
1717
}
1818

19-
export interface ScheduleTransitionSettleOptions {
20-
includeCSSTransitions?: boolean;
21-
isVisuallyComplete?: (element: HTMLElement) => boolean;
22-
}
23-
24-
/** After double-RAF, optionally poll visual completion, then wait for animations to settle. */
25-
export function scheduleTransitionSettle(
26-
element: HTMLElement,
27-
isCurrent: () => boolean,
28-
onSettled: () => void,
29-
options: ScheduleTransitionSettleOptions = {}
30-
): () => void {
31-
let pollRaf = 0;
32-
let settled = false;
33-
34-
function settle(): void {
35-
if (settled || !isCurrent()) return;
36-
settled = true;
37-
cancelAnimationFrame(pollRaf);
38-
pollRaf = 0;
39-
onSettled();
40-
}
41-
42-
function pollVisualComplete(): void {
43-
pollRaf = 0;
44-
if (settled || !isCurrent()) return;
45-
46-
if (options.isVisuallyComplete?.(element)) {
47-
settle();
48-
return;
49-
}
50-
51-
pollRaf = requestAnimationFrame(pollVisualComplete);
52-
}
53-
54-
afterDoubleAnimationFrame(isCurrent, () => {
55-
pollVisualComplete();
56-
57-
waitForAnimations(element, {
58-
includeCSSTransitions: options.includeCSSTransitions ?? false,
59-
}).then(() => {
60-
settle();
61-
});
62-
});
63-
64-
return () => {
65-
settled = true;
66-
cancelAnimationFrame(pollRaf);
67-
};
68-
}
69-
7019
/**
7120
* Manages open/close transition lifecycle via `createState`.
7221
*
@@ -109,8 +58,6 @@ export function createTransition(): TransitionApi {
10958
element = el;
11059
}
11160

112-
const animationTarget = element;
113-
11461
state.patch({ active: true, status: 'starting', transitioning: true });
11562

11663
return new Promise<void>((resolve) => {
@@ -120,7 +67,7 @@ export function createTransition(): TransitionApi {
12067
rafId2 = 0;
12168
if (destroyed || currentTransitionId !== transitionId || !state.current.active) return resolve();
12269
state.patch({ status: 'idle' });
123-
waitForAnimations(animationTarget).finally(() => {
70+
waitForAnimations(element).finally(() => {
12471
if (destroyed || currentTransitionId !== transitionId || !state.current.active) return resolve();
12572
state.patch({ transitioning: false });
12673
resolve();

0 commit comments

Comments
 (0)