react-native
react-native copied to clipboard
[iOS][TextInput]Add numberOfLines and maxNumberOfLines props to TextInput on iOS
This pull-request adds rows, numberOfLines props to TextInput component on iOS so that it we can use it on all platforms. Additionally, the adds maxNumberOfLines prop that lets people express maximumHeight in lines (which is useful due to multithreaded react native architecture that can cause some delays caused by scheduling)
The main idea in this pr is to make use of maximumNumberOfLines property on NSTextContainer. As we don't have any native support for setting exact height expressed in lines I came up with the idea that we can add some lines of text at the layout stage what will let us implement numberOfLines using maximumNumberOfLines. The lines are not added in component that are actually displaying text so that it's not visible on the screen but is using on layout layer so ShadowNodes and TextLayoutManagers. Later similar idea was implemented on Android here( https://github.com/facebook/react-native/pull/35703) but I split the implementation to simplify the review.
So you can review this pr without a need to get familiar with original pr.
This pr was created to simplify review and contains only iOS changes from https://github.com/facebook/react-native/pull/35703
Summary:
Changelog:
[iOS] [Added] - Added support for numberOfLines/rows to TextInput on iOS platform (Fabric and Paper) [iOS] [Added] - Added MaxNumberOfLines prop to the same component that sets upper bound instead of exact value
Test Plan:
I added few Texts to RN tester
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| android | hermes | arm64-v8a | 8,894,103 | +1,157 |
| android | hermes | armeabi-v7a | 7,943,165 | +1,483 |
| android | hermes | x86 | 9,291,808 | +1,100 |
| android | hermes | x86_64 | 9,193,455 | +1,100 |
| android | jsc | arm64-v8a | 9,486,361 | +822 |
| android | jsc | armeabi-v7a | 8,428,381 | +1,145 |
| android | jsc | x86 | 9,470,232 | +777 |
| android | jsc | x86_64 | 9,784,606 | +773 |
Base commit: f9a63ec005184b3d59c59d3810ca08c4f47c8fbf Branch: main
@sammy-SC @NickGerleman I just fixed CI Tests. Sorry for not noticing it earlier. Is there anything I can do to make the review easier?
@NickGerleman @sammy-SC I know you have plenty of work and this pr may seems to be a bit complex at first but would really appreciate if you could take a look :)
| Warnings | |
|---|---|
| :warning: |
packages/rn-tester/js/examples/TextInput/TextInputExample.android.js#L445 - packages/rn-tester/js/examples/TextInput/TextInputExample.android.js line 445 – 'examples' is already declared in the upper scope on line 157 column 7. (no-shadow) |
Generated by :no_entry_sign: dangerJS against 018b0e91305864a82d70b61678d5963cadf23b50
@sammy-SC I just merged recent changes on Main so there is no merge conflicts. I also added descriptions the pr so that you don't need to read original pr. Please, let me know what you thing/how I can help. :)
I'm sorry, but what is the final goal here? By reading the comments it seems like that you'd like to add the Maximum Number of Rows so that you can add some padding at the beginning for layout purposes. Is that right?
If that's the case, I don't think that this is the right solution for your problem. We shouldn't introduce additional props and logic in the the semantic of a component (handling text) to get a final result on a different layer (layouting)
@cipolleschi It's a pr that adds numberOfLines prop on iOS. Android and web already have that prop :)
Additionally, I'm adding a prop that let people set maximum height expressed in lines
The problem is here. We cannot accept such logic that arbitrarily adds lines to a component. It will break a lot of internal tests.
That's why I was asking what's the problem you are trying to solve. If we know that, we can guide you to the right solution.
The goal is to add numberOfLines support to TextInput component on iOS. On Android and Web we can set exact height in lines using exactly this prop. On iOS it's not possible.
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...
@cipolleschi Those lines are only added on layout stage and are not visible on the screen.
https://reactnative.dev/docs/textinput#numberoflines-android That's the prop that I implemented on iOS in this pr
I understand, but why do you need the same behavior? The two platforms behaves differently natively and that's the strength of React Native. As I said, we cannot import this PR lightly as it will break internal tests which sets that property and expect iOS not to show multiple lines and it will break also all the product outside Meta which are built with that assumption.
On top of that, the maximumNumberOfLines is a confusing name: I tried on Android and the numberOfLines is not a maximum. If we set 5, for example, the app still alllow us to input more than 5 lines, so, effectively, the two platforms will still have different semantics.
I think that this change arrives because of some product need, where there is a text field that needs a certain height to accomodate some user input, right? Why can't the product change the layout code (with flexlayout) so that it occupies a portion of the screen that it's roughly the amount of space you need?
maximumNumberOfLines is a second props I add here as a bonus. But the main goal is numberOfLines.
My initial pr adds the same prop on android and fixes Fabric implementation. The other strength of React native is that you can write code that will work on multiple platforms so that's why want to be able to use numberOfLines prop on Android, Web and iOS. :)
https://github.com/facebook/react-native/pull/35703 <- here is there previous pr that I just split as @NickGerleman asked me to do it.
Can you explain how the pr can break internal tests? It only has impact on dimensions returned by yoga and then on frame size on UIViews.
Because we have screenshot tests: we have a reference file that states the expected result. We run the test and we generate a screenshot of the state of the app after the change. We compare the expected result with the actual result and if they differ, it is a failure.
Imagine that we have a JS file used both by iOS and Android, which sets the numberOfLines to 5.
Before, that prop was ignored on iOS, so the screenshot would end up having a TextInput that occupies the space of a single line.
After, iOS will not ignore that property anymore, and the generated screenshot will occupy the space of 5 lines, breaking the test for iOS.
We are not the only company having screenshot tests and this change will also mess up with the design of apps which were designed considering a single line for iOS.
I just created a RFC discussion on this here. Could everyone who's involved in this change give it a look? :)
@Noitidart i think you were interested in this feature too
I just created a RFC discussion on this here. Could everyone who's involved in this change give it a look? :)
@Noitidart i think you were interested in this feature too
Yes absolutely still interested will take a look thank you.
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.
This PR was closed because it has been stalled for 7 days with no activity.