Handle newCursorPosition correctly for Android's commitText()

third_party/WebKit/Source/core/editing/InputMethodController.cpp

Index: third_party/WebKit/Source/core/editing/InputMethodController.cpp
diff --git a/third_party/WebKit/Source/core/editing/InputMethodController.cpp b/third_party/WebKit/Source/core/editing/InputMethodController.cpp
index 343e89aad7a0aaf3644dca5f79449d6d75138bc9..09d563945dc910551ec4766dda2ea651b132ab67 100644
--- a/third_party/WebKit/Source/core/editing/InputMethodController.cpp
+++ b/third_party/WebKit/Source/core/editing/InputMethodController.cpp
@@ -183,12 +183,12 @@ void InputMethodController::selectComposition() const
     frame().selection().setSelection(selection, 0);
 }
 
-bool InputMethodController::confirmComposition()
+bool InputMethodController::replaceComposition()
 {
-    return confirmComposition(composingText());
+    return replaceComposition(composingText());
 }
 
-bool InputMethodController::confirmComposition(const String& text, ConfirmCompositionBehavior confirmBehavior)
+bool InputMethodController::replaceComposition(const String& text, ConfirmCompositionBehavior confirmBehavior)
 {
     if (!hasComposition())
         return false;
@@ -233,7 +233,7 @@ bool InputMethodController::confirmComposition(const String& text, ConfirmCompos
     return true;
 }
 
-bool InputMethodController::confirmCompositionOrInsertText(const String& text, ConfirmCompositionBehavior confirmBehavior)
+bool InputMethodController::confirmComposition(const String& text)
 {
     if (!hasComposition()) {
         if (!text.length())
@@ -247,15 +247,69 @@ bool InputMethodController::confirmCompositionOrInsertText(const String& text, C
     }
 
     if (text.length()) {
-        confirmComposition(text);
+        replaceComposition(text);
         return true;
     }
 
-    if (confirmBehavior == DoNotKeepSelection)
-        return confirmComposition(composingText(), DoNotKeepSelection);
-
     SelectionOffsetsScope selectionOffsetsScope(this);
-    return confirmComposition();
+    return replaceComposition();
+}
+
+bool InputMethodController::confirmCompositionWithCursor(const String& text, int relativeCursorPosition)
+{
+    // The relativeCursorPosition is relative to the composing text if any, or
+    // relative to the selection if no composing text.
+    // This is to match Android behavior. See:
+    // https://developer.android.com/reference/android/view/inputmethod/InputConnection.html#commitText(java.lang.CharSequence, int)
+    int textStart, textLength, absoluteCursorPosition;
+    if (hasComposition()) {
+        Element* rootEditableElement = frame().selection().rootEditableElement();
+        if (!rootEditableElement)
+            return false;
+        PlainTextRange compositionRange = PlainTextRange::create(*rootEditableElement, *m_compositionRange);
+
+        textStart = compositionRange.start();
+
+        // If |text| is empty, we will confirm the composing text; otherwise, we
+        // will insert the |text|.
+        textLength = text.length() ? text.length() : compositionRange.length();
+    } else {
+        PlainTextRange selectionRange = getSelectionOffsets();
+        if (selectionRange.isNull())
+            return false;
+
+        textStart = getSelectionOffsets().start();
+        textLength = text.length();
+    }
+
+    // If relativeCursorPosition > 0, it's relative to the end of the text - 1;
+    // if relativeCursorPosition <= 0, it is relative to the start of the text.
+    // This is also to match Android behavior.
+    if (relativeCursorPosition > 0)
+        relativeCursorPosition += textLength - 1;
+    absoluteCursorPosition = textStart + relativeCursorPosition;

[aelias_OOO_until_Jul13, 2016/08/25 03:02:10]

Since it isn't used before this line, please declare absoluteCursorPosition here
instead of above.

[yabinh, 2016/08/25 10:27:08]

Done.

+
+    if (!hasComposition()) {
+        if (!text.length())

[aelias_OOO_until_Jul13, 2016/08/25 02:50:57]

Can you move this duplicate code into a new private method "bool
insertText(String)?

[yabinh, 2016/08/25 10:27:08]

Done.

+            return false;
+
+        if (dispatchBeforeInputInsertText(frame().document()->focusedElement(), text) != DispatchEventResult::NotCanceled)
+            return false;
+
+        editor().insertText(text, 0);
+    } else if (text.length()) {
+        replaceComposition(text);

[aelias_OOO_until_Jul13, 2016/08/25 02:50:57]

Wouldn't it be simpler/more efficient to use DoNotKeepSelection here?

[yabinh, 2016/08/25 10:27:08]

Done.

+    } else {
+        if (!replaceComposition(composingText(), DoNotKeepSelection))
+            return false;

[aelias_OOO_until_Jul13, 2016/08/25 03:02:10]

Why do we return false here but not in the clause above in the same scenario?

[yabinh, 2016/08/25 10:27:08]

It seems it makes no difference to return false or not. So we'll all return
false in those scenarios in the latest patch.

+    }
+
+    frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
+
+    PlainTextRange selectedRange = createRangeForSelection(absoluteCursorPosition, absoluteCursorPosition, 0);
+    if (selectedRange.isNull())
+        return false;
+    return setEditableSelectionOffsets(selectedRange);
 }
 
 void InputMethodController::cancelComposition()
@@ -343,7 +397,7 @@ void InputMethodController::setComposition(const String& text, const Vector<Comp
     //    !hasComposition() && test.isEmpty().
     if (text.isEmpty()) {
         if (hasComposition()) {
-            confirmComposition(emptyString());
+            replaceComposition(emptyString());
         } else {
             // It's weird to call |setComposition()| with empty text outside composition, however some IME
             // (e.g. Japanese IBus-Anthy) did this, so we simply delete selection without sending extra events.