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.

Sign up for GitHub

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

Jump to bottom

[Android] Fix onKeyPress so it provides the correct key when TextInput is at max length #30751

Closed
wants to merge 8 commits into from
Closed

[Android] Fix onKeyPress so it provides the correct key when TextInput is at max length #30751

wants to merge 8 commits into from

Conversation

osamaqarem
Copy link

@osamaqarem osamaqarem commented Jan 17, 2021 •
edited
Loading

Summary

Fixes #30747 making Android behave the same as iOS under the same scenario.

When autosuggest is enabled (which is the default), then the keypress is inferred based on cursor position. The issue appears when max length is reached where we can no longer guess the keypress based on cursor location alone.

The problem can be reproduced using the expo snack provided in the original issue:
https://snack.expo.io/PuyXgucEf

Changelog

[Android] [Fixed] - Fix onKeyPress so it provides the correct key when TextInput has keybard suggestions and is at max length.

Test Plan

Tested using RNTester app in Components > TextInput > Event handling example and adding a maxLength prop.

  1. Enter text in the input until maxLength is reached.
  2. The value printed 'onKeyPress key' should be the actual key pressed.

Before

before.mov

After

after.mov

@facebook-github-bot
Copy link
Contributor

Hi @osamaqarem!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

analysis-bot
analysis-bot suggested changes Jan 17, 2021
View reviewed changes
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

...id/src/main/java/com/facebook/react/views/textinput/ReactEditTextInputConnectionWrapper.java Show resolved Hide resolved
...id/src/main/java/com/facebook/react/views/textinput/ReactEditTextInputConnectionWrapper.java Outdated Show resolved Hide resolved
...id/src/main/java/com/facebook/react/views/textinput/ReactEditTextInputConnectionWrapper.java Show resolved Hide resolved
...id/src/main/java/com/facebook/react/views/textinput/ReactEditTextInputConnectionWrapper.java Show resolved Hide resolved
@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 Jan 17, 2021
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@analysis-bot
Copy link

analysis-bot commented Jan 17, 2021 •
edited
Loading

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

Base commit: 5d500f4

@analysis-bot
Copy link

analysis-bot commented Jan 17, 2021 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,888,051 +46
android hermes armeabi-v7a 8,392,968 +42
android hermes x86 9,378,060 +47
android hermes x86_64 9,320,836 +46
android jsc arm64-v8a 10,338,035 +49
android jsc armeabi-v7a 9,826,044 +49
android jsc x86 10,389,557 +58
android jsc x86_64 10,972,577 +51

Base commit: 5d500f4

analysis-bot
analysis-bot suggested changes Jan 17, 2021
View reviewed changes
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

...id/src/main/java/com/facebook/react/views/textinput/ReactEditTextInputConnectionWrapper.java Show resolved Hide resolved
...id/src/main/java/com/facebook/react/views/textinput/ReactEditTextInputConnectionWrapper.java Show resolved Hide resolved
...id/src/main/java/com/facebook/react/views/textinput/ReactEditTextInputConnectionWrapper.java Show resolved Hide resolved
...id/src/main/java/com/facebook/react/views/textinput/ReactEditTextInputConnectionWrapper.java Show resolved Hide resolved
osamaqarem
osamaqarem commented Jan 17, 2021
View reviewed changes
...id/src/main/java/com/facebook/react/views/textinput/ReactEditTextInputConnectionWrapper.java
(currentSelectionStart < previousSelectionStart) || currentSelectionStart <= 0;
if (cursorMovedBackwardsOrAtBeginningOfInput || (!noPreviousSelection && cursorDidNotMove)) {
(currentSelectionStart < previousSelectionStart) || currentSelectionStart <= 0;
int maxLength = mEditText.getLengthFilterValue();
Copy link
Author

@osamaqarem osamaqarem Jan 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Another way of getting the maxLength would be looping through the InputFilters on the EditText until we find the LengthFilter. Let me know if that approach is preferrable.

@osamaqarem
Copy link
Author

osamaqarem commented Jan 17, 2021 •
edited
Loading

Regarding the commit: fix: backspace keypress not inferred when max length is 0:

This fixes a specific edge case when maxLength is set to 0.

When maxLength is set to 1 or more and the max length is reached, the composing text never changes. So backspace can be inferred correctly. However, when maxLength is set to 0 then whatever the user types is appended to the composing text and therefore we can no longer infer the backspace keypress.

The videos below demonstrate the issue and the result of the fix introduced in this commit.
In both theses videos, maxLength is set to 0.

Before

before.mov

After

after.mov

@osamaqarem osamaqarem changed the title [Draft][Android] Fix onKeyPress to return correct key when TextInput is at max length [Draft][Android] Fix onKeyPress so it provides the correct key when TextInput is at max length Jan 17, 2021
@osamaqarem osamaqarem changed the title [Draft][Android] Fix onKeyPress so it provides the correct key when TextInput is at max length [Android] Fix onKeyPress so it provides the correct key when TextInput is at max length Jan 17, 2021
@osamaqarem osamaqarem marked this pull request as ready for review January 17, 2021 12:05
@safaiyeh safaiyeh added Component: TextInput Related to the TextInput component. Platform: Android Android applications. labels Jan 17, 2021
@safaiyeh
Copy link
Contributor

cc @JoshuaGross to check out this PR.

Quick turn around for a reported issue here: #30747

@osamaqarem osamaqarem closed this Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PrayaAmadigaPitasa PrayaAmadigaPitasa PrayaAmadigaPitasa left review comments

@analysis-bot analysis-bot analysis-bot requested changes

Assignees
No one assigned
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Component: TextInput Related to the TextInput component. Platform: Android Android applications.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

On Android, the onKeyPress callback returns the wrong key value when the TextInput has the maxLength property
6 participants
@osamaqarem @facebook-github-bot @analysis-bot @safaiyeh @PrayaAmadigaPitasa @react-native-bot