-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
[TextInput] Add numberOfLines and maximumNumberOfLines props on iOS and fix Android implementation #35703
[TextInput] Add numberOfLines and maximumNumberOfLines props on iOS and fix Android implementation #35703
Conversation
Hi @Szymon20000! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Base commit: 28dfdb2 |
Base commit: 89ef5bd |
PR build artifact for a819cfc is ready. |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
* Sets the number of lines for a `TextInput`. Use it with multiline set to | ||
* `true` to be able to fill the lines. | ||
*/ | ||
numberOfLines?: ?number, |
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 intend for this prop to be replaced by the rows
prop (see above) as per /~https://github.com/necolas/discussions-and-proposals/blob/reduce-fragmentation/proposals/0000-reduce-fragmentation.md#textinput-component
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.
Make sense. Does it mean that I should change the name in this pr or are already working on one yourself?
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.
The rows
prop (alias for numberOfLines
) is already there on L530 ish. It might also make sense to rename (or alias) maximumNumberOfLines
to something like maxRows
.
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.
Is it android only for now? I haven't seen any logic related to rows on ios.
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.
I only see support for maximumNumberOfLines which is not the same. This pr adds support for numberOfLines so if rows hasn't been implemented yet for ios I can rename numberOfLines in the pr to rows.
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.
@necolas are you interested in this pr? Do you want me to rename the prop to rows?
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.
In the JS layer? Yes, I already mentioned that rows
is a thing. See #34488
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.
Ok, then I will adust this pr so it follows the new naming and then ping you here one more time thx.
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.
@necolas I've just renamed it. Sorry for the delay I was super busy with other tasks.
ReactCommon/react/renderer/textlayoutmanager/platform/ios/RCTTextLayoutManager.mm
Outdated
Show resolved
Hide resolved
Libraries/Text/Text.js
Outdated
@@ -228,6 +228,14 @@ const Text: React.AbstractComponent< | |||
|
|||
const _hasOnPressOrOnLongPress = | |||
props.onPress != null || props.onLongPress != null; | |||
let numberOfLinesKey = 'numberOfLines'; |
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 do we need to do this? Can we do it in the native implementation of the fabric view instead of having all these conditions in js?
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.
In Fabric Most of layout logic and props is shared between TextComponent
and TextInputComponen
t on the native side. I didn't wanted to split them and numberOfLines
for some reason works differently on Text
and TextInput
.
ForText
is actually sets maximum height while in TextInput
it's exact height. I can probably change it on paper architecture to use maximumNumberOfLines
on the native side. Let me know if that would be cleaner. :D
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.
Ok, done it this way
#pragma mark - Multiline <TextInput> (aka TextView) specific properties | ||
|
||
RCT_REMAP_VIEW_PROPERTY(dataDetectorTypes, backedTextInputView.dataDetectorTypes, UIDataDetectorTypes) | ||
|
||
RCT_CUSTOM_SHADOW_PROPERTY(maximumNumberOfLines, NSNumber, RCTBaseTextInputShadowView) |
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.
I think you can use something like this instead
RCT_REMAP_SHADOW_PROPERTY(numberOfLines, maximumNumberOfLines, NSInteger) |
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.
Probably dont need the remap version for the first one since it has same name
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.
done
There are also a few flow errors that will need fixing, you can see on CI. |
@@ -218,7 +218,16 @@ - (NSAttributedString *)measurableAttributedText | |||
|
|||
- (CGSize)sizeThatFitsMinimumSize:(CGSize)minimumSize maximumSize:(CGSize)maximumSize | |||
{ | |||
NSAttributedString *attributedText = [self measurableAttributedText]; | |||
NSMutableAttributedString *attributedText = [[self measurableAttributedText] mutableCopy]; |
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.
I think it might be worth adding a comment that explains that ios doesnt support setting fixed number of lines and what we are doing.
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.
Same for the fabric part
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.
Added! Let me know if the comments make sense.
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.
Is there a reason for adding the new lines before the text instead of after? Is there any cases where it would affect the width of the input? Also the paper implementation doesn’t need the extra K char to work? I also wonder if there would be a better character to use which has 0 width
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.
Yes, on fabric for some reason I need to add character with non 0 width. Otherwise it wasn't working properly.
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.
I think adding after should work well too.
I think we have differences now between platforms when it comes to how rows/numberOfLines works. (Tested it all in RNTester today) Android FabricAndroid Fabric rows actually means max number of rows. So it grows when we add line (as long as we don't go over the limit). iOS Fabric & Paperrows means that height is fixed and expressed in lines no matter what text we display. WEB (textarea)rows means that height is fixed and expressed in lines no matter what text we display. Android Paperrows means that height is fixed and expressed in lines no matter what text we display. @necolas I think it would make sense to have |
@@ -734,6 +734,11 @@ public void setNumLines(ReactEditText view, int numLines) { | |||
view.setLines(numLines); | |||
} | |||
|
|||
@ReactProp(name = ViewProps.MAXIMUM_NUMBER_OF_LINES, defaultInt = 1) |
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.
Isn't it better to have -1
(UNSET) as default? It feels a bit weird to me to have 1 max line by default. What happens if someone pass multiline={true}
?
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.
Yes, I'm rewriting android part now as I found few issues with current implementation. Hope to finish it today.
@matinzd What do you think about it? |
fb3b94b
to
9239ee6
Compare
Now both parts should be fine. |
d1b3ecc
to
2b7bda1
Compare
Thanks for working through the feedback so far. We'll need someone from the RN team to review the native code and import the PR |
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 you also add examples in RN Tester of the new props?
Overall looks good, if I understand correctly the changes to Text native component are needed because it is also used by TextInput right?
ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManagerMapBuffer.java
Outdated
Show resolved
Hide resolved
: UNSET; | ||
|
||
int lines = layout.getLineCount(); | ||
if (numberOfLines != UNSET && numberOfLines != 0 && numberOfLines > lines && text.length() > 0) { |
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.
Could we share this logic with TextLayoutManager? Or is this file already kind of just a copy of it?
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.
The whole file is a faster copy of TextLayoutManager that uses C++/Java shared buffer instead of using JNI.
...erer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTTextLayoutManager.mm
Outdated
Show resolved
Hide resolved
|
||
// for some reason a newline on end causes issues with computing height so we add a character | ||
if (text.toString().endsWith("\n")) { | ||
ssb.append("A"); |
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 we append "K\n" like on iOS to avoid having to do this?
ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManagerMapBuffer.java
Outdated
Show resolved
Hide resolved
Also could you rebase to see if it fixes CI, failures look unrelated. |
The changes are needed because Text native component shares few native objects with TextInput. For instance |
…o graphics folder Differential Revision: D45904748 Original commit changeset: a4e666d7c739 Original Phabricator Diff: D45904748 fbshipit-source-id: c2da28836cb51966854c81d4e380a2abeb742cda
Summary: Chronos Job Instance ID: 1125907889493757 Sandcastle Job Instance ID: 36028797978627335 allow-large-files ignore-conflict-markers opt-out-review Differential Revision: D46270525 fbshipit-source-id: 26beec108aa03c970338fdc4561af24798813acf
Summary: Pull Request resolved: facebook#37572 Currently, RNTester was using a completely custom AppDelegate and not leveraging the RCTAppDelegate we use in the OSS. This resulted in a misalignment between the two setups and duplicated work to test stuff internally furst and then in the OSS, with some more time needed to understand why one setup was working and the other wasn't. With this change, we are aligning the two, bringing RNTester closer to the OSS setup. There are still small differences, but we can iterate over those. ## Changelog: [iOS][Changed] - Make RNTester use RCTAppDelegate Reviewed By: cortinico Differential Revision: D46182888 fbshipit-source-id: 7c55b06de1a317b1f2d4ad0d18a390dc4d3356a4
Summary: Pull Request resolved: facebook#37596 This commit is a backport of this [commit](facebook@32327cc) so that we build the right version of hermes in CI. This also bumps the hermes keys to make sure that we are building it properly from now on. ## Changelog: [Internal] - Fix Hermes versioning and bump hermes workspace keys Reviewed By: cortinico Differential Revision: D46225179 fbshipit-source-id: f00c9d20d37f3bd5689e0742063f98e3bfe373c2
Summary: Pull Request resolved: facebook#37614 changelog: [internal] We do not control what `vImageBoxConvolve_ARGB8888` returns, it may return 0. If it does return 0, we will allocate memory chunk of size 0. Yes, malloc will let you do that. Well, it depends on the implementation, but according to the spec it is legal. The only requirement is to by able to call free on that without crash. If `vImageBoxConvolve_ARGB8888` does return 0 and we allocate memory of size 0. Call to `vImageBoxConvolve_ARGB8888` with tempBuffer of size 0 will lead to a crash. [The documentation](https://developer.apple.com/documentation/accelerate/1515945-vimageboxconvolve_argb8888#discussion) for `vImageBoxConvolve_ARGB8888` and tempBuffer states: > To determine the minimum size for the temporary buffer, the first time you call this function pass the kvImageGetTempBufferSize flag. Pass the same values for all other parameters that you intend to use in for the second call. The function returns the required minimum size, which **should be a positive value**. (A negative returned value indicates an error.) The kvImageGetTempBufferSize flag prevents the function from performing any processing other than to determine the minimum buffer size. I think the keyword word there is "should be a positive value". 0 is not a positive value. Reviewed By: javache, yungsters Differential Revision: D46263204 fbshipit-source-id: baa8fac5b3be6fb5bed02800cd725cc4cf43485a
…targets (facebook#37581) Summary: In Xcode projects with multiple targets, and in particular when targets are for different platforms (e.g. iOS and macOS), Cocoapods may add a suffix to a Pod name like `React-Core`. When this happens, the code in `new_architecture.rb` (which was looking for a pod with exact name `React-Core`) would not add the preprocessor definitions for Fabric as expected. This change fixes this issue. Fixes facebook#37102 . ## Changelog: [iOS] [Fixed] - Fix Fabric issue with React-Core pod when Xcode project has multiple targets Pull Request resolved: facebook#37581 Test Plan: Tested that this change fixes this issue which occurs 100% of the time in React Native TV projects. Reviewed By: dmytrorykun Differential Revision: D46264704 Pulled By: cipolleschi fbshipit-source-id: 8dfc8e342b5a110ef1f028636e01e5c5f2b6e2f0
Summary: This (`greet.yml`) action requires some _changes_ to "repo" settings which are _out of maintainers' controls_. So, instead of wasting compute; let's just delete this action :( 🤗🌏 ## Changelog: [GENERAL] [REMOVED] - [Actions] Remove `greet.yml` action Pull Request resolved: facebook#37587 Test Plan: - Not needed. Reviewed By: cortinico Differential Revision: D46275607 Pulled By: cipolleschi fbshipit-source-id: 80880568cbae1158006445e078e638e4e375cb73
Summary: Pull Request resolved: facebook#37616 Add tests in CircleCI to avoid that changes in Hermes can break the Xcode integration. ## Changelog: [Internal] - Add CI tests to avoid to break the Hermes-Xcode integration Reviewed By: cortinico Differential Revision: D46265656 fbshipit-source-id: 176f1a33fe6944a68fb14b62e5c626fe30d296cc
Summary: Pull Request resolved: facebook#37410 Incremental adoption of new bridgeless API's, where they are semantically equivalent to the old one. Changelog: [Internal] Reviewed By: christophpurrer Differential Revision: D45736529 fbshipit-source-id: e41f6840e7c329f6051530e53773fae760584842
1d9ae2c
to
908eeca
Compare
@NickGerleman Just added code in js that checks if maximumNumberOfLines prop is define on the native side if not then it uses the previous name. |
Please split this into different PRs, as per the last comment. This change is too large to review as-is. |
Sure, Would it be enough to split it into 2 pr android and iOS? |
Just wanted to keep your morale high by saying thanks for hanging in there @Szymon20000. It's rough going through all the steps after doing so much already. I sincerely appreciate it! This is going to be an amazing feature, I currently use weird workarounds for. |
I would recommend as granular as possible. iOS/Android is a good split, especially since there are folks on the React Native team with expertise in one, but not the other. Another split that is common (depending on how much is shared vs not) is splitting between Paper and Fabric. |
@NickGerleman Here is the iOS only pr #38021 |
...ive/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java
Outdated
Show resolved
Hide resolved
…react/views/textinput/ReactTextInputManager.java Co-authored-by: Jakub Piasecki <jakubpiasecki67@gmail.com>
This pr turned out to be too complex to review it in one go so I extracted iOS part and moved here -> |
Summary
We want to be able to set height of TextArea/TextInput/EditText components on all platforms in the same way. Was is currently missing on iOS is expressing the height of a component in lines and this pr is supposed to fix it. Additionally in some cases we want to set maximum number of lines a "TextArea" like component can have. Because of Different screen sizes and accessibility settings it's sometimes hard to compute how many lines a current content takes so the purpose of maximumNumberOfLines prop is to simplify that process.
iOS How it was implemented?
On Android the native "TextArea" like component (EditText) exposes setLines method that sets height of the component in lines. However, I wasn't able to find anything similar on iOS. What is a common method for both platforms is setting maximum height in lines. On iOS we can set .maximumNumberOfLines property on TextContainer and similarly on Android we can call setMaxLines() method to limit number of lines. So Adding maximumNumberOfLines is not a problem.
I realised that when we add enough number of newLines in order to implement numberOfLines prop using maximumNumberOfLines functionality and did exactly that. Probably it's possible to get a font size and based on that set exact height but I didn't want to miss any edge cases and adding few newlines doesn't seem to be an overhead at all.
Also probably otherwise we would need to take into account things like space between lines, padding...
Android
It turned out that Android implementation has some issues on Paper and on Fabric the behaviour is shared with TextComponent which limits number of lines instead of setting exact height. So I rewrote the android implementation too.
Fabric
I implemented numberOfLines and maximumNumberOfLines similarly to what I did for paper architecture but it turned out that there is one edge case. Whenever you tapped the enter button it added a new line to the text input. Even though NSTextContainer clearly had set maximumNumberOfLines property. I checked what are differences between paper and Fabric implementation when it comes to using NSTextContainer but I wasn't able to spot any difference. Fonts, spaces, other text attributes were exactly the same for every character. In RN docs I found that TextInput already have support for numberOfLines to I checked if the problem occurs also there. Turned out that problem is there too and moreover it's also in paper architecture. I checked differences on how NSContainer is used and spotted a different order of methods called on NSTextContainer, NSTextStorage and NSLayoutManager. After changing the order the problem is gone everywhere.
Changelog
[ANDROID][FIXED] - fix numberOfLines for TextInput component on Paper
[ANDROID][CHANGED] - change numberOfLines for TextInput component on Fabric so it's consistent with paper implementation and WEB
[ANDROID][ADDED] - add maximumNumberOfLines prop to TextInput
[ISO][ADDED] - add numberOfLines for TextInput component on Paper and Fabric
[IOS][ADDED] - add maximumNumberOfLines prop to TextInput (Paper and Fabric)
Test Plan