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

Szymon20000 · 2023-06-22T13:01:45Z

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( #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 #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

Co-authored-by: Janic Duplessis <janicduplessis@gmail.com>

…ines_rn

…acebook#37609) Summary: Pull Request resolved: facebook#37609 changelog: [internal] Moving LayoutMetrics and LayoutPrimitives from core to graphics module. This is to enable different implementation for different platforms. Reviewed By: rubennorte Differential Revision: D45904748 fbshipit-source-id: a4e666d7c7390e87abdb09235f96655b63f451f9

…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

…nes_rn

analysis-bot · 2023-06-22T13:27:41Z

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: f9a63ec
Branch: main

Szymon20000 · 2023-07-08T09:44:58Z

@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 · 2023-07-24T10:12:23Z

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

github-actions · 2023-08-07T05:10:36Z

	Warnings
⚠️	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 🚫 dangerJS against 018b0e9

Szymon20000 · 2023-08-07T05:25:49Z

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

cipolleschi · 2023-08-22T09:40:42Z

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)

Szymon20000 · 2023-08-22T14:46:10Z

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

Szymon20000 · 2023-08-22T14:47:08Z

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

cipolleschi · 2023-08-23T11:42:19Z

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.

Szymon20000 · 2023-08-24T11:42:29Z

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 · 2023-08-24T11:47:41Z

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

Szymon20000 · 2023-08-24T11:48:51Z

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

cipolleschi · 2023-08-24T13:08:17Z

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?

Szymon20000 · 2023-08-24T14:55:55Z

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 · 2023-08-24T14:56:34Z

#35703 <- here is there previous pr that I just split as @NickGerleman asked me to do it.

Szymon20000 · 2023-08-24T14:58:20Z

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.

cipolleschi · 2023-08-24T15:05:12Z

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.

chrispader · 2023-11-16T15:01:55Z

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

Noitidart · 2023-11-16T16:18:59Z

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.

github-actions · 2024-05-15T05:01:11Z

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.

Szymon20000 and others added 26 commits March 15, 2023 10:02

android part

c9e11d5

ios changes

e55eed8

update types, configs

cd78e0d

fix NativeTextComponent Props

34acc74

fix fabric issue with newlines

22d7189

Apply suggestions from code review

e5388e1

Co-authored-by: Janic Duplessis <janicduplessis@gmail.com>

add example to rntester

448a5eb

Merge remote-tracking branch 'upstream/main' into @szymon/numner_of_l…

8080b34

…ines_rn

fix text component on android - (is was stipping off natest views)

b58d914

apply suggestions

b29648d

Merge remote-tracking branch 'upstream/main' into @szymon/numner_of_l…

036d5ad

…ines_rn

update rntester examples

24614c6

Merge branch 'main' into @szymon/numner_of_lines_rn

2551355

Revert D45904748: Move LayoutMetrics and LayoutPrimitives from core t…

f77567d

…o graphics folder Differential Revision: D45904748 Original commit changeset: a4e666d7c739 Original Phabricator Diff: D45904748 fbshipit-source-id: c2da28836cb51966854c81d4e380a2abeb742cda

Merge remote-tracking ranch 'upstream/main' into @szymon/numner_of_li…

5a90e0d

…nes_rn

choose proper numberOfLines prop name based on native config

908eeca

remove android changes in java files

6d108f3

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 22, 2023

Szymon20000 mentioned this pull request Jun 22, 2023

[TextInput] Add numberOfLines and maximumNumberOfLines props on iOS and fix Android implementation #35703

Closed

4 tasks

NickGerleman requested a review from sammy-SC June 26, 2023 18:37

Szymon20000 added 5 commits July 8, 2023 10:12

fix flow check

d328d71

fix linting

06a854f

revert weird merge issue

f39c184

fix tests and linting

ef502b6

fix flow again

eccd5a1

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jul 20, 2023

Szymon20000 changed the title ~~Add numberOfLines and maxNumberOfLines props to TextInput on iOS~~ [iOS][TextInput]Add numberOfLines and maxNumberOfLines props to TextInput on iOS Aug 3, 2023

Merge branch 'main' into @szymon/number_of_lines_ios_part

018b0e9

AndrewGable mentioned this pull request Aug 18, 2023

Android build times are very slow Expensify/App#25501

Closed

fabOnReact mentioned this pull request Sep 14, 2023

Upgrade react native to 0.72.4 Expensify/App#27320

Merged

59 tasks

github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Szymon20000 commented Jun 22, 2023 •

edited

analysis-bot commented Jun 22, 2023 •

edited

Szymon20000 commented Jul 8, 2023

Szymon20000 commented Jul 24, 2023

github-actions bot commented Aug 7, 2023 •

edited

Szymon20000 commented Aug 7, 2023

cipolleschi commented Aug 22, 2023

Szymon20000 commented Aug 22, 2023

Szymon20000 commented Aug 22, 2023

cipolleschi commented Aug 23, 2023

Szymon20000 commented Aug 24, 2023

Szymon20000 commented Aug 24, 2023 •

edited

Szymon20000 commented Aug 24, 2023

cipolleschi commented Aug 24, 2023

Szymon20000 commented Aug 24, 2023

Szymon20000 commented Aug 24, 2023 •

edited

Szymon20000 commented Aug 24, 2023

cipolleschi commented Aug 24, 2023

chrispader commented Nov 16, 2023 •

edited

Noitidart commented Nov 16, 2023

github-actions bot commented May 15, 2024

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

Are you sure you want to change the base?

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

Conversation

Szymon20000 commented Jun 22, 2023 • edited

Summary:

Changelog:

Test Plan:

analysis-bot commented Jun 22, 2023 • edited

Szymon20000 commented Jul 8, 2023

Szymon20000 commented Jul 24, 2023

github-actions bot commented Aug 7, 2023 • edited

Szymon20000 commented Aug 7, 2023

cipolleschi commented Aug 22, 2023

Szymon20000 commented Aug 22, 2023

Szymon20000 commented Aug 22, 2023

cipolleschi commented Aug 23, 2023

Szymon20000 commented Aug 24, 2023

Szymon20000 commented Aug 24, 2023 • edited

Szymon20000 commented Aug 24, 2023

cipolleschi commented Aug 24, 2023

Szymon20000 commented Aug 24, 2023

Szymon20000 commented Aug 24, 2023 • edited

Szymon20000 commented Aug 24, 2023

cipolleschi commented Aug 24, 2023

chrispader commented Nov 16, 2023 • edited

Noitidart commented Nov 16, 2023

github-actions bot commented May 15, 2024

Szymon20000 commented Jun 22, 2023 •

edited

analysis-bot commented Jun 22, 2023 •

edited

github-actions bot commented Aug 7, 2023 •

edited

Szymon20000 commented Aug 24, 2023 •

edited

Szymon20000 commented Aug 24, 2023 •

edited

chrispader commented Nov 16, 2023 •

edited