fix(Fabric): back gesture activates Pressable elements
#2131
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
tboba
requested changes
May 14, 2024
WoLewicki
reviewed
May 14, 2024
2 tasks
kkafar
added a commit
that referenced
this pull request
Jul 2, 2024
…sted stack (#2223) ## Description In #2193 I've made a mistake - on Paper, when there is no touch handler held in `_touchHandler` field I've started lookup for touch handler in ancestor chain from `self` -> which leads to infinite loop when swiping back in nested-stack navigation scenario. ## Changes * Started lookup from superview. ## Test code and steps to reproduce `Test2223` 1. Navigate to NestedStack. 2. Navigate to NestedStack details screen. 3. Use swipe gesture to go-back W/o this PR the application will crash hitting infinite loop. Also test that this behaviour remains fixed: * #2131 ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes
This was referenced Jul 17, 2024
This was referenced Aug 20, 2024
This was referenced Aug 23, 2024
This was referenced Aug 30, 2024
This was referenced Sep 24, 2024
ja1ns
pushed a commit
to WiseOwlTech/react-native-screens
that referenced
this pull request
Oct 9, 2024
…nsion#2131) ## Description When cancelling touches we previously relied on `touchHandler` instance being exposed by some react managed view above us (screen stack) in the view hierarchy (on Paper it was `RCTRootContentView`). However this changed with some recent Fabric version and no view exposes such property any longer, despite the fact that touch handler is obviously still utilised by these views even on Fabric, but the field has been removed from public API. Currently on Fabric (as of version `0.74.1`) `RCTSurfaceTouchHandler` is owned by instance of `RCTFabricSurface` owned by instance of `RCTSurfaceView` which we can expect to live above us in the view hierarchy (RN mounts it in the very beginning of application runtime). To get access to the private touchHandler (which is a `UIGestureRecognizer`) we observe that it is attached to the `RCTSurfaceView` and thus it is retained in its `gestureRecognizer` array (native API). > [!tip] > We could potentially cache the instance of `RCTSurfaceView`, but for now I've come to a conclusion that this is an minor minor computation (there is only one gesture recognizer in the array & the parent lookup is O(log n) (I know this is not a balanced tree, but you know what I mean). However this is always an option for future. > [!note] > I thought of adding warning in case of `RCTSurfaceView` being not present above screen stack in view hierarchy but this might be the case with `modal` stack presentation. Closes software-mansion#2118 ## Changes * Added utility extensions on `RCTTouchHandler` & `RCTSurfaceTouchHandler` classes to make the code more expressive * Splited `RNSScreenStackView.cancelTouchesInParent` implementation into two separate for Paper & Fabric * In case of Fabric added lookup for `RCTSurfaceView` & got access to its touch handler. ## Test code and steps to reproduce `Test2118` See software-mansion#2118 for issue description. ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes
ja1ns
pushed a commit
to WiseOwlTech/react-native-screens
that referenced
this pull request
Oct 9, 2024
…sted stack (software-mansion#2223) ## Description In software-mansion#2193 I've made a mistake - on Paper, when there is no touch handler held in `_touchHandler` field I've started lookup for touch handler in ancestor chain from `self` -> which leads to infinite loop when swiping back in nested-stack navigation scenario. ## Changes * Started lookup from superview. ## Test code and steps to reproduce `Test2223` 1. Navigate to NestedStack. 2. Navigate to NestedStack details screen. 3. Use swipe gesture to go-back W/o this PR the application will crash hitting infinite loop. Also test that this behaviour remains fixed: * software-mansion#2131 ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes
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.
Description
When cancelling touches we previously relied on
touchHandlerinstance being exposed by some react managed view above us (screen stack) in the view hierarchy (on Paper it wasRCTRootContentView). However this changed with some recent Fabric version and no view exposes such property any longer, despite the fact that touch handler is obviously still utilised by these views even on Fabric, but the field has been removed from public API.Currently on Fabric (as of version
0.74.1)RCTSurfaceTouchHandleris owned by instance ofRCTFabricSurfaceowned by instance ofRCTSurfaceViewwhich we can expect to live above us in the view hierarchy (RN mounts it in the very beginning of application runtime).To get access to the private touchHandler (which is a
UIGestureRecognizer) we observe that it is attached to theRCTSurfaceViewand thus it is retained in itsgestureRecognizerarray (native API).Tip
We could potentially cache the instance of
RCTSurfaceView, but for now I've come to a conclusion that this is an minor minor computation (there is only one gesture recognizer in the array & the parent lookup is O(log n) (I know this is not a balanced tree, but you know what I mean). However this is always an option for future.Note
I thought of adding warning in case of
RCTSurfaceViewbeing not present above screen stack in view hierarchy but this might be the case withmodalstack presentation.Closes #2118
Changes
RCTTouchHandler&RCTSurfaceTouchHandlerclasses to make the code more expressiveRNSScreenStackView.cancelTouchesInParentimplementation into two separate for Paper & FabricRCTSurfaceView& got access to its touch handler.Test code and steps to reproduce
Test2118See #2118 for issue description.
Checklist