[Fix] touch event handling#3
Conversation
…ssified as mouse In onPointerPressed(), activeTouch.touchType was unconditionally set to UITouchType::Mouse. Now reads pointerPoint.PointerDeviceType() to correctly classify touch, pen, and mouse inputs, matching the pattern already used in WindowsTextInputComponentView. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…local screenPoint was incorrectly set to ptLocal (element-local coordinates), same as offsetPoint. Changed to ptScaled (document-relative coordinates) which is consistent with how other React Native platforms report screenX/screenY in touch events. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR fixes two clear bugs:
Confidence Score: 4/5Safe to merge the two bug fixes, but the new cancel helper is dead code indicating an incomplete feature. The screenPoint and touchType fixes are clearly correct. However, DispatchSynthesizedTouchCancelForActiveTouch has no callers, suggesting the cancel-synthesizing feature it enables is not yet connected — shipping dead code that emits events could mask a missing behavior or confuse future contributors. CompositionEventHandler.cpp — specifically the missing call site for DispatchSynthesizedTouchCancelForActiveTouch Important Files Changed
Sequence DiagramsequenceDiagram
participant W as Windows Input
participant CEH as CompositionEventHandler
participant EE as TouchEventEmitter
W->>CEH: onPointerPressed(args)
CEH->>CEH: detect PointerDeviceType (Touch/Pen/Mouse)
CEH->>CEH: UpdateActiveTouch(ptScaled, ptLocal)
note right of CEH: pagePoint=ptScaled, screenPoint=ptScaled (fixed), offsetPoint=ptLocal
CEH->>EE: DispatchTouchEvent(Start, ...)
W->>CEH: onPointerMoved / onPointerReleased
CEH->>CEH: UpdateActiveTouch
CEH->>EE: DispatchTouchEvent(Move/End, ...)
note over CEH,EE: New function — no call site yet
CEH-->>EE: DispatchSynthesizedTouchCancelForActiveTouch
note right of EE: onPointerCancel + onTouchCancel
Reviews (1): Last reviewed commit: "Fix touch cancel to include all active t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| void CompositionEventHandler::DispatchSynthesizedTouchCancelForActiveTouch( | ||
| const ActiveTouch &cancelledTouch, | ||
| const winrt::Microsoft::ReactNative::Composition::Input::PointerPoint &pointerPoint, | ||
| winrt::Windows::System::VirtualKeyModifiers keyModifiers) { | ||
| if (!cancelledTouch.eventEmitter) { | ||
| return; | ||
| } | ||
|
|
||
| facebook::react::PointerEvent pointerEvent = | ||
| CreatePointerEventFromActiveTouch(cancelledTouch, TouchEventType::Cancel); | ||
| winrt::Microsoft::ReactNative::ComponentView targetView{nullptr}; | ||
| facebook::react::SharedTouchEventEmitter emitter = cancelledTouch.eventEmitter; | ||
| auto pointerHandler = [emitter, pointerEvent](std::vector<winrt::Microsoft::ReactNative::ComponentView> &) { | ||
| emitter->onPointerCancel(pointerEvent); | ||
| }; | ||
| HandleIncomingPointerEvent(pointerEvent, targetView, pointerPoint, keyModifiers, pointerHandler); | ||
|
|
||
| facebook::react::TouchEvent touchEvent; | ||
| touchEvent.changedTouches.insert(cancelledTouch.touch); | ||
|
|
||
| for (const auto &pair : m_activeTouches) { | ||
| if (!pair.second.eventEmitter) { | ||
| continue; | ||
| } | ||
|
|
||
| if (touchEvent.changedTouches.find(pair.second.touch) != touchEvent.changedTouches.end()) { | ||
| continue; | ||
| } | ||
|
|
||
| touchEvent.touches.insert(pair.second.touch); | ||
| } | ||
|
|
||
| for (const auto &pair : m_activeTouches) { | ||
| if (pair.second.eventEmitter == cancelledTouch.eventEmitter) { | ||
| touchEvent.targetTouches.insert(pair.second.touch); | ||
| } | ||
| } | ||
|
|
||
| cancelledTouch.eventEmitter->onTouchCancel(touchEvent); | ||
| } |
There was a problem hiding this comment.
DispatchSynthesizedTouchCancelForActiveTouch is never called
This new function is defined and declared in the header, but there are no call sites anywhere in the codebase. If it was intended to synthesize cancel events for a specific scenario (e.g. pointer capture loss or view detachment), the call site was not added. Dead-code that emits events is unlikely to be intentional — please either wire it up where needed or remove it if it's reserved for a follow-up.
| for (const auto &pair : m_activeTouches) { | ||
| if (pair.second.eventEmitter == cancelledTouch.eventEmitter) { | ||
| touchEvent.targetTouches.insert(pair.second.touch); | ||
| } | ||
| } |
There was a problem hiding this comment.
targetTouches includes the cancelled touch itself
Per the W3C Touch Events spec, targetTouches for a touchcancel should only list touches that are still active on the same target — the cancelled touch should be excluded (it belongs only in changedTouches). The loop here doesn't filter out cancelledTouch.touch, so targetTouches will incorrectly contain the touch that was just cancelled.
Note that DispatchTouchEvent has the same pattern (lines ~1621-1624), so this is a pre-existing issue being replicated; however, correcting it in the new function would be straightforward:
for (const auto &pair : m_activeTouches) {
if (pair.second.eventEmitter == cancelledTouch.eventEmitter &&
touchEvent.changedTouches.find(pair.second.touch) == touchEvent.changedTouches.end()) {
touchEvent.targetTouches.insert(pair.second.touch);
}
}
gmacmaster
force-pushed
the
fix/touch-event-handling
branch
from
be8c697 to
e316584
Compare
…spec compliance) DispatchSynthesizedTouchCancelForActiveTouch() was filtering the touches array to only include touches from the same eventEmitter. Per W3C spec, touches should contain all active touch points on the surface, matching the behavior in DispatchTouchEvent(). targetTouches remains correctly scoped to the target element. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gmacmaster
force-pushed
the
fix/touch-event-handling
branch
from
e316584 to
1926ab9
Compare
|
@grepile review |
1 participant
No description provided.