Skip to content

fix(hover): Defer pointerleave during press to prevent whileHover flash#3540

Merged
mattgperry merged 2 commits intomainfrom
drag-while-hover
Feb 5, 2026
Merged

fix(hover): Defer pointerleave during press to prevent whileHover flash#3540
mattgperry merged 2 commits intomainfrom
drag-while-hover

Conversation

@mattgperry
Copy link
Collaborator

Summary

  • Reverts pointer capture from drag system — the previous approach (fix(drag): Fix whileHover persisting after drag ends #3537) didn't cover the pre-drag window between pointerdown and drag start, causing whileHover to deactivate when the pointer moved outside the element during that gap
  • Moves hover-during-press protection into hover() itself — tracks press state per-element and defers pointerleave processing while the pointer is pressed, covering all press scenarios (press-only, press+drag, press+tap)
  • On pointer release: if a leave was deferred and no drag is active, hover ends; if the pointer is still over the element, hover stays active

Test plan

  • Updated existing drag-hover interaction tests to reflect press-based flow
  • Added test: whileHover stays active during press and deactivates on release outside element
  • Added test: whileHover stays active during press when pointer leaves before drag starts
  • All 37 hover + press tests pass

🤖 Generated with Claude Code

@claude
Copy link

claude bot commented Feb 5, 2026

Code Review

I've thoroughly reviewed this PR and the changes look excellent. This is a well-thought-out solution that properly addresses the hover-during-press issue that the previous pointer capture approach missed.

✅ Strengths

1. Correct Architecture

  • Moving hover-during-press protection into hover() itself is the right approach. This ensures consistent behavior across all press scenarios (press-only, press+drag, press+tap)
  • The per-element state tracking (isPressed, deferredHoverEnd, hoverEndCallback) properly encapsulates the hover lifecycle

2. Comprehensive Coverage

  • The solution correctly handles the critical edge case: the pre-drag window between pointerdown and drag start, which the previous pointer capture approach missed
  • Window-level pointerup listeners ensure cleanup happens regardless of where the pointer is released

3. Excellent Test Coverage

  • All three new tests target specific scenarios:
    • Press-only hover (no drag involved) - packages/framer-motion/src/gestures/tests/hover.test.tsx:279
    • Press before drag starts - packages/framer-motion/src/gestures/tests/hover.test.tsx:312
    • Hover remains active when pointer stays over element after drag - packages/framer-motion/src/gestures/tests/hover.test.tsx:249
  • Tests now correctly simulate the full interaction flow with pointerDown and pointerUp

4. Clean Code

  • Proper cleanup of event listeners in removePointerLeave() and onPointerUp()
  • Clear state management with deferredHoverEnd flag
  • Simplified drag code by removing pointer capture complexity

🔍 Potential Issues

1. Event Listener Memory Leak Risk (Medium Priority)

The onPointerUp listener is added to window but there's a potential leak if the element is unmounted while pressed:

// In hover.ts:84-95
const onPointerDown = () => {
    isPressed = true
    window.addEventListener("pointerup", onPointerUp as EventListener, eventOptions)
    window.addEventListener("pointercancel", onPointerUp as EventListener, eventOptions)
}

If the cancel() function is called (via AbortController) while isPressed = true, the window listeners will be removed by the signal, but isPressed will remain true and hoverEndCallback may leak.

However, looking at the code more closely, I see that eventOptions includes the AbortController signal from setupGesture(), which means the window listeners WILL be cleaned up when cancel() is called. This is actually correct! The signal propagates to all listeners using those options.

2. Pointer Event Type Casting (Low Priority)

In onPointerUp, the event is cast from Event to PointerEvent:

// hover.ts:67-81
const onPointerUp = (event: Event) => {
    // ...
    endHover(event as PointerEvent)  // Type assertion here
}

This is safe in practice since pointerup always fires a PointerEvent, but it would be cleaner to type the parameter correctly:

const onPointerUp = (event: PointerEvent) => {
    // ... no casting needed
}

3. State Reset on Hover Re-entry

The code correctly resets deferredHoverEnd = false in onPointerEnter (packages/motion-dom/src/gestures/hover.ts:112), which is good. However, if a pointer is pressed, leaves, re-enters, and then releases, the deferred state is cleared. This appears correct but deserves a test case to verify the edge case:

// Suggested test case
test("hover state resets correctly on re-entry during press", async () => {
    // pointerEnter -> pointerDown -> pointerLeave -> pointerEnter -> pointerUp
    // Expected: hover should remain active since pointer is over element on release
})

🎯 Performance Considerations

  • Event listener churn: Each press adds two window-level listeners that are removed on release. This is appropriate for the use case and unavoidable
  • Closure memory: Each element gets its own closure with state variables. This is clean and properly scoped
  • No performance regressions expected: The solution is lightweight and event-driven

🔒 Security Considerations

No security concerns identified. The code:

  • Doesn't expose user data
  • Properly validates pointer types
  • Uses standard browser APIs correctly
  • No XSS, injection, or other OWASP top 10 vulnerabilities introduced

📝 Minor Suggestions

1. Code Comment for Clarity

The isPressed || isDragActive() check in onPointerLeave could benefit from a comment explaining why both conditions defer hover end:

// hover.ts:98-107
const onPointerLeave = (leaveEvent: PointerEvent) => {
    if (leaveEvent.pointerType === "touch") return

    // Defer hover end during press (covers pre-drag window) or active drag
    // to prevent whileHover from flashing off during interaction
    if (isPressed || isDragActive()) {
        deferredHoverEnd = true
        return
    }

    endHover(leaveEvent)
}

2. Test Comment Enhancement

The test comment on line 330 mentions "needs movement threshold" which is accurate but could be slightly clearer about why this matters for the test scenario.

📊 Test Coverage Assessment

Excellent coverage with 37 passing hover + press tests. The three new tests specifically cover:

  1. ✅ Press without drag causing hover flash
  2. ✅ Pointer leaving before drag threshold is met
  3. ✅ Hover persistence when pointer remains over element

All critical edge cases are covered.

✅ Verdict

This is a high-quality PR that correctly solves a tricky UX issue. The approach is sound, test coverage is comprehensive, and the code is clean.

Strengths:

  • ✅ Correct architectural approach
  • ✅ Comprehensive test coverage (3 new tests covering all edge cases)
  • ✅ Proper cleanup via AbortController signal
  • ✅ Cleaner than the previous pointer capture approach
  • ✅ No performance or security concerns

Minor suggestions:

  • Consider adding inline comments for clarity
  • Optional: stricter TypeScript typing for event handlers
  • Optional: additional edge case test for pointer re-entry during press

Recommendation: APPROVED

The code quality is excellent and follows the repository's style conventions from CLAUDE.md. The solution is production-ready.

🤖 Generated with Claude Code

…g before drag starts

The previous pointer capture approach in the drag system didn't cover the
window between pointerdown and drag start. Move the protection into
hover() itself by tracking press state per-element and deferring
pointerleave processing while the pointer is pressed.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Feb 5, 2026

Code Review Summary: This PR correctly moves hover-during-press protection from drag into hover itself, fixing whileHover deactivation during the gap between pointerdown and drag start. Strengths: (1) Correct architecture - handles press-only, press+drag, and press+tap scenarios (2) Excellent test coverage for edge cases (3) Clean per-element state management (4) Proper event cleanup. Critical Issue: Memory leak risk - the cancel function does not clean up window-level pointerup/pointercancel listeners if hover is canceled while pointer is pressed (line 142). These listeners will remain attached. Recommend: Track cleanup functions and call them on cancel. Medium Priority: Multi-pointer support - current implementation does not track pointerId, so multiple simultaneous pointers could confuse the isPressed state. Low Priority: Type safety for onPointerUp parameter, changelog should mention behavioral fix not just types. Overall: Approve with fix for memory leak before merge. Great solution to a tricky interaction problem!

@claude
Copy link

claude bot commented Feb 5, 2026

Detailed Code Suggestions

Issue 1: Memory Leak Fix (Required)

The returned cancel function needs to clean up window listeners. Here is the pattern to follow:

export function hover(
    elementOrSelector: ElementOrSelector,
    onHoverStart: OnHoverStartEvent,
    options: EventOptions = {}
): VoidFunction {
    const [elements, eventOptions, cancel] = setupGesture(
        elementOrSelector,
        options
    )

    const cleanupFunctions: VoidFunction[] = []

    elements.forEach((element) => {
        let isPressed = false
        let deferredHoverEnd = false
        let hoverEndCallback: OnHoverEndEvent | undefined

        const removeWindowListeners = () => {
            window.removeEventListener(
                "pointerup",
                onPointerUp as EventListener
            )
            window.removeEventListener(
                "pointercancel",
                onPointerUp as EventListener
            )
        }

        const onPointerUp = (event: Event) => {
            isPressed = false
            removeWindowListeners()

            if (deferredHoverEnd) {
                deferredHoverEnd = false
                endHover(event as PointerEvent)
            }
        }

        // ... rest of implementation

        cleanupFunctions.push(removeWindowListeners)
    })

    return () => {
        cancel()
        cleanupFunctions.forEach(fn => fn())
    }
}

Issue 2: Multi-Pointer Support (Optional Enhancement)

For handling multiple simultaneous pointers:

const pressedPointers = new Set<number>()

const onPointerDown = (event: PointerEvent) => {
    pressedPointers.add(event.pointerId)
    if (pressedPointers.size === 1) {
        window.addEventListener("pointerup", onPointerUp as EventListener, eventOptions)
        window.addEventListener("pointercancel", onPointerUp as EventListener, eventOptions)
    }
}

const onPointerUp = (event: Event) => {
    const pointerEvent = event as PointerEvent
    pressedPointers.delete(pointerEvent.pointerId)
    
    if (pressedPointers.size === 0) {
        removeWindowListeners()
        
        if (deferredHoverEnd) {
            deferredHoverEnd = false
            endHover(pointerEvent)
        }
    }
}

const onPointerLeave = (leaveEvent: PointerEvent) => {
    if (leaveEvent.pointerType === "touch") return

    if (pressedPointers.size > 0) {
        deferredHoverEnd = true
        return
    }

    endHover(leaveEvent)
}

@mattgperry mattgperry merged commit f81d462 into main Feb 5, 2026
3 of 5 checks passed
@mattgperry mattgperry deleted the drag-while-hover branch February 5, 2026 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant