From b60a727adbcfa785e3d1b13bf069b766216e60f8 Mon Sep 17 00:00:00 2001 From: Josh Hargreaves Date: Tue, 27 Feb 2018 09:59:34 -0800 Subject: [PATCH] Fix crashes onKeyPress Android 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 https://github.com/facebook/react-native/issues/17974 & https://github.com/facebook/react-native/issues/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](https://github.com/facebook/react-native/issues/17974#issuecomment-368471737), 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 https://github.com/facebook/react-native/issues/17974#issuecomment-368471737. 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 https://github.com/facebook/react-native/pull/18114 Differential Revision: D7099570 Pulled By: hramos fbshipit-source-id: 75b2dc468c1ed398a33eb00487c6aa14ae04e5c2 --- .../facebook/react/views/textinput/ReactEditText.java | 8 +++++--- .../ReactEditTextInputConnectionWrapper.java | 11 ++++++----- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java b/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java index 0dde905faed0ee..e15dd5a7969cda 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java @@ -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 diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditTextInputConnectionWrapper.java b/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditTextInputConnectionWrapper.java index f0e12e61f340e5..cc2976efdf8679 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditTextInputConnectionWrapper.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditTextInputConnectionWrapper.java @@ -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;