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

[Android][Fix]Properly set borderRadius on Android #25626

Closed
wants to merge 1 commit into from

Conversation

cabelitos
Copy link
Contributor

@cabelitos cabelitos commented Jul 13, 2019

Summary

In order to properly set the view's borderRadius the innerRadiusX/innerRadiusY should not
be used, since their values are obtained via the following formula:

int innerTopLeftRadiusX = Math.max(topLeftRadius - borderWidth.left, 0);

If topLeftRadius and borderWidth.left have the same value innerTopLeftRadiusX will
be zero and it will cause the top left radius to never be set since
"(innerTopLeftRadiusX > 0 ? extraRadiusForOutline : 0)" will evaluate to zero.
In order to prevent this issue the condition will only consider topLeftRadius, topRightRadius, and etc.

I took a closer look to see if this fix does not causes a regression in #22511

Changelog

[Android] [FIX] - Correctly set the border radius on android

Test Plan

Using the following code and Android certify that the border radius is correctly applied.

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

export default class App extends React.Component<Props> {
  render() {
    return (
      <ScrollView style={styles.container}>
        <View style={styles.box1}>
          <Text>borderWidth: 2</Text>
          <Text>borderRadius: 2</Text>
        </View>
        <View style={styles.box2}>
          <Text>borderWidth: 2</Text>
          <Text>borderRadius: 2.000001</Text>
        </View>
        <View style={styles.box3}>
          <Text>borderWidth: 5</Text>
          <Text>borderRadius: 5</Text>
        </View>
        <View style={styles.box4}>
          <Text>borderWidth: 5</Text>
          <Text>borderRadius: 5.000001</Text>
        </View>
        <View style={styles.box5}>
          <Text>borderWidth: 10</Text>
          <Text>borderRadius: 10</Text>
        </View>
        <View style={styles.box6}>
          <Text>borderWidth: 10</Text>
          <Text>borderRadius: 11</Text>
        </View>
        <View style={styles.box7}>
          <Text>borderWidth: 10</Text>
          <Text>borderRadius: 5</Text>
        </View>
        <Text>Testing if this does not cause a regression in #22511</Text>
        <View style={{
          margin: 10,
          backgroundColor: 'red',
          width: 100,
          height: 100,
          borderWidth: 5,
          borderBottomLeftRadius: 15,
        }} />
        <View style={{
          margin: 10,
          backgroundColor: 'green',
          borderWidth: 5,
          width: 100,
          height: 100,
        }} />
      </ScrollView>
    );
  }
}

const styles = StyleSheet.create({
  container: {
    flex: 1
  },
  box1: {
    margin: 10,
    height: 100,
    borderRadius: 2,
    borderWidth: 2,
    borderColor: "#000000"
  },
  box2: {
    margin: 10,
    height: 100,
    borderRadius: 2.000001,
    borderWidth: 2,
    borderColor: "#000000"
  },
  box3: {
    margin: 10,
    height: 100,
    borderRadius: 5,
    borderWidth: 5,
    borderColor: "#000000"
  },
  box4: {
    margin: 10,
    height: 100,
    borderRadius: 5.000001,
    borderWidth: 5,
    borderColor: "#000000"
  },
  box5: {
    margin: 10,
    height: 100,
    borderRadius: 10,
    borderWidth: 10,
    borderColor: "#000000"
  },
  box6: {
    margin: 10,
    height: 100,
    borderRadius: 11,
    borderWidth: 10,
    borderColor: "#000000"
  },
  box7: {
    margin: 10,
    height: 100,
    borderRadius: 5,
    borderWidth: 10,
    borderColor: "#000000"
  }
});

Without the fix

not-working

With the fix

fixed

Closes #25591

In orde to properly set the view's borderRadius the inner*RadiusX/inner*RadiusY should not
be used, since their values are obtained via the following formula:

int innerTopLeftRadiusX = Math.max(topLeftRadius - borderWidth.left, 0);

If topLeftRadius and borderWidth.left have the same value innerTopLeftRadiusX will
be zero and it will cause the top left radius to never be set since
"(innerTopLeftRadiusX > 0 ? extraRadiusForOutline : 0)" will evaluate to zero.
In order to prevent this issue the condition will only consider topLeftRadius, topRightRadius, and etc.
@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 Jul 13, 2019
@react-native-bot react-native-bot added the Platform: Android Android applications. label Jul 13, 2019
@pull-bot
Copy link

Messages
📖

📋 Verify Changelog Format - A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 139c462

@cabelitos cabelitos changed the title [Android][FixProperly set borderRadius on Android [Android][Fix]Properly set borderRadius on Android Jul 13, 2019
@mjmasn
Copy link
Contributor

mjmasn commented Jul 13, 2019

Awesome, thanks @cabelitos!

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.

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

Copy link
Contributor

@mdvacca mdvacca left a comment

Choose a reason for hiding this comment

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

This PR was approved internally and we are landing it.

@mdvacca
Copy link
Contributor

mdvacca commented Jul 17, 2019

Thanks for working on this @cabelitos

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @cabelitos in b432b8f.

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 17, 2019
@cabelitos
Copy link
Contributor Author

thank you @mdvacca and your coworkers for reviewing and approving this fix. I'm glad that I could help.

grabbou pushed a commit that referenced this pull request Aug 7, 2019
Summary:
In order to properly set the view's borderRadius the inner*RadiusX/inner*RadiusY should not
be used, since their values are obtained via the following formula:

int innerTopLeftRadiusX = Math.max(topLeftRadius - borderWidth.left, 0);

If topLeftRadius and borderWidth.left have the same value innerTopLeftRadiusX will
be zero and it will cause the top left radius to never be set since
"(innerTopLeftRadiusX > 0 ? extraRadiusForOutline : 0)" will evaluate to zero.
In order to prevent this issue the condition will only consider topLeftRadius, topRightRadius, and etc.

I took a closer look to see if this fix does not causes a regression in #22511

[Android] [FIX] - Correctly set the border radius on android
Pull Request resolved: #25626

Test Plan:
Using the following code and Android certify that the border radius is correctly applied.
```javascript
import React from "react";
import { ScrollView, StyleSheet, Text, View } from "react-native";

export default class App extends React.Component<Props> {
  render() {
    return (
      <ScrollView style={styles.container}>
        <View style={styles.box1}>
          <Text>borderWidth: 2</Text>
          <Text>borderRadius: 2</Text>
        </View>
        <View style={styles.box2}>
          <Text>borderWidth: 2</Text>
          <Text>borderRadius: 2.000001</Text>
        </View>
        <View style={styles.box3}>
          <Text>borderWidth: 5</Text>
          <Text>borderRadius: 5</Text>
        </View>
        <View style={styles.box4}>
          <Text>borderWidth: 5</Text>
          <Text>borderRadius: 5.000001</Text>
        </View>
        <View style={styles.box5}>
          <Text>borderWidth: 10</Text>
          <Text>borderRadius: 10</Text>
        </View>
        <View style={styles.box6}>
          <Text>borderWidth: 10</Text>
          <Text>borderRadius: 11</Text>
        </View>
        <View style={styles.box7}>
          <Text>borderWidth: 10</Text>
          <Text>borderRadius: 5</Text>
        </View>
        <Text>Testing if this does not cause a regression in https://github.com/facebook/react-native/issues/22511</Text>
        <View style={{
          margin: 10,
          backgroundColor: 'red',
          width: 100,
          height: 100,
          borderWidth: 5,
          borderBottomLeftRadius: 15,
        }} />
        <View style={{
          margin: 10,
          backgroundColor: 'green',
          borderWidth: 5,
          width: 100,
          height: 100,
        }} />
      </ScrollView>
    );
  }
}

const styles = StyleSheet.create({
  container: {
    flex: 1
  },
  box1: {
    margin: 10,
    height: 100,
    borderRadius: 2,
    borderWidth: 2,
    borderColor: "#000000"
  },
  box2: {
    margin: 10,
    height: 100,
    borderRadius: 2.000001,
    borderWidth: 2,
    borderColor: "#000000"
  },
  box3: {
    margin: 10,
    height: 100,
    borderRadius: 5,
    borderWidth: 5,
    borderColor: "#000000"
  },
  box4: {
    margin: 10,
    height: 100,
    borderRadius: 5.000001,
    borderWidth: 5,
    borderColor: "#000000"
  },
  box5: {
    margin: 10,
    height: 100,
    borderRadius: 10,
    borderWidth: 10,
    borderColor: "#000000"
  },
  box6: {
    margin: 10,
    height: 100,
    borderRadius: 11,
    borderWidth: 10,
    borderColor: "#000000"
  },
  box7: {
    margin: 10,
    height: 100,
    borderRadius: 5,
    borderWidth: 10,
    borderColor: "#000000"
  }
});

```

![not-working](https://user-images.githubusercontent.com/984610/61164801-e1dc6580-a4ee-11e9-91f3-6ca2fab7c461.gif)

![fixed](https://user-images.githubusercontent.com/984610/61164794-db4dee00-a4ee-11e9-88d7-367c29c785ec.gif)

Closes #25591

Reviewed By: osdnk

Differential Revision: D16243602

Pulled By: mdvacca

fbshipit-source-id: 1e16572fdf6936aa19c3d4c01ff6434028652942
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. Merged This PR has been merged. Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[0.60.0] Regression in border radius rendering
6 participants