Handle newCursorPosition correctly for Android's commitText()

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

 /*
  * Copyright (C) 2006, 2007, 2008, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 167 matching lines @@
 
     // The composition can start inside a composed character sequence, so we have to override checks.
     // See <http://bugs.webkit.org/show_bug.cgi?id=15781>
     VisibleSelection selection;
     selection.setWithoutValidation(range.startPosition(), range.endPosition());
     frame().selection().setSelection(selection, 0);
 }
 
 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;
 
     Optional<Editor::RevealSelectionScope> revealSelectionScope;
     if (confirmBehavior == KeepSelection)
         revealSelectionScope.emplace(&editor());
 
     // If the composition was set from existing text and didn't change, then
     // there's nothing to do here (and we should avoid doing anything as that
@@ skipping 24 matching lines @@
     // Event handler might destroy document.
     if (!frame().document())
         return false;
 
     // No DOM update after 'compositionend'.
     dispatchCompositionEndEvent(frame(), text);
 
     return true;
 }
 
-bool InputMethodController::confirmCompositionOrInsertText(const String& text, ConfirmCompositionBehavior confirmBehavior)
+bool InputMethodController::commitComposition(const String& text)
 {
-    if (!hasComposition()) {
-        if (!text.length())
-            return false;
+    if (!hasComposition())
+        return insertText(text);
 
-        if (dispatchBeforeInputInsertText(frame().document()->focusedElement(), text) != DispatchEventResult::NotCanceled)
-            return false;
-
-        editor().insertText(text, 0);
+    if (text.length()) {
+        replaceComposition(text, KeepSelection);
         return true;
     }
 
-    if (text.length()) {
-        confirmComposition(text);
-        return true;
-    }
-
-    if (confirmBehavior == DoNotKeepSelection)
-        return confirmComposition(composingText(), DoNotKeepSelection);
-
     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);
+
+        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;
+
+        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;
+    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;
+
+    frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
+
+    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()
 {
     if (!hasComposition())
         return;
 
     Editor::RevealSelectionScope revealSelectionScope(&editor());
 
     if (frame().selection().isNone())
         return;
 
@@ skipping 65 matching lines @@
     //    Sending a compositionupdate event at this time ensures that at least one
     //    compositionupdate event is dispatched.
     // 2. Updating the existing composition node.
     //    Send a compositionupdate event when this function updates the existing composition
     //    node, i.e. hasComposition() && !text.isEmpty().
     // 3. Canceling the ongoing composition.
     //    Send a compositionend event when function deletes the existing composition node, i.e.
     //    !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.
             TypingCommand::deleteSelection(*frame().document(), TypingCommand::PreventSpellChecking);
         }
 
         setEditableSelectionOffsets(selectedRange);
         return;
     }
 
@@ skipping 218 matching lines @@
     TypingCommand::deleteSelection(*frame().document());
 }
 
 DEFINE_TRACE(InputMethodController)
 {
     visitor->trace(m_frame);
     visitor->trace(m_compositionRange);
 }
 
 } // namespace blink