react-native icon indicating copy to clipboard operation
react-native copied to clipboard

[iOS][TextInput]Add numberOfLines and maxNumberOfLines props to TextInput on iOS

Open Szymon20000 opened this issue 2 years ago • 20 comments

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

Szymon20000 avatar Jun 22 '23 13:06 Szymon20000

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

analysis-bot avatar Jun 22 '23 13:06 analysis-bot

@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?

Szymon20000 avatar Jul 08 '23 09:07 Szymon20000

@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 :)

Szymon20000 avatar Jul 24 '23 10:07 Szymon20000

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

github-actions[bot] avatar Aug 07 '23 05:08 github-actions[bot]

@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. :)

Szymon20000 avatar Aug 07 '23 05:08 Szymon20000

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 avatar Aug 22 '23 09:08 cipolleschi

@cipolleschi It's a pr that adds numberOfLines prop on iOS. Android and web already have that prop :)

Szymon20000 avatar Aug 22 '23 14:08 Szymon20000

Additionally, I'm adding a prop that let people set maximum height expressed in lines

Szymon20000 avatar Aug 22 '23 14:08 Szymon20000

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.

cipolleschi avatar Aug 23 '23 11:08 cipolleschi

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...

Szymon20000 avatar Aug 24 '23 11:08 Szymon20000

@cipolleschi Those lines are only added on layout stage and are not visible on the screen.

Szymon20000 avatar Aug 24 '23 11:08 Szymon20000

https://reactnative.dev/docs/textinput#numberoflines-android That's the prop that I implemented on iOS in this pr

Szymon20000 avatar Aug 24 '23 11:08 Szymon20000

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?

cipolleschi avatar Aug 24 '23 13:08 cipolleschi

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. :)

Szymon20000 avatar Aug 24 '23 14:08 Szymon20000

https://github.com/facebook/react-native/pull/35703 <- here is there previous pr that I just split as @NickGerleman asked me to do it.

Szymon20000 avatar Aug 24 '23 14:08 Szymon20000

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.

Szymon20000 avatar Aug 24 '23 14:08 Szymon20000

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.

cipolleschi avatar Aug 24 '23 15:08 cipolleschi

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

chrispader avatar Nov 16 '23 15:11 chrispader

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.

Noitidart avatar Nov 16 '23 16:11 Noitidart

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.

github-actions[bot] avatar May 15 '24 05:05 github-actions[bot]

This PR was closed because it has been stalled for 7 days with no activity.

github-actions[bot] avatar May 22 '24 05:05 github-actions[bot]