Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix border drawn over children on iOS #44777

Conversation

j-piasecki
Copy link
Contributor

Summary:

Fixes #44690

In the code responsible for drawing border on iOS there's a comment saying:

iOS draws borders in front of the content whereas CSS draws them behind the content. For this reason, only use iOS border drawing when clipping or when the border is hidden.

The condition that follows checks whether the content is clipped and the width and alpha channel of the border:

borderMetrics.borderWidths.left == 0 ||
colorComponentsFromColor(borderMetrics.borderColors.left).alpha == 0 || self.clipsToBounds);
.

The problem is when the color is not set at all - colorComponentsFromColor(borderMetrics.borderColors.left).alpha will be equal to 0 since the relevant SharedColor is null:

int32_t ColorFromUIColor(const std::shared_ptr<void> &uiColor)
{
UIColor *color = (UIColor *)unwrapManagedObject(uiColor);
if (color) {
UITraitCollection *currentTraitCollection = [UITraitCollection currentTraitCollection];
color = [color resolvedColorWithTraitCollection:currentTraitCollection];
return ColorFromUIColor(color);
}
return 0;
}

Then it uses the path with the default iOS behavior (drawing the border on top of the content) instead of the custom one (with the border below) and it seems like it defaults to drawing black when the passed color is nil.

This PR simply adds one more check to make sure the color is actually set before choosing the default platform behavior.

Changelog:

[IOS] [FIXED] - Fixed border being drawn over children when no color was set

Test Plan:

Tested on the code from the issue.

Before After
Screenshot 2024-06-04 at 11 18 14 Screenshot 2024-06-04 at 11 17 38

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Software Mansion Partner: Software Mansion Partner labels Jun 4, 2024
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Amazing job @j-piasecki! Thank you so much for this fix!

@facebook-github-bot
Copy link
Contributor

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

@j-piasecki
Copy link
Contributor Author

Are the failing actions something I should be concerned with 😅?

@cortinico
Copy link
Contributor

Are the failing actions something I should be concerned with 😅?

Nope it's a flaky internal test

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jun 4, 2024
@facebook-github-bot
Copy link
Contributor

@cipolleschi merged this pull request in 91c5a6d.

Copy link

github-actions bot commented Jun 4, 2024

This pull request was successfully merged by @j-piasecki in 91c5a6d.

When will my fix make it into a release? | How to file a pick request?

kosmydel pushed a commit to kosmydel/react-native that referenced this pull request Jun 11, 2024
Summary:
Fixes facebook#44690

In the code responsible for drawing border on iOS there's a comment saying:
> iOS draws borders in front of the content whereas CSS draws them behind the content. For this reason, only use iOS border drawing when clipping or when the border is hidden.

The condition that follows checks whether the content is clipped and the width and alpha channel of the border: https://github.com/facebook/react-native/blob/e0a2e86d0346bd7e40adf69311daa538ca8c9c5f/packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm#L643-L644.

The problem is when the color is not set at all - `colorComponentsFromColor(borderMetrics.borderColors.left).alpha` will be equal to 0 since the relevant `SharedColor` is `null`: https://github.com/facebook/react-native/blob/e0a2e86d0346bd7e40adf69311daa538ca8c9c5f/packages/react-native/ReactCommon/react/renderer/graphics/platform/ios/react/renderer/graphics/HostPlatformColor.mm#L76-L86

Then it uses the path with the default iOS behavior (drawing the border on top of the content) instead of the custom one (with the border below) and it seems like it defaults to drawing black when the passed color is `nil`.

This PR simply adds one more check to make sure the color is actually set before choosing the default platform behavior.

## Changelog:

[IOS] [FIXED] - Fixed border being drawn over children when no color was set

Pull Request resolved: facebook#44777

Test Plan:
Tested on the code from the issue.

|Before|After|
|-|-|
|<img width="546" alt="Screenshot 2024-06-04 at 11 18 14" src="https://github.com/facebook/react-native/assets/21055725/f13250a9-2e99-41c5-a9bc-02d65c00a6c0">|<img width="546" alt="Screenshot 2024-06-04 at 11 17 38" src="https://github.com/facebook/react-native/assets/21055725/f4571a5f-dfc4-4191-854c-fd3faf698b29">|

Reviewed By: cortinico

Differential Revision: D58131337

Pulled By: cipolleschi

fbshipit-source-id: 7da247d81ecec586de6f0023e0cb399f9966213d
Titozzz pushed a commit that referenced this pull request Jun 18, 2024
Summary:
Fixes #44690

In the code responsible for drawing border on iOS there's a comment saying:
> iOS draws borders in front of the content whereas CSS draws them behind the content. For this reason, only use iOS border drawing when clipping or when the border is hidden.

The condition that follows checks whether the content is clipped and the width and alpha channel of the border: https://github.com/facebook/react-native/blob/e0a2e86d0346bd7e40adf69311daa538ca8c9c5f/packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm#L643-L644.

The problem is when the color is not set at all - `colorComponentsFromColor(borderMetrics.borderColors.left).alpha` will be equal to 0 since the relevant `SharedColor` is `null`: https://github.com/facebook/react-native/blob/e0a2e86d0346bd7e40adf69311daa538ca8c9c5f/packages/react-native/ReactCommon/react/renderer/graphics/platform/ios/react/renderer/graphics/HostPlatformColor.mm#L76-L86

Then it uses the path with the default iOS behavior (drawing the border on top of the content) instead of the custom one (with the border below) and it seems like it defaults to drawing black when the passed color is `nil`.

This PR simply adds one more check to make sure the color is actually set before choosing the default platform behavior.

## Changelog:

[IOS] [FIXED] - Fixed border being drawn over children when no color was set

Pull Request resolved: #44777

Test Plan:
Tested on the code from the issue.

|Before|After|
|-|-|
|<img width="546" alt="Screenshot 2024-06-04 at 11 18 14" src="https://github.com/facebook/react-native/assets/21055725/f13250a9-2e99-41c5-a9bc-02d65c00a6c0">|<img width="546" alt="Screenshot 2024-06-04 at 11 17 38" src="https://github.com/facebook/react-native/assets/21055725/f4571a5f-dfc4-4191-854c-fd3faf698b29">|

Reviewed By: cortinico

Differential Revision: D58131337

Pulled By: cipolleschi

fbshipit-source-id: 7da247d81ecec586de6f0023e0cb399f9966213d
Titozzz pushed a commit that referenced this pull request Jun 18, 2024
Summary:
Fixes #44690

In the code responsible for drawing border on iOS there's a comment saying:
> iOS draws borders in front of the content whereas CSS draws them behind the content. For this reason, only use iOS border drawing when clipping or when the border is hidden.

The condition that follows checks whether the content is clipped and the width and alpha channel of the border: https://github.com/facebook/react-native/blob/e0a2e86d0346bd7e40adf69311daa538ca8c9c5f/packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm#L643-L644.

The problem is when the color is not set at all - `colorComponentsFromColor(borderMetrics.borderColors.left).alpha` will be equal to 0 since the relevant `SharedColor` is `null`: https://github.com/facebook/react-native/blob/e0a2e86d0346bd7e40adf69311daa538ca8c9c5f/packages/react-native/ReactCommon/react/renderer/graphics/platform/ios/react/renderer/graphics/HostPlatformColor.mm#L76-L86

Then it uses the path with the default iOS behavior (drawing the border on top of the content) instead of the custom one (with the border below) and it seems like it defaults to drawing black when the passed color is `nil`.

This PR simply adds one more check to make sure the color is actually set before choosing the default platform behavior.

## Changelog:

[IOS] [FIXED] - Fixed border being drawn over children when no color was set

Pull Request resolved: #44777

Test Plan:
Tested on the code from the issue.

|Before|After|
|-|-|
|<img width="546" alt="Screenshot 2024-06-04 at 11 18 14" src="https://github.com/facebook/react-native/assets/21055725/f13250a9-2e99-41c5-a9bc-02d65c00a6c0">|<img width="546" alt="Screenshot 2024-06-04 at 11 17 38" src="https://github.com/facebook/react-native/assets/21055725/f4571a5f-dfc4-4191-854c-fd3faf698b29">|

Reviewed By: cortinico

Differential Revision: D58131337

Pulled By: cipolleschi

fbshipit-source-id: 7da247d81ecec586de6f0023e0cb399f9966213d
This was referenced Jun 28, 2024
# for free to join this conversation on GitHub. Already have an account? # 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. p: Software Mansion Partner: Software Mansion Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parent border is drawn over the child content
4 participants