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

Set rounded rectangle mask on TouchableNativeFeedback's ripples #25342

Closed

Conversation

nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Jun 21, 2019

Summary

This commit fixes an issue where ripple touch feedback extends beyond the border radius of a view.

Before

After

The fix

It achieves this by adding a mask to the RippleDrawable background, collecting that information from two new methods on ReactViewGroup:

  1. getBorderRadiusMask() returns a drawable rounded rectangle matching the view's border radius properties
  2. getBorderRadius() produces a float[] with the border radius information required to build a RoundedRectShape in getBorderRadiusMask()

Additionally, this commit updates setBorderRadius in ReactViewManager to re-apply the background whenever it is set, which is necessary to update the mask on the RippleDrawable background image as the border radius changes.

Related issues:
#6480

Changelog

[Android][fixed] - Adding border radius styles to TouchableNative react-native run-android --port correctly connects to dev server and related error messages display the correct port

Test Plan

Link this branch to a new React native project with the following App.js class:

import React, { Component } from "react";
import { StyleSheet, Text, View, TouchableNativeFeedback } from "react-native";

export default class App extends Component {
  render() {
    const ripple = TouchableNativeFeedback.Ripple("#ff0000");

    return (
      <View style={styles.container}>
        <TouchableNativeFeedback background={ripple}>
          <View
            style={{
              width: 96,
              borderRadius: 12,
              borderTopLeftRadius: 10,
              borderBottomRightRadius: 37,
              height: 96,
              alignItems: "center",
              justifyContent: "center",
              borderColor: "black",
              borderWidth: 2
            }}
          >
            <Text>{"CLICK CLICK"}</Text>
          </View>
        </TouchableNativeFeedback>
      </View>
    );
  }
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: "center",
    alignItems: "center",
    backgroundColor: "#F5FCFF"
  }
});

It's important to ensure that updates to border radius are accounted for. I did this by enabling hot module reloading and updating the border radius styles to verify that the ripple remains correct.

@facebook-github-bot 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 Jun 21, 2019
@@ -154,16 +165,18 @@ public void setPointerEvents(ReactViewGroup view, @Nullable String pointerEvents

@ReactProp(name = "nativeBackgroundAndroid")
public void setNativeBackground(ReactViewGroup view, @Nullable ReadableMap bg) {
mBg = bg;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a better way to track the current background style object passed into the React Component? I need to keep track of the last background style to reapply the ripple drawable with new border radius styles.

I wasn't sure if assigning this to a variable was the best approach, but I couldn't find another way to do it.

@nhunzaker
Copy link
Contributor Author

CI Failing with:

/opt/ndk/android-ndk-r20/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/8.0.2 doesn't exist.

Huh. Maybe a fluke, upstreaming to retrigger the build.

@nhunzaker nhunzaker force-pushed the fix-corner-radius-ripple branch 2 times, most recently from 6dfe068 to 644afca Compare July 10, 2019 18:15
This commit fixes an issue where ripple touch feedback extends beyond
the border radius of a view.

It achieves this by adding a mask to the RippleDrawable background,
collecting that information from two new methods on ReactViewGroup:

1. getBorderRadiusMask() returns a drawable rounded rectangle matching
the view's border radius properties
2. getBorderRadius() produces a float[] with the border radius
information required to build a RoundedRectShape in
getBorderRadiusMask()

Additionally, this commit updates setBorderRadius in ReactViewManager
to re-apply the background whenever it is set, which is necessary to
update the mask on the RippleDrawable background image as the border
radius changes.

Related issues:
facebook#6480
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@makovkastar
Copy link
Contributor

Thanks a lot for working on it, looks good! I've made a few changes to add the RTL support for the borderRadius prop (so that borderBottom[Top]StartRadius or borderBottom[Top]EndRadius are not ignored when creating the RippleDrawable mask).

Regarding storing the background and foreground ReadableMaps in class member variables, I don't know another way to do it, so let's keep it.

@nhunzaker
Copy link
Contributor Author

borderBottom[Top]StartRadius or borderBottom[Top]EndRadius

Totally forgot about these. Great!

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @nhunzaker in 14b455f.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jul 15, 2019
@grabbou
Copy link
Contributor

grabbou commented Oct 1, 2019

Heads up: This PR introduced regression: #26544. We are reverting it.

Does the regression look anything similar that could be caused by the changes in this commit?

@mjmasn
Copy link
Contributor

mjmasn commented Oct 1, 2019

Aside from the background/elevation regressions this would be a really nice improvement. Not sure if this helps but I found the following in my testing:

With TouchableNativeFeedback.Ripple('#ff0000') if overflow: 'hidden' is not added to the view then the red ripple is masked correctly but what I assume is the standard greyish ripple is also shown and is visible in the corners outside the border radius. With overflow: 'hidden' it renders correctly.

With TouchableNativeFeedback.Ripple('#ff0000', true) even with overflow: 'hidden' it just renders the entire ripple circle and no mask is applied.

TouchableNativeFeedback.SelectableBackground() also doesn't mask correctly even with overflow: 'hidden' (although maybe this is expected behaviour?)

For TouchableNativeFeedback.SelectableBackgroundBorderless() with or without overflow: 'hidden' the full circle ripple is shown with no mask.

For a null background (this probably isn't supported?) with overflow: 'hidden' the ripple is masked correctly. Both with or without the overflow: 'hidden' the View's background colour disappears (or changes to white?).

Plus if the view or any (later?) sibling views have elevation, then the elevation shadow is rendered in front of rather than behind those elevated views when using TouchableNativeFeedback.SelectableBackgroundBorderless() or Ripple.TouchableNativeFeedback('#ff0000', true).

@satya164
Copy link
Contributor

satya164 commented Oct 1, 2019

This is definitely a much needed fix. But the background related regression is much worse. So it's not possible to keep this commit right now. A new PR based on this which addresses the regression is very welcome.

grabbou added a commit that referenced this pull request Oct 2, 2019
grabbou added a commit that referenced this pull request Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Component: TouchableNativeFeedback Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants