onAnimatePlay callback

[mattgperry]

This implements the feature requested in issue #2765. The new onAnimationPlay callback fires when an animation actually begins playing in the DOM, after any delay or staggerChildren settings have completed.

Key changes:

• Add onDelayComplete callback to AnimationPlaybackLifecycles
• Track delay phase exit in JSAnimation.tick() with hasExitedDelayPhase flag
• Add timeout-based delay tracking in NativeAnimation for WAAPI
• Thread onDelayComplete through visual-element-target.ts with "fire once" wrapper to ensure it only fires when the first property animation starts
• Add AnimationPlay event to VisualElement event system
• Add onAnimationPlay prop type with JSDoc documentation
• Fire AnimationPlay event for children at their respective delay times
• Add tests for the new callback behavior

This allows users to know when their animation truly starts, not just when it's queued, which is particularly useful for coordinating UI updates with delayed or staggered animations.

Closes #2765

[claude]

PR Review: onAnimationPlay callback

I've reviewed this PR implementing the onAnimationPlay callback feature. Overall, this is a well-structured implementation with good test coverage. Here's my detailed feedback:

---

✅ Strengths

1. Clean Architecture

• The implementation correctly threads the onDelayComplete callback through the animation system layers (JSAnimation, NativeAnimation → visual-element-target → visual-element)
• Proper separation of concerns: internal onDelayComplete callback triggers the public AnimationPlay event
• Good use of the "fire once" wrapper pattern in visual-element-target.ts to ensure the callback only fires when the first property animation starts

2. Comprehensive Test Coverage

The test suite covers key scenarios:

• ✅ Immediate firing when no delay is present
• ✅ Delayed firing after a delay period
• ✅ Single invocation with multiple animated properties
• ✅ Correct definition argument (object or variant name)
• ✅ Proper ordering (onAnimationStart → onAnimationPlay)

3. Good Documentation

• Clear JSDoc comments explaining the feature
• Helpful inline code comments explaining implementation details

---

⚠️ Issues & Concerns

1. CRITICAL: Timing Inconsistency in Tests ⚠️

Location: packages/framer-motion/src/motion/__tests__/delay.test.tsx:243-256

onAnimationStart(() => {
    onAnimationStart()
    startTime = performance.now()  // ❌ Using performance.now()
}}
onAnimationPlay={() => {
    onAnimationPlay()
    playTime = performance.now()    // ❌ Using performance.now()
}}

Issue: According to CLAUDE.md guidelines, the codebase uses time.now() from motion-dom/src/frameloop/sync-time.ts instead of performance.now() for frame-synced timestamps. This ensures consistent time measurements within synchronous contexts.

Impact: The test might have timing inconsistencies, though it may still pass due to the generous threshold (90ms for a 100ms delay).

Recommendation: Replace performance.now() with time.now() from motion-dom:

import { time } from 'motion-dom/src/frameloop/sync-time'

// Then use:
startTime = time.now()
playTime = time.now()

---

2. Missing Test: Staggered Children 🔴

The existing delay.test.tsx file has extensive tests for staggered children with delayChildren and staggerChildren, but the new onAnimationPlay test suite doesn't include these scenarios.

Missing test case:

test("fires for each staggered child at correct times", async () => {
    const childPlayTimes: number[] = []
    const Component = () => (
        <motion.div 
            variants={{
                visible: { 
                    transition: { staggerChildren: 0.1, type: false }
                }
            }}
            animate="visible"
        >
            <motion.div 
                variants={{ visible: { x: 10 } }}
                onAnimationPlay={() => childPlayTimes.push(time.now())}
            />
            <motion.div 
                variants={{ visible: { x: 10 } }}
                onAnimationPlay={() => childPlayTimes.push(time.now())}
            />
        </motion.div>
    )
    // Assert that children fire at staggered intervals
})

Why this matters: The implementation in visual-element-variant.ts:95-98 creates individual onDelayComplete callbacks for each child, which is correct. However, without a test, we can't verify this behavior works as expected with real stagger timing.

---

3. Minor: Cleanup in NativeAnimation 🟡

Location: packages/motion-dom/src/animation/NativeAnimation.ts:162-172 and :174-182

The timeout cleanup code is duplicated in both cancel() and stop() methods:

// Duplicated cleanup code
if (this.delayCompleteTimeout) {
    clearTimeout(this.delayCompleteTimeout)
    this.delayCompleteTimeout = undefined
}

Recommendation: Extract to a private method for DRY:

private clearDelayTimeout() {
    if (this.delayCompleteTimeout) {
        clearTimeout(this.delayCompleteTimeout)
        this.delayCompleteTimeout = undefined
    }
}

cancel() {
    this.clearDelayTimeout()
    // ... rest of cancel logic
}

stop() {
    if (this.isStopped) return
    this.isStopped = true
    this.clearDelayTimeout()
    // ... rest of stop logic
}

---

4. Edge Case: Animation Interruption 🟡

Scenario: What happens if a new animation starts before onDelayComplete fires?

Looking at the code flow:

1. Animation A starts with 1s delay, sets up a timeout for onDelayComplete
2. 0.5s later, Animation B starts (new animate prop)
3. Animation A gets cancelled, clearing its timeout ✅
4. Animation B sets up its own timeout ✅

Verdict: The implementation handles this correctly by clearing timeouts in cancel() and stop(). However, an explicit test case would provide confidence:

test("onAnimationPlay doesn't fire if animation is interrupted", async () => {
    const onPlay1 = jest.fn()
    const onPlay2 = jest.fn()
    
    const { rerender } = render(
        <motion.div 
            animate={{ x: 100 }} 
            transition={{ delay: 0.2, type: false }}
            onAnimationPlay={onPlay1}
        />
    )
    
    // Interrupt after 100ms (before delay completes)
    await wait(100)
    rerender(
        <motion.div 
            animate={{ x: 200 }} 
            transition={{ type: false }}
            onAnimationPlay={onPlay2}
        />
    )
    
    await wait(200)
    expect(onPlay1).toBeCalledTimes(0) // Should not fire
    expect(onPlay2).toBeCalledTimes(1) // New animation should fire
})

---

5. Naming: onAnimatePlay vs onAnimationPlay ℹ️

The PR title says onAnimatePlay but the implementation uses onAnimationPlay. The implementation follows the existing pattern (onAnimationStart, onAnimationComplete), which is correct. Just noting this minor inconsistency in the PR title.

---

🔒 Security Considerations

✅ No security concerns identified. The callbacks are user-provided and don't introduce XSS, injection, or other vulnerabilities.

---

⚡ Performance Considerations

✅ Minimal performance impact:

• The "fire once" wrapper prevents redundant callback invocations
• Timeout cleanup prevents memory leaks
• JSAnimation uses a simple boolean flag instead of complex state tracking

---

📋 Summary & Recommendations

Must Fix (Before Merge):

1. Replace performance.now() with time.now() in the delay test (line 243-256)

Should Add (Strong Recommendation):

2. Add test for staggered children to verify per-child onAnimationPlay timing
3. Add test for animation interruption to verify timeout cleanup works correctly

Nice to Have:

4. Refactor timeout cleanup into a shared method in NativeAnimation
5. Update PR title to match the actual prop name (onAnimationPlay)

---

🎯 Overall Assessment

This is a solid implementation that solves a real user need (issue #2765). The core logic is sound, and the test coverage is good. Addressing the timing inconsistency and adding stagger tests would make this production-ready.

Recommendation: ✅ Approve after addressing the performance.now() issue and adding stagger tests.

[claude]

PR Review: onAnimationPlay Callback

Thank you for implementing this feature! The implementation addresses the issue raised in #2765.

Strengths

1. Well-tested with comprehensive test coverage
2. Clear JSDoc documentation
3. Consistent with existing event system patterns
4. Fire-once logic correctly implemented

Potential Issues

1. Typo in PR Title

Title says onAnimatePlay but implementation uses onAnimationPlay

2. performance.now() in Tests

Tests use performance.now() but CLAUDE.md recommends time.now() from motion-dom/src/frameloop/sync-time.ts

3. Timing Assertion May Be Flaky

Test expects 90ms for 100ms delay - could be flaky on slow CI

4. Race Condition in NativeAnimation

Promise.resolve().then() for zero-delay may fire before first frame. Consider requestAnimationFrame

5. Duplicated Cleanup

Consider extracting clearDelayTimeout() method

Suggestions

Test coverage gaps: staggered children, cancellation, repeats, WAAPI

Documentation: mention repeat behavior and relationship with onAnimationStart

Summary

Solid implementation. Main concerns: timing assertions and zero-delay callback timing. Recommend addressing performance.now() usage before merging.

[claude]

PR Review: onAnimationPlay callback

Overview

This PR adds the onAnimationPlay callback feature that fires when an animation actually begins playing after delays. This addresses issue #2765 and is a useful addition for coordinating UI updates with delayed/staggered animations.

Positive Aspects

✅ Well-structured implementation: The feature is implemented cleanly across both JS and WAAPI animation systems
✅ Good test coverage: The test file includes comprehensive scenarios (no delay, with delay, multiple properties, variants)
✅ Proper cleanup: Timeout cleanup in NativeAnimation.cancel() and stop() methods
✅ Fire-once pattern: Correctly implements a wrapper to ensure onDelayComplete only fires once for the first property
✅ Documentation: JSDoc comments added for the new callback

Issues & Concerns

1. Bug: NativeAnimation delay timing may be inaccurate

Location: packages/motion-dom/src/animation/NativeAnimation.ts:104-110

The implementation uses setTimeout(onDelayComplete, delay) but this doesn't account for:

• The time already elapsed since the animation was created/started
• The animation's playbackRate (speed) which could affect effective delay duration
• Whether the animation is paused immediately after creation

If there's any gap between construction and when the WAAPI animation starts, the timeout will fire too early relative to when the animation actually begins.

Suggestion: Consider using the WAAPI animation's own ready promise or tracking the actual animation start time instead of a fixed setTimeout.

2. Edge case: Negative playback speed (reverse animations)

Location: packages/motion-dom/src/animation/JSAnimation.ts:242-245

The JSAnimation correctly handles delay phase detection for negative speeds:

const isInDelayPhase =
    this.playbackSpeed >= 0
        ? timeWithoutDelay < 0
        : timeWithoutDelay > totalDuration

However, I don't see tests for reverse animations. Should onAnimationPlay fire at the beginning of a reversed animation (which would be at the end time minus delay)?

Suggestion: Add test cases for animations with negative playbackSpeed or direction: "reverse".

3. Potential race condition with Promise.resolve()

Location: packages/motion-dom/src/animation/NativeAnimation.ts:106

When delay <= 0:

Promise.resolve().then(onDelayComplete)

This fires on the next microtask, but the WAAPI animation might not have started yet. The animation could theoretically be cancelled before the callback fires.

Suggestion: Check this.isStopped before invoking the callback, or clear a microtask ID similar to how you clear the timeout.

4. Missing staggerChildren test

The PR description mentions "Fire AnimationPlay event for children at their respective delay times" and the code handles this in visual-element-variant.ts:95-98, but there's no test verifying that parent and child onAnimationPlay callbacks fire at different times when staggerChildren is used.

Suggestion: Add a test case like:

test("fires at staggered times for children", async () => {
    const parentPlayTime = jest.fn()
    const child1PlayTime = jest.fn()
    const child2PlayTime = jest.fn()
    
    <motion.div
        initial="hidden"
        animate="visible"
        variants={{
            visible: { transition: { staggerChildren: 0.1 } }
        }}
        onAnimationPlay={parentPlayTime}
    >
        <motion.div variants={{ visible: { x: 10 } }} onAnimationPlay={child1PlayTime} />
        <motion.div variants={{ visible: { x: 10 } }} onAnimationPlay={child2PlayTime} />
    </motion.div>
})

5. Type inconsistency in is-transition-defined.ts

Location: packages/motion-dom/src/animation/utils/is-transition-defined.ts:20

The parameter destructuring adds onDelayComplete: _onDelayComplete but this feels like a code smell. This function is checking if a transition is "defined" by examining if it has any transition properties, but onDelayComplete is a callback, not a transition property.

The underscore prefix naming suggests it should be ignored, but this could be confusing.

Suggestion: Consider if there's a cleaner way to handle this, perhaps by separating lifecycle callbacks from transition config at the type level.

6. Naming: AnimationPlay event name

Location: Multiple files

The event is named "AnimationPlay" which is consistent with the Web Animation API's play() method. However, it might be confused with programmatic .play() calls. The callback fires when the delay completes, not when .play() is called.

Consider: Would "AnimationDelayComplete" or "AnimationBegin" be clearer? Though I understand "Play" matches the user-facing prop name onAnimationPlay.

7. Performance: Timer cleanup on re-render

If a component re-renders and a new animation is created before the old one's delay completes, the timeout should be cleaned up. This appears to be handled, but ensure that cancel() or stop() is called on the old animation when a new one starts.

Minor Issues

• Line 106: Using performance.now() instead of time.now() (see CLAUDE.md guidance about using time.now() for frame-synced timestamps). Though this might be intentional since we're measuring wall-clock time, not animation time.

• Documentation: Consider adding an example in the docs showing the difference between onAnimationStart and onAnimationPlay to clarify the use case.

Security & Performance

✅ No security concerns identified
✅ Performance impact is minimal (single timeout per animation with delay)
✅ Proper cleanup prevents memory leaks

Recommendation

The PR is well-implemented overall, but I recommend addressing:

1. Critical: The NativeAnimation timing accuracy issue (Initial API (WIP) #1)
2. Important: Add staggerChildren test (adding circleci config #4)
3. Important: Add reverse animation test (Fixing types and es references #2)
4. Nice to have: Promise.resolve() race condition (Add motionValue prop #3)

The feature is valuable and the implementation approach is sound. With the suggested fixes, this will be a solid addition to the library.