Reland: Fix setComposingText with empty text when newCursorPosition != 1

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 303 matching lines @@
 
     selectComposition();
 
     if (frame().selection().isNone())
         return;
 
     Element* target = frame().document()->focusedElement();
     if (!target)
         return;
 
+    int selectionOffsetsStart = static_cast<int>(getSelectionOffsets().start());
+    int start = selectionOffsetsStart + selectionStart;
+    int end = selectionOffsetsStart + selectionEnd;
+    PlainTextRange selectedRange = createRangeForSelection(start, end, text.length());
+    if (selectedRange.isNull())
+        return;
+
     // Dispatch an appropriate composition event to the focused node.
     // We check the composition status and choose an appropriate composition event since this
     // function is used for three purposes:
     // 1. Starting a new composition.
     //    Send a compositionstart and a compositionupdate event when this function creates
     //    a new composition node, i.e.
     //    !hasComposition() && !text.isEmpty().
     //    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());
-            return;
+        } 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);
         }
-        // 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;
     }
 
     // We should send a 'compositionstart' event only when the given text is not empty because this
     // function doesn't create a composition node when the text is empty.
     if (!hasComposition()) {
         target->dispatchEvent(CompositionEvent::create(EventTypeNames::compositionstart, frame().domWindow(), frame().selectedText()));
         if (!frame().document())
             return;
     }
@@ skipping 26 matching lines @@
     m_isDirty = true;
     m_hasComposition = true;
     if (!m_compositionRange)
         m_compositionRange = Range::create(baseNode->document());
     m_compositionRange->setStart(baseNode, baseOffset);
     m_compositionRange->setEnd(baseNode, extentOffset);
 
     if (baseNode->layoutObject())
         baseNode->layoutObject()->setShouldDoFullPaintInvalidation();
 
-    // In case of exceeding the left boundary.
-    int selectionOffsetsStart = static_cast<int>(getSelectionOffsets().start());
-    int start = std::max(selectionOffsetsStart + selectionStart, 0);
-    int end = std::max(selectionOffsetsStart + selectionEnd, start);
-
-    Element* rootEditableElement = frame().selection().rootEditableElement();
-    if (!rootEditableElement)
-        return;
-
-    // In case of exceeding the right boundary.
-    // If both |value1| and |value2| exceed right boundary,
-    // PlainTextRange(value1, value2)::createRange() will return a default
-    // value, which is [0,0]. In order to get the correct Position in that case,
-    // we should make sure |value1| is within range at least.
-    const EphemeralRange& startRange = PlainTextRange(0, start).createRange(*rootEditableElement);
-    const EphemeralRange& endRange = PlainTextRange(0, end).createRange(*rootEditableElement);
-
-    // TODO(yabinh): There should be a better way to create |startPosition| and
-    // |endPosition|. But for now, since we can't get |anchorNode| and |offset|,
-    // we can't create the 2 Position objects directly. So we use
-    // PlainTextRange::createRange as a workaround.
-    const Position& startPosition = startRange.endPosition();
-    const Position& endPosition = endRange.endPosition();
-    const EphemeralRange selectedRange(startPosition, endPosition);
-    frame().selection().setSelectedRange(selectedRange, TextAffinity::Downstream, SelectionDirectionalMode::NonDirectional, NotUserTriggered);
+    setEditableSelectionOffsets(selectedRange);
 
     if (underlines.isEmpty()) {
         frame().document()->markers().addCompositionMarker(m_compositionRange->startPosition(), m_compositionRange->endPosition(), Color::black, false, LayoutTheme::theme().platformDefaultCompositionBackgroundColor());
         return;
     }
     for (const auto& underline : underlines) {
         unsigned underlineStart = baseOffset + underline.startOffset;
         unsigned underlineEnd = baseOffset + underline.endOffset;
         EphemeralRange ephemeralLineRange = EphemeralRange(Position(baseNode, underlineStart), Position(baseNode, underlineEnd));
         if (ephemeralLineRange.isNull())
@@ skipping 80 matching lines @@
     return frame().selection().setSelectedRange(range, VP_DEFAULT_AFFINITY, SelectionDirectionalMode::NonDirectional, FrameSelection::CloseTyping);
 }
 
 bool InputMethodController::setEditableSelectionOffsets(const PlainTextRange& selectionOffsets)
 {
     if (!editor().canEdit())
         return false;
     return setSelectionOffsets(selectionOffsets);
 }
 
+PlainTextRange InputMethodController::createRangeForSelection(int start, int end, size_t textLength)
+{
+    // In case of exceeding the left boundary.
+    start = std::max(start, 0);

[yosin_UTC9, 2016/06/07 08:11:20]

Adjusting |start| and |end| should be done in call sites rather than here.
Passing wrong range indicates bad thing happened in call sites.
This kind of treatment is error prone and makes hard to find bug in callers.

[yabinh, 2016/06/07 08:47:50]

It's easy to handle the left boundary. But for the right boundary, we need do
more things.

Do we need to apply boundary-checking logic in InputMethodController, or
RenderWidget, or other place?

FYI, there are other issues also need this logic, like:
https://codereview.chromium.org/1995333002/
https://codereview.chromium.org/1889053003/

Another question. PlainTextRange::create(start, end) can return a range staying
at right boudary when |start| or |end| exceeds right boundary. But when both of
them exceed the right boundary, it will return a default range [0,0], instead of
staying at the right boundary. If we change it to let it stay at the right
boundary, most of code in this function will be unnecessary. The problem is, if
we do that, many Layout Tests will fail.
Do you think we should keep the logic in this function, or we should change
PlainTextRange::create()?

+    end = std::max(end, start);
+
+    // In case of exceeding the right boundary.
+    Element* rootEditableElement = frame().selection().rootEditableElement();
+    if (!rootEditableElement)
+        return PlainTextRange();
+    const EphemeralRange& range = EphemeralRange::rangeOfContents(*rootEditableElement);
+
+    TextIteratorBehaviorFlags behaviorFlags = TextIteratorEmitsObjectReplacementCharacter;

[yosin_UTC9, 2016/06/07 08:11:20]

nit: Should have |const|. We can express this in one statement.

+    behaviorFlags |= TextIteratorEmitsCharactersBetweenAllVisiblePositions;
+    TextIterator it(range.startPosition(), range.endPosition(), behaviorFlags);
+
+    int rightBoundary = 0;
+    for (; !it.atEnd(); it.advance()) {

[yosin_UTC9, 2016/06/07 08:11:20]

nit: No need to have {}

+        rightBoundary += it.length();
+    }
+
+    if (hasComposition())
+        rightBoundary -= (compositionRange()->endOffset() - compositionRange()->startOffset());
+
+    rightBoundary += textLength;
+
+    start = std::min(start, rightBoundary);
+    end = std::min(end, rightBoundary);
+
+    return PlainTextRange(start, end);
+}
+
 void InputMethodController::extendSelectionAndDelete(int before, int after)
 {
     if (!editor().canEdit())
         return;
     PlainTextRange selectionOffsets(getSelectionOffsets());
     if (selectionOffsets.isNull())
         return;
 
     // A common call of before=1 and after=0 will fail if the last character
     // is multi-code-word UTF-16, including both multi-16bit code-points and
@@ skipping 20 matching lines @@
     TypingCommand::deleteSelection(*frame().document());
 }
 
 DEFINE_TRACE(InputMethodController)
 {
     visitor->trace(m_frame);
     visitor->trace(m_compositionRange);
 }
 
 } // namespace blink