[fabric] Fixes to core fabric components #2117
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
shwanton
force-pushed
the
shwanton/fabric-core-fixes
branch
from
b991978 to
6b0fd9c
Compare
This was referenced Nov 15, 2024
|
These commits will be handled separately, closing. |
This was referenced Sep 4, 2025
Saadnajmi
added a commit
that referenced
this pull request
Sep 11, 2025
## Summary: This is part of a series of PRs where we are cherry-picking fixes from #2117 to update our Fabric implementation on macOS. This is furthermore, a subset of commits from #2675 . This set specifically implements the text selection APIs on TextInput for macOS, along with another set of commits to reduce our diffs in `RCTCopyBackedTextInput` ## Test Plan: Tested the TextInput selection examples in RNTester. All seem to work well. --------- Co-authored-by: Shawn Dempsey <[email protected]> Co-authored-by: Nick Lefever <[email protected]>
Saadnajmi
added a commit
that referenced
this pull request
Sep 19, 2025
Needs #2690 to land first. ## Summary: Implement focus on RCTViewComponentView. Much of the implementation is taken from #1437, #2117 and comparing against `RCTView`. The border path used for `drawFocusRingMask` is the same as what is used for box shadows and cursors. ## Test Plan: The focus loop seems nonexistent on both paper and Fabric in RNTester... but I can verify that calling `ref.current?/.focus()` on a Pressable displays the focus ring
This was referenced Oct 1, 2025
Saadnajmi
added a commit
that referenced
this pull request
Oct 6, 2025
## Summary: Cherry pick a change from #2117 to add tooltip support ## Test Plan: Existing test page works --------- Co-authored-by: Nick Lefever <[email protected]>
Saadnajmi
added a commit
that referenced
this pull request
Oct 7, 2025
## Summary: Cherry pick more commits from #2117 to implement Drag and Drop Support on View. ## Test Plan: Moved the TextInput example (which doesn't work on Fabric yet) to a new page, and added a View example (which does work on Fabric) https://github.com/user-attachments/assets/b00b25fc-24b9-4587-865f-a05f305d0f40 --------- Co-authored-by: Nick Lefever <[email protected]> Co-authored-by: Tommy Nguyen <[email protected]>
This was referenced Oct 7, 2025
Saadnajmi
added a commit
that referenced
this pull request
Oct 17, 2025
## Summary: Cherry pick another change from #2117 around Fabric We actually already had this prop in Paper, but I had removed it in one RNM upgrade. It turns out that onPress / pointer events don't expose a "count" property, so there isnt' an easy way to listen to double click. Adding this back + changes to add to Fabric. ## Test Plan: Event fires in Pressable Feedback example
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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