See #419 for more context on approach.

Looks like the test is still flaky:

Failing tests:
RNTester-macOSIntegrationTests:
-[RNTesterLoadAllPages ScrollViewExample]

This task has failed 8 times across 36 pipeline runs in the last 14 days.

Isn't this relying on the exact same undefined behavior as #419 was?

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.

https://developer.apple.com/library/archive/releasenotes/AppKit/RN-AppKit/#10_13Layer-backed%20Views

It seems to me we're still modifying the masksToBounds property on a layer that is the .layer of an NSView. Note that how the layer is created doesn't matter according to Apple, so even making our own layer and assigning it to the view wouldn't be safe.

The concern is that we would then be explicitly relying on undefined behavior that has been explicitly called out as undefined for years. Since we at this moment can't find a supported way to achieve the behavior we want, if Apple changes that undefined behavior as they are totally justified in doing, we have no workarounds or ideas. We would have just broken clients with no way to fix them if and when Apple changes this undefined behavior.

That's why I don't think taking this change is the right approach

Isn't this relying on the exact same undefined behavior as #419 was?

This approach is practically different in two major ways. First, it modifies the layer provided by displayLayer: rather than accessing self.layer or view.layer, and it does it every time, making no assumption that the state will be preserved indefinitely - this was necessary to get transforms to work (in a separate diff) because the layer does get recreated by the system and not all properties are preserved, so this technique avoids that potential class of bug that definitely happens with transforms. Second, we are already knee deep in this approach for a bunch of border rendering and other logic, so we are at least self consistent. I believe @tom-un is planning on scoping out a separate project to make all of this done "the right way" whatever that ends up being.

This approach is practically different in two major ways. First, it modifies the layer provided by displayLayer: rather than accessing self.layer or view.layer, and it does it every time, making no assumption that the state will be preserved indefinitely - this was necessary to get transforms to work (in a separate diff) because the layer does get recreated by the system and not all properties are preserved, so this technique avoids that potential class of bug that definitely happens with transforms.

The code in this review does seem to explicitly call self.layer from RCTView.

Second, we are already knee deep in this approach for a bunch of border rendering and other logic, so we are at least self consistent. I believe @tom-un is planning on scoping out a separate project to make all of this done "the right way" whatever that ends up being.

I had not been aware of these other spots in the project where we're explicitly relying on undefined behavior, but those are definitely of concern. I'm concerned this debt will continue to be pushed off until the day Apple releases an OS version that changes this undefined behavior, at which point we will be unable to support existing behaviors on new OSs, breaking existing clients and apps.

The code in this review does seem to explicitly call self.layer from RCTView.

lol, that's a typo! Let me fix that...

The code in this review does seem to explicitly call self.layer from RCTView.

lol, that's a typo! Let me fix that...

But how is that different? It's still the same layer that Apple has documented that we shouldn't mess with, whether we're grabbing it from our ivar or using it from the layer drawing callback? Or is it a different layer and I'm missing the subtlety?

The code in this review does seem to explicitly call self.layer from RCTView.

lol, that's a typo! Let me fix that...

But how is that different? It's still the same layer that Apple has documented that we shouldn't mess with, whether we're grabbing it from our ivar or using it from the layer drawing callback? Or is it a different layer and I'm missing the subtlety?

I debugged it and the layer passed in to displayLayer: is the same as the self.layer.

As @sahrens has pointed out, there are several places in react-native-macos where we are modifying a layer backed view. The current implementation relies on several layer prop changes for several features: z-order, rotation, shadows, and clipping. All of these features are critical for react-native platform parity.

I propose that I open a new issue to track a larger refactoring of all these undefined behavior layer changes and annotate the points where we violate it, including this new contribution. I opened this issue: #466 .