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

RN 0.72: Broken border colors / radius on Android #37753

Closed
MicroDroid opened this issue Jun 7, 2023 · 33 comments
Closed

RN 0.72: Broken border colors / radius on Android #37753

MicroDroid opened this issue Jun 7, 2023 · 33 comments
Labels
Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. Needs: Attention Issues where the author has responded to feedback. Platform: Android Android applications. Priority: High Resolution: Fixed A PR that fixes this issue has been merged.

Comments

@MicroDroid
Copy link

MicroDroid commented Jun 7, 2023

Description

When borderRadius is set, borderColor is backgroundColor by default (which is wrong), and only borderColor can change its color, borderTopColor etc. don't work (also wrong).

React Native Version

0.72.0-rc.5

Output of npx react-native info

info Fetching system and libraries information...
System:
  OS: Linux 5.15 Arch Linux
  CPU: (16) x64 AMD Ryzen 7 5800H with Radeon Graphics
  Memory: 13.05 GB / 31.35 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 18.14.2
    path: ~/.nvm/versions/node/v18.14.2/bin/node
  Yarn:
    version: 3.5.0
    path: /usr/sbin/yarn
  npm:
    version: 9.5.0
    path: ~/.nvm/versions/node/v18.14.2/bin/npm
  Watchman:
    version: 20221016.020512.0
    path: /usr/sbin/watchman
SDKs:
  Android SDK:
    API Levels:
      - "31"
      - "33"
      - "33"
    Build Tools:
      - 30.0.2
      - 30.0.3
      - 31.0.0
      - 33.0.0
      - 33.0.2
    System Images:
      - android-33 | Google APIs Intel x86_64 Atom
    Android NDK: Not Found
IDEs:
  Android Studio: AI-222.4459.24.2221.9971841
Languages:
  Java:
    version: 11.0.17
    path: /usr/sbin/javac
  Ruby: Not Found
npmPackages:
  "@react-native-community/cli": Not Found
  react: Not Found
  react-native: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: false
iOS:
  hermesEnabled: true
  newArchEnabled: Not found

Steps to reproduce

  1. Use the example code I provided
  2. Observe border color is green (same as background)
  3. Change borderTopColor to borderColor
  4. Observe border color becomes orange

Snack, code example, screenshot, or link to a repository

<View
	style={{
		width: 128,
		height: 128,
		borderWidth: 5,
		borderRadius: 5,
		borderTopColor: 'orange',
		backgroundColor: 'green',
	}}
/>
@github-actions github-actions bot added the Platform: Android Android applications. label Jun 7, 2023
@cortinico
Copy link
Contributor

@MicroDroid could you test on 0.71 and check if this is a regression introduced in 0.72?

@MicroDroid
Copy link
Author

@cortinico this bug appeared after upgrading to 0.72 so presumably yeah

@MicroDroid
Copy link
Author

(the app was on 0.71.8 iirc)

@NickGerleman
Copy link
Contributor

NickGerleman commented Jun 8, 2023

Iirc there were some changes here added for impl of web props. Let me see if I can find the commit.

@NickGerleman
Copy link
Contributor

NickGerleman commented Jun 8, 2023

Relevant changes:

  1. feat: Add logical border radius implementation #35572
  2. feat: Add logical border block color properties  #35999

cc @gabrieldonadel @lunaleaps @necolas

@kelset
Copy link
Contributor

kelset commented Jun 8, 2023

thanks for reporting! This looks like something that needs to be addressed ASAP

@MicroDroid
Copy link
Author

So once you fix this, could I install react-native package from main GitHub branch so that I can make a new release for the app I'm working on? Or would that not work?

I ask because I am not sure how native code is bundled or whatever.

@MicroDroid
Copy link
Author

(and I don't want to wait 2 more weeks for another RC)

@Pranav-yadav
Copy link
Contributor

So once you fix this, could I install react-native package from main GitHub branch so that I can make a new release for the app I'm working on? Or would that not work?

I ask because I am not sure how native code is bundled or whatever.

@MicroDroid Actually, main is NOT stable. The stable versions' code lives in their own *-stable branch (e.g.: 0.71-stable) and the respective RCs, stable, and patches are released from these respective branches only.
Once the PR w/ fix is opened/merged you can expect more details about the same under this issue only :)

(and I don't want to wait 2 more weeks for another RC)

Hopefully, that won't be the case, as Lorenzo said 👇:

"This looks like something that needs to be addressed ASAP" - #37753 (comment)

@MicroDroid
Copy link
Author

👍

@NickGerleman
Copy link
Contributor

@gabrieldonadel is this something that you might be willing to take a look at as the author of the changes?

We can also otherwise revert these and see if it resolves the issue. Though due to the nature of the source build, it does seem like that might need another RC if we cannot locally repro.

@fabOnReact
Copy link
Contributor

I can start working on this task. I remain available. What timeframe do I have? Thanks

@fabOnReact fabOnReact self-assigned this Jun 9, 2023
@fabOnReact
Copy link
Contributor

@fabOnReact
Copy link
Contributor

fabOnReact commented Jun 10, 2023

  1. setBorderWidth does not set borderWidthTop which causes no border applied

  2. colorTop seems to use the default of black instead of the color set from javascript

CLICK TO OPEN TESTS RESULTS

Before After

@MicroDroid
Copy link
Author

@fabriziobertoglio1987 I don't know if the example I put up is accurate, but I expect my UI elements to not lose borders due to a RN upgrade, is all

@fabOnReact
Copy link
Contributor

fabOnReact commented Jun 10, 2023

Individual border colors do not work with border radius

The logic that does not work when using border radius + borderTopColor

if (borderWidth.top > 0) {
final float x1 = left - mGapBetweenPaths;
final float y1 = top;
final float x2 = mInnerTopLeftCorner.x - mGapBetweenPaths;
final float y2 = mInnerTopLeftCorner.y;
final float x3 = mInnerTopRightCorner.x + mGapBetweenPaths;
final float y3 = mInnerTopRightCorner.y;
final float x4 = right + mGapBetweenPaths;
final float y4 = top;
drawQuadrilateral(canvas, colorTop, x1, y1, x2, y2, x3, y3, x4, y4);
}

Using borderColorTop instead of borderColor (commit)

const styles = StyleSheet.create({
  view: {
    width: 128,
    height: 128,
    borderTopWidth: 5,
    borderRadius: 5,
    borderColor: 'orange',
    backgroundColor: 'green',
  },
  button: {},
});

function TextExample() {
  const [redColor, setRedColor] = React.useState(true);
  return (
    <>
      <View
        style={[
          styles.view,
          // works with borderColor 
          {borderTopColor: redColor ? '#ff0000' : '#3399ff'},
        ]}
      />
      <Button title="change color" onPress={() => setRedColor(prev => !prev)} />
      <Text>
        Current color is {redColor ? '#ff0000' : '#3399ff'} which corresponds to{' '}
        {redColor ? 'red color' : 'blue color'}
      </Text>
    </>
  );
}
borderColor borderTopColor

@fabOnReact
Copy link
Contributor

fabOnReact commented Jun 10, 2023

Individual border color work with individual border width

The logic without border-radius does not experience this issue.

if (borderRight > 0) {
final float x1 = left + width;
final float y1 = top;
final float x2 = left + width;
final float y2 = top + height;
final float x3 = left + width - borderRight;
final float y3 = top + height - borderBottom;
final float x4 = left + width - borderRight;
final float y4 = top + borderTop;
drawQuadrilateral(canvas, colorRight, x1, y1, x2, y2, x3, y3, x4, y4);
}

float x2,
float y2,
float x3,
float y3,
float x4,
float y4) {
if (fillColor == Color.TRANSPARENT) {
return;
}
if (mPathForBorder == null) {
mPathForBorder = new Path();
}
mPaint.setColor(fillColor);
mPathForBorder.reset();
mPathForBorder.moveTo(x1, y1);
mPathForBorder.lineTo(x2, y2);
mPathForBorder.lineTo(x3, y3);
mPathForBorder.lineTo(x4, y4);
mPathForBorder.lineTo(x1, y1);
canvas.drawPath(mPathForBorder, mPaint);
}
private int getBorderWidth(int position) {
if (mBorderWidth == null) {
return 0;
}

Screenshot 2023-06-10 at 8 54 42 PM Screenshot 2023-06-10 at 8 55 37 PM

@fabOnReact
Copy link
Contributor

fabOnReact commented Jun 10, 2023

The issue is caused by the missing clipping effect of the top border when using borderColorTop.
The same issue does not reproduce when using borderColor.

This is the result after commenting the logic responsible for drawing top border.

  • When using borderColor, the rectangle is clipped on the top to display the border
  • When using borderTopColor, the rectangle is not clipped on the top and the border is not displayed
borderColor (fixed) borderTopColor (not fixed)

The clipping is done from updatePath:


/**
* Rounded Multi-Colored Border Algorithm:
*
* <p>Let O (for outer) = (top, left, bottom, right) be the rectangle that represents the size
* and position of a view V. Since the box-sizing of all React Native views is border-box, any
* border of V will render inside O.
*
* <p>Let BorderWidth = (borderTop, borderLeft, borderBottom, borderRight).
*
* <p>Let I (for inner) = O - BorderWidth.
*
* <p>Then, remembering that O and I are rectangles and that I is inside O, O - I gives us the
* border of V. Therefore, we can use canvas.clipPath to draw V's border.
*
* <p>canvas.clipPath(O, Region.OP.INTERSECT);
*
* <p>canvas.clipPath(I, Region.OP.DIFFERENCE);
*
* <p>canvas.drawRect(O, paint);
*
* <p>This lets us draw non-rounded single-color borders.
*
* <p>To extend this algorithm to rounded single-color borders, we:

@fabOnReact
Copy link
Contributor

Borders are drawn by using an inner and outer rectangle responsible for clipping the original view.

  • The inner rectangle clips the view and draws the border inside it
  • The outer rectangle clips the view and applies the borderRadius

STEP 1 - Drawing inner rectangle with rounded borders (1 for each side when using individual colors with borderTopColor).

  • Calculating the outer rectangle with rounded borders for each size (top, left, bottom, right) (sourcecode)
  • Clipping the View to draw the border (sourcecode)

After: Changed logic to drawQuadrilateral(canvas, -65536, -0.8f, 0.0f, -0.8f, 27.5f, 352.8f, 27.5f, 352.8f, 0.0f);

Before After

STEP 2 - The borderRadius is applied by clipping the View with an outside rectangle with rounded corners.

Before After

@fabOnReact
Copy link
Contributor

fabOnReact commented Jun 11, 2023

STEP 1) Using hardcoded logic to draw top border (step 1 from this comment)

drawQuadrilateral(canvas, -65536, -0.8f, 0.0f, -0.8f, 27.5f, 352.8f, 27.5f, 352.8f, 0.0f);

STEP 2) Commenting line clip path inner border

STEP 3) Using borderTopWidth instead of borderWidth (separate issue with setBorderWidth)

const styles = StyleSheet.create({
  view: {
    width: 128,
    height: 128,
    borderTopWidth: 5,
    borderRadius: 5,
    backgroundColor: 'green',
  },
  button: {},
});

function TextExample() {
  const [redColor, setRedColor] = React.useState(true);
  return (
    <>
      <View
        style={[
          styles.view,
          {borderTopColor: redColor ? '#ff0000' : '#3399ff'},
        ]}
      />
      <Button title="change color" onPress={() => setRedColor(prev => !prev)} />
      <Text>
        Current color is {redColor ? '#ff0000' : '#3399ff'} which corresponds to{' '}
        {redColor ? 'red color' : 'blue color'}
      </Text>
    </>
  );
}
Before After

@fabOnReact
Copy link
Contributor

The top border is drawn correctly when using borderTopWidth and borderColor, which confirm issue with the borderTopColor implementation in Java as explained above.

const styles = StyleSheet.create({
  view: {
    width: 128,
    height: 128,
    borderTopWidth: 5,
    borderRadius: 5,
    backgroundColor: 'green',
  },
  button: {},
});

function TextExample() {
  const [redColor, setRedColor] = React.useState(true);
  return (
    <>
      <View
        style={[styles.view, {borderColor: redColor ? '#ff0000' : '#3399ff'}]}
      />
      <Button title="change color" onPress={() => setRedColor(prev => !prev)} />
      <Text>
        Current color is {redColor ? '#ff0000' : '#3399ff'} which corresponds to{' '}
        {redColor ? 'red color' : 'blue color'}
      </Text>
    </>
  );
}

@fabOnReact
Copy link
Contributor

The functionality works on Expo

Screenshot 2023-06-11 at 8 32 43 PM

The functionality does not work on 0.71.8, adding a black full border (borderColor: "black") will remove the top border (borderTopColor: "red")

Screenshot 2023-06-11 at 8 07 27 PM

@fabOnReact
Copy link
Contributor

Borders are drawn by using an inner and outer rectangle responsible for clipping the original view.

  • The inner rectangle clips the view and draws the border inside it
  • The outer rectangle clips the borders inside the view

STEP 1 - The borderRadius is applied by clipping the View with an outside rectangle with rounded corners.

Before After

STEP 2 - Clipping inner rectangle (borders)

Difference when clipping view with 3 borders instead of 4 borders

Clipping a View with 3 borders

const styles = StyleSheet.create({
  view: {
    width: 128,
    height: 128,
    borderTopWidth: 5,
    borderLeftWidth: 5,
    borderRightWidth: 5,
    borderRadius: 5,
    borderTopColor: 'orange',
    backgroundColor: 'green',
  },
});
Before After

Clipping a View with 4 borders

const styles = StyleSheet.create({
  view: {
    width: 128,
    height: 128,
    borderTopWidth: 5,
    borderLeftWidth: 5,
    borderRightWidth: 5,
    borderBottomWidth: 5,
    borderRadius: 5,
    borderTopColor: 'orange',
    backgroundColor: 'green',
  },
});
Before After

STEP 3 - Drawing inner rectangle with rounded borders (1 for each side when using individual colors with borderTopColor).

Before After

Conclusions:

  • The black borders are drawn on top of the red border

@fabOnReact fabOnReact removed their assignment Jun 11, 2023
@kelset kelset added Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. Needs: Attention Issues where the author has responded to feedback. labels Jun 12, 2023
@kelset
Copy link
Contributor

kelset commented Jun 12, 2023

Thanks @fabriziobertoglio1987 for the investigation!

I've spent some time this morning diving a bit more into this from the perspective of trying to figure out which commit created the problem; here are a few things I've learned.

First off, to repro this problem is also possible to quickly just add the sample code in RNTester, under packages/rn-tester/js/examples/View/ViewExample.js; here's how things look in the 0.72-stable branch with the test code added:
Screenshot 2023-06-12 at 11 23 21

So, yes, the Android issue is fully confirmed there. After setting that up, I made a local branch and tried to revert the commits that @NickGerleman pointed out.

By reverting "feat: Add logical border block color properties" 597a1ff I actually got some improvement to the situation, shown here:
revert just logical border block

Reverting "feat: Add logical border radius implementation" 4ae4984 on top of the other revert didn't change anything:
revert both

And this is consistent with what Fabrizio is saying, meaning that there was a second underlying bug already present since 0.71.x. I didn't dig into what in 0.71 might have caused the problem.

So this leaves us into a position where:

  • there's a bug with Android and borders that was already existing in 0.71, that should be handled
  • there's a bigger regression on top of it, the one reported here, that is caused by 597a1ff by @gabrieldonadel

I think that if viable, we should properly fix both.

If that's too complicated/would take more than a couple days, we should revert the commit by Gabriel, bring back the status of things to the one we had in 0.71, and proceed with 0.72.0 while proper fixes are landed in main and then backported to 0.71 and 0.72.

@gabrieldonadel
Copy link
Collaborator

  • there's a bigger regression on top of it, the one reported here, that is caused by 597a1ff by @gabrieldonadel

This is fixed here #37828

@cipolleschi
Copy link
Contributor

For sake of completeness, @cortinico found this commit which touched that file in 0.72

  • gabrieldonadel@1d51032
    It does not seems related to the issue to me, tbh, but l'm putting it there to keep track of it.

facebook-github-bot pushed a commit that referenced this issue Jun 12, 2023
Summary:
Instead of requiring all  types of border color values to be present we should only take into consideration the left, top, right, bottom, and allEdges values and inject block values into  colorBottom and colorTop.

This PR only addresses the first issue described here (#37753 (comment)) by kelset

## Changelog:

[ANDROID] [FIXED] - Fix border clip check

Pull Request resolved: #37828

Test Plan:
Test through rn-tester if border color is being applied

<img width="482" alt="image" src="https://github.com/facebook/react-native/assets/11707729/c8c8772c-da8d-4393-bc3f-5868eca5df15">

Reviewed By: lunaleaps

Differential Revision: D46643773

Pulled By: cipolleschi

fbshipit-source-id: efb1ea81bf2462c14767a2554880eb7c44989975
@gabrieldonadel
Copy link
Collaborator

Ok, I did some more investigation on this. I tested multiple versions of react-native until I found the one where borderColor + borderRadius was working, the last version where this was working 100% was 0.70.6, which means that something on 0.71 broke this

image

@lunaleaps
Copy link
Contributor

Thanks @gabrieldonadel for your amazing help!

We'll pick the following into 0.72

  • Revert of cd6a913 - reverting this fixes ignored side-specific borderColor when borderRadius set
  • 7d1f7f3 - fix for the default borderColor matching the backgroundColor when borderRadius set.

@leymytel
Copy link

Hello everyone.

This issue seems to have similarities with a previously reported issue in version 0.71.x.

Here's the link to the previous issue for reference: #36569.

@kelset
Copy link
Contributor

kelset commented Jun 13, 2023

great stuff @gabrieldonadel! I locally applied both of your PRs on top of 0.72 locally and I can confirm that with both, the regression is fully addressed 🎉

Screenshot 2023-06-13 at 10 52 58

And by doing the revert of relevant commit in 0.71 I can confirm it fixes that problem too -> #36569 (comment)

kelset pushed a commit that referenced this issue Jun 13, 2023
Summary:
Instead of requiring all  types of border color values to be present we should only take into consideration the left, top, right, bottom, and allEdges values and inject block values into  colorBottom and colorTop.

This PR only addresses the first issue described here (#37753 (comment)) by kelset

## Changelog:

[ANDROID] [FIXED] - Fix border clip check

Pull Request resolved: #37828

Test Plan:
Test through rn-tester if border color is being applied

<img width="482" alt="image" src="https://github.com/facebook/react-native/assets/11707729/c8c8772c-da8d-4393-bc3f-5868eca5df15">

Reviewed By: lunaleaps

Differential Revision: D46643773

Pulled By: cipolleschi

fbshipit-source-id: efb1ea81bf2462c14767a2554880eb7c44989975
kelset pushed a commit that referenced this issue Jun 13, 2023
…37840)

Summary:
In an effort to fix #37753, this PR reverts the changes introduced by cd6a913 and 1d51032 so border-radius width calculations work as expected

## Changelog:

[ANDROID] [FIXED] - Fix border-radius width calculations

Pull Request resolved: #37840

Test Plan:
Test border colors along with border-radius through RNTester
<img width="538" alt="image" src="https://github.com/facebook/react-native/assets/11707729/4b148d4b-35dc-4737-be00-c5ff156b0865">

Reviewed By: dmytrorykun

Differential Revision: D46684071

Pulled By: cipolleschi

fbshipit-source-id: cf7a80d0d63009b457f03d690b632e332a9b4a02
@MicroDroid
Copy link
Author

RC.6 LET'S GOOOOOOOOO

@kelset
Copy link
Contributor

kelset commented Jun 13, 2023

we've done the cherry picking, we're starting testing, unless something unexpected goes horribly wrong in the next 12/24hrs RC6 should come out and soon after 0.71.11

Kudo pushed a commit to expo/react-native that referenced this issue Jun 15, 2023
Summary:
Instead of requiring all  types of border color values to be present we should only take into consideration the left, top, right, bottom, and allEdges values and inject block values into  colorBottom and colorTop.

This PR only addresses the first issue described here (facebook#37753 (comment)) by kelset

## Changelog:

[ANDROID] [FIXED] - Fix border clip check

Pull Request resolved: facebook#37828

Test Plan:
Test through rn-tester if border color is being applied

<img width="482" alt="image" src="https://github.com/facebook/react-native/assets/11707729/c8c8772c-da8d-4393-bc3f-5868eca5df15">

Reviewed By: lunaleaps

Differential Revision: D46643773

Pulled By: cipolleschi

fbshipit-source-id: efb1ea81bf2462c14767a2554880eb7c44989975
(cherry picked from commit 2d15f50)
Kudo pushed a commit to expo/react-native that referenced this issue Jun 15, 2023
…acebook#37840)

Summary:
In an effort to fix facebook#37753, this PR reverts the changes introduced by facebook@cd6a913 and facebook@1d51032 so border-radius width calculations work as expected

## Changelog:

[ANDROID] [FIXED] - Fix border-radius width calculations

Pull Request resolved: facebook#37840

Test Plan:
Test border colors along with border-radius through RNTester
<img width="538" alt="image" src="https://github.com/facebook/react-native/assets/11707729/4b148d4b-35dc-4737-be00-c5ff156b0865">

Reviewed By: dmytrorykun

Differential Revision: D46684071

Pulled By: cipolleschi

fbshipit-source-id: cf7a80d0d63009b457f03d690b632e332a9b4a02
(cherry picked from commit 0817eaa)
@kelset
Copy link
Contributor

kelset commented Jun 15, 2023

update, released both:

thanks again everyone for your work, it's really important to find and tackle this type of issues before stable release 🙇‍♂️

@Pranav-yadav Pranav-yadav added the Resolution: Fixed A PR that fixes this issue has been merged. label Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. Needs: Attention Issues where the author has responded to feedback. Platform: Android Android applications. Priority: High Resolution: Fixed A PR that fixes this issue has been merged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants