Modify TouchableNativeFeedback (Ripple) to follow borderRadius #6515

jeffchienzabinet · 2016-03-18T01:05:12Z

Using TouchableNativeFeedback has been a problem for me because the ripples it makes don't follow the child view's border radii so the ripples stick out of the child view's rounded corners. This PR should fix this problem with a minor caveat: this only works for TouchableNativeFeedback.Ripple and not TouchableNativeFeedback.SelectableBackground. I searched how I could apply corner radius to selectableItemBackground and it doesn't seem to be possible (the prevalent advice is to create the ripple manually which is equivalent to using TNF.Ripple in our case), though I could be wrong.

I added an example to UIExplorer (TouchableExample). This is my first PR to this repo so let me know if something's wrong. Cheers!

facebook-github-bot · 2016-03-18T01:05:34Z

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

facebook-github-bot · 2016-03-18T01:05:35Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

janicduplessis · 2016-03-18T05:02:11Z

Examples/UIExplorer/TouchableExample.js

@@ -111,6 +111,17 @@ exports.examples = [
   },
 }];

+if(Platform.OS === 'android') {


You can add a platform prop to the example object instead of checking the platform manually. See the '3D Touch' example above that is ios only.

Gotcha, updated.

facebook-github-bot · 2016-03-18T05:02:20Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

facebook-github-bot · 2016-03-18T20:23:22Z

@jeffchienzabinet updated the pull request.

mkonicek · 2016-03-19T13:54:19Z

Thanks for the PR! Is there a way to do this only by changing implementation of TouchableNativeFeedback and not react/views/view directly?

cc @kmagiera who wrote TouchableNativeFeedback, can you take a look please? :)

kmagiera · 2016-03-21T08:22:16Z

ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java

+  private void refreshTranslucentBackgroundDrawable() {
+    Drawable background = null;
+    if (mNativeBackground != null) {
+      float[] cornerRadii = getOrCreateReactViewBackground().getBorderRadii();


I think you may want to check if mReactBackgroundDrawable != null first. Otherwise this line will create that background object for every view with native background which is undesirable

Ah good catch, updated.

kmagiera · 2016-03-21T08:24:40Z

I left one comment inline. Otherwise this looks good to me.

facebook-github-bot · 2016-03-21T11:21:19Z

@jeffchienzabinet updated the pull request.

kmagiera · 2016-03-21T21:11:14Z

@jeffchienzabinet sorry for bugging you again: would you be able to squash your commit so that it only adds 1 commit instead of 9. This helps to keep the history of changes cleaner

facebook-github-bot · 2016-03-21T21:52:20Z

@jeffchienzabinet updated the pull request.

facebook-github-bot · 2016-03-21T22:21:22Z

@jeffchienzabinet updated the pull request.

jeffchienzabinet · 2016-03-21T22:26:51Z

I messed up the first rebase attempt so I had to hard reset and copy over the changes. I think it should be right...I hope

eslint-bot · 2016-03-21T22:29:15Z

Examples/UIExplorer/TouchableExample.js

+      <View>
+        <TouchableNativeFeedback
+          style={[styles.row, styles.block]}
+          onPress={() => console.log('TNF Ripple with Border Radius has been clicked')}


quotes: Strings must use singlequote.

nit: I personally find TNF a bit unclear. Can you use TouchableNativeFeedback?

mkonicek · 2016-03-21T22:36:55Z

Thanks a lot for the review @kmagiera! I'll wait for the tests to pass before merging. You can also use the shipit bot if you want.

mkonicek · 2016-04-01T15:00:25Z

I've just restarted the tests.

mkonicek · 2016-04-01T15:00:54Z

@facebook-github-bot shipit

jeffchienzabinet · 2016-04-12T19:53:41Z

Alright, done. Also fixed some conflicts.

facebook-github-bot · 2016-04-20T23:33:14Z

@jeffchienzabinet updated the pull request.

mkonicek · 2016-04-21T14:36:52Z

ReactAndroid/src/main/java/com/facebook/react/views/view/ReactDrawableHelper.java

      if (!drawableDescriptionDict.hasKey("borderless") ||
          drawableDescriptionDict.isNull("borderless") ||
          !drawableDescriptionDict.getBoolean("borderless")) {
-        mask = new ColorDrawable(Color.WHITE);
+        mask = new PaintDrawable(Color.WHITE);
+        if (cornerRadii != null)


nit: Please always add braces to keep the formatting consistent with the rest of the codebase:

if (foo) { oneLiner(); }

It's just a coding convention.

mkonicek · 2016-04-21T14:38:42Z

Thanks @jeffchienzabinet! Just some small nits, then this is good to go! 👍

Non-ripples generated by TNF.Ripple are now clipped by border radii the same way that the ReactViewGroup it's a background of gets clipped. This is achieved by refreshing those backgrounds in ReactViewGroup whenever nativeBackground or any of the borderRadii is updated, rather than ReactViewManager just setting the background Drawable. Note that TNF.SelectableBackground is not clipped because it's not possible to clip the selectableItemBackground Drawable.

jeffchienzabinet · 2016-04-21T18:56:16Z

Alright, done!

facebook-github-bot · 2016-04-21T18:57:14Z

@jeffchienzabinet updated the pull request.

mkonicek · 2016-04-22T15:19:58Z

Thanks @jeffchienzabinet!

@facebook-github-bot shipit

facebook-github-bot · 2016-04-22T15:20:35Z

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

Summary:Using TouchableNativeFeedback has been a problem for me because the ripples it makes don't follow the child view's border radii so the ripples stick out of the child view's rounded corners. This PR should fix this problem with a minor caveat: this only works for TouchableNativeFeedback.Ripple and not TouchableNativeFeedback.SelectableBackground. I searched how I could apply corner radius to selectableItemBackground and it doesn't seem to be possible (the prevalent advice is to create the ripple manually which is equivalent to using TNF.Ripple in our case), though I could be wrong. I added [an example to UIExplorer (TouchableExample)](http://i.imgur.com/CHY9xjW.png). This is my first PR to this repo so let me know if something's wrong. Cheers! Closes #6515 Differential Revision: D3126513 Pulled By: AaaChiuuu fb-gh-sync-id: 4a00e7177ee4ffd8dffeca143f4f43f08c99b5a1 fbshipit-source-id: 4a00e7177ee4ffd8dffeca143f4f43f08c99b5a1

Summary:Using TouchableNativeFeedback has been a problem for me because the ripples it makes don't follow the child view's border radii so the ripples stick out of the child view's rounded corners. This PR should fix this problem with a minor caveat: this only works for TouchableNativeFeedback.Ripple and not TouchableNativeFeedback.SelectableBackground. I searched how I could apply corner radius to selectableItemBackground and it doesn't seem to be possible (the prevalent advice is to create the ripple manually which is equivalent to using TNF.Ripple in our case), though I could be wrong. I added [an example to UIExplorer (TouchableExample)](http://i.imgur.com/CHY9xjW.png). This is my first PR to this repo so let me know if something's wrong. Cheers! Closes facebook#6515 Differential Revision: D3126513 Pulled By: mkonicek fb-gh-sync-id: 1d3e92243abf9706132ae47c485d9e04a9b47d81 fbshipit-source-id: 1d3e92243abf9706132ae47c485d9e04a9b47d81

mkonicek · 2016-04-25T11:24:36Z

@jeffchienzabinet Product teams at fb reported this broke a few things, such as standard borderRadius and JS-based modals.

The commit was reverted in 475f1b5.

Feel free to submit this again. Can you implement this in a way that only affects TouchableNativeFeedback and nothing else?

Sorry about that, I should have asked for a very thorough Test plan on this pull request that not only tests the new functionality but also the existing one (e.g. borders on views), especially since this modifies such core parts of the codebase (ReactViewGroup, ReactViewBackgroundDrawable).

jeffchienzabinet · 2016-04-26T19:22:01Z

Ahh, damn. Alright, give me some time. Do you guys know what the problem is exactly?

Summary:Using TouchableNativeFeedback has been a problem for me because the ripples it makes don't follow the child view's border radii so the ripples stick out of the child view's rounded corners. This PR should fix this problem with a minor caveat: this only works for TouchableNativeFeedback.Ripple and not TouchableNativeFeedback.SelectableBackground. I searched how I could apply corner radius to selectableItemBackground and it doesn't seem to be possible (the prevalent advice is to create the ripple manually which is equivalent to using TNF.Ripple in our case), though I could be wrong. I added [an example to UIExplorer (TouchableExample)](http://i.imgur.com/CHY9xjW.png). This is my first PR to this repo so let me know if something's wrong. Cheers! Closes facebook#6515 Differential Revision: D3126513 Pulled By: mkonicek fb-gh-sync-id: 1d3e92243abf9706132ae47c485d9e04a9b47d81 fbshipit-source-id: 1d3e92243abf9706132ae47c485d9e04a9b47d81

Summary:Using TouchableNativeFeedback has been a problem for me because the ripples it makes don't follow the child view's border radii so the ripples stick out of the child view's rounded corners. This PR should fix this problem with a minor caveat: this only works for TouchableNativeFeedback.Ripple and not TouchableNativeFeedback.SelectableBackground. I searched how I could apply corner radius to selectableItemBackground and it doesn't seem to be possible (the prevalent advice is to create the ripple manually which is equivalent to using TNF.Ripple in our case), though I could be wrong. I added [an example to UIExplorer (TouchableExample)](http://i.imgur.com/CHY9xjW.png). This is my first PR to this repo so let me know if something's wrong. Cheers! Closes facebook#6515 Differential Revision: D3126513 Pulled By: AaaChiuuu fb-gh-sync-id: 4a00e7177ee4ffd8dffeca143f4f43f08c99b5a1 fbshipit-source-id: 4a00e7177ee4ffd8dffeca143f4f43f08c99b5a1

cancan101 · 2016-05-25T16:45:26Z

Any updates on this?

Summary:Using TouchableNativeFeedback has been a problem for me because the ripples it makes don't follow the child view's border radii so the ripples stick out of the child view's rounded corners. This PR should fix this problem with a minor caveat: this only works for TouchableNativeFeedback.Ripple and not TouchableNativeFeedback.SelectableBackground. I searched how I could apply corner radius to selectableItemBackground and it doesn't seem to be possible (the prevalent advice is to create the ripple manually which is equivalent to using TNF.Ripple in our case), though I could be wrong. I added [an example to UIExplorer (TouchableExample)](http://i.imgur.com/CHY9xjW.png). This is my first PR to this repo so let me know if something's wrong. Cheers! Closes facebook#6515 Differential Revision: D3126513 Pulled By: mkonicek fb-gh-sync-id: 1d3e92243abf9706132ae47c485d9e04a9b47d81 fbshipit-source-id: 1d3e92243abf9706132ae47c485d9e04a9b47d81

Summary:Using TouchableNativeFeedback has been a problem for me because the ripples it makes don't follow the child view's border radii so the ripples stick out of the child view's rounded corners. This PR should fix this problem with a minor caveat: this only works for TouchableNativeFeedback.Ripple and not TouchableNativeFeedback.SelectableBackground. I searched how I could apply corner radius to selectableItemBackground and it doesn't seem to be possible (the prevalent advice is to create the ripple manually which is equivalent to using TNF.Ripple in our case), though I could be wrong. I added [an example to UIExplorer (TouchableExample)](http://i.imgur.com/CHY9xjW.png). This is my first PR to this repo so let me know if something's wrong. Cheers! Closes facebook#6515 Differential Revision: D3126513 Pulled By: AaaChiuuu fb-gh-sync-id: 4a00e7177ee4ffd8dffeca143f4f43f08c99b5a1 fbshipit-source-id: 4a00e7177ee4ffd8dffeca143f4f43f08c99b5a1

antoinerousseau · 2016-07-14T01:13:42Z

@mkonicek @jeffchienzabinet @kmagiera any plan to make this again? it sounds awesome ^^

Summary:Using TouchableNativeFeedback has been a problem for me because the ripples it makes don't follow the child view's border radii so the ripples stick out of the child view's rounded corners. This PR should fix this problem with a minor caveat: this only works for TouchableNativeFeedback.Ripple and not TouchableNativeFeedback.SelectableBackground. I searched how I could apply corner radius to selectableItemBackground and it doesn't seem to be possible (the prevalent advice is to create the ripple manually which is equivalent to using TNF.Ripple in our case), though I could be wrong. I added [an example to UIExplorer (TouchableExample)](http://i.imgur.com/CHY9xjW.png). This is my first PR to this repo so let me know if something's wrong. Cheers! Closes facebook#6515 Differential Revision: D3126513 Pulled By: mkonicek fb-gh-sync-id: 1d3e92243abf9706132ae47c485d9e04a9b47d81 fbshipit-source-id: 1d3e92243abf9706132ae47c485d9e04a9b47d81

Summary:Using TouchableNativeFeedback has been a problem for me because the ripples it makes don't follow the child view's border radii so the ripples stick out of the child view's rounded corners. This PR should fix this problem with a minor caveat: this only works for TouchableNativeFeedback.Ripple and not TouchableNativeFeedback.SelectableBackground. I searched how I could apply corner radius to selectableItemBackground and it doesn't seem to be possible (the prevalent advice is to create the ripple manually which is equivalent to using TNF.Ripple in our case), though I could be wrong. I added [an example to UIExplorer (TouchableExample)](http://i.imgur.com/CHY9xjW.png). This is my first PR to this repo so let me know if something's wrong. Cheers! Closes facebook#6515 Differential Revision: D3126513 Pulled By: AaaChiuuu fb-gh-sync-id: 4a00e7177ee4ffd8dffeca143f4f43f08c99b5a1 fbshipit-source-id: 4a00e7177ee4ffd8dffeca143f4f43f08c99b5a1

facebook-github-bot added the GH Review: review-needed label Mar 18, 2016

janicduplessis reviewed Mar 18, 2016
View reviewed changes

facebook-github-bot 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 Mar 18, 2016

kmagiera assigned sebmck and kmagiera and unassigned sebmck Mar 21, 2016

kmagiera reviewed Mar 21, 2016
View reviewed changes

kmagiera added GH Review: needs-revision and removed GH Review: review-needed labels Mar 21, 2016

kmagiera removed their assignment Mar 21, 2016

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Mar 21, 2016

jeffchienzabinet force-pushed the master branch from 053ce97 to c93bb99 Compare March 21, 2016 22:20

eslint-bot reviewed Mar 21, 2016
View reviewed changes

mkonicek added the GH Review: accepted label Mar 21, 2016

facebook-github-bot removed the GH Review: review-needed label Apr 1, 2016

jeffchienzabinet force-pushed the master branch from 8b6c830 to 8e48b27 Compare April 20, 2016 23:33

mkonicek reviewed Apr 21, 2016
View reviewed changes

jeffchienzabinet force-pushed the master branch from 8e48b27 to ade5cc9 Compare April 21, 2016 18:56

facebook-github-bot added Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: needs-revision labels Apr 22, 2016

ghost closed this in e438954 Apr 22, 2016

antoinerousseau mentioned this pull request Jul 14, 2016

TouchableNativeFeedback's ripples aren't affected by borderRadius on Android 9 #6480

Closed

kherock mentioned this pull request May 31, 2017

Improve Android button appearances (closes #849, #896) GeekyAnts/NativeBase#907

Closed

ConnorDY mentioned this pull request Apr 17, 2021

Attempt to fix FAB Android ripple GeekyAnts/NativeBase#3653

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify TouchableNativeFeedback (Ripple) to follow borderRadius #6515

Modify TouchableNativeFeedback (Ripple) to follow borderRadius #6515

jeffchienzabinet commented Mar 18, 2016

facebook-github-bot commented Mar 18, 2016

facebook-github-bot commented Mar 18, 2016

janicduplessis Mar 18, 2016

jeffchienzabinet Mar 18, 2016

facebook-github-bot commented Mar 18, 2016

facebook-github-bot commented Mar 18, 2016

mkonicek commented Mar 19, 2016

kmagiera Mar 21, 2016

jeffchienzabinet Mar 21, 2016

kmagiera commented Mar 21, 2016

facebook-github-bot commented Mar 21, 2016

kmagiera commented Mar 21, 2016

facebook-github-bot commented Mar 21, 2016

facebook-github-bot commented Mar 21, 2016

jeffchienzabinet commented Mar 21, 2016

eslint-bot Mar 21, 2016

mkonicek Apr 21, 2016

mkonicek commented Mar 21, 2016

mkonicek commented Apr 1, 2016

mkonicek commented Apr 1, 2016

jeffchienzabinet commented Apr 12, 2016

facebook-github-bot commented Apr 20, 2016

mkonicek Apr 21, 2016 •

edited

Loading

mkonicek commented Apr 21, 2016

jeffchienzabinet commented Apr 21, 2016

facebook-github-bot commented Apr 21, 2016

mkonicek commented Apr 22, 2016

facebook-github-bot commented Apr 22, 2016

mkonicek commented Apr 25, 2016 •

edited

Loading

jeffchienzabinet commented Apr 26, 2016

cancan101 commented May 25, 2016

antoinerousseau commented Jul 14, 2016 •

edited

Loading

Modify TouchableNativeFeedback (Ripple) to follow borderRadius #6515

Modify TouchableNativeFeedback (Ripple) to follow borderRadius #6515

Conversation

jeffchienzabinet commented Mar 18, 2016

facebook-github-bot commented Mar 18, 2016

facebook-github-bot commented Mar 18, 2016

janicduplessis Mar 18, 2016

Choose a reason for hiding this comment

jeffchienzabinet Mar 18, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Mar 18, 2016

facebook-github-bot commented Mar 18, 2016

mkonicek commented Mar 19, 2016

kmagiera Mar 21, 2016

Choose a reason for hiding this comment

jeffchienzabinet Mar 21, 2016

Choose a reason for hiding this comment

kmagiera commented Mar 21, 2016

facebook-github-bot commented Mar 21, 2016

kmagiera commented Mar 21, 2016

facebook-github-bot commented Mar 21, 2016

facebook-github-bot commented Mar 21, 2016

jeffchienzabinet commented Mar 21, 2016

eslint-bot Mar 21, 2016

Choose a reason for hiding this comment

mkonicek Apr 21, 2016

Choose a reason for hiding this comment

mkonicek commented Mar 21, 2016

mkonicek commented Apr 1, 2016

mkonicek commented Apr 1, 2016

jeffchienzabinet commented Apr 12, 2016

facebook-github-bot commented Apr 20, 2016

mkonicek Apr 21, 2016 • edited Loading

Choose a reason for hiding this comment

mkonicek commented Apr 21, 2016

jeffchienzabinet commented Apr 21, 2016

facebook-github-bot commented Apr 21, 2016

mkonicek commented Apr 22, 2016

facebook-github-bot commented Apr 22, 2016

mkonicek commented Apr 25, 2016 • edited Loading

jeffchienzabinet commented Apr 26, 2016

cancan101 commented May 25, 2016

antoinerousseau commented Jul 14, 2016 • edited Loading

mkonicek Apr 21, 2016 •

edited

Loading

mkonicek commented Apr 25, 2016 •

edited

Loading

antoinerousseau commented Jul 14, 2016 •

edited

Loading