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

Align multi-line TextInput onSubmitEditing behavior #29177

Conversation

tido64
Copy link
Collaborator

@tido64 tido64 commented Jun 19, 2020

Summary

Aligns behavior to be consistent with Android and the documentation. onSubmitEditing should not be called when blurOnSubmit=false.

Changelog

[iOS] [Fixed] - Align multi-line TextInput onSubmitEditing behavior

Test Plan

textinput-blur-align

Aligns behavior to be consistent with Android and the documentation.
`onSubmitEditing` should not be called when `blurOnSubmit=false`.
@tido64 tido64 added the Platform: iOS iOS applications. label Jun 19, 2020
@tido64 tido64 requested review from shergin and elicwhite June 19, 2020 10:02
@facebook-github-bot 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. p: Microsoft Partner: Microsoft Partner labels Jun 19, 2020
@analysis-bot
Copy link

analysis-bot commented Jun 19, 2020

Platform Engine Arch Size (bytes) Diff
ios - universal 829440 n/a

Base commit: ff69028

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 6,764,112 -1,145
android hermes armeabi-v7a 6,426,889 -1,609
android hermes x86 7,150,421 -1,992
android hermes x86_64 7,040,363 -2,018
android jsc arm64-v8a 8,937,393 -1,144
android jsc armeabi-v7a 8,592,583 -1,561
android jsc x86 8,766,841 -1,968
android jsc x86_64 9,342,401 -2,008

Base commit: ff69028

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@shergin
Copy link
Contributor

shergin commented Jul 12, 2020

IDK. Where exactly the documentation says that? I only found this:

blurOnSubmit
If true, the text field will blur when submitted. The default value is true for single-line fields and false for multiline fields. Note that for multiline fields, setting blurOnSubmit to true means that pressing return will blur the field and trigger the onSubmitEditing event instead of inserting a newline into the field.

To me, it does not say anything about the case where blurOnSubmit is false.
I don't really know what's the right and most useful and versatile behavior.

Do we have any motivation for this use-case? Maybe we should change Android?

We have to unify the behavior, that's for sure.

@tido64
Copy link
Collaborator Author

tido64 commented Jul 12, 2020

To me, it does not say anything about the case where blurOnSubmit is false.
I don't really know what's the right and most useful and versatile behavior.

My interpretation of "instead of`" in the documentation describes the opposite case, i.e.:

  • blurOnSubmit=true: pressing return will blur the field and trigger the onSubmitEditing event
  • blurOnSubmit=false: inserting a newline into the field

If you're asking me what I think should be the correct behaviour, I think blurOnSubmit=false should just submit and not insert a newline or blur. I can't think of a single use case where inserting a newline in addition to submitting makes any sense.

Regardless, this change aligns iOS with Android. Right now, macOS is aligned with Android (as of v0.61.49). If you think we should change the behaviour to something that is more useful, I am happy to do that but I need to know what that something is 😄

@shergin
Copy link
Contributor

shergin commented Jul 16, 2020

Oh, IDK, it's a tough choice.
Seems you put a lot of thoughts into that. Let's do the right thing. What should we do?

@tido64
Copy link
Collaborator Author

tido64 commented Aug 7, 2020

@shergin: This is what I'm thinking:

Pressing Enter... Submits Blurs Inserts newline
(none) no no yes
blurOnSubmit no no yes
onSubmitEditing yes no no
onSubmitEditing + blurOnSubmit yes yes no

In all cases, pressing Shift+Enter will always insert a newline, and will neither submit nor blur.

Pressing Shift+Enter... Submits Blurs Inserts newline
(none) no no yes
blurOnSubmit no no yes
onSubmitEditing no no yes
onSubmitEditing + blurOnSubmit no no yes

I think this matches most chat clients and forms that I've interacted with. What do you think?

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @tido64 in 521b167.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Sep 29, 2020
facebook-github-bot pushed a commit that referenced this pull request Oct 8, 2020
Summary:
This is a revert of D22488870 (521b167). (#29177)
We have to revert it because we realized that it's a breaking change without a very good reason. We have to figure out a better way to unify platform behaviors.

Changelog:
[iOS][Fixed] - Reverted recent change in TextInput (#29177)

Reviewed By: fkgozali

Differential Revision: D24200517

fbshipit-source-id: af0e561a6b8d9ade487be6b197a5d79d326442b6
amgleitman pushed a commit to amgleitman/react-native-macos that referenced this pull request Sep 27, 2021
Summary:
This is a revert of D22488870 (facebook@521b167). (facebook#29177)
We have to revert it because we realized that it's a breaking change without a very good reason. We have to figure out a better way to unify platform behaviors.

Changelog:
[iOS][Fixed] - Reverted recent change in TextInput (facebook#29177)

Reviewed By: fkgozali

Differential Revision: D24200517

fbshipit-source-id: af0e561a6b8d9ade487be6b197a5d79d326442b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants