Android click sounds on press #6825

mikemonteith · 2016-04-05T19:57:48Z

Android should play a sound effect on click by default.
Since the ReactNative touch event model circumvents the usual Android OnClickListener, we trigger the click sound manually.

eslint-bot · 2016-04-05T20:03:32Z

Libraries/Components/Touchable/Touchable.js

        this.touchableHandlePress(e);
      }
    }

    this.touchableDelayTimeout && clearTimeout(this.touchableDelayTimeout);
    this.touchableDelayTimeout = null;
+  },
+


keyword-spacing: Expected space(s) after "if".

facebook-github-bot · 2016-04-05T23:16:04Z

By analyzing the blame information on this pull request, we identified @mkonicek, @sahrens and @kmagiera to be potential reviewers.

facebook-github-bot · 2016-04-09T18:42:14Z

@mikemonteith updated the pull request.

ghost · 2016-04-30T17:00:30Z

@sahrens would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

sahrens · 2016-04-30T17:58:04Z

Nice, but I doubt all apps want this - can you add a global config to turn it off? Like Touchable.DISABLE_DEFAULT_SOUNDS that people can set when there app first initializes, and a comment in the docs? cc @dmmiller and @mkonicek for thoughts.

For the propTypes, does it work to factor them out into a shared class, TouchablePropTypes? Make sure that if there are legit differences between the same propType docs on different touchables that those differences are preserved.

Also, please rebase and fix the conflicts and get tests passing.

satya164 · 2016-05-01T14:00:06Z

It's rare for apps in Android to play a sound on touch. So I'd say it should be off by default.

mikemonteith · 2016-05-01T15:01:07Z

@satya164 @sahrens if you turn on system sounds in android sound settings you'll notice that almost all apps play a click sound on touch, as its default onClick handler behaviour. (I think almost everyone has system sounds turned off for this reason)

satya164 · 2016-05-01T15:02:19Z

@mikemonteith Ah. Weird.

janicduplessis · 2016-05-16T00:40:10Z

Tested a few apps on Android and every touchable element always produces a sound so I think this should be enabled by default and not even be a prop. It's up to the user to enable / disable touch sounds but apps should not decide whether or not there should be one.

Also right now react native apps touch sounds are inconsistent since touchable elements don't produce sounds but other native controls like Toolbar buttons do.

mkonicek · 2016-05-16T18:50:19Z

Tested a few apps on Android and every touchable element always produces a sound

@janicduplessis You're right, as long as it's enabled in the system settings. On my Nexus 5X I actually had sounds disabled in Settings -> Sounds & notifications -> Other sounds -> Touch sounds. Now when I enabled it all touchables produce a sound, as long as the media volume is not muted.

@mikemonteith Let's play the sounds by default, agree with @janicduplessis we don't need the prop, as long as the behavior respects the system setting.

ide · 2016-05-16T18:51:53Z

Also right now react native apps touch sounds are inconsistent since touchable elements don't produce sounds but other native controls like Toolbar buttons do.

Is there a way we could cleanly fake this? Ex: by calling into an audio library directly, or sending fake touch events to a single offscreen button?

janicduplessis · 2016-05-16T18:59:09Z

@ide I'm pretty sure this PR does exactly what we want by calling https://developer.android.com/reference/android/view/View.html#playSoundEffect(int)

mikemonteith · 2016-05-17T08:25:35Z

Looks like there's some interest in this PR. Would you like me to make any changes to it?

janicduplessis · 2016-05-20T06:57:15Z

@mikemonteith Yes, could you remove the prop you added and always play the touch sounds, could you also make sure it repects the system setting for touch sounds.

facebook-github-bot · 2016-05-20T19:49:14Z

@mikemonteith updated the pull request.

mkonicek · 2016-05-23T13:55:21Z

Libraries/Components/Touchable/Touchable.js

@@ -712,12 +714,21 @@ var TouchableMixin = {

      var shouldInvokePress =  !IsLongPressingIn[curState] || pressIsLongButStillCallOnPress;
      if (shouldInvokePress && this.touchableHandlePress) {
+        this._playClickSound();


This is called on iOS as well as Android. Is that intentional?

No it should just be android. Should I add a Platform.os === 'android' check here?

Should probably add the platform check to make sure it doesn't do unnecessary stuff on ios.

mkonicek · 2016-05-23T13:57:32Z

Thanks for updating the pull request @mikemonteith 👍 I added one question.

Can you please add a test plan demonstrating the system setting is respected (you can hear sounds if and only if this is enabled in system settings)?
For an explanation of what a test plan is, see https://github.com/facebook/react-native/blob/master/PULL_REQUEST_TEMPLATE.md

dmmiller · 2016-07-20T10:47:13Z

@facebook-github-bot shipit

facebook-github-bot · 2016-07-20T10:48:40Z

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

dmmiller · 2016-07-20T16:03:00Z

Tried importing and this is failing on some internal tests. The problem is that you call findNodeHandle on things that might not end up in the tree. In particular, it is failing on a test like this :

<Text style={styles.centeredText} onPress={this.handlePress.bind(this, 'text2')}>
          <Text onPress={this.handlePress.bind(this, 'nested1')}>link</Text>
          {' text '}
          <Text onPress={this.handlePress.bind(this, 'nested2')}>link</Text>
</Text>

Try clicking on one of the nested text pieces. This fires an event on a virtual node which you then dispatch on. That doesn't actually exist in the tree. I think you can get around this by perhaps just always passing the root or something.

ghost · 2016-07-20T23:48:14Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

gousta · 2016-08-06T20:03:20Z

Hi, any updates on this?

mkonicek · 2016-09-09T10:12:49Z

@mikemonteith I'm going through all pull requests and noticed this one hasn't been updated in a while and the last comment requests changes. I'll close this pull request so it doesn't stay open indefinitely but please send a new one if you want to continue working on this.

I think this is useful! If anyone wants to send a new PR with this code, addressing the comment about findNodeHandle failing tests by @dmmiller it would be cool to get this in!

deehuey · 2017-09-22T01:29:07Z

Hey guys, why did we not implement this? It's driving me crazy as a new react-native user. It totally pulls me away from the 'native' feeling.

bcalik · 2017-10-09T01:17:36Z

I can't believe this issue is still not fixed for years.

deehuey · 2017-10-09T01:19:21Z

I gave up.

…

On Oct 8, 2017 21:18, "Burak Çalık" ***@***.***> wrote: I can't believe this issue is still not fixed for years. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6825 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AcCuP_b7xBMJK4TFTMT90Ea8De_58Ddsks5sqXRvgaJpZM4IAb0i> .

mikemonteith · 2017-10-09T07:45:12Z

Hi. I haven't worked on a react-native project for over a year now so this is very low on my priority list. The pull request is so old that I'm sure it needs to be looked at anew.

Hopefully someone else can pick this up before I eventually get round to looking at it again.

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

Memo check compared elements of prevProps to nextProps but failed to take new props into account. Fixed the logic and added a new test.

mikemonteith mentioned this pull request Apr 5, 2016

[Accessibility] Touchable components need "Touch Sounds" for more native feeling. #6188

Closed

eslint-bot reviewed Apr 5, 2016
View reviewed changes

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 Apr 5, 2016

mikemonteith force-pushed the android-click-sounds branch from c895571 to 7cd5ea0 Compare April 9, 2016 18:41

sahrens added the GH Review: needs-revision label Apr 30, 2016

sahrens removed the GH Review: review-needed label Apr 30, 2016

mikemonteith force-pushed the android-click-sounds branch from 7cd5ea0 to e0bad29 Compare May 20, 2016 19:22

ghost added GH Review: review-needed and removed GH Review: needs-revision labels May 20, 2016

mkonicek reviewed May 23, 2016
View reviewed changes

ghost added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jul 20, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 20, 2016

ghost added Import Failed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Jul 20, 2016

javache added GH Review: needs-revision and removed GH Review: accepted labels Aug 4, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 4, 2016

mkonicek closed this Sep 9, 2016

doomsower mentioned this pull request Feb 24, 2017

Adds touchSoundEnabled prop to Touchables #11136

Closed

akriger mentioned this pull request Dec 13, 2017

Adds Android click sound to Touchables #17183

Closed

react-one pushed a commit to react-one/react-native that referenced this pull request Sep 24, 2021

fix: static container memo check (facebook#6825)

2bf0958

Memo check compared elements of prevProps to nextProps but failed to take new props into account. Fixed the logic and added a new test.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android click sounds on press #6825

Android click sounds on press #6825

mikemonteith commented Apr 5, 2016

eslint-bot Apr 5, 2016

facebook-github-bot commented Apr 5, 2016

facebook-github-bot commented Apr 9, 2016

ghost commented Apr 30, 2016

sahrens commented Apr 30, 2016

satya164 commented May 1, 2016

mikemonteith commented May 1, 2016

satya164 commented May 1, 2016

janicduplessis commented May 16, 2016 •

edited

Loading

mkonicek commented May 16, 2016 •

edited

Loading

ide commented May 16, 2016

janicduplessis commented May 16, 2016

mikemonteith commented May 17, 2016

janicduplessis commented May 20, 2016

facebook-github-bot commented May 20, 2016

mkonicek May 23, 2016

mikemonteith May 25, 2016

janicduplessis May 25, 2016

mkonicek commented May 23, 2016

dmmiller commented Jul 20, 2016

facebook-github-bot commented Jul 20, 2016

dmmiller commented Jul 20, 2016

ghost commented Jul 20, 2016

gousta commented Aug 6, 2016

mkonicek commented Sep 9, 2016

deehuey commented Sep 22, 2017

bcalik commented Oct 9, 2017

deehuey commented Oct 9, 2017 via email

mikemonteith commented Oct 9, 2017

Android click sounds on press #6825

Android click sounds on press #6825

Conversation

mikemonteith commented Apr 5, 2016

eslint-bot Apr 5, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Apr 5, 2016

facebook-github-bot commented Apr 9, 2016

ghost commented Apr 30, 2016

sahrens commented Apr 30, 2016

satya164 commented May 1, 2016

mikemonteith commented May 1, 2016

satya164 commented May 1, 2016

janicduplessis commented May 16, 2016 • edited Loading

mkonicek commented May 16, 2016 • edited Loading

ide commented May 16, 2016

janicduplessis commented May 16, 2016

mikemonteith commented May 17, 2016

janicduplessis commented May 20, 2016

facebook-github-bot commented May 20, 2016

mkonicek May 23, 2016

Choose a reason for hiding this comment

mikemonteith May 25, 2016

Choose a reason for hiding this comment

janicduplessis May 25, 2016

Choose a reason for hiding this comment

mkonicek commented May 23, 2016

dmmiller commented Jul 20, 2016

facebook-github-bot commented Jul 20, 2016

dmmiller commented Jul 20, 2016

ghost commented Jul 20, 2016

gousta commented Aug 6, 2016

mkonicek commented Sep 9, 2016

deehuey commented Sep 22, 2017

bcalik commented Oct 9, 2017

deehuey commented Oct 9, 2017 via email

mikemonteith commented Oct 9, 2017

janicduplessis commented May 16, 2016 •

edited

Loading

mkonicek commented May 16, 2016 •

edited

Loading