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

[TextInput] Implements 'onKeyPress' for Android #10665

Closed
wants to merge 1 commit into from
Closed

[TextInput] Implements 'onKeyPress' for Android #10665

wants to merge 1 commit into from

Conversation

petergarnaes
Copy link

An attempt at the Android implementation. Attempted to mirror the iOS implementation by supporting 'Enter' and 'Backspace'.

The implementation utilises the TextWatcher, because the [documentation](https://developer.android.com/reference/android/view/View.OnKeyListener.html#onKey(android.view.View, int, android.view.KeyEvent)) for OnKeyListener->onKey states:

Key presses in software keyboards will generally NOT trigger this method, although some may elect to do so in some situations. Do not assume a software input method has to be key-based; even if it is, it may use key presses in a different way than you expect, so there is no way to reliably catch soft input key presses.

Because of this, in my implementation pressing backspace in an empty text field will not trigger onKeyPress. Using cut on selected text and pasting a single character will be interpreted as key presses.

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @janicduplessis and @rigdern to be potential reviewers.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

@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 1, 2016
@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!

* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This header should be at the top of the file.

int diff = count - before;
if(diff == 1) {
// Mirrors behaviour of iOS
String key = ""+s.charAt(start+count-1);
Copy link
Contributor

Choose a reason for hiding this comment

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

spaces around +

private WritableMap serializeEventData() {
WritableMap eventData = Arguments.createMap();

//WritableMap selectionData = Arguments.createMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

space after //

//WritableMap selectionData = Arguments.createMap();
eventData.putString("key", mKey);

//eventData.putMap("selection", selectionData);
Copy link
Contributor

Choose a reason for hiding this comment

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

space after //


public ReactKeyDownEvent(
int viewId,
String key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

single line

@mkonicek
Copy link
Contributor

Key presses in software keyboards will generally NOT trigger this method, although some may elect to do so in some situations. Do not assume a software input method has to be key-based; even if it is, it may use key presses in a different way than you expect, so there is no way to reliably catch soft input key presses.

Oh Android 😐

// If the string is only 1 character longer, we interpret it as a key press. It also triggers
// if only 1 character was pasted, but there is no way to monitor soft/virtual key presses
int diff = count - before;
if(diff == 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

space after "if"

}
// If the text is shorter we interpret as a backspace press (could also be a Cut from a
// selection)
if(diff < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

space after "if"

@lacker
Copy link
Contributor

lacker commented Nov 30, 2016

Do we need to change the docs for this? Also do we need to communicate a breaking change - is this breaking the behavior of existing stuff, or just adding a new event?

Could you add a test to ensure this behavior is working properly?

@lacker lacker self-assigned this Nov 30, 2016
@petergarnaes
Copy link
Author

petergarnaes commented Dec 1, 2016

Do we need to change the docs for this?

Yes, right now this event is marked as iOS only. It is also maybe worth mentioning the limitation of key events on Android?

Also do we need to communicate a breaking change - is this breaking the behavior of existing stuff, or just adding a new event?

I have simply just added a new event, and triggers in the appropriate places.

Could you add a test to ensure this behavior is working properly?

Sure, could you give me a small pointer in the right direction? Which file(s) should I look at for inspiration?

@lacker
Copy link
Contributor

lacker commented Dec 1, 2016

Are we testing other similar events at all? I'm not quite sure what the right way to test this is, but we really need to figure that out to reduce the rate of React Native accidental breakage.

@lacker
Copy link
Contributor

lacker commented Dec 15, 2016

@petergarnaes I would try adding an Android integration test here, since this changes both js and java. Check out ReactAndroid/src/androidTest to see how the integration tests work.

@hramos
Copy link
Contributor

hramos commented Jan 19, 2017

Ping @petergarnaes are you still working on this PR?

@lacker
Copy link
Contributor

lacker commented Feb 8, 2017

It seems like this PR has gone stale so I am going to close it. Please feel free to reopen if you are still working on this!

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.

5 participants