feat(iOS): add all supported ReturnKeyTypes #43362

elencho · 2024-03-07T10:40:04Z

Summary:

Related issue: #43243
As documentation stated for the ReturnKeyType prop on the input there are different options, like "next", "go" and etc. They are actually supported on the iOS side but we can't use it in our react native app, because of the hardcoded version of DoneButton on existing code:

So, I decided to add support for types which were in documentation

Changelog:

[IOS] [ADDED]: ReturnKeyTypes

Test Plan:

ran yarn jest react-native-codegen and yarn jest react-native, both successfully:

analysis-bot · 2024-03-07T11:09:37Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	19,544,176	-15,551
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	22,900,330	-12,606
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: c13790f
Branch: main

facebook-github-bot · 2024-04-25T12:28:58Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cipolleschi

Hi @elencho, thanks for the contribution! I'll import it and see how it behaves internally!

cipolleschi

Sorry for getting back to this after the approval.
A colleague was doing a second pass and he raised some relevant concerns.

I left a couple of suggestions in the code.

Plus, this is a file for the old architecture only. It is great that you fixed this, but could you apply the same changes to this other file, for the New Architecture?

Thank you so much! 🙏

cipolleschi · 2024-04-26T09:24:24Z

packages/react-native/Libraries/Text/TextInput/RCTBaseTextInputView.mm

+  NSArray<NSNumber *> *returnKeyTypes = @[
+        @(UIReturnKeyDone),
+        @(UIReturnKeyGo),
+        @(UIReturnKeyDefault),
+        @(UIReturnKeyNext),
+        @(UIReturnKeySearch),
+        @(UIReturnKeySend),
+        @(UIReturnKeyYahoo),
+        @(UIReturnKeyGoogle),
+        @(UIReturnKeyRoute),
+        @(UIReturnKeyJoin),
+        @(UIReturnKeyRoute),
+        @(UIReturnKeyEmergencyCall)
+  ];


A couple of suggestions here:

Can we create a NSSet<NSNumber *> instead of an NSArray? This will be more efficient when looking up as we won't have to iterate over the array

Can we move this to a static variable out of the implementation?
(before the @implementation keyword on top)?

The reason for 2 is that this Collection will be created and destroyed every time the method setDefaultInputAccessoryView is invoked, so it is not very efficient.

cipolleschi · 2024-05-01T13:07:28Z

@elencho gentle reminder that there are some changes requested.

elencho · 2024-05-01T14:21:24Z

@elencho gentle reminder that there are some changes requested.

Sorry I just saw your comment, will fix in few days and get back to you. Thanks for your feedback!!

…elencho/add_returnKeyTypes

elencho · 2024-05-08T22:05:40Z

NOTE: I removed Default value, because it was causing a bug: when returnKey was undefined from js it was still showing "Default"

cipolleschi · 2024-05-20T16:03:22Z

NOTE: I removed Default value, because it was causing a bug: when returnKey was undefined from js it was still showing "Default"

Why you think it is a bug? I feel like it is the right behavior to fall back to Default if nothing is specified, no?

elencho · 2024-05-21T13:35:06Z

NOTE: I removed Default value, because it was causing a bug: when returnKey was undefined from js it was still showing "Default"

Why you think it is a bug? I feel like it is the right behavior to fall back to Default if nothing is specified, no?

Okay i will revert

facebook-github-bot · 2024-05-30T14:22:38Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cipolleschi · 2024-05-30T14:22:50Z

/rebase - this comment automatically rebase on top of main

elencho · 2024-06-04T10:35:26Z

/rebase - this comment automatically rebase on top of main

is something wrong with the tests? that is blocking the merge

cipolleschi · 2024-06-05T14:41:23Z

@elencho No, nothing wrong.
I have to run the internal linter (which is different from the OSS one, unfortunately) and ask for a review internally.
I'm still catching up from AppJS and the ReactConf, that's why I have been a little slow, these days.

facebook-github-bot · 2024-06-06T16:26:52Z

@cipolleschi merged this pull request in ed9978b.

github-actions · 2024-06-06T16:26:55Z

This pull request was successfully merged by @elencho in ed9978b.

^{When will my fix make it into a release? | How to file a pick request?}

Summary: Related issue: facebook#43243 As [documentation](https://reactnative.dev/docs/textinput#returnkeytype) stated for the ReturnKeyType prop on the input there are different options, like "next", "go" and etc. They are actually supported on the iOS side but we can't use it in our react native app, because of the hardcoded version of DoneButton on existing code: <img width="887" alt="image" src="https://github.com/facebook/react-native/assets/53994979/9ecaf63b-675c-45f0-b737-7ae3e937584a"> So, I decided to add support for types which were in documentation ## Changelog: [IOS] [ADDED]: ReturnKeyTypes <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#43362 Test Plan: ran yarn jest react-native-codegen and yarn jest react-native, both successfully: <img width="420" alt="image" src="https://github.com/facebook/react-native/assets/53994979/c36b61f7-ef45-4062-ac5b-1dd2d0a9e544"> <img width="420" alt="image" src="https://github.com/facebook/react-native/assets/53994979/af83c22c-d110-4c28-94c1-d48ee27bfcfe"> Reviewed By: cortinico Differential Revision: D56571845 Pulled By: cipolleschi fbshipit-source-id: 74dbffb3795ab0b5c9eafa685761c2e770cd5cf9

kirillzyusko · 2024-08-12T13:47:48Z

@elencho @cipolleschi so, I tested [email protected] and... Is there a way to hide this inputAccessoryView? I always have it right now (I specified returnKeyType="none" but it still present):

And presence of two toolbars looks really weird (if I'm using KeyboardToolbar from react-native-keyboard-toolbar):

For me it looks like a breaking change and it prevents me to migrate to 0.75 version 😐

kirillzyusko · 2024-08-12T14:10:41Z

I have a feeling like it had to be removed #43362 (comment)

Because right now when you didn't specify returnTypeKey (i. e. it's undefined) the toolbar always shows "Default" and this button doesn't do anything (i. e. I press, but nothing happens).

I have a strong feeling that it's a regression 😅 Correct me if I'm wrong 😅

elencho · 2024-08-12T18:53:44Z

Hello @kirillzyusko , thank you for feedback.
Just tested and you are right, I would love to contribute again here.
Can I investigate this and come back with fixed solution?

kirillzyusko · 2024-08-13T08:06:25Z

Can I investigate this and come back with fixed solution?

Yes, please 🙏

I also would like to get @cipolleschi opinion on that - whether it should be considered as a release blocker or not and what is the best way to resolve it here.

The straightforward approach is to remove UIReturnKeyDefault from the Set. Or if undefined or null is coming from JS, then do not substitute the value with UIReturnKeyDefault by default and allow that value to be nil, but it may affect other places of the framework (just afraid to get NullPointerException somewhere in the code 😅).

Anyway, it's just suggestions 🙃

cipolleschi · 2024-08-13T10:12:16Z

Hi There, sorry for the issue!

I also would like to get @cipolleschi opinion on that - whether it should be considered as a release blocker or not and what is the best way to resolve it here.

We cannot stop the release, which is happening tomorrow, at this point, especially for this that has not been reported by anyone else.
However, we are planning todo 75.1 next week, so this fix can go in there.

kirillzyusko · 2024-08-13T11:27:47Z

We cannot stop the release, which is happening tomorrow, at this point, especially for this that has not been reported by anyone else.

@cipolleschi got it!

However, we are planning todo 75.1 next week, so this fix can go in there.

That would be amazing to include this fix in 0.75.1 👍 Do you think that removing UIReturnKeyDefault from a set can be included in 0.75.1? Or do you want to have a different fix?

elencho · 2024-08-13T11:31:57Z

@kirillzyusko I will try to include it, but if there wont be another fix I will remove that

cipolleschi · 2024-08-13T15:35:51Z

@elencho just to understand, do you plan to look into this issue?

elencho · 2024-08-13T15:37:36Z

@elencho just to understand, do you plan to look into this issue?

Yes, I will contribute tomorrow

elencho · 2024-08-14T15:09:22Z

New PR link
@cipolleschi @kirillzyusko

feat(ios): add all supported ReturnKeyTypes

3d73a23

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Mar 7, 2024

elencho mentioned this pull request Mar 7, 2024

TextInput with prop keyboardtype number-pad doesnt render returnKeyType next? #43243

Closed

cipolleschi approved these changes Apr 25, 2024

View reviewed changes

cipolleschi requested changes Apr 26, 2024

View reviewed changes

elencho added 3 commits May 3, 2024 12:51

Merge branch 'main' of https://github.com/facebook/react-native into …

6f0e026

…elencho/add_returnKeyTypes

feat(ios): add fabric support

17ea3e1

feat(ios): removed default value

edf5c8b

feat(ios): revert default value

5f111ab

cipolleschi approved these changes May 30, 2024

View reviewed changes

facebook-github-bot closed this in ed9978b Jun 6, 2024

facebook-github-bot added the Merged This PR has been merged. label Jun 6, 2024

kirillzyusko mentioned this pull request Aug 12, 2024

chore: react-native 0.75 kirillzyusko/react-native-keyboard-controller#530

Merged

2 tasks

feat(iOS): add all supported ReturnKeyTypes #43362

feat(iOS): add all supported ReturnKeyTypes #43362

Uh oh!

Conversation

elencho commented Mar 7, 2024

Summary:

Changelog:

Test Plan:

Uh oh!

analysis-bot commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Apr 25, 2024

Uh oh!

cipolleschi left a comment

Choose a reason for hiding this comment

Uh oh!

cipolleschi left a comment

Choose a reason for hiding this comment

Uh oh!

cipolleschi Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

cipolleschi commented May 1, 2024

Uh oh!

elencho commented May 1, 2024

Uh oh!

elencho commented May 8, 2024

Uh oh!

cipolleschi commented May 20, 2024

Uh oh!

elencho commented May 21, 2024

Uh oh!

facebook-github-bot commented May 30, 2024

Uh oh!

cipolleschi commented May 30, 2024

Uh oh!

elencho commented Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cipolleschi commented Jun 5, 2024

Uh oh!

facebook-github-bot commented Jun 6, 2024

Uh oh!

github-actions bot commented Jun 6, 2024

Uh oh!

kirillzyusko commented Aug 12, 2024

Uh oh!

kirillzyusko commented Aug 12, 2024

Uh oh!

elencho commented Aug 12, 2024

Uh oh!

kirillzyusko commented Aug 13, 2024

Uh oh!

cipolleschi commented Aug 13, 2024

Uh oh!

kirillzyusko commented Aug 13, 2024

Uh oh!

elencho commented Aug 13, 2024

Uh oh!

cipolleschi commented Aug 13, 2024

Uh oh!

elencho commented Aug 13, 2024

Uh oh!

elencho commented Aug 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

analysis-bot commented Mar 7, 2024 •

edited

Loading

elencho commented Jun 4, 2024 •

edited

Loading