Fix overflow:'hidden' github issue #399

[sahrens]

Please select one of the following

• I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
• I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
• I am making a fix / change for the macOS implementation of react-native
• I am making a change required for Microsoft usage of react-native

Summary

Fixes overflow:'hidden' #399 by overriding setClipsToBounds to also set self.layer.masksToBounds = clipsToBounds;

overflow: 'hidden' doesn't work in MacOS, but it does work in iOS and Android (not sure about Windows). This change brings functional parity with minimal complexity/conflict.

This also fixes borderRadius on <Image> which broke recently because the implementation of RCTImage changes from subclass to composition with the image view being a subview and thus now relies on functional clipping behavior.

Changelog

[MacOS] [Fixed] - Fix overflow:'hidden' #399

Test Plan

Before	After

Microsoft Reviewers: Open in CodeFlow

[sahrens]

Looks like the failing test might be flaky since it also failed on #408:

https://dev.azure.com/ms/react-native/_build/results?buildId=81251&view=results
https://dev.azure.com/ms/react-native/_pipeline/analytics/stageawareoutcome?definitionId=220

Also, shouldn't snapshot tests have caught the image borderRadius regression before it landed?

[anandrajeswaran]

This is explicitly documented as undefined behavior. See https://developer.apple.com/library/archive/releasenotes/AppKit/RN-AppKit/#10_13Layer-backed%20Views.
macOS 10.13 is more consistent around updates to properties of views' layers (that is, layers that are the .layer of some NSView, however created). As before, if an application modifies such a layer by changing any of the following properties, the behavior of the application is undefined: bounds, position, zPosition, anchorPoint, anchorPointZ, transform, affineTransform, frame, hidden, geometryFlipped, masksToBounds, opaque, compositingFilter, filters, shadowColor, shadowOpacity, shadowOffset, shadowRadius, shadowPath, layoutManager.
I'm not super clear on what the overflow:'hidden' behavior is supposed to do and why it's valuable, but we need to come up with another approach

[tom-un]

Perhaps we could set the view's layer to a custom layer. I tried the following locally and it seemed to work:

in RCTView.m I created a

@interface RCTNoClippingLayer: CALayer
@end
@implementation RCTNoClippingLayer
-(BOOL) masksToBounds {
  return NO;
}
@end

and then added a setClipsToBounds: override that does

- (void)setClipsToBounds:(BOOL)clipsToBounds {
  [super setClipsToBounds:clipsToBounds];
  if (clipsToBounds) {
    self.layer = [RCTNoClippingLayer new];
  }
}

In RNTester it works. The impl is probably too simple though. I guess we'd have to cache the original CALayer and restore it if the prop is set to NO. And the impl should probably be put up a level on RCTUIView since that's where clipsToBounds is defined. But I think in principal it avoids the undefined behavior as documented by Apple of setting props on existing layers.

[tom-un]

Perhaps we could set the view's layer to a custom layer. I tried the following locally and it seemed to work:

in RCTView.m I created a

@interface RCTNoClippingLayer: CALayer
@end
@implementation RCTNoClippingLayer
-(BOOL) masksToBounds {
  return NO;
}
@end

and then added a setClipsToBounds: override that does

- (void)setClipsToBounds:(BOOL)clipsToBounds {
  [super setClipsToBounds:clipsToBounds];
  if (clipsToBounds) {
    self.layer = [RCTNoClippingLayer new];
  }
}

In RNTester it works. The impl is probably too simple though. I guess we'd have to cache the original CALayer and restore it if the prop is set to NO. And the impl should probably be put up a level on RCTUIView since that's where clipsToBounds is defined. But I think in principal it avoids the undefined behavior as documented by Apple of setting props on existing layers.

[tom-un]

Looks like the failing test might be flaky since it also failed on #408:

https://dev.azure.com/ms/react-native/_build/results?buildId=81251&view=results
https://dev.azure.com/ms/react-native/_pipeline/analytics/stageawareoutcome?definitionId=220

Also, shouldn't snapshot tests have caught the image borderRadius regression before it landed?

Indeed it seems the snapshot tests should have caught the change. I'll take a look, thanks!

[sahrens]

Would it be better to set masksToBounds in this function?

[sahrens]

I'm pretty wary of fundamentally changing the way RCTView works with respect to layers if a particular prop is changed, which would also be a further divergence from iOS (and probably break a bunch of stuff without a lot of additional complexity, like copying all the properties as you mention).

Also note that we already muck with view.layer in multiple places to implement stuff like shadows, borders, etc. so I think that ship has sailed, but also, it works fine in practice so I don't think there is much risk with this change as-is.

[tom-un]

I'm pretty wary of fundamentally changing the way RCTView works with respect to layers if a particular prop is changed, which would also be a further divergence from iOS (and probably break a bunch of stuff without a lot of additional complexity, like copying all the properties as you mention).

Also note that we already muck with view.layer in multiple places to implement stuff like shadows, borders, etc. so I think that ship has sailed, but also, it works fine in practice so I don't think there is much risk with this change as-is.

I'm inclined to agree given the other layer changes have been in place for sometime and still work. I have to guess that with Catalyst and UIKit for macOS its unlikely Apple could change the behavior now. @anandrajeswaran ?

[anandrajeswaran]

We should probably fix the other places where we already muck with layers? I get that the alternative of changing the entire layer model is scary, but it seems like an easy decision to not take further dependency on explicitly documented undefined behavior.

I haven't had much of a chance to dig into this case yet and probably won't until next week, but I'm sure we can come up with something that doesn't change the entire layer model but avoids adding dependency on behavior that is documented to be undefined.

Also worth noting that Catalyst was already in the pipeline when the documentation was updated in 10.13 to mark that behavior as undefined. We should definitely not take Catalyst as a sign that Apple is going to stop investing in AppKit

[sahrens]

If we want to rework the way we use layers, can we do that as a separate project?

This PR fixes a major regression that was recently introduced (border radius stopped working) and is blocking us from clean upgrades to the latest version which has critical fixes we need (like tintColor and resizeMode: ‘cover’). We’ve had to apply patches internally to move forward.

[sahrens]

So I just spent some time fixing transforms and discovered some weirdness about how layers work with NSView - the main issue I came across is that the backing layer can change to another one without all the properties getting copied over, so if you just set view.layer.transform, it gets wiped out later. The displayLayer: method seems safe though, so I think it's ok to set masksToBounds in that function as I proposed in my earlier inline comment.

[sahrens]

Also, I haven't been able to properly fork this repo - github blocks me because I already have a fork of facebok/react-native, so I just copy-pasted changes into the github web editor for this PR. Anyone know the best way to set up a proper fork? Do I need to delete my other fork? Or is there a way to bypass the github check? Or maybe I should muck with the origin for my existing fork?

[tom-un]

Also, I haven't been able to properly fork this repo - github blocks me because I already have a fork of facebok/react-native, so I just copy-pasted changes into the github web editor for this PR. Anyone know the best way to set up a proper fork? Do I need to delete my other fork? Or is there a way to bypass the github check? Or maybe I should muck with the origin for my existing fork?

Ya, this is a common friction point that comes up a lot. We tried to document the process in our Contribution Guide.

The issue is really just the nature of how GitHub works. A single user can only have a single fork of a repo. The solution is to manually add upstream remote aliases to your clone via git remote add ....

For example, your private fork of react-native was most likely made from facebook/react-native, so if you run git remote -v in your clone, you probably see:

origin	git@github.com:YOUR-ALIAS/react-native.git (fetch)
origin	git@github.com:YOUR-ALIAS/react-native.git (push)
upstream	https://github.com/facebook/react-native.git (fetch)
upstream	https://github.com/facebook/react-native.git (push)

You can manually add an addition upstream alias for react-native-macos via something like git remote add ms git@github.com:microsoft/react-native-macos.git
Then you can run git fetch ms to pull down the react-native-macos changes.
What I typically do when making a PR to one fork or another is to do git checkout ms/master which puts the clone in a detach head state, then git branch YOUR-BRANCH, git checkout YOUR-BRANCH, then make my commit and push.

[sahrens]

Awesome, thanks for the instructions. The contributor guide reads "...start by creating your own fork of the repository. This is essential....Click on the Fork button at the top right corner of the page."

Which doesn't work, so I wasn't sure if the rest of the instructions were recommended since they also don't apply very closely.

[tom-un]

Ya, I saw that and made note to myself to add a paragraph for "if you have already forked react-native...".

[sahrens]

Ok, replacing this with #457 which should avoid the undefined layer behavior issues.