Adds `touchSoundEnabled` prop to Touchables #11136

doomsower · 2016-11-25T16:48:37Z

This PR addresses issue #6188
It allows touchables to play system default sound when touched, and adds a prop to control this behavior (off by default)
It also allows to play default touch sound by calling UIManager.playTouchSound(), which can be used manually in gesture responders.

Test plan (required)

Tested this manually on following code fragment on both iOS and Android physical devices:

        <TouchableOpacity onPress={this.handlePress.bind(this, 'button')} touchSoundEnabled={true}>
          <View style={styles.btn}>
            <Text>Button</Text>
          </View>
        </TouchableOpacity>
        <Text style={styles.instructions} onPress={this.handlePress.bind(this, 'text2')}>
          <Text onPress={this.handlePress.bind(this, 'nested1')}>link</Text>
          {' text '}
          <Text onPress={this.handlePress.bind(this, 'nested2')} touchSoundEnabled={true}>link</Text>
        </Text>

Which, when set to true, allows them to play system default touch sound both on iOS ans Android. Relasted to #6188

facebook-github-bot · 2016-11-25T16:48:46Z

By analyzing the blame information on this pull request, we identified @ericvicenti and @JoelMarcey to be potential reviewers.

brentvatne · 2016-11-27T01:24:54Z

ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java

+  public void playTouchSound() {
+    AudioManager audioManager =
+            (AudioManager) getReactApplicationContext().getSystemService(Context.AUDIO_SERVICE);
+    if (audioManager != null)


iirc in this codebase braces should be used even on single line if statements

brentvatne · 2016-11-27T01:25:21Z

React/Modules/RCTUIManager.m

@@ -1119,6 +1119,10 @@ - (void)batchDidComplete
  [self _layoutAndMount];
 }

+RCT_EXPORT_METHOD(playTouchSound){


{ should be on newline to follow code style conventions

brentvatne · 2016-11-27T01:26:38Z

Libraries/Text/Text.js

@@ -151,6 +151,10 @@ const Text = React.createClass({
     */
    testID: React.PropTypes.string,
    /**
+     * If true, plays system sound on touch
+     **/
+    touchSoundEnabled: React.PropTypes.bool,


is this actually doing anything on Text?

doomsower · 2016-11-27T12:21:11Z

I fixed code style issues @brentvatne mentioned above.
And yes, it works on text too, and it respects nested text nodes.
It is possible to enable touch sound on top-level text but disable it on nested Text nodes and vice versa.
Here is my test app.

mkonicek · 2016-11-28T10:40:32Z

Check Brent's question:

is this actually doing anything on Text?

doomsower · 2016-11-28T10:53:57Z

@mkonicek
As I commented above, it works on Text too.

<Text touchSoundEnabled={true} style={styles.text} onPress={() => this.handlePress('Top-level text Sound ON')}>
  <Text style={styles.nestedText} onPress={() => this.handlePress('Nested text Sound OFF')}>Nested</Text>
  {' text '}
  <Text style={styles.nestedText} onPress={() => this.handlePress('Nested text Sound ON')} touchSoundEnabled={true}> sound </Text>
  {' ON '}
</Text>

In this fragment, it plays sound when you press on text, sound and ON words, but doesn't make sound when you press Nested word.
If there is no onPress prop on Text, then no sound is played, regardless of touchSoundEnabled value

brentvatne · 2016-11-28T17:29:56Z

@janicduplessis pointed out:

From what I remember for Android we wanted to enable touch sounds by default since that's how all native apps work. If sounds are enabled is controlled by a system setting.

Thoughts @doomsower?

Also there is a related PR that was closed: https://github.com/facebook/react-native/pull/6825/files

Thanks for the PR by the way! :)

doomsower · 2016-11-28T18:11:18Z

This PR was abandoned and got closed, because it used reactTag to obtain native view and then call method on of this view. As it was mentioned in the discussion there, reactTag doesn't always resolve to actual native view. Moreover, in this PR only ReactViewManager is modified to play sounds, but Touchables can be represented by other native views with different manager classes (Text, maybe Image). I take different approach and use system service to play sounds.

I don't play sound by default for two reasons. Firstly this change affects both iOS and Android, and I want to have similar behavior on both platforms. Secondly Touchable is not necessary a button, it can be text (link), icon, image, etc. (in the end, it is TouchableMixin that makes sound ), and I'm not sure if all of possible Touchable use cases produce sounds on Android.

It's possible to set different default prop value based on platform and component class (Button, Text, TouchableOpacity/TouhcableNativeFeedback).

brentvatne · 2016-12-11T20:11:59Z

This seems reasonable to me. Thoughts @ide / @janic / @satya / @mkonicek?

janicduplessis · 2016-12-12T01:47:07Z

I'm fine with merging this

iSimonWeb · 2017-02-13T21:22:49Z

Seems like we're almost there! Who can merge this by solving conflicts?

janicduplessis · 2017-02-13T21:46:57Z

@facebook-github-bot shipit

facebook-github-bot · 2017-02-13T21:47:13Z

@janicduplessis has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

shergin · 2017-02-24T06:18:31Z

React/Modules/RCTUIManager.m

@@ -1143,6 +1143,11 @@ - (void)batchDidComplete
  [self _layoutAndMount];
 }

+RCT_EXPORT_METHOD(playTouchSound)


I dislike an idea to put something related to sound into RCTUIManager. We just have to have dedicated AudioManager (if we still don't have one) and then implement whole feature on Javascript side.

Adding dedicated managers is beyond my area of expertise. I found no other classes dealing with sound or audio in RN, so I think it's ok to do it this way until more sound business is needed.

shergin · 2017-02-24T06:21:39Z

Libraries/Text/Text.js

@@ -177,6 +177,10 @@ const Text = React.createClass({
     */
    testID: PropTypes.string,
    /**
+     * If true, plays system sound on touch


Wait, why <Text>? Text has nothing to do with sound. A property of <Touchable*> would be enough.

Because Text has onPress too, and it can by used without being wrapped in touchable. If something can handle press, then it should be able to make sound. Besides, in discussion of first PR #6825 Text was used in internal tests.

filipencus · 2017-03-20T22:17:27Z

Any news or some progress on this? Thanks

ericvicenti · 2017-04-09T17:28:36Z

It sounds like we do not want to move forward with an implementation like this. If the change was exclusively to TouchableNativeFeedback, it would be more likely to get merged

deehuey · 2017-09-22T01:30:55Z

Hey guys, was this ever implemented in any way? My native app feels so empty and un androidy :(

Summary: Android apps play a touch sound on press, as long as you have "Touch sounds" enabled in the settings. As and Android user, when building my app using React Native, one of the first things I noticed was that there were not any touch sounds. This is missing from React Native and there have been multiple PRs to have this implemented, but no success. This PR iterates over [#6825](#6825) and [#11136](#11136) This PR keeps it simple by only implementing the enhancement for Android, as iOS apps typically do not use touch sounds, and follows the users' system settings for whether or not the sound is played. I have manually tested this on multiple devices and emulators with zero problems [ANDROID] [ENHANCEMENT] [UIManagerModule.java]- Adds Android click sound to touchables [ANDROID] [ENHANCEMENT] [Touchable] - Adds Android click sound to touchables Closes #17183 Differential Revision: D7560327 Pulled By: hramos fbshipit-source-id: ce1094c437541bc677c7d64b0dba343dd9574422

Summary: Android apps play a touch sound on press, as long as you have "Touch sounds" enabled in the settings. As and Android user, when building my app using React Native, one of the first things I noticed was that there were not any touch sounds. This is missing from React Native and there have been multiple PRs to have this implemented, but no success. This PR iterates over [facebook#6825](facebook#6825) and [facebook#11136](facebook#11136) This PR keeps it simple by only implementing the enhancement for Android, as iOS apps typically do not use touch sounds, and follows the users' system settings for whether or not the sound is played. I have manually tested this on multiple devices and emulators with zero problems [ANDROID] [ENHANCEMENT] [UIManagerModule.java]- Adds Android click sound to touchables [ANDROID] [ENHANCEMENT] [Touchable] - Adds Android click sound to touchables Closes facebook#17183 Differential Revision: D7560327 Pulled By: hramos fbshipit-source-id: ce1094c437541bc677c7d64b0dba343dd9574422

yurykorzun · 2019-04-18T14:12:36Z

Just found that any touchable always plays sound on Android and there is no way to disable it. Any plans to fix it and introduce a touchSoundEnabled prop?

janicduplessis · 2019-04-19T05:31:40Z

@yurykorzun #17183 was merged to add touch sounds but does not include a prop to disable it. I don't think there are any plans to add a prop currently but feel free to submit a PR.

Summary: Currently, every time a touchable is pressed on Android, a system sound is played. It was added in the PR #17183. There is no way to disable it, except disabling touch on sound on the system level. I am pretty sure there are cases when touches should be silent and there should be an option to disable it. Related PRs - #17183, #11136 [Android][added] - Added a touchSoundDisabled prop to Touchable. If true, doesn't system sound on touch. Pull Request resolved: #24666 Differential Revision: D15166582 Pulled By: cpojer fbshipit-source-id: 48bfe88f03f791e3b9c7cbd0e2eed80a2cfba8ee

Adds touchSoundEnabled prop to Touchables

6052549

Which, when set to true, allows them to play system default touch sound both on iOS ans Android. Relasted to #6188

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Nov 25, 2016

Fix TouchableHightlight tests

4be9233

brentvatne reviewed Nov 27, 2016

View reviewed changes

Fixed code style issues

96b6293

mkonicek added Needs more information and removed GH Review: review-needed labels Nov 28, 2016

brentvatne removed the Needs more information label Dec 11, 2016

Merge branch 'master' into touchableClickSound

f0c448a

hramos self-requested a review January 20, 2017 00:22

hramos approved these changes Jan 20, 2017

View reviewed changes

Merge branch 'master' into touchableClickSound

63006ae

Merge branch 'master' into touchableClickSound

71dfdc8

shergin reviewed Feb 24, 2017

View reviewed changes

ericvicenti closed this Apr 9, 2017

akriger mentioned this pull request Dec 13, 2017

Adds Android click sound to Touchables #17183

Closed

yurykorzun mentioned this pull request Apr 30, 2019

Adds a touchSoundDisabled prop to Touchable #24666

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds `touchSoundEnabled` prop to Touchables #11136

Adds `touchSoundEnabled` prop to Touchables #11136

doomsower commented Nov 25, 2016

facebook-github-bot commented Nov 25, 2016

brentvatne Nov 27, 2016

brentvatne Nov 27, 2016

brentvatne Nov 27, 2016

doomsower commented Nov 27, 2016

mkonicek commented Nov 28, 2016

doomsower commented Nov 28, 2016

brentvatne commented Nov 28, 2016

doomsower commented Nov 28, 2016

brentvatne commented Dec 11, 2016

janicduplessis commented Dec 12, 2016

iSimonWeb commented Feb 13, 2017

janicduplessis commented Feb 13, 2017

facebook-github-bot commented Feb 13, 2017

shergin Feb 24, 2017

doomsower Feb 24, 2017

shergin Feb 24, 2017

doomsower Feb 24, 2017

filipencus commented Mar 20, 2017

ericvicenti commented Apr 9, 2017

deehuey commented Sep 22, 2017

yurykorzun commented Apr 18, 2019

janicduplessis commented Apr 19, 2019

Adds touchSoundEnabled prop to Touchables #11136

Adds touchSoundEnabled prop to Touchables #11136

Conversation

doomsower commented Nov 25, 2016

facebook-github-bot commented Nov 25, 2016

brentvatne Nov 27, 2016

Choose a reason for hiding this comment

brentvatne Nov 27, 2016

Choose a reason for hiding this comment

brentvatne Nov 27, 2016

Choose a reason for hiding this comment

doomsower commented Nov 27, 2016

mkonicek commented Nov 28, 2016

doomsower commented Nov 28, 2016

brentvatne commented Nov 28, 2016

doomsower commented Nov 28, 2016

brentvatne commented Dec 11, 2016

janicduplessis commented Dec 12, 2016

iSimonWeb commented Feb 13, 2017

janicduplessis commented Feb 13, 2017

facebook-github-bot commented Feb 13, 2017

shergin Feb 24, 2017

Choose a reason for hiding this comment

doomsower Feb 24, 2017

Choose a reason for hiding this comment

shergin Feb 24, 2017

Choose a reason for hiding this comment

doomsower Feb 24, 2017

Choose a reason for hiding this comment

filipencus commented Mar 20, 2017

ericvicenti commented Apr 9, 2017

deehuey commented Sep 22, 2017

yurykorzun commented Apr 18, 2019

janicduplessis commented Apr 19, 2019

Adds `touchSoundEnabled` prop to Touchables #11136

Adds `touchSoundEnabled` prop to Touchables #11136