react-native-gesture-handler icon indicating copy to clipboard operation
react-native-gesture-handler copied to clipboard

<RectButton> becomes unresponsive when used within a List

Open nujhong opened this issue 1 year ago • 3 comments

Description

Using a RectButton in a List component (FlatList/SectionList) will become unresponsive in certain scenarios. I am not sure whether this is an incorrect usage of this library or it's something new with RN. This wasn't an issue in production before.

  • It's observed that the event handler is not called when it's unresponsive
  • It's observed that when RectButton is unresponsive, it's also unresponsive to swipe up to close the android app
  • It's observed that using the same example, it cannot be reproduced on Snack
  • Finally, setting shouldActivateOnStart=true on RectButton seems to solve the issue
  • Additionally, I initially thought it's because I'm not using RectButton inside RNGH's ScrollView so I've added a SectionList export via patch-package. This also seems to solve the issue other than setting shouldActivateOnStart=true.

Platforms

  • [ ] iOS
  • [x] Android
  • [ ] Web

Screenshots

https://user-images.githubusercontent.com/23186058/183584450-ceebbea3-9298-4fb4-abcf-b0bf30b29cd5.mov

https://user-images.githubusercontent.com/23186058/183584650-a8e130da-a4db-4100-94f3-fcbbed814f91.mov

Steps To Reproduce

Using RN's SectionList

  1. Scroll multiple times to the bottom of the list
  2. All <RectButton> instances become unresponsive
  3. Navigate to "Solution" by pressing the button in Header
  4. All <RectButton shouldActivateOnStart> instances are still responsive (however the ripple effect is gone)
  5. Exit and reload the app and navigate straight to "Solution" and repeat Step 1
  6. Everything works fine

Using re-exported SectionList by overriding its ScrollViewComponent with RNGH's ScrollView via renderScrollComponent

  1. Apply the package by running yarn patch-package
  2. Scroll multiple times to the bottom of the list
  3. Everything works fine

Expected behavior

  • RectButton does not become unresponsive when used within a List component on Android

Actual behavior

  • RectButton becomes unresponsive when used within a List component Android

Snack or minimal code example

https://github.com/nujhong/RNGH-RectButton-In-a-List

Package versions

  • React: 18.0.0
  • React Native: 0.69.1
  • React Native Gesture Handler: 2.5.0
  • React Native Reanimated: 2.9.1

nujhong avatar Aug 09 '22 07:08 nujhong

Hi! At the first glance it might be related to the overscroll animation introduced in Android 12, however when you want to use gestures or components from Gesture Handler in a list, you should use the ScrollView exported from Gesture Handler. Using renderScrollComponent to override the default ScrollView is the correct solution in my opinion.

j-piasecki avatar Aug 09 '22 12:08 j-piasecki

I was about to file the same exact issue. And the unresponsiveness is coming randomly. My App is wrapped correctly with GestureRoot and it was not happening before I've upgraded to SDK 46 @j-piasecki.

I also passed renderScrollComponent={(props) => <ScrollView {...props} />}while ScrollView is an import from rngh.

Elements are not clickable anymore. Swapping to Pressable for example works. When I reload the app, it works like for 30 seconds or so and becomes broken.

https://user-images.githubusercontent.com/504909/184400660-72dcb03a-d4dc-4849-8b2b-09ff79241e3c.mp4

And its not only lists. Even my header buttons, which are BorderlessButton will break as well.

P.S: adding shouldActivateOnStart TO EVERY Button fixes the issue, but thats not a fix imho

hirbod avatar Aug 12 '22 16:08 hirbod

There is a much deeper issue with SDK 46 and RNGH. It also happens on iOS, there is basically no "ACTIVE" state anymore triggered. Breaking interactions with ScrollViews, also breaking @gorhom Bottom Sheet on iOS. We've tested this with Bare RN and Expo. Only happening with Expo (SDK 46, working fine with SDK 45) together with RNGH.

Will report back with more proof soon

hirbod avatar Aug 13 '22 10:08 hirbod

Yes we also found that the problem still persists when using RNGH's ScrollView.

Adding to @hirbod's investigation with ours, I summarised this which may be used to conclude that it's happening only for RN 0.69+ and very likely due to the Android 12 overscroll issue @j-piasecki has mentioned.

Framework RN Version Working
Expo SDK 45 0.68 🟢
Expo SDK 46 0.69 🔴
Bare 0.69 🔴

In terms of the shouldActivateOnStart workaround, we had to carry on with it for now and took it even further because that:

  • the problem is still being reported after replacing all ScrollView from RNGH, one scenario is that it happens when the screen times out after inactivity
  • it created another new problem with nested touch handling https://github.com/software-mansion/react-native-gesture-handler/issues/784#issuecomment-786471220, so I ended up adding another prop disallowInterruption=true to the child touch component as a workaround for us now

https://user-images.githubusercontent.com/23186058/184568225-7e03f95d-e7af-447f-a535-e672becf1afc.mov

https://user-images.githubusercontent.com/23186058/184569349-5926e741-d495-4fcb-83c8-dc0fa1fb00f7.mov

nujhong avatar Aug 15 '22 03:08 nujhong

I also experienced issues with nested buttons, good point with the interruption prop

hirbod avatar Aug 15 '22 18:08 hirbod

@nujhong Thanks for the update! I'll try to figure out why it doesn't work out-of-the-box, and look into the nested buttons.

@hirbod This seems to be a different issue than the one on iOS you mentioned. This also happens on bare workflow, and is related only to buttons. That said, have you experienced problems with nested buttons on Android, iOS, or both?

j-piasecki avatar Aug 16 '22 12:08 j-piasecki

Both, when I use the workaround to make them work in first place

hirbod avatar Aug 16 '22 14:08 hirbod

I summarised this which may be used to conclude that it's happening only for RN 0.69+ and very likely due to the Android 12 overscroll issue

I can reproduce this issue on React Native 0.68.2 (bare) with RNGH 2.5.0.

The way to reproduce it is quite simple: All you need is a scrollview (of any kind) with a RectButton or other pressables from RNGH.

  1. Press one button
  2. Drag down (with stretch effect)
  3. Drag up (with stretch effect)
  4. repeat until buttons are no longer pressable, usually repeating these steps once produces the issue

JoniVR avatar Aug 18 '22 14:08 JoniVR

An update that is hopefully useful @j-piasecki

This seems to be a problem specific to [email protected] + Android 12, without regards to react-native, as Expo SDK 46 also bumped it from 2.2.0 to 2.5.0.

For the peers, we eventually downgraded it to 2.3.2 and it works for us without any workarounds now.

nujhong avatar Aug 24 '22 05:08 nujhong

Could you check if the linked PR solves the issue in your case? If not, could you post how to reproduce it further?

j-piasecki avatar Aug 29 '22 13:08 j-piasecki

Will test tomorrow and report back! Thanks for addressing the issue.

hirbod avatar Aug 29 '22 13:08 hirbod

This is not an issue only with lists. I am experiencing the same problem. Basically the four buttons shown in a non list component, neither a ScrollView nor a Flatlist. The whole app is wrapped using GestureHandlerRootView yet this is the result

One of the buttons was not responsive after numerous tries, then another became responsive. Coming to first pressed button is again responsive.

  • Android 12 only

https://user-images.githubusercontent.com/80689446/189156842-83aced93-c84a-46fe-8467-48773e1f219a.mp4

itsramiel avatar Sep 08 '22 15:09 itsramiel

@itsramiel yes, in my example, you can also see that I can't even press the top right header button. It was only working when I added shouldActivateOnStart. I'm back to business now, I will try to pull in the draft to see if that resolves the issue.

hirbod avatar Sep 08 '22 15:09 hirbod

I was having similar issue with @gorhom/bottom-sheet and we just didn't have GestureHandlerRootView properly wrapping the root of the app case someone has similar issue. - https://docs.swmansion.com/react-native-gesture-handler/docs/installation/#js

AndreiCalazans avatar Sep 09 '22 16:09 AndreiCalazans

Looks like the fix works @j-piasecki - can we get this shipped quickly please? But I am still not sure if it fixed the issue in general. Its happen sporadically and I can't say with 100% confidence that it has fixed it, because casual RectButtons are breaking as well.

hirbod avatar Sep 24 '22 14:09 hirbod

I just wanted to add a little extra side note that the unresponsiveness does also happen on Android 10. I am currently doing some more tests (with no patch applied currently)

hirbod avatar Sep 25 '22 22:09 hirbod

After a bit more testing, I could not reproduce the issue anymore - neither with Android 10 nor with Android 12 So the PR #2187 fixed this issue for me

Yarn 3 patchfile (react-native-gesture-handler-npm-2.6.2-d70380740b.patch):

diff --git a/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerButtonViewManager.kt b/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerButtonViewManager.kt
index 7f878bf8f6b012e83c748f9d127a45a104ec2d81..049e06269d02818a44a260edf7e2a87307c421a8 100644
--- a/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerButtonViewManager.kt
+++ b/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerButtonViewManager.kt
@@ -167,6 +167,10 @@ class RNGestureHandlerButtonViewManager : ViewGroupManager<ButtonViewGroup>(), R
      * [com.swmansion.gesturehandler.NativeViewGestureHandler.onHandle]  */
     @SuppressLint("ClickableViewAccessibility")
     override fun onTouchEvent(event: MotionEvent): Boolean {
+      if (event.action == MotionEvent.ACTION_CANCEL) {
+        tryFreeingResponder()
+      }
+
       val eventTime = event.eventTime
       val action = event.action
       // always true when lastEventTime or lastAction have default value (-1)
@@ -267,7 +271,7 @@ class RNGestureHandlerButtonViewManager : ViewGroupManager<ButtonViewGroup>(), R
     }
 
     override fun drawableHotspotChanged(x: Float, y: Float) {
-      if (responder == null || responder === this) {
+      if (touchResponder == null || touchResponder === this) {
         super.drawableHotspotChanged(x, y)
       }
     }
@@ -280,19 +284,31 @@ class RNGestureHandlerButtonViewManager : ViewGroupManager<ButtonViewGroup>(), R
       return isResponder
     }
 
+    override fun afterGestureEnd(event: MotionEvent) {
+      tryFreeingResponder()
+      isTouched = false
+    }
+
+    private fun tryFreeingResponder() {
+      if (touchResponder === this) {
+        touchResponder = null
+        soundResponder = this
+      }
+    }
+
     private fun tryGrabbingResponder(): Boolean {
       if (isChildTouched()) {
         return false
       }
 
-      if (responder == null) {
-        responder = this
+      if (touchResponder == null) {
+        touchResponder = this
         return true
       }
       return if (exclusive) {
-        responder === this
+        touchResponder === this
       } else {
-        !(responder?.exclusive ?: false)
+        !(touchResponder?.exclusive ?: false)
       }
     }
 
@@ -313,7 +329,8 @@ class RNGestureHandlerButtonViewManager : ViewGroupManager<ButtonViewGroup>(), R
     override fun performClick(): Boolean {
       // don't preform click when a child button is pressed (mainly to prevent sound effect of
       // a parent button from playing)
-      return if (!isChildTouched()) {
+      return if (!isChildTouched() && soundResponder == this) {
+        soundResponder = null
         super.performClick()
       } else {
         false
@@ -327,20 +344,22 @@ class RNGestureHandlerButtonViewManager : ViewGroupManager<ButtonViewGroup>(), R
       // when canStart is called eventually, tryGrabbingResponder will return true if the button
       // already is a responder
       if (pressed) {
-        tryGrabbingResponder()
+        if (tryGrabbingResponder()) {
+          soundResponder = this
+        }
       }
 
       // button can be pressed alongside other button if both are non-exclusive and it doesn't have
       // any pressed children (to prevent pressing the parent when children is pressed).
-      val canBePressedAlongsideOther = !exclusive && responder?.exclusive != true && !isChildTouched()
+      val canBePressedAlongsideOther = !exclusive && touchResponder?.exclusive != true && !isChildTouched()
 
-      if (!pressed || responder === this || canBePressedAlongsideOther) {
+      if (!pressed || touchResponder === this || canBePressedAlongsideOther) {
         // we set pressed state only for current responder or any non-exclusive button when responder
         // is null or non-exclusive, assuming it doesn't have pressed children
         isTouched = pressed
         super.setPressed(pressed)
       }
-      if (!pressed && responder === this) {
+      if (!pressed && touchResponder === this) {
         // if the responder is no longer pressed we release button responder
         responder = null
         isTouched = false
@@ -354,6 +373,8 @@ class RNGestureHandlerButtonViewManager : ViewGroupManager<ButtonViewGroup>(), R
 
     companion object {
       var resolveOutValue = TypedValue()
+      var touchResponder: ButtonViewGroup? = null
+      var soundResponder: ButtonViewGroup? = null
       var responder: ButtonViewGroup? = null
       var dummyClickListener = OnClickListener { }
     }

hirbod avatar Sep 25 '22 22:09 hirbod

@hirbod Sorry for the delay, we will try to ship it ASAP.

j-piasecki avatar Oct 03 '22 06:10 j-piasecki