WIP - Touchhandler changes for Paper/Fabric #9
Conversation
This file contains 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
…hHandler
Summary: Since our `NSWindow` subclass already sends mouseup events to its `RCTTouchHandler` for manual mouse tracking loops (since D25026407), these synthetic mouseup events are no longer necessary and prevent `onLongPress` from working on images.
Test Plan:
|**Before**|**After**|
|{F641423485}{F641425218}|{F641423396}{F641425554}|
Reviewers: lyahdav, ackchiu, mowang
Reviewed By: mowang
Subscribers: eliwhite
Differential Revision: https://phabricator.intern.facebook.com/D30172688
Tasks: T92436951
Signature: 30172688:1628744633:5a1d6ff50cbbeeb358de65df06625776e859895f
# Conflicts:
# React/Base/RCTTouchHandler.m
# Conflicts:
# React/Base/RCTTouchHandler.m
# Conflicts:
# React/Base/RCTTouchHandler.m
# Conflicts:
# packages/react-native/React/Base/RCTTouchHandler.m
…teractions
Summary:
Take 2 after reverting the previous fix attempt (D38506367):
Instead of ending the incomplete/suspended touch (which can cause unexpected side effects on a completely different component that the user is interacting with at the moment), cancel it instead
Test Plan:
Use the secure text field (which is the only example known to not properly notify us of a context menu)
# Before:
1. Right click on the secure text input and switch to another window without any additional interactions (use Alt Tab)
2. Observe Pressability for the secure text input component (react tag 6375) moved the Responder to intermediate state (RESPONDER_ACTIVE_LONG_PRESS_IN)
3. Switch back to the app using Alt Tab
4. Click on "Switch". Observe that JS sides receives a multi touch with tag 6375 and 4957 ("Switch" label) combined
5. Pressability ignores 4957 and proceeds to end the interaction for 6375
6. As a result, the click on 4957 is simply ignored
https://pxl.cl/29wrD
# After:
1. Right click on the secure text input and switch to another window without any additional interactions (use Alt Tab)
2. Observe Pressability for the secure text input component (react tag 10607) moved the Responder to intermediate state (RESPONDER_ACTIVE_LONG_PRESS_IN)
3. Switch back to the app using Alt Tab
4. Click on "Switch". Observe the click is correctly processed and:
6. Responder for 10607 gets released, the click on "Switch" is processed correctly
5. Click on toggle just in case to confirm the clicks works as usual and 10607 is not mentioned going forward.
https://pxl.cl/29wnf
Reviewers: lyahdav, shawndempsey
Reviewed By: lyahdav
Subscribers: generatedunixname499725568, #worchon
Differential Revision: https://phabricator.intern.facebook.com/D38540910
Tasks: T119232032
# Conflicts:
# React/Base/RCTTouchHandler.m
# Conflicts:
# React/Base/RCTTouchHandler.m
…nt is unavailable Summary: As discovered by Shawn here: T119232032 (see comments), long press (RightMouseDown + Pressure) might break the touch tracking on macOS: while RCTTouchTracker ignores Pressure events, they leak into [ContextMenuNativeModule-macOS.mm](https://www.internalfb.com/code/fbsource/[ef0109a115c9a0482274aad9c2a7077d6e5f840d]/xplat/archon/native/rn/src/ContextMenuNativeModule-macOS.mm?lines=129]) which uses `NSApp.currentEvent` to determine the event that triggered the context menu, but while the context menu is triggered by RightMouseDown, `NSApp.currentEvent` is often Pressure in this scenario. What happens then is ContextMenuNativeModule fails to end the interaction and that breaks the state machine in [Pressability.js](https://www.internalfb.com/code/fbsource/[4c718f3046ee0cb90ac7ef45b6b933ddabef8ff2]/xplat/js/react-native-github/Libraries/Pressability/Pressability.js?lines=349) until the next interaction which will not be processed correctly due to invalid state. To work around this, since we cannot pass the full NSEvent through JS to ContextMenuNativeModule, we will rely on isRightClick instead in ContextMenuNativeModule - in a separate diff. This diff only adds the method we'll need. Test Plan: clone.sh these changes and test that (with corresponding changes in the context menu handler - separate diff) the responder in Pressability.js ends up in the right state after Pressure: ``` RESPONDER_GRANT: NOT_RESPONDER -> RESPONDER_INACTIVE_PRESS_IN DELAY: RESPONDER_INACTIVE_PRESS_IN -> RESPONDER_ACTIVE_PRESS_IN RESPONDER_RELEASE: RESPONDER_ACTIVE_PRESS_IN -> NOT_RESPONDER ``` Reviewers: lyahdav, shawndempsey Reviewed By: shawndempsey Subscribers: #worchon Differential Revision: https://phabricator.intern.facebook.com/D38515665 Tasks: T119232032 # Conflicts: # React/Base/RCTTouchHandler.h # React/Base/RCTTouchHandler.m # Conflicts: # React/Base/RCTTouchHandler.h # React/Base/RCTTouchHandler.m
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D43558608
…nts as opt-in Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D43558607 # Conflicts: # React/Base/RCTTouchHandler.h # React/Base/RCTTouchHandler.m
…icate events Summary: Update the duplicate event detection logic to allow mouse drag events to be sent out continuously. Mouse drag events maintain the same event type and event number, but update the location of the event. This diff checks for location updates so that mouse drag events wouldn't be flagged as duplicates. Test Plan: Launch RNTester, click and drag in and out of a Pressable in the Pressable demo. Without the fix: https://pxl.cl/2wq4R With the fix: https://pxl.cl/2wq4W Reviewers: shawndempsey, chpurrer Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D43985809 Tasks: T147732113
…ch handler on mouse up. (microsoft#1815) Summary: This stack picks all fabric changes merged on RN 0.71 macOS that have no side-effects on paper. Cherry-pick of Fabric fixes: microsoft#1815 Test Plan: Tested later in this stack. Reviewers: chpurrer, #rn-desktop Reviewed By: chpurrer Differential Revision: https://phabricator.intern.facebook.com/D48138672 Tasks: T158583598 # Conflicts: # React/Fabric/RCTSurfaceTouchHandler.mm
…ents Summary: Add the equivalent of the RCTTouchHandler functionality to get the touch handler for a view and request to cancel an ongoing touch. This allows submitting mouse events to native components without breaking the internal touch handler state. Test Plan: Tested later in this stack. Reviewers: shawndempsey, chpurrer, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D49465176 Tasks: T163838519 # Conflicts: # React/Fabric/RCTSurfaceTouchHandler.mm
Summary: **Context** - When we migrated from `RCTRootView` to `RCTSurfaceHostingView` for Paper & Fabric, we didn't port over the right click dev menu functionality **Change** - Only show dev menu if in dev mode - Show the dev menu on right click - Find the correct touch handler for surface type & cancel the native click Test Plan: |Fabric| | https://pxl.cl/3sn8V| |Paper| | https://pxl.cl/3sndK | Reviewers: ericroz, lefever, #rn-desktop Reviewed By: ericroz Subscribers: taskcreeper Differential Revision: https://phabricator.intern.facebook.com/D49638464 # Conflicts: # React/Fabric/RCTSurfaceTouchHandler.mm # packages/react-native/React/Base/Surface/SurfaceHostingView/RCTSurfaceHostingView.mm
Summary: **Context** - When a native component receives touches, it needs to clear the active touches and reset the Identifier pool **Change** - expose `reset` on SurfaceTouchHandler so it can be called from native components Test Plan: |Fabric| |https://pxl.cl/3wxJR| Reviewers: lefever, #rn-desktop Differential Revision: https://phabricator.intern.facebook.com/D49971420
Summary: This diff adds a helper method that can broadcast and event to all surface touch handlers. Events happening outside the view hierarchy of the surface touch handler have to be broadcast to all surface touch handler so that each instance can update the internal state based on the mouse up event. Test Plan: Tested later in this stack (D50480942) Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D50480946 # Conflicts: # React/Fabric/RCTSurfaceTouchHandler.mm
…cast Summary: This diff registers all surface touch handler instances as observers of the event broadcasting notification. The broadcasted event can be used to update or reset internal mouse tracking state that is completed by the event. Test Plan: Tested later in this stack (D50480942) Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D50480945 # Conflicts: # React/Fabric/RCTSurfaceTouchHandler.mm
Summary: This diff applies the same method used in Paper to support catching events received by the application but not dispatched to the surface touch handler. Some of these are events happening while the cursor is outside all the app windows. Event tracking run loop modes also swallow events (e.g. right mouse up after opening a contextual menu). By using a method swizzle on the nextEvent pull method we can track all events handled by the app and broadcast the mouse up events happening outside the app windows. This allows all the surface touch handlers to update their internal state based on the event. Test Plan: Tested later in this stack (D50480942) Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D50480941 # Conflicts: # React/Fabric/RCTSurfaceTouchHandler.mm
…tracking loop Summary: This diff tracks right mouse up events being pulled from an event tracking loop. This is typically used by contextual/popup menus and was causing the right mouse up event to never reach the surface touch handler. By catching these events and forwarding them to the surface touch handler, the ongoing mouse event state can be cancelled or reset. Test Plan: Tested later in this stack (D50480942) Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D50480943 # Conflicts: # React/Fabric/RCTSurfaceTouchHandler.mm
…face touch handler Summary: To avoid having breaking changes if the default settings for the gesture recognizer changes in AppKit, the required setting for delayed event dispatching/cancellation is explicitly set. This was done in Paper macOS and in Fabric on iOS. Test Plan: Tested later in this stack (D50480942) Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D50480944 # Conflicts: # React/Fabric/RCTSurfaceTouchHandler.mm
…face touch handler Summary: With the changes applied in this stack there is no need for notifying the surface touch handler of contextual menu's being displayed. The event catching and broadcasting is able to catch the breaking event swallowing and cancel the ongoing gesture started by the right mouse down. Test Plan: Force enable Fabric in Zeratul by setting the `useFabric` property to `YES` on the created `ReactRootViewManager` instances. - Right click in the main window. - Toggle between appearance styles in the settings more than 11 times. - Right click on a thread and click away. https://pxl.cl/3DhQ1 Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D50480942
…umption of it Summary: Native components can consume mouse up events through an event tracking run loop. Those events won't reach the surface touch handler and potentially lead to filling up the 11 slots for touch sessions. This diff updates the event tracker to catch events processed by native components and submit a touch cancellation to the surface touch handler if a left mouse up was consumed. This change removes the need to manually notify the surface touch handler of cancellations in native components. Test Plan: Tested later in this stack Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D50704270 # Conflicts: # React/Fabric/RCTSurfaceTouchHandler.mm
…event tracking loops Summary: AppKit NSTextField uses an event tracking run loop on mouse down, waiting for and dequeuing the mouse up event to enable editing of the field. The dequeuing of the mouse up event means it won't be submitted to the touch handler and leaving an ongoing touch session started by the mouse down. This diff catches the deque and sends it asynchronously to let the touch handler process it after the event tracking exited. This fixes press events for views located above a text field. The change will submit the mouse down and up event, allowing the press event to be triggered. Test Plan: - Run Messenger Desktop - Log out - Log in with email and password - Enter a password and click on the "show password" button | Before | After | |--| | https://pxl.cl/4G0gc | https://pxl.cl/4G0fV | - Log in - Search and clear the search by clicking on the discard icon https://pxl.cl/4G0kC Reviewers: lyahdav, shawndempsey, #rn-desktop Reviewed By: lyahdav Subscribers: ericroz, ngerlem Differential Revision: https://phabricator.intern.facebook.com/D55951356 Tasks: T183747164, T184150391
…e of tracking loop Summary: **Context** - TouchHandler was fixed in D55951356 to support `mouseUp` from an event tracking loop (TextView) - Add this change to Fabric Test Plan: |RN Tester| | https://pxl.cl/4KLph| Reviewers: chpurrer, lefever, #rn-desktop Reviewed By: chpurrer Differential Revision: https://phabricator.intern.facebook.com/D56484002 # Conflicts: # React/Fabric/RCTSurfaceTouchHandler.mm
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
WIP - do not merge