fix(iOS): displaying irregular borders on iOS Fabric #45973
Conversation
facebook-github-bot
added
CLA Signed
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
I am trying to replicate the same mechanism (creating a
|
| @@ -681,7 +681,7 @@ - (void)invalidateLayer | |||
| } else { | |||
| if (!_borderLayer) { | |||
| CALayer *borderLayer = [CALayer new]; | |||
| borderLayer.zPosition = -1024.0f; | |||
| borderLayer.zPosition = 1024.0f; | |||
There was a problem hiding this comment.
Hi, maybe you can check the code history, and it introduce borderLayer is because CSS draws the border behind the content :) .
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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: :)
We hope to see this:
There was a problem hiding this comment.
Unfortunately, the iOS docs are not too descriptive about how the zPosition works 😕 I will try to think about other solution.
There was a problem hiding this comment.
The approach is promising, but it breaks another use case: Animated-MovingView
| BEFORE | AFTER |
|---|---|
| You can see that the blue background is not clipped and is present outside the rounded corners. |
The suggested approach from @joevilches would be to take out the background color entirely from that method and implement the border drawing in CG correctly without it.
Also, we should test for dotted and dashed borders if possible.
Many thanks for working on this!
| UIImage *image = RCTGetBorderImage( | ||
| RCTBorderStyleFromBorderStyle(borderMetrics.borderStyles.left), | ||
| layer.bounds.size, | ||
| RCTCornerRadiiFromBorderRadii(borderMetrics.borderRadii), | ||
| RCTUIEdgeInsetsFromEdgeInsets(borderMetrics.borderWidths), | ||
| borderColors, | ||
| backgroundColor, | ||
| transparentBackgroundColor, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
The new approach clips the inner image separately (as this bug occurs for images only) according to its bounds and the border's corner insets. We keep the previous value of |
|
@coado could you please resolve the merge conflict? 🙏 |
|
What do you think of this solution?
Btw, this area has a ton of churn right now. There is this PR, @jorge-cab is working on asymmetrical border radius % in #46009, and I am prototyping how to get box shadows to work with clipping (which relates to borders and masks). I feel like we just need to clean this area up some, and have a good story for how each element of the background/border area works. Lmk if the steps above would miss some edge case you have thought about |
|
Hey @coado, was thinking about this more and the above refactor I mentioned would in theory fix some problem I am running across with trying to get outset box shadows to work with clipping, so I went ahead and started on some of that refactor (specifically the second bullet point). I think we can just land this as is, but I might refactor some of it in the coming days |
Hey, so I've resolved the merge conflicts. I was thinking about the solution that you provided, but I wasn't sure if we wanted to keep the border in front of the image. I think it will work, but I haven't tested it myself. Should I proceed with it or is it something that you want to implement in the future? |
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@cipolleschi merged this pull request in 94407f5. |
Summary:
This PR solves issue with displaying irregular borders on Fabric. The same issue appears on the old architecture, but I am having a problems there, so I am pushing this fix for now.
The problem is solved by decoupling
backgroundColorfromborderLayerand settingzPositiononborderLayerto1024.0f, so that the border is always in front of the layer. ThezPositionis compared within a layer, so it shouldn't impact outside components. I would love to hear your opinion if there is a case in which this could break.Changelog:
[IOS] [FIXED] - changed border display
Test Plan:
I've checked that on RNTester Images.