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 e36cea49358ce24b90a500afbec5b97eb29f8242..ec71e2aa4bfa78c27d564891d18ccf876133cdc0 100644
--- a/third_party/WebKit/Source/core/editing/InputMethodController.cpp
+++ b/third_party/WebKit/Source/core/editing/InputMethodController.cpp
@@ -233,8 +233,35 @@ 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;
+        // if newCursorPosition <= 0, it is relative to the start of the text.
+        if (newCursorPosition > 0)
+            newCursorPosition = newCursorPosition + text.length() - 1;

[Changwan Ryu, 2016/08/03 07:37:30]

newCursorPosition += text.length() - 1;

[yabinh, 2016/08/08 07:33:43]

Done.

+
+        // According to Android documentation, 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;

[yabinh, 2016/08/03 05:12:09]

Note that |m_compositionRange->startOffset()| is relative to its  anchor node,
not root node. So we need to convert it for multiple nodes.

+            PlainTextRange compositionRange = PlainTextRange::create(*rootEditableElement, *m_compositionRange);
+
+            if (text.length() == 0)
+                newCursorPosition += compositionRange.end();

[Changwan Ryu, 2016/08/03 07:37:30]

Why should this differ from the else line?

[yabinh, 2016/08/08 07:33:44]

There are 2 cases when hasCompositio() == true:


Case 1. text.length() != 0
This happens when we call ImeTest#setComposingText("hello", newCursorPosition),
then call ImeTest#commitText("world", newCursorPosition).

In this case, we should drop the composing "hello", and insert the committing
"world".


Case 2.text.length() == 0
This happens in the test RenderViewImplTest#ImeComposition. 
(https://cs.chromium.org/chromium/src/content/renderer/render_view_browsertest...)

(in Scenario 2 and Scenario 3)

Firstly, we call RenderWidget#OnImeSetComposition to set the composing text. 
Then we call RenderWidget#OnImeConfirmComposition with empty text(I.E.,
IME_CONFIRMCOMPOSITION, false, -1, -1, L"", L"\x6F22\x5B57"). 
Here, we mean to confirm the composing text, not to replace it with empty text.

By the way, it may seem confusing here. Consider such a case: if we call
ImeTest#commitText("", newCursorPosition) after
ImeTest#setComposingText("hello", newCursorPosition), will it cancel the
composing "hello", or confirm it? Actually, there is no problem in this case,
because it won't go to WebWidget#confirmComposition(). Instead, it will go to
WebWidget#setComposition.

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

Acknowledged, but checking "text.length() == 0" is a nonobvious way to make this
distinction.  On Android, there is no such thing as "confirm with no text", so
it sounds like what is going on here is different platform-specific semantics. 
We should check for this more explicitly instead of relying on implicit signals
like "text == 0".  For example, does the enum value "ConfirmCompositionBehavior"
help?  If not, can we add another enum value, or possibly make the other
platforms not run this code at all?

[aelias_OOO_until_Jul13, 2016/08/24 08:16:01]

Sorry, please disregard this particular comment, it's a draft comment I meant to
delete (I realized later it's not platform-specific at all really).  I proposed
a clearer common way of handling text == 0 below.

+            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 +270,23 @@ bool InputMethodController::confirmCompositionOrInsertText(const String& text, C
             return false;
 
         editor().insertText(text, 0);
-        return true;
-    }
-
-    if (text.length()) {
+    } else if (text.length()) {
         confirmComposition(text);
-        return true;
+    } else if (confirmBehavior == DoNotKeepSelection) {
+        if (!confirmComposition(composingText(), DoNotKeepSelection))
+            return false;
+    } else {
+        SelectionOffsetsScope selectionOffsetsScope(this);
+        return confirmComposition();
     }
 
-    if (confirmBehavior == DoNotKeepSelection)
-        return confirmComposition(composingText(), DoNotKeepSelection);
+    if (confirmBehavior == KeepSelection)
+        return true;
 
-    SelectionOffsetsScope selectionOffsetsScope(this);
-    return confirmComposition();
+    PlainTextRange selectedRange = createRangeForSelection(newCursorPosition, newCursorPosition, 0);
+    if (selectedRange.isNull())
+        return false;
+    return setEditableSelectionOffsets(selectedRange);
 }
 
 void InputMethodController::cancelComposition()
@@ -524,6 +555,9 @@ PlainTextRange InputMethodController::createRangeForSelection(int start, int end
     if (range.isNull())
         return PlainTextRange();
 

[yabinh, 2016/08/03 05:12:09]

There is a DCHECK in the constructor of TextIterator:
DCHECK(!start.document()->needsLayoutTreeUpdate());

So it's better to make sure the condition is met here.

+    if (frame().document()->needsLayoutTreeUpdate())

[Changwan Ryu, 2016/08/03 07:37:30]

You probably need to call updateStyleAndLayoutTree() somewhere else.

[yabinh, 2016/08/08 07:33:44]

I tried to call it in InputMethodController#confirmCompositionOrInsertText, but
it doesn't work. However, updateStyleAndLayoutIgnorePendingStylesheets() works.
It's also called in InputMethodController#setComposition after inserting text.

+        return PlainTextRange();
+
     const TextIteratorBehaviorFlags behaviorFlags = TextIteratorEmitsObjectReplacementCharacter | TextIteratorEmitsCharactersBetweenAllVisiblePositions;
     TextIterator it(range.startPosition(), range.endPosition(), behaviorFlags);