Implement onKeyPress Android

[joshjhargreaves]

Motivation

This implements onKeyPress for Android on TextInputs and addresses #1882.
N.B. that this PR has not yet addressed hardware keyboard inputs, but doing will be fairly trivial. The main challenge was doing this for soft keyboard inputs.

I've tried to match the style as much as I could. Will happily make any suggested edits be they architectural or stylistic design (edit: and of course implementation), but hopefully this is a good first pass :).
I think important to test this on the most popular keyboard types; maybe different languages too.
I have not yet added tests to test implementation, but will be happy to do that also.

Test Plan

• Build & run RNTester project for Android and open TextInput.
• Enter keys into 'event handling' TextInput.
• Verify that keys you enter appear in onKeyPress below the text input
• Test with autocorrect off, on same input and validate that results are the same.

Below is a gif of PR in action.

[shergin]

Looks good to me!
I think it is a highly requested and useful feature which unlikely can be implemented perfectly right at the first attempt. So, we probably should fix all currently known issues (with Swift Keyboard) and then polish this down the road.

@achen1 What do you think?

[shergin]

I am totally n00b in Android, but maybe we should call it inputConnectionWrapper instead?

[joshjhargreaves]

'totally n00b in Android': that makes two of us then ;). Thank-you yes, good point!

[shergin]

space character before class ...

[joshjhargreaves]

@shergin, thank-you for taking a look at this, very much appreciated :).

I actually have only just seem this ticket on the `Better <TextInput>' board, already triaged. They have taken the approach of detecting the key by changes to the text itself, just like the iOS implementation. This was the first route I went down, but found that TextWatching really didn't give you much in the way of helping you distinguish between different types of input, be they autocorrection's, cut/paste etc. Although it's hard to say at this point if key detection as done in this PR is too hard/a good idea with the potential wide variety of behavior from different IMEs.

I will say, you do at least need to use the InputConnection override for detecting the 'empty delete' case I mentioned, for detecting deletes which result in no text changes. Maybe even this could be used in combination with a TextWatcher, to reason about if text changes were results of changes from the soft keyboard.

I do think this is a really fiddly problem, and I'd say that this implementation handles pretty much every case (with stock Android keyboard). But perhaps we've ended up with a solution that's overfitted, and we'll have to go more generic solution.

[joshjhargreaves]

status update
I think it can be quite easy to get bogged down in implementation details sometimes, instead of thinking bigger picture!

So I think it's worth asking, 'what is the real value add' with implementing onKeyPress on Android for users? The thing that users cannot currently do currently on RN Android is detect 'empty deletes' or distinguish between key presses and cut and/or pastes. This we can do natively, and this is what I think we should effectively expose through this API. We should support the same features as onKeyPress on iOS: 'Backspace' & 'Enter' & any other key press events should only be triggered by keyboard input and not cut/copy & we should support the empty delete case.

From further investigation with SwiftKey vs stock Android keyboard and what we cannot do natively is know what key was pressed to 'complete' a word from a word suggestion, thus we cannot infer the keyPress in this case; we do not get that level granularity. So given we cannot do this natively, (or at least very easily at all, see the distinct lack of stack overflow answers on the topic), IMO we should not worry about this use case; RN on iOS also has these same issues.

I'll update the issue tracking this feature with a link to think.

[PardeepK]

@joshyhargreaves , with this implementation, we won't get onKeyPress event for keys like up/down/left/right or hardware keyboard's ENTER key.
So why don't we use native Android control EditText.onKeyup() (or KeyListener.onKeyUp()) to propagate key event to JS? That way we will also get this event if user press non-printable keys like up/down/left/right arrow keys?

[PardeepK]

@joshyhargreaves also is there any timeline by when we will get this merged to master?

[joshjhargreaves]

@PardeepK, onKeyUp events only work with hardware keyaboards as of ~Android 4.1, so not possible. We can of course still forward the events for hardware keyboards. This would be unlike the iOS inplementstion which works by watching the text.

I also think I’ve made too many assumptions about how different IMEs work based on the behaviour of the Android built in keyboard. For one, I don’t think you can assume beginBatchEdit is going to be called before any input events, and it’s up to the IME to call this, but not a requirement.

I have no timeline on when this will be merged; I think it needs to be integrated upon a few more times. Ultimately I think the behaviour RN users want but do not currently have is th ability to detect delete key presses on an empty text field, and perhaps this is the real value add of implementing this API and is what we should mainly be focussing upon as I don’t think this API is really possible to implement in the same way as the web.

[PardeepK]

Thanks @joshyhargreaves for quick reply!
I need onKeyPress event for h/w keyboard up/down arrow key. I was investigating and found that in vanilla (non-react) Android app, I get EditText::onKeyDown/onKeyUp() callback for all keys for both s/w and h/w keyboard. But when I am using react-native TextInput in Android app, I am not getting these onkeyDown() onKeyUp() methods invoked. any reason?
Also onKeyDown/onKeyUp() methods in InternalKeyListener (inner class in ReactEditText) are not getting invoked when user presses left/right/up/down key on h/w keyboard. I am not sure where these keyEvents are getting consumed. In fact I added these onKeyUp/onKeyDown methods in ReactRootView class, those also not getting invoked when I used h/w keyboard.

Appreciate any help!

[joshjhargreaves]

Copied from stackoveflow answer: from http://developer.android.com/reference/android/view/KeyEvent.html

As soft input methods can use multiple and inventive ways of inputting text, there is no guarantee that any key press on a soft keyboard will generate a key event: this is left to the IME's discretion, and in fact sending such events is discouraged. You should never rely on receiving KeyEvents for any key on a soft input method. In particular, the default software keyboard will never send any key event to any application targetting Jelly Bean or later, and will only send events for some presses of the delete and return keys to applications targetting Ice Cream Sandwich or earlier. Be aware that other software input methods may never send key events regardless of the version. Consider using editor actions like IME_ACTION_DONE if you need specific interaction with the software keyboard, as it gives more visibility to the user as to how your application will react to key presses.

I have set up a vanilla project to test this and have observed that onKeyPress etc so perhaps the above is why you’re not seeing the events? I didn’t try arrow keys actually so they might still be sent by the IME and maybe that’s what you’re seeing? Hmm, not sure why you’d be seeing differences actually I haven’t observed different bevahour in the different cases. I have noticed subtle differences when autocorrect is on and off, so that might be worth checking.

No problem, I’d like to see this solved too!

[PardeepK]

@joshyhargreaves any recommendation/help how can we capture hardware keyboard key press for TextInput (ReactEditText) RN control on Android?

[facebook-github-bot]

@joshyhargreaves I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[shergin]

@joshyhargreaves Are you personally satisfied with this?
Could you please resolve conflicts?

[joshjhargreaves]

@shergin sorry I missed the email mention!

Yes I can fix up the PR, sorry this fell off my radar with a talk I was doing! But now I'm free to finish this up; thank you for the continued input, it'd be great to get this landed!

So @shergin, I'm not quite happy with this exact approach, it's a solution which is definitely overfit to the stock Android stock keyboard IME lifecycle events and could be a bit fragile with other keyboards as a result. I think the solution is to make the logic a bit more naive (and cover more keyboards) by using a TextWatcher implementation similar to the onKeyPress iOS implementation.

The key thing I think we should enable as part of this PR is backspace detection on an empty text-input, as that's something that users need. For example: jwohlfert23/react-native-tag-input#31. Would you agree?

So I think I'll aim for a combined approach of what I have here for backspace on empty text-input detection, along with simplified text-watcher logic.

[joshjhargreaves]

@shergin, it looks like the logic of can be a lot simpler by using TextWatcher; but using an the InputConnection to catch the ‘empty deletes’. Hopefully this should be fairly trivial, but looking into detecting when a text input update came from a cut/paste, but fingers crossed that should be easy enough

[joshjhargreaves]

Looking to update this PR after the weekend, will keep everyone posted.

[joshjhargreaves]

@shergin I implemented the logic using the TextWatcher API and I am happy with the resultant functionality! I haven't updated this PR, as I may swing back to fixing up the original method given now I have a greater understand of the finer details of the implementation: https://github.com/facebook/react-native/compare/master...joshyhargreaves:implement_edittext_onkeypress_android_textwatcher_rework?diff=unified&name=implement_edittext_onkeypress_android_textwatcher_rework

Functionality wise, it seems to work as I'd expect with both SwiftKey and stock Android keyboard :). However it turns out that the logic needed to be a bit more hacky (as you can probably see) to implement the API. We know less about how the input was changed with the TextWatcher approach, so we need to have some additional state around whether it was a cut/paste which changed the input, for example.

Regardless of this, I'm positive that with a bit more refinement & testing, we will have something soon that should be good enough to use!

[joshjhargreaves]

@shergin I revised the methodology with the original implementation; code is a lot cleaner than the alternate TextWatcher approach I linked above.

I have tested on SwiftKey and stock Android keyboard, and it seems to work well :).

[joshjhargreaves]

@lako90, would you also mind having a look?

[DarrylD]

Any movement on this?

[joshjhargreaves]

@DarrylD I'm happy to whatever needs to be done to get this merged. @shergin @hramos could you provide some guidance on this?

[kmagiera]

Great stuff @joshyhargreaves

I've put some minor comments here and there. I hope that my comments will help improve this PR and get it merged sooner.

[kmagiera]

The logic of instantiating InputConnection seems overly complicated. If you don't need to keep a reference to inputConnection outside of ReactEditText class why not instantiate it directly in ReactEditText#onCreateInputConnection ? This will make setEditText and setTarget methods no longer needed and result in a cleaner contract for ReactTextInputInputConnection.

a) setEditText will not be necessary if you are instantiating it from onCreateInputConnection because you can have the reference to ReactEditText there, just use this.
b) setTarget can be replaced by just passing InputConnection we want to wrap directly via constructor of ReactTextInputInputConnection

[joshjhargreaves]

Thank you, very good point!

[kmagiera]

Would be good to have some high level comment here on what this class is for.

Also I think you didn't want to name this class this way ("InputInput"). As you need to change the name anyways I suggest that the class name include the word "Wrapper" (e.g. "ReactEditTextInputConnectionWrapper")

[joshjhargreaves]

Sounds like a more sensible name, thank you :P.

[kmagiera]

public static final fields should be all caps with underscores: NEW_LINE_RAW_VALUE etc

[joshjhargreaves]

Thanks!

[kmagiera]

If you use @Nullable annotation you should make sure to do null checks before calling methods on that object or use assertNoNull. Although as per my other comment about removing setEditText I think if you follow that comment you won't need @Nullable here.

[joshjhargreaves]

Thanks, with your suggested changes, @Nullable no longer necessary!

[joshjhargreaves]

@kmagiera made the changes you suggested, let me know what you think.

[kmagiera]

Just few more minor comments I missed last time. Otherwise this looks good!

[kmagiera]

As we are no longer updating target we can just default to mutable=false. Also there seem to be no need to expose mutable as an argument of your subclass constructor (ReactEditTextInputConnectionWrapper). I recommend to replace this call with:

super(target, false);

And remove boolean mutable from the list of constructor arguments.

[joshjhargreaves]

Good spot, thanks.

[kmagiera]

can you drop all the final keywords here. It is not common to use final in this context in Java (unlike in JS where you'd use const). Usually final is only used for scoped vars when they are being referenced in anonymous classes ("lambda classes") – in which case you get a compiler error if you forget to use final

[joshjhargreaves]

Thank you!

[kmagiera]

space: if(mKey -> if (mKey

[joshjhargreaves]

@kmagiera made the changes!

[edo1493]

Can we make this happen before Christmas? 🙏

[edo1493]

Hey @joshyhargreaves and @shergin, what's the status on this? 🤓

[joshjhargreaves]

@kmagiera and I are looking to move this forward after the holiday period.

[brentvatne]

@facebook-github-bot shipit

[facebook-github-bot]

@brentvatne is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[vision72]

Hi @joshjhargreaves, I hope you are doing well.
The onKeyPress Android is an amazing implementation, and I have been using it since as soon as it was launched, as many other developers do.

There is one issue with the onKeyPress property, which I am looking at, and I wanted to know more about the implementation of the event.preventDefault prototype of the SyntheticEvent class.

What I found out was there is a ReactEditTextInputConnectionWrapper class that extends the InputConnectionWrapper class which generates a key press native event and updates the input text, basically adding text on the input field.

I wanted to know in scenarios of using the event.preventDefault or event.stopPropagation from the event object of the onKeyPress property, can the setComposingText function from the ReactEditTextInputConnectionWrapper class handle a similar condition as,

boolean cursorMovedBackwardsOrAtBeginningOfInput =
        (currentSelectionStart < previousSelectionStart) || currentSelectionStart <= 0; 

Where we specify that the preventDefault will not initiate a dispatchKeyEventOrEnqueue, which indirectly overwrites the bubbling of the input key, thereby achieving an event.stopPropagation behavior.

Now, I have yet to check out your PR raised with the onKeyPress property, it could be that it's working in the earlier versions and not now.

I will keep you posted, and await your inputs on the preventDefault/stopPropagation events.

Link to the Issue: #38155.
CC: @cortinico