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.

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

Add sound to Touchables on Android #1172

Merged
merged 2 commits into from
Nov 23, 2020

Conversation

jakub-gonet
Copy link
Member

@jakub-gonet jakub-gonet commented Aug 22, 2020

Description

Fixes #1146.

Seems like even TouchableWithoutFeedback plays sound in RN implementation, so we will stick to that.

Extracted from changes:

Buttons in RN are wrapped in NativeViewGestureHandler which manages onTouchEvent call after activation of the handler. Problem is, in order to verify that the underlying button implementation is interested in receiving touches we have to call onTouchEvent and check if the button is pressed.
This leads to invoking onTouchEvent twice which isn't idempotent in View - it calls OnClickListener and plays a sound effect if OnClickListener was set.

To mitigate this behavior we use mLastEventTime variable to check that we already handled the event in onInterceptTouchEvent. We assume here that different events will have different event times. We can't use ids of the event objects because they can be recycled and reused.

Test plan

Tested on example code:

import React from 'react';
import {
  Text,
  View,
  TouchableNativeFeedback,
  TouchableOpacity,
  TouchableWithoutFeedback,
} from 'react-native';

import {
  TouchableNativeFeedback as GHTouchableNativeFeedback,
  TouchableOpacity as GHTouchableOpacity,
  TouchableWithoutFeedback as GHTouchableWithoutFeedback,
} from 'react-native-gesture-handler';

export default () => {
  return (
    <View
      style={{
        flex: 1,
        justifyContent: 'center',
        alignItems: 'center',
        alignContent: 'space-around',
      }}>
      <TouchableOpacity onPress={() => console.log('pressed TouchableOpacity')}>
        <View
          style={{
            width: 200,
            height: 50,
            backgroundColor: 'lightyellow',
          }}>
          <Text>TouchableOpacity</Text>
        </View>
      </TouchableOpacity>
      <TouchableWithoutFeedback
        onPress={() => console.log('pressed TouchableWithoutFeedback')}>
        <View
          style={{
            width: 200,
            height: 50,
            backgroundColor: 'lightyellow',
          }}>
          <Text>TouchableWithoutFeedback</Text>
        </View>
      </TouchableWithoutFeedback>
      <TouchableNativeFeedback
        onPress={() => console.log('pressed TouchableNativeFeedback')}>
        <View
          style={{
            width: 200,
            height: 50,
            backgroundColor: 'lightyellow',
          }}>
          <Text>TouchableNativeFeedback</Text>
        </View>
      </TouchableNativeFeedback>
      <GHTouchableOpacity
        onPress={() => console.log('pressed RNGH TouchableOpacity')}>
        <View
          style={{
            width: 200,
            height: 50,
            backgroundColor: 'lightblue',
          }}>
          <Text>RNGH TouchableOpacity</Text>
        </View>
      </GHTouchableOpacity>
      <GHTouchableWithoutFeedback
        onPress={() => console.log('pressed RNGH TouchableWithoutFeedback')}>
        <View
          style={{
            width: 200,
            height: 50,
            backgroundColor: 'lightblue',
          }}>
          <Text>RNGH TouchableWithoutFeedback</Text>
        </View>
      </GHTouchableWithoutFeedback>
      <GHTouchableNativeFeedback
        onPress={() => console.log('pressed RNGH TouchableNativeFeedback')}>
        <View
          style={{
            width: 200,
            height: 50,
            backgroundColor: 'lightblue',
          }}>
          <Text>RNGH TouchableNativeFeedback</Text>
        </View>
      </GHTouchableNativeFeedback>
    </View>
  );
};

@jakub-gonet jakub-gonet self-assigned this Aug 22, 2020
@jakub-gonet jakub-gonet requested a review from jkadamczyk August 22, 2020 12:57
Copy link
Contributor

@jkadamczyk jkadamczyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@jakub-gonet
Copy link
Member Author

While writing this PR I noticed that even after moving outside of Touchable it still runs onPress and plays a sound, it's something we want to fix in the future.

@jakub-gonet jakub-gonet force-pushed the @kuba/add-sound-feedback-to-touchable branch from 5fcdd70 to e34b1ba Compare November 20, 2020 14:26
@jakub-gonet
Copy link
Member Author

The previous implementation used Sound Manager from RN, which was failing detox tests due to turbo module inclusion.

I prepared a new implementation that uses native tap sound triggered by presence of OnClickListener.

This revealed that we call onTouchEvent multiple times on the same event, so we block subsequent calls by comparing the last event's time with the current event time. This is written under the assumption that every event will have a different event time. This approach may have unexpected consequences in the future if this isn't true (e.g. device sends events in batches with the same event time), causing some events to be skipped.

@jakub-gonet jakub-gonet requested a review from kmagiera November 20, 2020 14:40
@Guuz
Copy link

Guuz commented Nov 20, 2020

Using the "presence of OnClickListener" as you linked above sounds like a much better strategy, also for RN itself... might be worth it to suggest it there too!

@jakub-gonet
Copy link
Member Author

For the record: the new implementation fixed tap sound when moving outside of the touchable.

@jakub-gonet jakub-gonet changed the title Add sound to TouchableNativeFeedback in android Add sound to Touchables on Android Nov 23, 2020
@jakub-gonet jakub-gonet merged commit 6070a36 into master Nov 23, 2020
@jakub-gonet jakub-gonet deleted the @kuba/add-sound-feedback-to-touchable branch November 23, 2020 15:45
braincore pushed a commit to braincore/react-native-gesture-handler that referenced this pull request Mar 4, 2021
## Description

Fixes software-mansion#1146.

Seems like even `TouchableWithoutFeedback` plays sound in RN implementation, so we will stick to that.

Extracted from code comment:

Buttons in RN are wrapped in NativeViewGestureHandler which manages `onTouchEvent` call after activation of the handler. Problem is, in order to verify that the underlying button implementation is interested in receiving touches we have to call onTouchEvent and check if the button is pressed.
This leads to invoking onTouchEvent twice which isn't idempotent in View - it calls OnClickListener and plays a sound effect if OnClickListener was set.

To mitigate this behavior we use `mLastEventTime` variable to check that we already handled the event in `onInterceptTouchEvent`. We assume here that different events will have different event times. We can't use ids of the event objects because they can be recycled and reused.

## Test plan

Tested on example code:
```jsx
import React from 'react';
import {
  Text,
  View,
  TouchableNativeFeedback,
  TouchableOpacity,
  TouchableWithoutFeedback,
} from 'react-native';

import {
  TouchableNativeFeedback as GHTouchableNativeFeedback,
  TouchableOpacity as GHTouchableOpacity,
  TouchableWithoutFeedback as GHTouchableWithoutFeedback,
} from 'react-native-gesture-handler';

export default () => {
  return (
    <View
      style={{
        flex: 1,
        justifyContent: 'center',
        alignItems: 'center',
        alignContent: 'space-around',
      }}>
      <TouchableOpacity onPress={() => console.log('pressed TouchableOpacity')}>
        <View
          style={{
            width: 200,
            height: 50,
            backgroundColor: 'lightyellow',
          }}>
          <Text>TouchableOpacity</Text>
        </View>
      </TouchableOpacity>
      <TouchableWithoutFeedback
        onPress={() => console.log('pressed TouchableWithoutFeedback')}>
        <View
          style={{
            width: 200,
            height: 50,
            backgroundColor: 'lightyellow',
          }}>
          <Text>TouchableWithoutFeedback</Text>
        </View>
      </TouchableWithoutFeedback>
      <TouchableNativeFeedback
        onPress={() => console.log('pressed TouchableNativeFeedback')}>
        <View
          style={{
            width: 200,
            height: 50,
            backgroundColor: 'lightyellow',
          }}>
          <Text>TouchableNativeFeedback</Text>
        </View>
      </TouchableNativeFeedback>
      <GHTouchableOpacity
        onPress={() => console.log('pressed RNGH TouchableOpacity')}>
        <View
          style={{
            width: 200,
            height: 50,
            backgroundColor: 'lightblue',
          }}>
          <Text>RNGH TouchableOpacity</Text>
        </View>
      </GHTouchableOpacity>
      <GHTouchableWithoutFeedback
        onPress={() => console.log('pressed RNGH TouchableWithoutFeedback')}>
        <View
          style={{
            width: 200,
            height: 50,
            backgroundColor: 'lightblue',
          }}>
          <Text>RNGH TouchableWithoutFeedback</Text>
        </View>
      </GHTouchableWithoutFeedback>
      <GHTouchableNativeFeedback
        onPress={() => console.log('pressed RNGH TouchableNativeFeedback')}>
        <View
          style={{
            width: 200,
            height: 50,
            backgroundColor: 'lightblue',
          }}>
          <Text>RNGH TouchableNativeFeedback</Text>
        </View>
      </GHTouchableNativeFeedback>
    </View>
  );
};
```
jakub-gonet added a commit that referenced this pull request Aug 17, 2021
## Description

After updating from 1.8.0 to 1.10.3 some of my users reported problems about buttons getting stuck on Android.

In #1172 an override for `onTouchEvent` was introduced to block subsequent calls. A check for event times was added with the assumption that every event will have a different event time. You can find further information in #1172 (comment) and in the Javadoc of `onTouchEvent`.

Unfortunately, events with the same event time can indeed occur on some devices for different actions, which has glaring consequences. Some of my users and myself experienced situations where - after a quick touch - the ripple effect was halted, the onPress was not fired, no touch sound was played and no other button was accepting any inputs. It seemed like the app thought the finger was still on the button.

This is due to the following sequence of events sometimes happening (fictional event times):
1. `ACTION_DOWN` is sent at EventTime 1000
2. `ACTION_MOVE` is sent at EventTime 2000 (Android sends at least one, even without a move)
3. `ACTION_UP` is **also** sent at EventTime 2000, resulting in not being handled and keeping the button touched

Furthermore, my thesis is confirmed by the fact that touching the same button again is sending another `ACTION_UP` and thus the and all other buttons are unstucked. The problem can even be forced to a certain extent by combining the quick touch with a small swiping gesture, which sends even more `ACTION_MOVE` events.

The only inexplicable part for me is that the frequency of this issue happening varies greatly from device to device. It happened every ~5 touches on a Huawei P30 (and on nearly every touch with the little forced swipe), infrequently but multiple times on a Samsung Galaxy S21, and couldn't be reproduced on an Oppo Find X2 Lite, LG G6, and the Android emulator.

It is also worth mentioning that a reproduction on statically placed buttons is easier, since buttons in scrollable views send an `ACTION_CANCEL` instead of an `ACTION_UP` on the slightest movement in the scrolling direction. I have so far not had a case where those event times were the same.

## How it's fixed

- Analogous to the check of the last event time, I have added another check for the last action.
- Because 0 is a possible value for an action (`ACTION_DOWN` with first pointer), I set the initial value to -1. To keep everything consistent, I've also set the default value of the event time to -1.
- By using `getAction` instead of `getActionMasked`, this solution is also pointer-sensitive.
- Of course I have also expanded the Javadoc. :)

After implementing these changes, I was no longer able to reproduce the issue on the P30 and S21 (or the other devices).

## Considerations

This is kind of an extension to @jakub-gonet's assumption that there will not be two or more events that have the same event time and action in immediate succession. There may still be another way in which unexpected consequences can occur, but my change ensures that the problems that have arisen so far in my app in production under normal use are eliminated. In addition, the check in `onTouchEvent` is much more robust.

Even if it did not happen in my tests, I can also very well imagine that the same event time could occur within scrollable views between `ACTION_MOVE` and `ACTION_CANCEL`. If so, my change would prevent such problems as well.

## Test plan

Tested with the example code of #1172 on the above mentioned 5 devices, where I appended the following `BaseButton` and `RectButton`. The fix naturally applies to all Touchables since they use `BaseButton` internally.

```jsx
import {
  BaseButton,
  RectButton,
} from 'react-native-gesture-handler';

...

<>
  <BaseButton
    style={{
      width: 200,
      height: 50,
      backgroundColor: 'lightgreen',
    }}
    onPress={() => console.log('pressed RNGH BaseButton')}>
    <Text>RNGH BaseButton</Text>
  </BaseButton>
  <RectButton
    style={{
      width: 200,
      height: 50,
      backgroundColor: 'lightgreen',
    }}
    onPress={() => console.log('pressed RNGH RectButton')}>
    <Text>RNGH RectButton</Text>
  </RectButton>
</>
```

Co-authored-by: Jakub Gonet <[email protected]>
fluiddot pushed a commit to wordpress-mobile/react-native-gesture-handler that referenced this pull request Feb 25, 2022
## Description

After updating from 1.8.0 to 1.10.3 some of my users reported problems about buttons getting stuck on Android.

In software-mansion#1172 an override for `onTouchEvent` was introduced to block subsequent calls. A check for event times was added with the assumption that every event will have a different event time. You can find further information in software-mansion#1172 (comment) and in the Javadoc of `onTouchEvent`.

Unfortunately, events with the same event time can indeed occur on some devices for different actions, which has glaring consequences. Some of my users and myself experienced situations where - after a quick touch - the ripple effect was halted, the onPress was not fired, no touch sound was played and no other button was accepting any inputs. It seemed like the app thought the finger was still on the button.

This is due to the following sequence of events sometimes happening (fictional event times):
1. `ACTION_DOWN` is sent at EventTime 1000
2. `ACTION_MOVE` is sent at EventTime 2000 (Android sends at least one, even without a move)
3. `ACTION_UP` is **also** sent at EventTime 2000, resulting in not being handled and keeping the button touched

Furthermore, my thesis is confirmed by the fact that touching the same button again is sending another `ACTION_UP` and thus the and all other buttons are unstucked. The problem can even be forced to a certain extent by combining the quick touch with a small swiping gesture, which sends even more `ACTION_MOVE` events.

The only inexplicable part for me is that the frequency of this issue happening varies greatly from device to device. It happened every ~5 touches on a Huawei P30 (and on nearly every touch with the little forced swipe), infrequently but multiple times on a Samsung Galaxy S21, and couldn't be reproduced on an Oppo Find X2 Lite, LG G6, and the Android emulator.

It is also worth mentioning that a reproduction on statically placed buttons is easier, since buttons in scrollable views send an `ACTION_CANCEL` instead of an `ACTION_UP` on the slightest movement in the scrolling direction. I have so far not had a case where those event times were the same.

## How it's fixed

- Analogous to the check of the last event time, I have added another check for the last action.
- Because 0 is a possible value for an action (`ACTION_DOWN` with first pointer), I set the initial value to -1. To keep everything consistent, I've also set the default value of the event time to -1.
- By using `getAction` instead of `getActionMasked`, this solution is also pointer-sensitive.
- Of course I have also expanded the Javadoc. :)

After implementing these changes, I was no longer able to reproduce the issue on the P30 and S21 (or the other devices).

## Considerations

This is kind of an extension to @jakub-gonet's assumption that there will not be two or more events that have the same event time and action in immediate succession. There may still be another way in which unexpected consequences can occur, but my change ensures that the problems that have arisen so far in my app in production under normal use are eliminated. In addition, the check in `onTouchEvent` is much more robust.

Even if it did not happen in my tests, I can also very well imagine that the same event time could occur within scrollable views between `ACTION_MOVE` and `ACTION_CANCEL`. If so, my change would prevent such problems as well.

## Test plan

Tested with the example code of software-mansion#1172 on the above mentioned 5 devices, where I appended the following `BaseButton` and `RectButton`. The fix naturally applies to all Touchables since they use `BaseButton` internally.

```jsx
import {
  BaseButton,
  RectButton,
} from 'react-native-gesture-handler';

...

<>
  <BaseButton
    style={{
      width: 200,
      height: 50,
      backgroundColor: 'lightgreen',
    }}
    onPress={() => console.log('pressed RNGH BaseButton')}>
    <Text>RNGH BaseButton</Text>
  </BaseButton>
  <RectButton
    style={{
      width: 200,
      height: 50,
      backgroundColor: 'lightgreen',
    }}
    onPress={() => console.log('pressed RNGH RectButton')}>
    <Text>RNGH RectButton</Text>
  </RectButton>
</>
```

Co-authored-by: Jakub Gonet <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TouchableNativeFeedback is missing touch sound
4 participants