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.

Sign up for GitHub

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

Jump to bottom

fix(iOS): displaying irregular borders on iOS Fabric #45973

Closed
wants to merge 5 commits into from
Closed

fix(iOS): displaying irregular borders on iOS Fabric #45973

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9f84b13
fix borders issue on Fabric
coado Aug 12, 2024
fa06485
remove comment
coado Aug 12, 2024
24cbbff
creating transparentColor with UIColor clearColor
coado Aug 12, 2024
e5477b4
clipping image
coado Aug 13, 2024
ffaf037
Merge branch 'main' into @dmalecki/borders-issue
coado Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,7 @@ - (void)invalidateLayer
borderMetrics.borderWidths.left == 0 || self.clipsToBounds ||
(colorComponentsFromColor(borderMetrics.borderColors.left).alpha == 0 &&
(*borderMetrics.borderColors.left).getUIColor() != nullptr));

CGColorRef backgroundColor = [_backgroundColor resolvedColorWithTraitCollection:self.traitCollection].CGColor;

if (useCoreAnimationBorderRendering) {
Expand All @@ -681,7 +681,7 @@ - (void)invalidateLayer
} else {
if (!_borderLayer) {
CALayer *borderLayer = [CALayer new];
borderLayer.zPosition = -1024.0f;
borderLayer.zPosition = 1024.0f;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, maybe you can check the code history, and it introduce borderLayer is because CSS draws the border behind the content :) .

https://github.com/facebook/react-native/pull/45973/files#diff-fce237eeca8733cbef40d994a0c0a154026be79e681b103b4afde567dd42330bL659-L661

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, css draws a border behind the component's children. In the provided solution, the border is still drawn behind the inner content of the element. I think that layer's zPosition applies only within a layer, so it shouldn't have an impact outside of that. The above problem looks like this after the changes on Fabric:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Emm, zPosition also affects the view or layer for the same hierarchy, For example like below, setting border layer's zPosition to 1024 can cover the content of SubView1 I think.

View
---border layer
---SubView1
------Subview of SubView1

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, I'm trying to refactor the Text to support border, you can try #45972 and applied your code, you can see border is cover the text like below: :)

image

We hope to see this:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, the iOS docs are not too descriptive about how the zPosition works 😕 I will try to think about other solution.

borderLayer.frame = layer.bounds;
borderLayer.magnificationFilter = kCAFilterNearest;
[layer addSublayer:borderLayer];
Expand All @@ -694,14 +694,16 @@ - (void)invalidateLayer
layer.cornerRadius = 0;

RCTBorderColors borderColors = RCTCreateRCTBorderColorsFromBorderColors(borderMetrics.borderColors);

UIColor *transparentColor = [UIColor clearColor];
CGColorRef transparentBackgroundColor = [transparentColor resolvedColorWithTraitCollection:self.traitCollection].CGColor;

UIImage *image = RCTGetBorderImage(
RCTBorderStyleFromBorderStyle(borderMetrics.borderStyles.left),
layer.bounds.size,
RCTCornerRadiiFromBorderRadii(borderMetrics.borderRadii),
RCTUIEdgeInsetsFromEdgeInsets(borderMetrics.borderWidths),
borderColors,
backgroundColor,
transparentBackgroundColor,
Copy link
Contributor

Choose a reason for hiding this comment

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

Feedback @joevilches on this line:

I feel like we should just get rid of this param at this point. It's still used in the old arch but they are both relying on iOS graphics at that point so we should be able to refactor both of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if we can get rid of the background here. If we set the background in the layer instead of borderLayer, components that have clipsToBounds = false may not be drawn correctly. This causes the above-mentioned bug of not clipping the background.

self.clipsToBounds);

RCTReleaseRCTBorderColors(borderColors);
Expand All @@ -714,7 +716,7 @@ - (void)invalidateLayer
CGRect contentsCenter = CGRect{
CGPoint{imageCapInsets.left / imageSize.width, imageCapInsets.top / imageSize.height},
CGSize{(CGFloat)1.0 / imageSize.width, (CGFloat)1.0 / imageSize.height}};

_borderLayer.contents = (id)image.CGImage;
_borderLayer.contentsScale = image.scale;

Expand Down Expand Up @@ -751,6 +753,7 @@ - (void)invalidateLayer
}

layer.cornerRadius = cornerRadius;
layer.backgroundColor = backgroundColor;
layer.mask = maskLayer;
}

Expand Down
Loading