Fix touch event handling, improve reliability, and optimize performance#16015
Open
gmacmaster wants to merge 9 commits intomicrosoft:mainfrom
Open
Fix touch event handling, improve reliability, and optimize performance#16015gmacmaster wants to merge 9 commits intomicrosoft:mainfrom
gmacmaster wants to merge 9 commits intomicrosoft:mainfrom
Conversation
- Fix touch/pen pointer device type detection and screenPoint coordinates - Fix touch cancel to include all active touches per W3C spec - Synthesize touch-cancel for stale pointers and releases outside views - Fix TextInput pointer message translation (use mouse-style messages for RichEdit) - Fix ShouldSubmit modifier key checks (altDown, ctrlKey) - Add null safety to RootComponentView() for island teardown - Fix Pressability hover timeout and tabIndex focusable mapping - Cache event path to root to avoid repeated tree walks - Use unordered_set for pointer capture tracking - Eliminate O(n²) hit testing by caching visual children - Skip snap scroll reconfiguration when unchanged - Improve TextInput reliability: thread-safe loading, null safety, use-after-free fix - Fix Timing data race and remove duplicate image error allocation - Use unordered_set for animated node and component registry lookups - Clean up dead code in ScrollView and simplify Modal event emitter init
iamAbhi-916
reviewed
Apr 20, 2026
| return event; | ||
| } | ||
|
|
||
| bool CompositionEventHandler::IsPointerWithinInitialTree(const ActiveTouch &activeTouch) noexcept { |
Contributor
There was a problem hiding this comment.
This now does a live hitTest instead of checking the stored [touch.target] against the ancestor chain.
Finger drift during a tap will suppress onClick. Test with Pressable on touch devices.
iamAbhi-916
reviewed
Apr 20, 2026
iamAbhi-916
reviewed
Apr 20, 2026
| } else { | ||
| msg = WM_POINTERDOWN; | ||
| wParam = PointerPointToPointerWParam(pp); | ||
| if (IsDoubleClick()) { |
Contributor
There was a problem hiding this comment.
please also test touch long-press text selection and pen barrel button with real hardware.
iamAbhi-916
reviewed
Apr 20, 2026
iamAbhi-916
reviewed
Apr 20, 2026
iamAbhi-916
reviewed
Apr 20, 2026
acoates-ms
reviewed
Apr 20, 2026
Contributor
acoates-ms
left a comment
acoates-ms
left a comment
There was a problem hiding this comment.
Generally, a great set of changes. Thanks for the PR!
| } else { | ||
| msg = WM_POINTERUP; | ||
| wParam = PointerPointToPointerWParam(pp); | ||
| msg = WM_LBUTTONUP; |
Contributor
There was a problem hiding this comment.
Shouldn't we be sending pointer events to richedit? I imagine there could be cases where richedit could get confused by multiple pointers if we are sending both touch and mouse as mouse events?
Just wondering on the reasoning for dropping the pointer events?
Author
There was a problem hiding this comment.
My understanding (could be wrong) is that TxSendMessage into RichEdit is really the classic mouse-message path—selection/caret/drag behavior assumes WM_LBUTTON* / WM_MOUSEMOVE with MK_* and POINTS in lParam, not the WM_POINTER* packing. That’s why I ended up synthesizing mouse for non-mouse pointers instead of forwarding pointer messages.
Happy to adjust if you’d rather we try pointer messages for touch/pen in some cases, or if you know of a supported pattern here I missed.
| const winrt::Microsoft::ReactNative::Composition::Experimental::IVisual &visual, | ||
| uint32_t index) noexcept { | ||
| assert(index <= m_childrenCache.size()); | ||
| if (index > m_childrenCache.size()) { |
Contributor
There was a problem hiding this comment.
I think I'd rather just crash here if we are getting an invalid index input. Otherwise, it's an incorrect behavior that people might start relying on.
| void InsertAt( | ||
| const winrt::Microsoft::ReactNative::Composition::Experimental::IVisual &visual, | ||
| uint32_t index) noexcept { | ||
| assert(index <= m_childrenCache.size()); |
Contributor
There was a problem hiding this comment.
Crash rather than hiding error
| onStartShouldSetResponder: (): boolean => { | ||
| const {disabled} = this._config; | ||
| return !disabled ?? true; | ||
| return disabled !== true; // falsy/undefined disabled => responder allowed |
Contributor
There was a problem hiding this comment.
This looks like this is from upstream? Can you make a PR with this in react-native rather than adding behavior differences in windows?
| if (delayHoverOut > 0) { | ||
| event.persist(); | ||
| this._hoverInDelayTimeout = setTimeout(() => { | ||
| this._hoverOutDelayTimeout = setTimeout(() => { |
Contributor
There was a problem hiding this comment.
This looks like this is from upstream? Can you make a PR with this in react-native rather than adding behavior differences in windows?
|
|
||
| if (tabIndex !== undefined) { | ||
| processedProps.focusable = !tabIndex; | ||
| processedProps.focusable = tabIndex >= 0; |
Contributor
There was a problem hiding this comment.
This looks like this is from upstream? Can you make a PR with this in react-native rather than adding behavior differences in windows?
Contributor
|
/azp run |
Contributor
|
Azure Pipelines successfully started running 1 pipeline(s). |
Performance Test ResultsBranch: ❌ Regressions DetectedFlatList
✅ Passed147 scenario(s) across 27 suite(s) — no regressionsSectionList
TouchableOpacity
ScrollView
TouchableHighlight
Pressable
Modal
Image
ActivityIndicator
Switch
Button
TextInput
View
Text
SectionList.native-perf-test.ts
FlatList.native-perf-test.ts
TouchableHighlight.native-perf-test.ts
TouchableOpacity.native-perf-test.ts
Pressable.native-perf-test.ts
ScrollView.native-perf-test.ts
ActivityIndicator.native-perf-test.ts
TextInput.native-perf-test.ts
Switch.native-perf-test.ts
Button.native-perf-test.ts
Modal.native-perf-test.ts
Image.native-perf-test.ts
View.native-perf-test.ts
Text.native-perf-test.ts
|
Author
|
@acoates-ms I addressed your comments. I see https://dev.azure.com/ms/react-native-windows/_build/results?buildId=630300&view=logs&j=251f7bac-43ee-511a-ef4a-213809139086&t=6ca16664-8a2c-5a6f-a7e7-c40e742e8544 failed with error: What should I put? Anything else you'd like me to change? Until we can get this merged, is there a way I can bring this changes to our local project? We're currently on .83 preview but can move around. Our biggest blocker for launching our app is that text inputs don't currently focus when tapped. We don't need the full keyboard type, just the ability to focus |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR bundles several touch stability and performance improvements for the Windows React Native Fabric renderer: it adds a m_childrenCache to avoid O(n) WinRT iterator traversal on every GetAt, hardens the codebase against a null RootComponentView, converts m_capturedPointers / m_children / m_componentNames to std::unordered_set for O(1) operations, and adds snap-point change detection in CompScrollerVisual to skip redundant reconfiguration.
Also brings over issues found in #16009
Type of Change
Why
Touch functionality often drops/misses touches from the user
What
What changes were made to the codebase to solve the bug, add the functionality, etc. that you specified above.
Changelog
Should this change be included in the release notes: yes
Fix touch event handling, improve reliability, and optimize performance
Important Files Changed
Sequence Diagram
sequenceDiagram participant W as WinRT Input participant EH as CompositionEventHandler participant RC as RootComponentView* participant FUI as FabricUIManager participant JS as JS EventEmitter W->>EH: PointerPressed/Moved/Released EH->>EH: RootComponentView() → raw ptr (null-safe) alt rootView == nullptr EH-->>W: early return (no crash) else rootView valid EH->>RC: hitTest(clientPoint, ptLocal) RC-->>EH: targetTag EH->>FUI: GetViewRegistry().componentViewDescriptorWithTag(targetTag) FUI-->>EH: targetComponentView EH->>EH: DispatchTouchEvent(eventType, pointerId, ...) loop for each activeTouch EH->>RC: hitTest (null-checked) EH->>EH: handler λ [this, &activeTouch, &pointerEvent] note over EH: IsPointerWithinInitialTree() walks parent chain EH->>JS: onPointerDown/Move/Up/Cancel + onClick/onAuxClick end loop for each uniqueEventEmitter EH->>JS: onTouchStart/Move/End/Cancel end endMicrosoft Reviewers: Open in CodeFlow