Fix touch event handling, improve reliability, and optimize performance#16015
Fix touch event handling, improve reliability, and optimize performance#16015gmacmaster wants to merge 16 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
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; |
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?
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.
There was a problem hiding this comment.
I checked RichEdit, and there are some differences in how it handles pointer events vs mouse events. Let's not lose the information here.
| const winrt::Microsoft::ReactNative::Composition::Experimental::IVisual &visual, | ||
| uint32_t index) noexcept { | ||
| assert(index <= m_childrenCache.size()); | ||
| if (index > m_childrenCache.size()) { |
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.
There was a problem hiding this comment.
Changed to crash
| void InsertAt( | ||
| const winrt::Microsoft::ReactNative::Composition::Experimental::IVisual &visual, | ||
| uint32_t index) noexcept { | ||
| assert(index <= m_childrenCache.size()); |
There was a problem hiding this comment.
Crash rather than hiding error
There was a problem hiding this comment.
Changed to crash
| onStartShouldSetResponder: (): boolean => { | ||
| const {disabled} = this._config; | ||
| return !disabled ?? true; | ||
| return disabled !== true; // falsy/undefined disabled => responder allowed |
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?
There was a problem hiding this comment.
Created facebook/react-native#56526 and reverted
| if (delayHoverOut > 0) { | ||
| event.persist(); | ||
| this._hoverInDelayTimeout = setTimeout(() => { | ||
| this._hoverOutDelayTimeout = setTimeout(() => { |
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?
There was a problem hiding this comment.
Created facebook/react-native#56526 and reverted
|
|
||
| if (tabIndex !== undefined) { | ||
| processedProps.focusable = !tabIndex; | ||
| processedProps.focusable = tabIndex >= 0; |
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?
There was a problem hiding this comment.
Created facebook/react-native#56526 and reverted
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Performance Test ResultsBranch: ✅ Passed161 scenario(s) across 28 suite(s) — no regressionsSectionList
FlatList
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
|
|
@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 |
|
The change file should be marked as pre-release for changes to main. What kind of input are you using for textinput not getting focused when tapped? -- Thats working in main, and wasn't a bug I'm aware of. |
|
We're just using the base TextInput from react native. In this video all we have is No matter what we do, we can't get a touch to focus any text input IMG_4695.1.mp4 |
|
Hey @acoates-ms or @iamAbhi-916 is there a way to bring these changes over to our project locally? Our project is pulling from nugest so I don't see a way to create a patch in our project. |
Forward WM_POINTER* messages to RichEdit for non-mouse pointers RichEdit handles pointer events differently from mouse events. Previously, touch and pen input was converted to WM_LBUTTON*/WM_MOUSEMOVE before being sent to TxSendMessage, losing pointer-specific information. Now non-mouse pointers send WM_POINTERDOWN, WM_POINTERUP, and WM_POINTERUPDATE with proper pointer wParam packing and screen coordinates, preserving the full pointer context for RichEdit.
It is probably easiest to just build the project separately then replace the Microsoft.ReactNative.dll in your app. Which version are you on, I can look at backporting this to the appropriate branch? |
We're on Thank you so much and let me know if we can be helpful at all! |
3 participants
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