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..d1068fc47dfb7c251998170b9cec3fb90db7c0b2 100644
--- a/third_party/WebKit/Source/core/editing/InputMethodController.cpp
+++ b/third_party/WebKit/Source/core/editing/InputMethodController.cpp
@@ -233,8 +233,34 @@ bool InputMethodController::confirmComposition(const String& text, ConfirmCompos
     return true;
 }
 
-bool InputMethodController::confirmCompositionOrInsertText(const String& text, ConfirmCompositionBehavior confirmBehavior)
+bool InputMethodController::confirmCompositionOrInsertText(const String& text, ConfirmCompositionBehavior confirmBehavior, int newCursorPosition)
 {
+    if (confirmBehavior == DoNotKeepSelection) {
+        // If newCursorPosition > 0, it's relative to the end of the text - 1;

[aelias_OOO_until_Jul13, 2016/08/24 08:13:07]

Please add a comment that this is to match Android behavior with a link to
https://developer.android.com/reference/android/view/inputmethod/InputConnect...,
int)

[yabinh, 2016/08/25 02:32:21]

Done.

+        // if newCursorPosition <= 0, it is relative to the start of the text.
+        if (newCursorPosition > 0)
+            newCursorPosition += text.length() - 1;
+
+        // The new cursor position is relative to the composing text if any, or
+        // relative to the selection if no composing text.
+        if (hasComposition()) {
+            Element* rootEditableElement = frame().selection().rootEditableElement();
+            if (!rootEditableElement)
+                return false;
+            PlainTextRange compositionRange = PlainTextRange::create(*rootEditableElement, *m_compositionRange);
+
+            if (text.length() == 0)
+                newCursorPosition += compositionRange.end();

[aelias_OOO_until_Jul13, 2016/08/24 08:13:07]

I read your explanation above that text.length() == 0 effectively means the
current composition is considered the text.  I think this should be expressed
more explicitly, along the lines of:

bool InputMethodController::confirmCompositionOrInsertText(... , int
relativeCursorPosition)

int textStart, textLength, absoluteCursorPosition;
textStart = compositionRange.start() [or getSelectionOffsets().start() ];
textLength = text.length() ? text.length() : compositionRange.length();
absoluteCursorPosition = relativeCursorPosition + textStart - 1;
if (relativeCursorPosition > 0) absoluteCursorPosition += textLength;

[yabinh, 2016/08/25 02:32:21]

Done.

+            else
+                newCursorPosition += compositionRange.start();
+        } else {
+            PlainTextRange selectionRange = getSelectionOffsets();
+            if (selectionRange.isNull())
+                return false;
+            newCursorPosition += getSelectionOffsets().start();
+        }
+    }
+
     if (!hasComposition()) {
         if (!text.length())
             return false;
@@ -243,19 +269,25 @@ bool InputMethodController::confirmCompositionOrInsertText(const String& text, C
             return false;
 
         editor().insertText(text, 0);
-        return true;
+    } else if (text.length()) {

[aelias_OOO_until_Jul13, 2016/08/24 08:13:07]

The code flow is getting difficult to follow.  The reason is that the behavior
varies too much based on whether we have KeepSelection or not.  This calls for a
small refactoring to split into two methods.  Here's what I propose:

1. Rename existing confirmComposition(text, ConfirmCompositionBehavior) to
replaceComposition() and make this method private (it seems to have no outside
callers).
2. Split "confirmCompositionOrInsertText(text, KeepSelection, 1) ->
"confirmComposition(text)"
3. Split "confirmCompositionOrInsertText(text, DoNotKeepSelection,
relativeCursorPos" -> "confirmCompositionWithCursor(text, relativeCursorPos)"

and WebViewImpl::confirmComposition should call the right one based on the value
of KeepSelection.

[yabinh, 2016/08/25 02:32:21]

For#1, The old InputMethodController#confirmComposition ( ika the new
inputMethodController#replaceComposition) is called in some places outside
InputMethodController, like InputMethodControllerTest, WebViewImpl,
WebLocalFrameImpl, and WebFrameWidgetImpl. So we'd better keep it public.

+        confirmComposition(text);
+    } else if (confirmBehavior == DoNotKeepSelection) {
+        if (!confirmComposition(composingText(), DoNotKeepSelection))
+            return false;
+    } else {
+        SelectionOffsetsScope selectionOffsetsScope(this);
+        return confirmComposition();
     }
 
-    if (text.length()) {
-        confirmComposition(text);
+    if (confirmBehavior == KeepSelection)
         return true;
-    }
 
-    if (confirmBehavior == DoNotKeepSelection)
-        return confirmComposition(composingText(), DoNotKeepSelection);
+    frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
 
-    SelectionOffsetsScope selectionOffsetsScope(this);
-    return confirmComposition();
+    PlainTextRange selectedRange = createRangeForSelection(newCursorPosition, newCursorPosition, 0);
+    if (selectedRange.isNull())
+        return false;
+    return setEditableSelectionOffsets(selectedRange);
 }
 
 void InputMethodController::cancelComposition()