Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Nested touchables do not work well with <ScrollView /> #2585

Closed
JoshuaJADaniel opened this issue Sep 11, 2023 · 6 comments · Fixed by #2586
Closed

Nested touchables do not work well with <ScrollView /> #2585

JoshuaJADaniel opened this issue Sep 11, 2023 · 6 comments · Fixed by #2586
Labels
Platform: Android This issue is specific to Android Repro provided A reproduction with a snack or repo is provided

Comments

@JoshuaJADaniel
Copy link

Description

If you have the following component structure: Touchable -> ScrollView -> Touchable, scrolling causes the outer touchable to prevent touches. Users have to click once to fix the unresponsiveness, and then click again to work normally. I have attached a demo + Snack to help demonstrate this better.

NOTES:

  1. I am using BaseButton in the demo + Snack to demonstrate what is happening more clearly; the issue here is still present when using regular touchables such as TouchableWithoutFeedback -- just that it is not immediately visible what the issue is.
  2. I have only tested this on Android, so I am not sure if iOS also harbors this issue.
demo.mp4

Steps to reproduce

  1. Open the Snack
  2. Select Android
  3. Run either locally or online
  4. Scroll while holding down on one of the cards
  5. Try to click on one of the cards

Snack or a link to a repository

https://snack.expo.dev/@joshuajadaniel/need-to-click-twice-with-nested-touchables

Gesture Handler version

~2.12.1

React Native version

0.72.3

Platforms

Android

JavaScript runtime

Hermes

Workflow

Expo managed workflow

Architecture

Paper (Old Architecture)

Build type

Debug mode

Device

Real device

Device model

Samsung Galaxy S22

Acknowledgements

Yes
@github-actions github-actions bot added Repro provided A reproduction with a snack or repo is provided Platform: Android This issue is specific to Android labels Sep 11, 2023
@m-bert m-bert mentioned this issue Sep 12, 2023
Merged
@m-bert
Copy link
Contributor

m-bert commented Sep 12, 2023

Hi @JoshuaJADaniel! I've looked at the snack that you've provided and I've found out what was the problem. Here is my PR that should fix this issue. Could you please give it a try and confirm that it indeed solves the problem?

@JoshuaJADaniel
Copy link
Author

Hey @m-bert, would I have to test this change via patch-package with a release build?

@HeroBanana HeroBanana mentioned this issue Sep 14, 2023
Closed
@m-bert
Copy link
Contributor

m-bert commented Sep 14, 2023

Yes. Alternatively you can install gesture handler directly from my branch - that should also work.

m-bert added a commit that referenced this issue Sep 18, 2023
@m-bert
Fix ripple effect staying after cancellation (#2586)
703fd3a 
## Description

Sometimes when our `Button` (like `BaseButton`) is cancelled, it gets stuck at ripple animation. This may also prevent other components form receiving events, until the button will be pressed again (which will release it). This PR changes behavior after receiving `ACTION_CANCEL`, so that buttons don't get stuck anymore.

Fixes #2585

## Test plan

Tested on example from [issue](#2585) and `NestedButtons` example from our example app.
@JoshuaJADaniel
Copy link
Author

Hey @m-bert, just updated to ^2.13.1, however, the issue appears to persist even with this PR.

Tested by running the same provided code locally (since Snack only allows ~2.12.0 for Expo SDK49).

@m-bert
Copy link
Contributor

m-bert commented Oct 5, 2023

Have you changed something in your code? I've copied your snack, tested it on Pixel 6 emulator and my S20+ and I no longer see this issue after applying my PR.

@JoshuaJADaniel
Copy link
Author

@m-bert After further testing, similar to #2619, this issue only appears in Expo Go (or Snack). Since everything works as expected in release builds, no further action is needed for this issue. Thanks.

HeroBanana pushed a commit to HeroBanana/react-native-gesture-handler that referenced this issue Oct 26, 2023
@m-bert @HeroBanana
Fix ripple effect staying after cancellation (software-mansion#2586)
14b5acb 
## Description

Sometimes when our `Button` (like `BaseButton`) is cancelled, it gets stuck at ripple animation. This may also prevent other components form receiving events, until the button will be pressed again (which will release it). This PR changes behavior after receiving `ACTION_CANCEL`, so that buttons don't get stuck anymore.

Fixes software-mansion#2585

## Test plan

Tested on example from [issue](software-mansion#2585) and `NestedButtons` example from our example app.

(cherry picked from commit 703fd3a)
@j-piasecki j-piasecki mentioned this issue May 14, 2024
Merged
j-piasecki added a commit that referenced this issue May 16, 2024
@j-piasecki
Fix buttons getting stuck after scrolling on them (#2693)
bcd9524 
## Description

I've noticed a weird interaction between Gesture Handler buttons and the
ScrollView wrapped with `NativeViewGestureHandler` after
#2551
- after starting the scroll on a button, that button (and only that one)
would become unresponsive for the next touch. The issue is apparent
after touching the same button after the scroll ends, where it doesn't
react to the first touch (see the before video).

After some debugging, I've found that
[cancelAndClearTouchTargets](https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/core/java/android/view/ViewGroup.java;l=2922?q=ViewGroup&ss=android%2Fplatform%2Fsuperproject%2Fmain)
was invoked before delivering the event to the children of the
`GestureHandlerRootView` when clicking on the same button. This, in a
nutshell, was resulting in the following scenario:
1. User touches the button, `MotionEvent.ACTION_DOWN` is dispatched
2. RootView tries to deliver the event to handlers, Button receives the
event and sets `lastAction` to `ACTION_DOWN` and `lastEventTime`.
3. No handler is active, RootView calls `super.dispatchTouchEvent`
4. `cancelAndClearTouchTargets` is invoked, `ACTION_CANCEL` event is
dispatched to children of the RootView
5. Button receives the cancel event and processes it internally, touch
is cancelled
6. Original `ACTION_DOWN` event is dispatched to children of RootView
7. Button receives the `ACTION_DOWN` event but it's the same event that
it received before the synthetic cancel event - it's the same action and
time as saved, so the event is ignored
8. `ACTION_UP` event is dispatched when finger is removed from the
screen, Button ignores it since there is no press active

This PR changes the logic in the Button component to also save the last
action type and time for `CANCEL` events, so that the second time the
`DOWN` event is delivered, it's not being ignored.

This is effectively a revert of
#2586,
which was fixing stuff that
#1601
broke in a wrong way. Hopefully, this one solves the issue once and for
all 🤞.

### From the review

> You could also consider adding a few words about ScrollView
interactions into PR description 😅

When running the reproducer code below, you may notice that the
scrolling will be cancelled by the buttons being pressed. This can be
avoided by setting `disallowInterruption` on the native gesture wrapping
the `ScrollView`, which is done by default on the `ScrollView` exported
by RNGH.

## Test plan

<details>
<summary>Tested on the Example app (most notably nested buttons
example), code from the issue #2585 and on the following code</summary>

```jsx
import React from 'react';
import { StyleSheet, Text, ScrollView } from 'react-native';
import {
  GestureDetector,
  Gesture,
  RectButton,
  GestureHandlerRootView,
} from 'react-native-gesture-handler';

export const DATA = new Array(100).fill(0).map((_, index) => `Item ${index}`);

export default function App() {
  const scrollGesture = Gesture.Native();

  return (
    <GestureDetector gesture={scrollGesture}>
      <ScrollView>
        <GestureHandlerRootView>
          {DATA.map((genre, index) => (
            <RectButton
              key={index}
              style={styles.selectableItem}
              onPress={() => {
                console.log('Pressed', genre);
              }}>
              <Text style={[styles.genreText, styles.horizontalMargin]}>
                {genre}
              </Text>
            </RectButton>
          ))}
        </GestureHandlerRootView>
      </ScrollView>
    </GestureDetector>
  );
}

export const styles = StyleSheet.create({
  genreText: {
    fontSize: 18,
    fontWeight: '600',
    marginVertical: 12,
  },
  selectableItem: {
    flexDirection: 'row',
    alignItems: 'center',
    paddingHorizontal: 16,
    paddingVertical: 8,
  },
  horizontalMargin: {
    marginHorizontal: 16,
  },
});
```

</details>

Before the change:


https://github.com/software-mansion/react-native-gesture-handler/assets/21055725/48befc20-b115-445d-8e2f-315f2d32e247

After the change:


https://github.com/software-mansion/react-native-gesture-handler/assets/21055725/ae49371d-0adc-4dfd-abcb-08de8a276a28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Platform: Android This issue is specific to Android Repro provided A reproduction with a snack or repo is provided
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix ripple effect staying after cancellation software-mansion/react-native-gesture-handler
2 participants
@JoshuaJADaniel @m-bert