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

[fabric] Fixes to core fabric components #2117

Draft
wants to merge 80 commits into
base: main
Choose a base branch
from

Conversation

shwanton
Copy link

@shwanton shwanton commented May 1, 2024

Summary:

Upstream Fabric fixes for Views, Text & TextInput

NOTE: TouchHandler & ScrollView changes will be added once this PR lands.

Test Plan:

Fabric

View

CleanShot.2024-05-01.at.16.53.12.mp4

Paper

View

CleanShot.2024-05-01.at.17.02.56.mp4

Nick Lefever and others added 30 commits April 30, 2024 23:24
Summary:
Fix AppKit exception throws when focusing text inputs by calling becomeFirstResponder directly on the backing text input view.

Making a view first responder has to happen through the window using makeFirstResponder.

Test Plan:
- Run Zeratul with Fabric
- Focus the search text input above the message threads
- Click inside the active message thread to trigger the auto-focus of the composer
- The composer gets focus without AppKit throwing an exception.

 https://pxl.cl/3dVMx

Reviewers: shawndempsey, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D48696690
…Component

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
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
…yout manager

Summary: To render text using an NSTextView, we need to gain access to a fully configured NSTextStorage to configure the text view to render the text with the right configuration.

Test Plan: Tested later in this stack.

Reviewers: shawndempsey, chpurrer, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D49465173

Tasks: T163838519
Summary: Use an NSTextView to render the text in RCTParagraphComponentView so that we would get UX interactions specific to desktop for free (e.g. text selection)

Test Plan:
- Run Zeratul with Fabric enabled
- Check that text is being rendered correctly with the right size and positioning.

{F1097505779}

Reviewers: shawndempsey, chpurrer, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D49465175

Tasks: T163838519
Summary: Override the hitTest and mouseDown handler in `RCTParagraphComponentView` to forward mouse drag events to the underlying NSTextView to get text selection support in Fabric with correct rendering.

Test Plan:
- Run Zeratul with Fabric enabled.
- Select text
 https://pxl.cl/3q3b3

Reviewers: shawndempsey, chpurrer, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D49465174

Tasks: T163838519
…er 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
Summary: See title

Test Plan: Build RNTester

Reviewers: shawndempsey, chpurrer, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D49924996
Summary:
This overrides the correct `hitTest` method to conditionally forward NSResponder events to the component or the underlying native text view to handle onPress and text selection.

RN will use `hitTest:withEvent:` while native components use `hitTest:`. Without overriding the method used by RN, the conditional forwarding is being bypassed and mouse clicks would get forwarded to the underlying native text view component.

Test Plan:
- Allowlist TU for the zeratul_enable_fabric_preferences_surface GK
- Run Zeratul
- Open the settings
- Select *Active status*
- Click on the *Learn more* link

With the fix the link loads in the browser:
 https://pxl.cl/3z70J

Reviewers: shawndempsey, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D50087800

Tasks: T166059211
Summary:
**Context**

- Our Fabric ViewComponentView supports the focus prop but it is not being sent via native props

**Change**

- Add the native prop to `ReactCommon/.../ViewProps.h`
- Updated `focusable` property on `RCTViewComponentView`

Test Plan:
|RNTester - Fabric|
| https://pxl.cl/3wLwF |

|RNTester - Paper|
||

Reviewers: lefever, #rn-desktop

Differential Revision: https://phabricator.intern.facebook.com/D49998664

[upstream][fabric] Add enableFocusRing for Fabric ViewComponentView

Summary:
**Context**

- `focusable` prop was added to Fabric ViewComponent in D49998664
- `enableFocusRing` is available on Paper view

**Change**

- Add the native prop to `ReactCommon/.../ViewProps.h`
- Updated `enableFocusView` property on RCTViewComponentView

Test Plan:
NOTE: Since views are flattened, the focusable view w/ enable focus ring won't be visible till D50102747

|Fabric|
| https://pxl.cl/3z9Wm|

Reviewers: lefever, #rn-desktop

Reviewed By: lefever

Differential Revision: https://phabricator.intern.facebook.com/D50102547
Summary:
**Context**

- Focusable props were added in D49998664 and D50102547
- The ViewComponentView is flattened so when the component had children, the focus view would be flattened away.

**Change**

- Do not flatten the view if `focusable` or `enableFocusView` are props on the Fabric View

Test Plan:
|Fabric|
| https://pxl.cl/3z9X4|

Reviewers: lefever, #rn-desktop

Reviewed By: lefever

Differential Revision: https://phabricator.intern.facebook.com/D50102747
Summary:
On macOS, Core Animation layers have their anchor point set to (0,0) which is the top-left. On iOS the anchor point is set to (0.5, 0.5) which matches the center.

Transforms assigned to views are built based on having the anchor point at the center.

This diff updates the transform matrix to apply to transform from the center.

Test Plan:
Check zoom/rotation animations in Zeratul.

| Before | After |
|--|
|  https://pxl.cl/3G8HG  |  https://pxl.cl/3G8HQ  |

Reviewers: shawndempsey, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D50628807

Tasks: T161413049
…stance

Summary:
Fix AppKit exception throws when blurring text inputs by calling `resignFirstResponder` directly on the backing text input view.
Resigning the first responder state has to happen through the window by calling `[window makeFirstResponder:nil]` which will:
- call `resignFirstResponder` on the current first responder
- if successful, the window will become the first responder

Test Plan:
- Run Zeratul with Fabric
- Focus the search text input above the message threads
- Click inside the active message thread to trigger the auto-focus of the composer and the blur on the search field
- The focused field resigns the first responder status without throwing an exception

 https://pxl.cl/3GvZD

Reviewers: shawndempsey, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D50700782
Summary:
Scroll views set up with an inverted vertical axis can't support view flattening due to the flattening not taking the axis inversion in consideration while repositioning the views.

This diff disables view flattening on the cells of the virtualized list so that the layout would be correct in inverted scroll views when using Fabric.

The change is not being applied to ScrollView directly because we can safely assume that vertical axis inversion will only be enabled on VirtualizedList/FlatList.

Test Plan:
Run Zeratul with Fabric and check that the vertical order of grouped bubble messages is correct.

| Before | After |
|--|
|  {F1136386200}  |  {F1136386364}  |

Reviewers: shawndempsey, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D50846483

Tasks: T167539420
…hit testing

Summary:
Native views use the AppKit api for hit testing which requires the point to be in the superview coordinate system.

This diff updates the hit testing in `RCTViewComponentView` to conditionally converts the point to the target view coordinate system only if the tested view is a react view.

Test Plan:
Run Zeratul with Fabric and select text inside message bubbles. The scroll view being a native view, the hit testing does not require a point conversion. With this change, the text selection works as expected.

| Before | After |
|--|
|  https://pxl.cl/3Mlpb  |  https://pxl.cl/3MllN  |

Reviewers: shawndempsey, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D51129375

Tags: uikit-diff
Summary: Picking out this from D51015931, will import to fbsource. Goal is upgrading visual studio tools on windows from the good ol' ancient vs2017.

Test Plan: CI on D46923145

Reviewers: lyahdav, ericroz

Reviewed By: ericroz

Differential Revision: https://phabricator.intern.facebook.com/D51140141
…ents of VirtualizedList

Summary:
View flattening was already disabled in the cell renderer used by the Virtualized List in this diff D50846483

This diff disables view flattening in the header, footer, empty and spacer cells to fix the layout being broken because of the vertical axis flipping used by the reverse order virtualized list.

Test Plan:
Run Zeratul with Fabric enabled and scroll to the top of a message thread to show the participants summary header.

| Before | After |
|--|
|  {F1145726580}  |  {F1145726618}  |

Reviewers: shawndempsey, chpurrer, #rn-desktop

Reviewed By: chpurrer

Differential Revision: https://phabricator.intern.facebook.com/D51182545

Tasks: T167539420
Summary:
**Context**

- Port Paper TextInput api's to Fabric

https://www.internalfb.com/code/fbsource/[dab91113a70a2f967fa2996f4aca0f49a58aea17]/third-party/microsoft-fork-of-react-native/react-native/Libraries/Text/TextInput/RCTBaseTextInputView.m?lines=440-444

**Change**

- Automatic spelling correction works on Fabric TextInput

Test Plan:
**Enable "Correct Spelling Automatically"**
|https://pxl.cl/3MFjJ| https://pxl.cl/3MFx6|

 {F1146645411}

Reviewers: chpurrer, lefever, #rn-desktop

Reviewed By: chpurrer

Differential Revision: https://phabricator.intern.facebook.com/D51158136
Summary:
**Context**

- Port Paper TextInput api's to Fabric

https://www.internalfb.com/code/fbsource/[25297dddce1b]/third-party/microsoft-fork-of-react-native/react-native-macos/packages/react-native/Libraries/Text/TextInput/RCTBaseTextInputView.mm?lines=500-505

**Change**

- Automatic spell checking works on Fabric TextInput

Test Plan:
**Enable "Check Spelling While Typing"**

https://pxl.cl/3MG01

Reviewers: chpurrer, lefever, #rn-desktop

Reviewed By: chpurrer

Differential Revision: https://phabricator.intern.facebook.com/D51177294
Summary:
**Context**

- Port Paper TextInput api's to Fabric

https://www.internalfb.com/code/fbsource/[25297dddce1b]/third-party/microsoft-fork-of-react-native/react-native-macos/packages/react-native/Libraries/Text/TextInput/RCTBaseTextInputView.mm?lines=507-512

**Change**

- Automatic grammar checking works on Fabric TextInput

Test Plan:
Enable "Check Grammar With Spelling"

https://pxl.cl/3MWj6

{F1146663064}

Reviewers: chpurrer, lefever, #rn-desktop

Reviewed By: chpurrer

Differential Revision: https://phabricator.intern.facebook.com/D51179163
Summary: `BOOL` being a char type, when building with our current BUCK config we need to static cast it to `bool` in Obj-C++ files when assigning those to a `bool`.

Test Plan:
Build Zeratul with BUCK.

```
BUILDING: FINISHED IN 5m 13.7s (100%) 11302/11302 JOBS, 6/11302 UPDATED

BUILD SUCCEEDED
```

Reviewers: shawndempsey

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D51258122
Summary:
**Context**
- Text selection for Fabric should match Paper

**Change**

- Use correct position for selection range
- Maintain cursor position with selection

Test Plan: https://pxl.cl/3NW99

Reviewers: lefever, #rn-desktop

Reviewed By: lefever

Differential Revision: https://phabricator.intern.facebook.com/D51282825
Summary:
Paper was rendering text as AXStaticText. This diff updates the `RCTParagraphComponentView` to propagate the same role in Fabric for text.

This change will allow to select UI elements based on the text contents.

Test Plan:
Use the Accessibility Inspector in Zeratul with Fabric enabled. With the changes the text elements are presented in the a11y hierarchy with AXStaticText:
 {F1162808272}

Reviewers: shawndempsey, chpurrer, #rn-desktop

Reviewed By: chpurrer

Differential Revision: https://phabricator.intern.facebook.com/D51736932

Tasks: T170938725

Tags: uikit-diff
Summary:
Paper was rendering text as AXStaticText. This diff updates the `RCTParagraphComponentView` to propagate the same role in Fabric for text.

This change will allow to select UI elements based on the text contents.

Test Plan:
Use the Accessibility Inspector in Zeratul with Fabric enabled. With the changes the text elements are presented in the a11y hierarchy with AXStaticText:
 {F1162808272}

Reviewers: shawndempsey, chpurrer, #rn-desktop

Reviewed By: chpurrer

Differential Revision: https://phabricator.intern.facebook.com/D51736931

Tasks: T170938725
Summary:
Paper was rendering text as AXStaticText. This diff updates the `RCTParagraphComponentView` to propagate the same role in Fabric for text.

This change will allow to select UI elements based on the text contents.

Test Plan:
Use the Accessibility Inspector in Zeratul with Fabric enabled. With the changes the text elements are presented in the a11y hierarchy with AXStaticText:
 {F1162808272}

Reviewers: shawndempsey, chpurrer, #rn-desktop

Reviewed By: chpurrer

Differential Revision: https://phabricator.intern.facebook.com/D51736933

Tasks: T170938725
Summary:
Paper was rendering text as AXStaticText. This diff updates the `RCTParagraphComponentView` to propagate the same role in Fabric for text.

This change will allow to select UI elements based on the text contents.

Test Plan:
Use the Accessibility Inspector in Zeratul with Fabric enabled. With the changes the text elements are presented in the a11y hierarchy with AXStaticText:
 {F1162808272}

Reviewers: shawndempsey, chpurrer, #rn-desktop

Reviewed By: chpurrer

Differential Revision: https://phabricator.intern.facebook.com/D51736954

Tasks: T170938725
Summary:
The multiline text input view on macOS needs its own view hierarchy, wrapping the RCTUITextView in a scroll view to support all the features offered by the React TextInput component.

This diff adds a wrapper class for RCTUITextView that provides the appropriate view hierarchy while still supporting the text input protocols required for text input.

The wrapper forwards all unimplemented methods to the RCTUITextView so that it can be used as a direct substitute for the RCTUITextView. This allows us to reduce the custom changes need for macOS in RCTTextInputComponentView while re-using all the logic in RCTUITextView.

Test Plan: Tested later in this stack.

Reviewers: shawndempsey, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D51962394

Tasks: T167538822, T157889591

Tags: uikit-diff
Summary:
Add a `responder` property to support assigning the first responder to the actual textfield/textview if the view is wrapped or not.

The wrapped text view already implements this property. This diff brings the same functionality to the text field and declares it on the common protocol.

Test Plan: Tested later in this stack.

Reviewers: shawndempsey, chpurrer, #rn-desktop

Reviewed By: shawndempsey, chpurrer

Differential Revision: https://phabricator.intern.facebook.com/D51962395

Tasks: T167538822, T157889591
Summary:
We're using wrapped text views with method forwarding, which enables a view to have fully supported text input interfaces.

The text input copy helper method signature was limiting its use to RCTUITextView. To support wrapped text views the typing was changed to RCTPlatformView.

All properties used in the implementation of the copy method are declared on RCTBackedTextInputViewProtocol.

Test Plan: Tested later in this stack.

Reviewers: shawndempsey, chpurrer, #rn-desktop

Reviewed By: shawndempsey, chpurrer

Differential Revision: https://phabricator.intern.facebook.com/D51962397

Tasks: T167538822, T157889591
Summary:
This diff updates the core TextInput RN component to use the wrapped text view for multiline TextInput.

Switching to `RCTWrappedTextView` enables correct `borderWidth` and `contentInsets` support for multi line text inputs while maintaining the same functionality for single line text input.

Scrolling text views are also supported correctly, with vertical height dependent scrollers.

Test Plan:
- Build Zeratul with Fabric enabled.
- Type in the search field to test the layout of the text contents
- Type in the composer to test multi line support and the layout of the text contents

| Before | After |
|--|
|  https://pxl.cl/3Xrrx  |  https://pxl.cl/3Xrr9  |

Reviewers: shawndempsey, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D51962396

Tasks: T167538822, T157889591
Nick Lefever and others added 9 commits April 30, 2024 23:24
Summary:
This diff adds support for double click event handling to the `View` component by overriding the `mouseUp` NSResponder method and track when a double click happened for a component that has JS double click handler set.

The mouse event generation method was refactored to use an enum for the event type to support enter/leave/double-click event emissions.

Test Plan:
* Run Zeratul with Fabric enabled
* Double click on the window drag handler area on the top of the window
* The JS-based window zoom action should be triggered

 https://pxl.cl/4nwrl

Reviewers: shawndempsey, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D54007810
…tsUpdateLayer

Summary: As title

Test Plan: TBD I believe this is tested after cloning changes to fbsource and then testing there?

Reviewers: shawndempsey, lefever, helenistic

Reviewed By: lefever

Differential Revision: https://phabricator.intern.facebook.com/D53783607

Tasks: T163838856
Summary: See title.

Test Plan: Tested later in this stack.

Reviewers: shawndempsey, chpurrer, #rn-desktop

Reviewed By: chpurrer

Differential Revision: https://phabricator.intern.facebook.com/D54045718
Summary: This diff adds the macOS `tooltip` property to the View component and assigns the provided text to the native NSView `toolTip` property.

Test Plan:
* Run Zeratul with Fabric enabled
* Hover over buttons and check that a tool tip is displayed.

 https://pxl.cl/4nM7h

Reviewers: shawndempsey, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D54045717
Summary:
This diff adds the `cursor` prop to the View component in Fabric.

The supported cursor types are converted to the `facebook::react::Cursor` type, supported the same set of values in Paper for the same prop.

Test Plan: Tested later in this stack.

Reviewers: shawndempsey, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D54098203

Tasks: T157889735
Summary: This diff implements the conversion of the react cursor value to a native `NSCursor` instance which is used by the native view to update the current cursor for the surface covered by the view bounds.

Test Plan:
* Run Zeratul with Fabric enabled
* Move the cursor to the vertical splitter
* Check that the cursor changes depending on the possible resize direction.

 https://pxl.cl/4p28W

Reviewers: shawndempsey, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D54098204

Tasks: T157889735
Summary:
**Context**

- TextInput focus prop was not wired up for Fabric

**Change**

- Focus the text input if `autofocus={true}`

Test Plan: Tested in top of stack

Reviewers: lefever, #rn-desktop

Reviewed By: lefever

Differential Revision: https://phabricator.intern.facebook.com/D54880714
Summary:
This diff resets the string content assigned to the internal text view on recycle for the Text component.

Because the text view update exits early when no text storage is defined on the state of the component, the previous text would still be set on the text view and displayed when using a recycled component view with undefined content assigned to it.

By resetting the text view to an empty string on recycle, that edge case will display an empty text component when being assigned undefined content.

Test Plan:
- Run Cosmo Studio and open the UI Reference
- Update an example to include a Text component with undefined content
- Check that no text is being rendered

| Before | After |
|--|
|  https://pxl.cl/4zTW2  |  {F1473555501}  |

Reviewers: shawndempsey, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D55368305

Tasks: T170085811
Comment on lines 55 to 61
@interface RCTParagraphComponentView () <NSTextViewDelegate>
@end
#endif // macOS]

#if !TARGET_OS_OSX // [macOS]
@interface RCTParagraphComponentView () <UIEditMenuInteractionDelegate>

@property (nonatomic, nullable) UIEditMenuInteraction *editMenuInteraction API_AVAILABLE(ios(16.0));

@end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer the code blocks that are the same class to be in the same ifdef group. Something like:

Suggested change
@interface RCTParagraphComponentView () <NSTextViewDelegate>
@end
#endif // macOS]
#if !TARGET_OS_OSX // [macOS]
@interface RCTParagraphComponentView () <UIEditMenuInteractionDelegate>
@property (nonatomic, nullable) UIEditMenuInteraction *editMenuInteraction API_AVAILABLE(ios(16.0));
@end
#endif // macOS]
#if !TARGET_OS_OSX // [macOS]
@interface RCTParagraphComponentView () <UIEditMenuInteractionDelegate>
@property (nonatomic, nullable) UIEditMenuInteraction *editMenuInteraction API_AVAILABLE(ios(16.0));
@end
#else // [macOS
@interface RCTParagraphComponentView () <NSTextViewDelegate>
@end
#endif //macOS]

Comment on lines +37 to +45
validKeysDown(
CoreFeatures::enablePropIteratorSetter
? sourceProps.validKeysDown
: convertRawProp(context, rawProps, "validKeysDown", sourceProps.validKeysDown, {})),
validKeysUp(
CoreFeatures::enablePropIteratorSetter
? sourceProps.validKeysUp
: convertRawProp(context, rawProps, "validKeysUp", sourceProps.validKeysUp, {})),
Copy link
Collaborator

Choose a reason for hiding this comment

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

uuuuuh any chance we can not use validKeysDown 😅

});
}
#endif // macOS]

Copy link
Author

Choose a reason for hiding this comment

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

These platform specific files were refactored in core. If we rebase on 0.74, I can move them to their correct folders.
facebook#41600
packages/react-native/ReactCommon/react/renderer/components/textinput/platform/macos/react/renderer/components/iostextinput/...

Comment on lines +154 to +157
inline NSAccessibilityRole RCTUIAccessibilityRoleFromAccessibilityTraits(
facebook::react::AccessibilityTraits accessibilityTraits)
{
using AccessibilityTraits = facebook::react::AccessibilityTraits;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably align to the w3c-compliant roles that @FalseLobster updated our paper implementation to:
#2101

Nick Lefever and others added 3 commits May 1, 2024 15:45
… for View component

Summary:
This diff fixes an invariant that wasn't valid anymore after having enabled `wantsUpdateLayer` statically for the View component in Fabric.

`RCTUIView` in RCTUIKit enables `wantsUpdateLayer` only if the instance implements the `displayLayer:` method. Because the View component always wants to have `wantsUpdateLayer` enabled, the assumption that `displayLayer:` exists wasn't valid anymore.

This diff only calls the `displayLayer:` method if the instance effectively responds to it. To avoid a perf hit on the check, we only test for it in the initialization and cache the result.

Test Plan:
* Run the Cosmo app
```
~/fbsource/xplat/arfx/cosmo/mac/run.sh
```

| Before | After |
|--|
|  {F1460101180}  |  {F1460101226}  |

Reviewers: shawndempsey, jorgecab, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D54090975

# Conflicts:
#	packages/react-native/React/Base/macOS/RCTUIKit.m
@shwanton shwanton force-pushed the shwanton/fabric-core-fixes branch from 909c136 to e722e95 Compare May 1, 2024 23:01
[self addSubview:_scrollView];

// a register for those notifications on the content view.
#if !TARGET_OS_OSX // [macOS]
Copy link
Author

Choose a reason for hiding this comment

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

Remove mobile code since this file is macOS only. @lenaic should we nest these files in a macos specific folder?

Copy link

Choose a reason for hiding this comment

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

The file is macOS specific, it could be placed in its own macOS subfolder to make it more obvious.

@shwanton shwanton force-pushed the shwanton/fabric-core-fixes branch from e722e95 to b991978 Compare May 1, 2024 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants