-
Notifications
You must be signed in to change notification settings - Fork 149
[DRAFT] fix(fabric, textinput): support some macOS only TextInput props #2675
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
base: main
Are you sure you want to change the base?
Conversation
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: 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
Summary: Pressing the escape key down in a TextInput on macOS will be captured on the native side as a cancel event. This diff adds support for sending the "Escape" key as a key pressed event and wires the delegate method of the backing text input to call the key press event handler with the correct payload. Test Plan: - Run Zeratul with Fabric enabled - Add a key press handler on the textfield logging the received data (e.g. on MDSTextInput) - Focus the textfield - Press the escape key - The key press handler should be called and print that the "Escape" key was pressed. https://pxl.cl/3XHR2 Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D52005857 Tasks: T167538822
Summary: This diff implements the scrollViewDidScroll delegate method call on the mutli line text input (`RCTWrappedTextView`). The text metrics are updated with the right values read directly from the emitting scroll view so that the TextInput would submit the right payload for scrolling multi line TextInput components at all times. Test Plan: - Run Zeratul with Fabric enabled - Log the onScroll callback data for TextInput used by the composer (e.g. MDSTextInput) - Type a message spanning over multiple lines in the composer until a scroll bar is visible - Scroll the composer contents - The logging shows the text metrics provided as payload for the onScroll handler. https://pxl.cl/3XHRl Reviewers: shawndempsey, chpurrer, #rn-desktop Reviewed By: chpurrer Differential Revision: https://phabricator.intern.facebook.com/D52005859 Tasks: T167538822
Summary: When text completions or special character symbols are displayed ahead of time, the content is present in the native text view attributed string. If a state update is applied, the cursor position will not be read as being at the end of the string while typing, which will shift the cursor position after the update. This diff disables native text view content updates while temporary context is displayed to fix the wrong cursor position updates. Test Plan: Run Zeratul with Fabric and type text in the message composer until macOS renders a text completion/suggestion. Keep typing and see if the cursor stays at the end of the string. | Before | After | |--| | https://pxl.cl/49PW4 | https://pxl.cl/49PWh | Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D52787094 Tasks: T174286406
Summary: This diff adds the macOS-only property to the TextInput component called `submitKeyEvents` which allows the user to provide key combinations that will trigger a submit action. This is being used to implement message sending on hitting the return key. Test Plan: Tested later in this stack Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Subscribers: taskcreeper Differential Revision: https://phabricator.intern.facebook.com/D52860413
Summary: This diff implements the key down event processing to check if any of the provided submit key combinations were entered in the TextInput. If a match is found, the submit callback is triggered. Test Plan: Tested later in this stack. Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D52860412
Summary: This diff adds the macOS-only `clearTextOnSubmit` property for the TextInput component. When set to `true`, the TextInput will clear its content after a submit was triggered through the configured `submitKeyEvents` key combinations. Test Plan: - Run Zeratul with Fabric enabled and enter a message in the composer. - Submit the message by pressing the `Return` key. - Check that `Shift + Return` still works without submitting. https://pxl.cl/4bw3D Reviewers: shawndempsey, chpurrer, #rn-desktop Reviewed By: shawndempsey, chpurrer Differential Revision: https://phabricator.intern.facebook.com/D52860414
Summary: This diff adds support for the secure text entry mode of the single line TextInput component. The backing text field is switched for an extension of the NSSecureTextField on macOS. This was implemented on the single line text input implementation in Paper. This logic is now combined with the multi line text input on the RCTTextInputComponentView on Fabric. The secure text field uses a macro define to re-include the text field source files with minor changes, this to avoid code duplication. To support the inclusion magic happening here, the `import` statement of the text field header had to be converted to an `include` so that the macro-toggling-magic of the secure text field would work. Test Plan: - Run Zeratul with Fabric enabled and use the auth screen of Messenger to test the secure text entry with the password field. https://pxl.cl/4c2CD Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D52924481
…ng view Summary: This diff adds the copy of the a11y attributes when switching the TextInput to the secure text entry which triggers the switch to a NSSecureTextField and runs the copy of the NSTextField attributes to the new backing text field. Test Plan: - Run Zeratul with Fabric enabled and inspect the Messenger password field on the auth screen using the Accessability Inspector to check that the attributes are passed down to the native text field. {F1332886925} Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D52924480
[self addSubview:_scrollView]; | ||
|
||
// a register for those notifications on the content view. | ||
#if !TARGET_OS_OSX // [macOS] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is there an iOS diff in a macOS only file?
@@ -38,6 +38,7 @@ NS_ASSUME_NONNULL_BEGIN | |||
@property (nonatomic, assign, readonly) BOOL textWasPasted; | |||
#else // [macOS | |||
@property (nonatomic, assign) BOOL textWasPasted; | |||
@property (nonatomic, readonly) NSResponder *responder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
macOS tag?
#import <React/RCTUITextField.h> | ||
#include <React/RCTUITextField.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate include?
RCTUIView<RCTBackedTextInputViewProtocol> *fromTextInput, | ||
RCTUIView<RCTBackedTextInputViewProtocol> *toTextInput | ||
#else // [macOS | ||
RCTUITextView<RCTBackedTextInputViewProtocol> *fromTextInput, | ||
RCTUITextView<RCTBackedTextInputViewProtocol> *toTextInput | ||
RCTPlatformView<RCTBackedTextInputViewProtocol> *fromTextInput, | ||
RCTPlatformView<RCTBackedTextInputViewProtocol> *toTextInput | ||
#endif // macOS] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably condense this all to RCTPlatformView?
RCTUIView<RCTBackedTextInputViewProtocol> *fromTextInput, | ||
RCTUIView<RCTBackedTextInputViewProtocol> *toTextInput | ||
#else // [macOS | ||
RCTUITextView<RCTBackedTextInputViewProtocol> *fromTextInput, | ||
RCTUITextView<RCTBackedTextInputViewProtocol> *toTextInput | ||
RCTPlatformView<RCTBackedTextInputViewProtocol> *fromTextInput, | ||
RCTPlatformView<RCTBackedTextInputViewProtocol> *toTextInput |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar comment, can condense the diff
#if !TARGET_OS_OSX // [macOS] | ||
RCTUIView<RCTBackedTextInputViewProtocol> *backedTextInputView = multiline ? [RCTUITextView new] : [RCTUITextField new]; | ||
#else // [macOS | ||
RCTUITextView<RCTBackedTextInputViewProtocol> *backedTextInputView = [RCTUITextView new]; | ||
RCTPlatformView<RCTBackedTextInputViewProtocol> *backedTextInputView = multiline ? [RCTWrappedTextView new] : [RCTUITextField new]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can maybe be condensed too
## 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]>
Summary:
Cherry pick some commits from #2117
Test Plan:
Todo