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..30fb652660c2cdb3543ca57ed3825908da7495e8 100644
--- a/third_party/WebKit/Source/core/editing/InputMethodController.cpp
+++ b/third_party/WebKit/Source/core/editing/InputMethodController.cpp
@@ -185,10 +185,10 @@ void InputMethodController::selectComposition() const
 
 bool InputMethodController::confirmComposition()
 {
-    return confirmComposition(composingText());
+    return replaceComposition(composingText(), KeepSelection);
 }
 
-bool InputMethodController::confirmComposition(const String& text, ConfirmCompositionBehavior confirmBehavior)
+bool InputMethodController::replaceComposition(const String& text, ConfirmCompositionBehavior confirmBehavior)
 {
     if (!hasComposition())
         return false;
@@ -233,29 +233,83 @@ bool InputMethodController::confirmComposition(const String& text, ConfirmCompos
     return true;
 }
 
-bool InputMethodController::confirmCompositionOrInsertText(const String& text, ConfirmCompositionBehavior confirmBehavior)
+bool InputMethodController::commitComposition(const String& text)
 {
-    if (!hasComposition()) {
-        if (!text.length())
+    if (!hasComposition())
+        return insertText(text);
+
+    if (text.length()) {
+        replaceComposition(text, KeepSelection);
+        return true;
+    }
+
+    SelectionOffsetsScope selectionOffsetsScope(this);
+    return confirmComposition();
+}
+
+bool InputMethodController::commitCompositionWithCursor(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;

[yosin_UTC9, 2016/08/26 10:06:11]

Can we introduce other functions to make this complex path simpler?

if (hasCoomposition())
  return insertTextWithCursor(...);
if (text.length())
  return replaceCompositionWithCursor(text, ...);
return replaceCompositionWithCursor(composingText()...);

I feel |int textStart, testLength| is old C-style coding.
  

[yabinh, 2016/08/29 05:49:53]

Done.

+    if (hasComposition()) {
+        Element* rootEditableElement = frame().selection().rootEditableElement();
+        if (!rootEditableElement)
             return false;
+        PlainTextRange compositionRange = PlainTextRange::create(*rootEditableElement, *m_compositionRange);
 
-        if (dispatchBeforeInputInsertText(frame().document()->focusedElement(), text) != DispatchEventResult::NotCanceled)
+        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();

[aelias_OOO_until_Jul13, 2016/08/30 17:26:13]

I don't see any equivalent to this line of logic starting in patch set #10, so
how can the latest patches be correct?

Furthermore, I believe tkent@'s desired semantics aren't feasible to implement
correctly if you bring back the correct behavior.  The up-to-date
compositionRange length is not known from the browser process.

If I'm right, please introduce a unit test covering this case to avoid this
regressing again.

[yabinh, 2016/08/31 11:23:27]

That logic is hidden in replaceCompositionAndMoveCaret. When text is empty,
textLength = composingText().length(); otherwise, textLength = text.length();
I'm not sure if it matters.
relativeCaretPosition is relative to the end of text, so we only need to be
careful with the case when replaceCompositionAndMoveCaret(composingText(),
relativeCaretPosition) is called with relativeCaretPosition < 0 in
InputMethodController::commitCompositionAndMoveCaret, because |composingText()|
here might be changed, and different from that in ime_adapter_android.

But in ImeTest, it's impossible to go to
replaceCompositionAndMoveCaret(composingText(), relativeCaretPosition). So it's
not a problem at least in ImeTest.

But there is another problem. We are able to set the caret within the composing
text with this patch. (See WebViewTest.cpp in patch set 14.) 
I'm not sure if anyone wants to do that, at least android doesn't want because
of the similar concern. 
(See
https://developer.android.com/reference/android/view/inputmethod/InputConnect...,
int)). 
Luckily, we are still unable to set the caret through ImeAdapter, because we
have some conversion in ime_adapter_android.cc. 

+    } else {
+        PlainTextRange selectionRange = getSelectionOffsets();
+        if (selectionRange.isNull())
             return false;
 
-        editor().insertText(text, 0);
-        return true;
+        textStart = getSelectionOffsets().start();
+        textLength = text.length();
     }
 
-    if (text.length()) {
-        confirmComposition(text);
-        return true;
+    // 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;
+    int absoluteCursorPosition = textStart + relativeCursorPosition;
+
+    bool result = false;
+    if (!hasComposition()) {
+        result = insertText(text);
+    } else if (text.length()) {
+        result = replaceComposition(text, DoNotKeepSelection);
+    } else {
+        result = replaceComposition(composingText(), DoNotKeepSelection);
     }
+    if (!result)
+        return false;
 
-    if (confirmBehavior == DoNotKeepSelection)
-        return confirmComposition(composingText(), DoNotKeepSelection);
+    frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
 
-    SelectionOffsetsScope selectionOffsetsScope(this);
-    return confirmComposition();
+    PlainTextRange selectedRange = createRangeForSelection(absoluteCursorPosition, absoluteCursorPosition, 0);
+    if (selectedRange.isNull())
+        return false;
+    return setEditableSelectionOffsets(selectedRange);
+}
+
+bool InputMethodController::insertText(const String& text)
+{
+    if (!text.length())
+        return false;
+
+    if (dispatchBeforeInputInsertText(frame().document()->focusedElement(), text) != DispatchEventResult::NotCanceled)
+        return false;
+
+    editor().insertText(text, 0);
+    return true;
 }
 
 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(), KeepSelection);
         } 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.