Skip to content
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

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

Closed
wants to merge 35 commits into from

Conversation

chrispader
Copy link

@chrispader chrispader commented Nov 16, 2023

Summary:

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

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 and others added 30 commits March 15, 2023 10:02
Co-authored-by: Janic Duplessis <janicduplessis@gmail.com>
…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
@facebook-github-bot 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 Nov 16, 2023
Copy link

github-actions bot commented Nov 16, 2023

Warnings
⚠️

packages/rn-tester/js/examples/TextInput/TextInputExample.android.js#L443 - packages/rn-tester/js/examples/TextInput/TextInputExample.android.js line 443 – 'examples' is already declared in the upper scope on line 155 column 7. (no-shadow)

Generated by 🚫 dangerJS against 94e9a28

@chrispader
Copy link
Author

Closing this PR since this will most probably not be accepted into the react-native repo as per this discussion

@chrispader chrispader closed this Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants