From ba0ed56d8c6bd05d1c28f165a05edf310b463006 Mon Sep 17 00:00:00 2001 From: Nick Lefever Date: Fri, 8 Sep 2023 04:22:54 +0200 Subject: [PATCH 1/3] [fabric] Fix hit testing calls going through LegacyViewManagerInteropComponent Summary: Hit testing in RN macOS uses the view coordinate space instead of the macOS superview coordinate space. The RCTView hitTest implementation gets called from Fabric when Paper components are loaded through the `LegacyViewManagerInteropComponent`. When the Paper component has Fabric child components, the hitTest implementation would treat those as native macOS views. This diff adds a selector check to detect Fabric RCTViewComponentView instances and apply the right point conversion. The selector check allows to have the right behavior without having to import Fabric classes in Paper code. Test Plan: - Run Workplace Chat with Fabric enabled - Click on a thread title in the thread list - With the fix, the thread gets selected Reviewers: shawndempsey, ericroz, chpurrer, #rn-desktop Reviewed By: shawndempsey, ericroz Differential Revision: https://phabricator.intern.facebook.com/D49083593 Tasks: T163162857 --- packages/react-native/React/Views/RCTView.m | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/react-native/React/Views/RCTView.m b/packages/react-native/React/Views/RCTView.m index deff71bc2c1bba..9fc3fbf413895d 100644 --- a/packages/react-native/React/Views/RCTView.m +++ b/packages/react-native/React/Views/RCTView.m @@ -256,9 +256,11 @@ - (RCTPlatformView *)hitTest:(CGPoint)point withEvent:(UIEvent *)event // [macOS #if !TARGET_OS_OSX // [macOS] pointForHitTest = [subview convertPoint:point fromView:self]; #else // [macOS - if ([subview isKindOfClass:[RCTView class]]) { + // Paper and Fabric components use the target view coordinate space for hit testing + if ([subview isKindOfClass:[RCTView class]] || [subview respondsToSelector:@selector(updateProps:oldProps:)]) { pointForHitTest = [subview convertPoint:point fromView:self]; } else { + // Native macOS views require the point to be in the super view coordinate space for hit testing. pointForHitTest = point; } #endif // macOS] From e6655507185b3988d2f4ab4828d1e107cf44c247 Mon Sep 17 00:00:00 2001 From: Nick Lefever Date: Wed, 13 Sep 2023 20:27:11 +0200 Subject: [PATCH 2/3] [fabric] Add support for clipsToBounds to RCTViewComponentView Summary: This diff conditionally sets the CALayer masksToBounds based on the clipsToBounds RN property so that the content of the view would be clipped by the view border. Test Plan: - Run Zeratul with Fabric enabled - Check that the profile pictures with no status indicator are clipped by the half size border radius set on the parent view. | Before | After | |--| | {F1090251649} | {F1090249259} | Reviewers: shawndempsey, chpurrer, #rn-desktop Reviewed By: chpurrer Differential Revision: https://phabricator.intern.facebook.com/D49239973 --- .../Mounting/ComponentViews/View/RCTViewComponentView.mm | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm b/packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm index d3a5748b4ecc41..1f5dbd7f8f7e91 100644 --- a/packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm +++ b/packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm @@ -862,6 +862,11 @@ - (void)invalidateLayer return; } +#if TARGET_OS_OSX // [macOS + // clipsToBounds is stubbed out on macOS because it's not part of NSView + layer.masksToBounds = self.clipsToBounds; +#endif // macOS] + const auto borderMetrics = _props->resolveBorderMetrics(_layoutMetrics); // Stage 1. Shadow Path From 5aacc3eb95f00829c5929d03fcce3099f23293c5 Mon Sep 17 00:00:00 2001 From: Nick Lefever Date: Wed, 4 Oct 2023 04:12:25 +0200 Subject: [PATCH 3/3] [fabric] Fix events being dispatched to wrong react component for Paper components Summary: The tag value is stored in the `reactTag` property on macOS. The adapter used by the RCTLegacyViewManagerInteropComponentView was being provided the `tag` property value which set all interceptors to tag -1 leading to incorrect event dispatching on the JS side. Test Plan: - Open the settings pane in Zeratul and select the appearance tab. - Toggle the theme PopUpButton With the fix, the theme changes while before the zoom would change. https://pxl.cl/3vZRd Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D49897662 --- .../RCTLegacyViewManagerInteropComponentView.mm | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/react-native/React/Fabric/Mounting/ComponentViews/LegacyViewManagerInterop/RCTLegacyViewManagerInteropComponentView.mm b/packages/react-native/React/Fabric/Mounting/ComponentViews/LegacyViewManagerInterop/RCTLegacyViewManagerInteropComponentView.mm index 0d6f570477c32c..1578b00fa31025 100644 --- a/packages/react-native/React/Fabric/Mounting/ComponentViews/LegacyViewManagerInterop/RCTLegacyViewManagerInteropComponentView.mm +++ b/packages/react-native/React/Fabric/Mounting/ComponentViews/LegacyViewManagerInterop/RCTLegacyViewManagerInteropComponentView.mm @@ -16,6 +16,10 @@ #import #import "RCTLegacyViewManagerInteropCoordinatorAdapter.h" +#if TARGET_OS_OSX // [macOS +#import +#endif // macOS] + using namespace facebook::react; static NSString *const kRCTLegacyInteropChildComponentKey = @"childComponentView"; @@ -210,8 +214,14 @@ - (void)finalizeUpdates:(RNComponentViewUpdateMask)updateMask }; if (!_adapter) { +#if !TARGET_OS_OSX // [macOS] _adapter = [[RCTLegacyViewManagerInteropCoordinatorAdapter alloc] initWithCoordinator:[self _coordinator] reactTag:self.tag]; +#else // [macOS + _adapter = [[RCTLegacyViewManagerInteropCoordinatorAdapter alloc] initWithCoordinator:[self _coordinator] + reactTag:self.reactTag.integerValue]; +#endif // macOS] + _adapter.eventInterceptor = ^(std::string eventName, folly::dynamic event) { if (weakSelf) { __typeof(self) strongSelf = weakSelf;