Commit 812d7fc0 authored by Ryan Landay's avatar Ryan Landay Committed by Commit Bot

Fix incorrect ending selection after InputMethodController::SetComposition()

InputMethodController::SetComposition() works as follows:

1. The current composition range is selected (we don't show this to users).
2. The selection is replaced with the new text, which is left selected.
3. The resulting selection is converted to a composition range, and we change
   the selection to whatever the IME requested be selected afterwards.

Step 2 causes the open TypingCommand's ending selection to be set to the
resulting composition range. We need to update it after step 3 to fix two
problems:

1. Pressing enter on a physical keyboard on Android with an open composition
   range causes the text in the composition range to be incorrectly deleted.

2. Pressing Ctrl-Z to undo causes each word to be selected in turn (see video on
   https://crbug.com/787598).

There's another problem with undo on Android, which is that the TypingCommand is
closed out after every call to SetComposition() when we select the composition,
which means undo only undoes one character at a time. I might fix this in a
separate CL.

Bug: 787598, 772565, 772584
Change-Id: I29f09e3c0bd97c6c8e17e5455c5579b53aa34c1b
Reviewed-on: https://chromium-review.googlesource.com/783770
Commit-Queue: Ryan Landay <rlanday@chromium.org>
Reviewed-by: default avatarChangwan Ryu <changwan@chromium.org>
Reviewed-by: default avatarXiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: default avatarYoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520857}
parent 5289cdfe
...@@ -673,8 +673,7 @@ public class ImeAdapter { ...@@ -673,8 +673,7 @@ public class ImeAdapter {
mInputMethodManagerWrapper.notifyUserAction(); mInputMethodManagerWrapper.notifyUserAction();
} }
@VisibleForTesting public void sendSyntheticKeyPress(int keyCode, int flags) {
protected void sendSyntheticKeyPress(int keyCode, int flags) {
long eventTime = SystemClock.uptimeMillis(); long eventTime = SystemClock.uptimeMillis();
sendKeyEvent(new KeyEvent(eventTime, eventTime, sendKeyEvent(new KeyEvent(eventTime, eventTime,
KeyEvent.ACTION_DOWN, keyCode, 0, 0, KeyEvent.ACTION_DOWN, keyCode, 0, 0,
...@@ -695,13 +694,6 @@ public class ImeAdapter { ...@@ -695,13 +694,6 @@ public class ImeAdapter {
CharSequence text, int newCursorPosition, boolean isCommit, int unicodeFromKeyEvent) { CharSequence text, int newCursorPosition, boolean isCommit, int unicodeFromKeyEvent) {
if (!isValid()) return false; if (!isValid()) return false;
// One WebView app detects Enter in JS by looking at KeyDown (http://crbug/577967).
if (TextUtils.equals(text, "\n")) {
sendSyntheticKeyPress(KeyEvent.KEYCODE_ENTER,
KeyEvent.FLAG_SOFT_KEYBOARD | KeyEvent.FLAG_KEEP_TOUCH_MODE);
return true;
}
onImeEvent(); onImeEvent();
long timestampMs = SystemClock.uptimeMillis(); long timestampMs = SystemClock.uptimeMillis();
nativeSendKeyEvent(mNativeImeAdapterAndroid, null, WebInputEventType.RAW_KEY_DOWN, 0, nativeSendKeyEvent(mNativeImeAdapterAndroid, null, WebInputEventType.RAW_KEY_DOWN, 0,
......
...@@ -323,6 +323,23 @@ class ThreadedInputConnection extends BaseInputConnection implements ChromiumBas ...@@ -323,6 +323,23 @@ class ThreadedInputConnection extends BaseInputConnection implements ChromiumBas
public boolean commitText(final CharSequence text, final int newCursorPosition) { public boolean commitText(final CharSequence text, final int newCursorPosition) {
if (DEBUG_LOGS) Log.i(TAG, "commitText [%s] [%d]", text, newCursorPosition); if (DEBUG_LOGS) Log.i(TAG, "commitText [%s] [%d]", text, newCursorPosition);
if (text == null) return false; if (text == null) return false;
// One WebView app detects Enter in JS by looking at KeyDown (http://crbug/577967).
if (TextUtils.equals(text, "\n")) {
beginBatchEdit();
// Clear the current composition range (the keypress alone wouldn't do this).
commitText("", 1);
ThreadUtils.postOnUiThread(new Runnable() {
@Override
public void run() {
mImeAdapter.sendSyntheticKeyPress(KeyEvent.KEYCODE_ENTER,
KeyEvent.FLAG_SOFT_KEYBOARD | KeyEvent.FLAG_KEEP_TOUCH_MODE);
}
});
endBatchEdit();
return true;
}
ThreadUtils.postOnUiThread(new Runnable() { ThreadUtils.postOnUiThread(new Runnable() {
@Override @Override
public void run() { public void run() {
......
...@@ -88,6 +88,8 @@ class CORE_EXPORT TypingCommand final : public CompositeEditCommand { ...@@ -88,6 +88,8 @@ class CORE_EXPORT TypingCommand final : public CompositeEditCommand {
static void CloseTyping(LocalFrame*); static void CloseTyping(LocalFrame*);
static TypingCommand* LastTypingCommandIfStillOpenForTyping(LocalFrame*); static TypingCommand* LastTypingCommandIfStillOpenForTyping(LocalFrame*);
static void UpdateSelectionIfDifferentFromCurrentSelection(TypingCommand*,
LocalFrame*);
void InsertText(const String& text, bool select_inserted_text, EditingState*); void InsertText(const String& text, bool select_inserted_text, EditingState*);
void InsertTextRunWithoutNewlines(const String& text, void InsertTextRunWithoutNewlines(const String& text,
...@@ -155,9 +157,6 @@ class CORE_EXPORT TypingCommand final : public CompositeEditCommand { ...@@ -155,9 +157,6 @@ class CORE_EXPORT TypingCommand final : public CompositeEditCommand {
should_prevent_spell_checking_ = prevent; should_prevent_spell_checking_ = prevent;
} }
static void UpdateSelectionIfDifferentFromCurrentSelection(TypingCommand*,
LocalFrame*);
void UpdatePreservesTypingStyle(ETypingCommand); void UpdatePreservesTypingStyle(ETypingCommand);
void TypingAddedToOpenCommand(ETypingCommand); void TypingAddedToOpenCommand(ETypingCommand);
bool MakeEditableRootEmpty(EditingState*); bool MakeEditableRootEmpty(EditingState*);
......
...@@ -762,6 +762,22 @@ void InputMethodController::SetComposition( ...@@ -762,6 +762,22 @@ void InputMethodController::SetComposition(
// We shouldn't close typing in the middle of setComposition. // We shouldn't close typing in the middle of setComposition.
SetEditableSelectionOffsets(selected_range, TypingContinuation::kContinue); SetEditableSelectionOffsets(selected_range, TypingContinuation::kContinue);
if (TypingCommand* const last_typing_command =
TypingCommand::LastTypingCommandIfStillOpenForTyping(&GetFrame())) {
// When we called InsertTextDuringCompositionWithEvents() with the
// kSelectInsertedText flag, it set what is now the composition range as the
// ending selection on the open TypingCommand. We now update it to the
// current selection to fix two problems:
//
// 1. Certain operations, e.g. pressing enter on a physical keyboard on
// Android, would otherwise incorrectly replace the composition range.
//
// 2. Using undo would cause text to be selected, even though we never
// actually showed the selection to the user.
TypingCommand::UpdateSelectionIfDifferentFromCurrentSelection(
last_typing_command, &GetFrame());
}
// Even though we would've returned already if SetComposition() were called // Even though we would've returned already if SetComposition() were called
// with an empty string, the composition range could still be empty right now // with an empty string, the composition range could still be empty right now
// due to Unicode grapheme cluster position normalization (e.g. if // due to Unicode grapheme cluster position normalization (e.g. if
......
...@@ -1147,9 +1147,9 @@ TEST_F(InputMethodControllerTest, InsertLineBreakWhileComposingText) { ...@@ -1147,9 +1147,9 @@ TEST_F(InputMethodControllerTest, InsertLineBreakWhileComposingText) {
EXPECT_EQ(5u, Controller().GetSelectionOffsets().End()); EXPECT_EQ(5u, Controller().GetSelectionOffsets().End());
GetFrame().GetEditor().InsertLineBreak(); GetFrame().GetEditor().InsertLineBreak();
EXPECT_STREQ("\n\n", div->innerText().Utf8().data()); EXPECT_STREQ("hello\n\n", div->innerText().Utf8().data());
EXPECT_EQ(1u, Controller().GetSelectionOffsets().Start()); EXPECT_EQ(6u, Controller().GetSelectionOffsets().Start());
EXPECT_EQ(1u, Controller().GetSelectionOffsets().End()); EXPECT_EQ(6u, Controller().GetSelectionOffsets().End());
} }
TEST_F(InputMethodControllerTest, InsertLineBreakAfterConfirmingText) { TEST_F(InputMethodControllerTest, InsertLineBreakAfterConfirmingText) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment