facebook
[TextInput] Add numberOfLines and maximumNumberOfLines props on iOS
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
- [ ] test iOS (Paper)
- [ ] test iOS (Fabric)
- [ ] test Android (Paper)
- [ ] test Android (Fabric)
Hi @Szymon20000!
Thank you for your pull request and welcome to our community.
Action Required
In 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.
Process
In 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 CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at [email protected]. Thanks!
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| android | hermes | arm64-v8a | 8,756,411 | -245,054 |
| android | hermes | armeabi-v7a | 8,069,058 | -186,321 |
| android | hermes | x86 | 9,248,684 | -261,828 |
| android | hermes | x86_64 | 9,098,523 | -257,850 |
| android | jsc | arm64-v8a | 9,318,059 | -296,472 |
| android | jsc | armeabi-v7a | 8,508,012 | -233,019 |
| android | jsc | x86 | 9,381,231 | -320,156 |
| android | jsc | x86_64 | 9,635,243 | -312,704 |
Base commit: 28dfdb22ebaff7055fd8bb4b8847e018181f849d Branch: main
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| ios | - | universal | n/a | -- |
Base commit: 89ef5bd6f9064298dfe55b0b18be4a770ee0872c Branch: main
PR build artifact for a819cfcd599723b82998e96fd9a9113e076847af is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!
There are also a few flow errors that will need fixing, you can see on CI.
I think we have differences now between platforms when it comes to how rows/numberOfLines works. (Tested it all in RNTester today)
Android Fabric
Android 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 & Paper
rows 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. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/textarea#attr-rows
Android Paper
rows means that height is fixed and expressed in lines no matter what text we display. edit: looks like on paper we set fixed height but it behaves more like minRows :D
@necolas I think it would make sense to have rows and maxRows. What do you think?
I think we have differences now between platforms when it comes to how rows/numberOfLines works. (Tested it all in RNTester today)
Android Fabric
Android 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 & Paper
rows 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. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/textarea#attr-rows
Android Paper
rows means that height is fixed and expressed in lines no matter what text we display. edit: looks like on paper we set fixed height but it behaves more like minRows :D
@necolas I think it would make sense to have
rowsandmaxRows. What do you think?
@matinzd What do you think about it?
@necolas @matinzd Here is my fix for android part. https://github.com/facebook/react-native/pull/36394 I will add it to this pr.
Now both parts should be fine. I will add test to RNTester and test it all one more time on {'iOS', 'Android'} x {'paper', 'fabric'} One more thing that's left is updating types.
@matinzd @necolas Could you guys review my changes? I think it's ready :)
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
Also could you rebase to see if it fixes CI, failures look unrelated.
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?
The changes are needed because Text native component shares few native objects with TextInput. For instance ParagraphAttributes or TextLayoutManager. However, numberOfLines prop behaves differently on Text and TextInput. In Text Component it actually works like maximumNumberOfLines while in TextInput it means an exact number.
| Fails | |
|---|---|
| :no_entry_sign: |
:clipboard: Missing Changelog - Please add a Changelog to your PR description. See Changelog format |
| :no_entry_sign: |
packages/react-native/Libraries/Text/Text.js#L21 - packages/react-native/Libraries/Text/Text.js line 21 – Replace |
| Warnings | |
|---|---|
| :warning: | One hour and a half have passed and the E2E jobs haven't finished yet. |
| :warning: |
packages/react-native/Libraries/Text/Text.js#L11 - packages/react-native/Libraries/Text/Text.js line 11 – Requires should be sorted alphabetically (lint/sort-imports) |
| :warning: |
packages/react-native/Libraries/Text/TextNativeComponent.js#L11 - packages/react-native/Libraries/Text/TextNativeComponent.js line 11 – Requires should be sorted alphabetically (lint/sort-imports) |
Generated by :no_entry_sign: dangerJS against bcb467d0d0ed98383566ecf011ca763a70c8c253
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
This work is so cool, I was just hoping to use this. Thanks so much for this hard work @Szymon20000 and team!
@NickGerleman I am very grateful that you took your time to check this pr. 🎉 At the beginning of next week I will correct/check the parts you pointed out. 🫡
@NickGerleman I applied suggestion that you mentioned and answered all questions. Would be great to get another round of review here. I try to reply asap.
@NickGerleman Unfortunately the changes on iOS and android are quite related and it's not easy to split them.
If that's a deal breaker I can add some if-s in Text.js and TextInput.js and other js files so my changes are platform specific. Then split it to iOS and Android prs.
When it comes to splitting js and native changes maybe I can just export
a constant doesNativeContainNewNumberOfLinesImpl to TextInputViewManager and TextViewManager?.
and then on js add proper if-s
That mechanism seems like it should work.
I would prefer that we still split this by implementation, and any of the refactoring/renaming, adding conditions to the JS if needed. Right now there is quite a bit of new logic here, along with refactoring which is a little difficult to follow what comes from what.
@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.
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?
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.
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?
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 https://github.com/facebook/react-native/pull/38021 This one will become android only once the iOS is merged. I still need to wrap some C++ parts in if-s as it's shared between android and iOS but the pr can already be reviewed on iOS.
This pr turned out to be too complex to review it in one go so I extracted iOS part and moved here ->
https://github.com/facebook/react-native/pull/38021
closing the pr so it that it doesn't scare away reviewers :)
I will open another pr with android fixes once the iOS one is merged.