Skip to content

fix: border rendering problem in Android - #36129

Closed
BeeMargarida wants to merge 2 commits into
react:mainfrom
BeeMargarida:fix/android-borders-not-rendered-correctly
Closed

fix: border rendering problem in Android#36129
BeeMargarida wants to merge 2 commits into
react:mainfrom
BeeMargarida:fix/android-borders-not-rendered-correctly

Conversation

@BeeMargarida

@BeeMargaridaBeeMargarida commented Feb 10, 2023

Copy link
Copy Markdown
Contributor

Summary

Fixes#36036

The problem was in ReactViewBackgroundDrawable.java that was not accounting for adjacent borders that add width set to 0.

Changelog

[Android] [Fixed] - Fix border rendering issue when bottom borders has no width

Test Plan

PreviouslyNow with the fix
imageimage

@facebook-github-botfacebook-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 Feb 10, 2023
@analysis-bot

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,479,906+140
androidhermesarmeabi-v7a7,800,769+145
androidhermesx868,955,600+138
androidhermesx86_648,813,441+150
androidjscarm64-v8a9,117,152+186
androidjscarmeabi-v7a8,313,222+184
androidjscx869,168,554+185
androidjscx86_649,427,379+176

Base commit: 96df8c0
Branch: main

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@cipolleschi

Copy link
Copy Markdown
Contributor

/rebase


mInnerBottomRightCorner.x = mInnerClipTempRectForBorderRadius.right;
mInnerBottomRightCorner.y = mInnerClipTempRectForBorderRadius.bottom * -2;
mInnerBottomRightCorner.y = borderWidth.bottom != 0

@javachejavacheFeb 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you explain why don't need this logic for mInnerTopLeftCorner and mInnerTopRightCorner?

@BeeMargaridaBeeMargaridaFeb 15, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, it can be added (my bad, I kinda had tunnel vision). In the cases where this bug happened it won't cause any difference though, because mInnerClipTempRectForBorderRadius.top was always 0. Do you want me to make a commit adding the same logic to mInnerTopLeftCorner and mInnerTopRightCorner?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If it doesn't make a difference, we shouldn't add it. So you're saying we don't need similar logic for the top corners?

@BeeMargaridaBeeMargaridaFeb 15, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm not totally sure. From what I saw from the code, mInnerClipTempRectForBorderRadius is set based on the getBounds of the drawable. In cases where the bounds don't change, top will always be 0. Is there any possibility of the bounds changing in this case?

Update: ah, there is if we change the size of drawable...I'll test it (nvm, not the case, from what I can tell top will always remain 0)

@javache

Copy link
Copy Markdown
Contributor

Looks like this logic was changed not too long ago in #34362. It seems there was no test-case added at that time, can you verify this doesn't regress it?

@BeeMargarida

Copy link
Copy Markdown
ContributorAuthor

Looks like this logic was changed not too long ago in #34362. It seems there was no test-case added at that time, can you verify this doesn't regress it?

Seems like there is no regression 👍
image

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label Feb 16, 2023
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@javache merged this pull request in 1d51032.

@zbranevichfinstek

zbranevichfinstek commented Jun 8, 2023

Copy link
Copy Markdown

@javache Please add this to 0.71

@javache

Copy link
Copy Markdown
Contributor

@zbranevichfinstek: please raise this in the 0.71 release thread - https://github.com/reactwg/react-native-releases/discussions

@lanphammm

lanphammm commented Apr 29, 2024

Copy link
Copy Markdown

I don't want to upgrade the react native version, so is there any way to fix this issue?
I have tried editing file ReactViewBackgroundDrawable.java inside node_modules but it's failed.

Dependencies: {
"react": "18.2.0",
"react-native": "0.71.3"
}

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BugCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Platform: AndroidAndroid applications.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[^0.71.0] - borderLeftColor and borderRightColor is not rendered properly

8 participants

@BeeMargarida@analysis-bot@facebook-github-bot@cipolleschi@javache@zbranevichfinstek@lanphammm@react-native-bot