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

fix(iOS): displaying irregular borders on iOS old architecture #46091

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

coado
Copy link
Collaborator

@coado coado commented Aug 19, 2024

Summary:

This PR solves the issue with drawing borders on the old architecture by setting a mask on the UIImageView subview (pretty much the same as in the earlier PR for Fabric). Also, it extracts the previous createMaskLayer function to RCTBorderDrawing.m, so it can be used in both RCTView.m and RCTViewComponentView.m files and changes its name to RCTCreateMaskLayer.

Changelog:

[IOS][FIXED] - fixes displaying irregular borders on the old architecture.

Test Plan:

old-arch-screen

@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 Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Aug 19, 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.

Great job here!

@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.

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.

See comment below.

layer.mask = mask;

for (UIView *subview in self.subviews) {
if ([subview isKindOfClass:[UIImageView class]]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There has been a suggestion from @NickGerleman to try and look for a more general approach.
It's not just images that should present this behavior, but all the components that have as tyle with overflow: hidden. Images have that style by default.

Do you think we can find a way to check if the style applied has the overflow property set to hidden, rather than checking if the subview is an Image?

Copy link
Collaborator Author

@coado coado Aug 20, 2024

Choose a reason for hiding this comment

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

Seems like the overflow property is used here to set clipsToBounds on the old architecture.

@coado
Copy link
Collaborator Author

coado commented Aug 20, 2024

I had a problem with the mask not correctly clipping other subviews. I am still unsure why that is, but I implemented the idea that @joevilches proposed about decoupling the border and background into two layers on Fabric. I don't know whether I should code the same on the old architecture since borders are implemented a little bit differently there. Also, I am not sure what initial value the zPosition on the border layer should have. It seems like it works for examples below, but I would love to hear your opinion about this.

BEFORE AFTER
scr-before scr

@coado coado requested a review from cipolleschi August 20, 2024 15:28
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.

a part for a nit, the code looks good for me. I'll forward the PR to @joevilches for his opinion.

…w/RCTViewComponentView.mm

Co-authored-by: Riccardo Cipolleschi <riccardo.cipolleschi@gmail.com>
@joevilches
Copy link
Contributor

Hey @coado, thanks for putting the extra time into this! Didn't see this and am about to put up a PR for the same thing, but the scope is limited to new arch. Do you mind limiting this PR to old arch? There are a few things with clipping that I added in as well that will make it more compliant.

Sorry for the inconvenience!

@coado
Copy link
Collaborator Author

coado commented Aug 23, 2024

Hey @coado, thanks for putting the extra time into this! Didn't see this and am about to put up a PR for the same thing, but the scope is limited to new arch. Do you mind limiting this PR to old arch? There are a few things with clipping that I added in as well that will make it more compliant.

Sorry for the inconvenience!

Hey, no problem! Do you think we could introduce separate layers for border and background also on the old architecture or is it better to not change current design?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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. p: Software Mansion Partner: Software Mansion Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants