-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[Android] onKeyPress event not fired with numeric keys #29046
Conversation
Base commit: 4324ca8 |
Base commit: 4324ca8 |
Thank you very much for this fix! There's only one adjustment that needs to be made on line 135: boolean isNumberKey = event.getUnicodeChar() < 56 && the number needs to be < 58 since event.key for the number 9 is 57. Now number 8 and 9 are excluded and don't trigger the event. |
@nukemonk thanks a lot for the correction, I will make the adjustment this weekend. I wish you a good day. Fabrizio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
google-java-format
found some issues. See https://github.com/google/google-java-format
@@ -132,10 +132,14 @@ public boolean deleteSurroundingText(int beforeLength, int afterLength) { | |||
@Override | |||
public boolean sendKeyEvent(KeyEvent event) { | |||
if (event.getAction() == KeyEvent.ACTION_DOWN) { | |||
boolean isNumberKey = event.getUnicodeChar() < 58 && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
google-java-format
suggested changes:
@@ -135,2 +135 @@
- boolean isNumberKey = event.getUnicodeChar() < 58 &&
- event.getUnicodeChar() > 47;
+ boolean isNumberKey = event.getUnicodeChar() < 58 && event.getUnicodeChar() > 47;
Any Updates on this one? |
@mariomurrent-softwaresolutions I need more time and have more detailed look at the detox failures. Thanks test-android
test-docker
|
Okay, cool & thx |
@fabriziobertoglio1987 Is it done? |
@fabriziobertoglio1987 this CI's issue you are experiencing seems to have been fixed in master. Would you mind updating your branch to master please? I'm more then happy to collaborate on this if you are busy. |
@vadimb thanks a lot for your feedback. Feel free to do a code review on this pr and collaborate with me. I'll be happy to accept your improvements.. Currently I'm working (in my free time) on other prs and I don't know of any problems with this pr, but I'll be happy to follow your advice/improvements. Thanks a lot for finding the solution to the issue with CI Tests. Thanks a lot 🙏 |
Just tested and seems all other keys(including Hopefully it will land ASAP 👍 |
thanks a lot @vadimb yes, but still reading your review and interacting with you is very valuable for me, as from our interaction we may get ideas on how to improve or break this functionality. I consider you comment a very valuable code review. I did not think about this scenario and use case, but luckily it was covered. |
@nukemonk can you please let us know if there is any eta when we this PR will be merged? |
@vadimb there is no eta. 🙏 😄 |
My bad - seeing that you approved thought you have super powers :) - but seems anyone can review/approve. |
I already updated the code from @fabriziobertoglio1987. But no PR approve on his side |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥇
Any estimate when this could be released? 🙂 |
@vastamaki you can check out https://github.com/sponsors/fabriziobertoglio1987 if you are interested, otherwise I can estimate 6-12 months. |
@vastamaki To answer the question of when this fix will be released in the next version of React Native, I suggest you to follow the discussions at https://github.com/react-native-community/releases/issues by subscribing to those threads. The process of releasing a new version with a bug fix is the following:
|
Same problem. But not only android. My problem on both. ( IOS + Android ). I don't know why pull request was technically rejected, but I have a specific need. I am faced with a case that works according to an algorithm and I have to process individual letters. Just; numbers, commas, periods and backspace. For this reason, is it possible to come up with a solution? |
@sshic has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
1 similar comment
@sshic has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@sshic merged this pull request in ee3e71f. |
Still persists, Using this
|
Summary
This issue fixes #19507 fixes #30475 onKeyPress event not fired for numeric keyboard
The TextInput onKeyPress event is not fired when pressing numeric keys on Android.
The method sendKeyEvent will dispatchKeyEvent only for:
The solution proposed is trigger dispatchKeyEvent for KeyEvents with event.getUnicodeChar() value included between 47 and 58 (numeric keys 0-9)
Changelog
[Android] [Fixed] - onKeyPress event not fired with numeric keys
Test Plan
CLICK TO OPEN TESTS RESULTS