Skip to content

Commit

Permalink
Fix crashes onKeyPress Android
Browse files Browse the repository at this point in the history
Summary:
There appear to be two different types of crashes related to the recent addition of `onKeyPress` on Android introduce in `0.53`. This PR addresses the cause of both of them.

Firstly, it seems possible to get an `indexOutOfBoundsException` with some 3rd-party keyboards as observed in #17974 & #17922. I have simplified the backspace determining logic slightly, and also put in an explicit check for zero case so it is not possible to get an indexOutOfBoundsException & it should make sense in the context of the onKeyPress logic.

Secondly, it appears that `EditText#onCreateInputConnection` can return null. In this case, if we set `null` to be the target of our subclass of `ReactEditTextInputConnectionWrapper`, we will see the crashes as seen [here](#17974 (comment)), whereby any of methods executed in the `InputConnection` interface can result in a crash. It's hard to reason about the state when `null` is returned from `onCreateInputConnection`, however I would might reason that any soft keyboard input cannot update the `EditText` with a `null` `input connection`, as there is no way of interfacing with the `EditText`. I'm am not sure, if there is a later point where we might return/set this input connection at a later point? As without the `InputConnection` onKeyPress will not work. But for now, this will fix this crash at least.

I have not managed to reproduce these crashes myself yet, but users have confirmed that the `indexOutOfBounds` exception is fixed with the 'zero' case and has been confirmed on the respective issues #17974 (comment).

For the `null` inputConnection target case, I have verified that explicitly setting the target as null in the constructor of `onCreateInputConnection` results in the same stack trace as the one linked. Here is also a [reference](https://github.com/stripe/stripe-android/pull/392/files#diff-6cc1685c98457d07fd4e2dd83f54d5bb) to the same issue closed with the same fix for another project on github.

It is also important to verify that the behavior of `onKeyPress` still functions the same after this change, which can be verified by running the RNTesterProject and the `KeyboardEvents` section in `InputText`.
The cases to check that I think are important to check are:
- Cursor at beginning of input & backspace
- Return key & return key at beginning of input
- Select text then press delete
- Selection then press a key
- Space key
- Different keyboard types

This should not be a breaking change.

 [ANDROID] [BUGFIX] [TextInput] - Fixes crashes with TextInput introduced in 0.53.
Closes #18114

Differential Revision: D7099570

Pulled By: hramos

fbshipit-source-id: 75b2dc468c1ed398a33eb00487c6aa14ae04e5c2
  • Loading branch information
joshjhargreaves authored and facebook-github-bot committed Feb 27, 2018
1 parent a759a44 commit b60a727
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -172,14 +172,16 @@ protected void onScrollChanged(int horiz, int vert, int oldHoriz, int oldVert) {
@Override
public InputConnection onCreateInputConnection(EditorInfo outAttrs) {
ReactContext reactContext = (ReactContext) getContext();
ReactEditTextInputConnectionWrapper inputConnectionWrapper =
new ReactEditTextInputConnectionWrapper(super.onCreateInputConnection(outAttrs), reactContext, this);
InputConnection inputConnection = super.onCreateInputConnection(outAttrs);
if (inputConnection != null) {
inputConnection = new ReactEditTextInputConnectionWrapper(inputConnection, reactContext, this);
}

if (isMultiline() && getBlurOnSubmit()) {
// Remove IME_FLAG_NO_ENTER_ACTION to keep the original IME_OPTION
outAttrs.imeOptions &= ~EditorInfo.IME_FLAG_NO_ENTER_ACTION;
}
return inputConnectionWrapper;
return inputConnection;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,15 @@ public boolean setComposingText(CharSequence text, int newCursorPosition) {
int previousSelectionEnd = mEditText.getSelectionEnd();
String key;
boolean consumed = super.setComposingText(text, newCursorPosition);
int currentSelectionStart = mEditText.getSelectionStart();
boolean noPreviousSelection = previousSelectionStart == previousSelectionEnd;
boolean cursorDidNotMove = mEditText.getSelectionStart() == previousSelectionStart;
boolean cursorMovedBackwards = mEditText.getSelectionStart() < previousSelectionStart;
if ((noPreviousSelection && cursorMovedBackwards)
|| !noPreviousSelection && cursorDidNotMove) {
boolean cursorDidNotMove = currentSelectionStart == previousSelectionStart;
boolean cursorMovedBackwardsOrAtBeginningOfInput =
(currentSelectionStart < previousSelectionStart) || currentSelectionStart <= 0;
if (cursorMovedBackwardsOrAtBeginningOfInput || (!noPreviousSelection && cursorDidNotMove)) {
key = BACKSPACE_KEY_VALUE;
} else {
key = String.valueOf(mEditText.getText().charAt(mEditText.getSelectionStart() - 1));
key = String.valueOf(mEditText.getText().charAt(currentSelectionStart - 1));
}
dispatchKeyEventOrEnqueue(key);
return consumed;
Expand Down

0 comments on commit b60a727

Please sign in to comment.