Introduce InsertIncrementalTextCommand to respect existing style for composition

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 50 matching lines @@
 void dispatchCompositionEndEvent(LocalFrame& frame, const String& text) {
   Element* target = frame.document()->focusedElement();
   if (!target)
     return;
 
   CompositionEvent* event = CompositionEvent::create(
       EventTypeNames::compositionend, frame.domWindow(), text);
   target->dispatchEvent(event);
 }
 
+bool static needsIncrementalInsertion(const LocalFrame& frame,

[chongz, 2016/12/02 20:34:44]

I was under the impression that the style guide prefers anonymous namespace than
static functions, yosin@ can you confirm please?
e.g. Remove |static| since it's already inside anonymous namespace.

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

Done.

+                                      const String& newText) {
+  DCHECK(frame.document());
+
+  // Since 'input' element doesn't support formated text, there is no need to
+  // apply incremental insertion.
+  if (frame.document()->focusedElement()->tagName() == String("INPUT"))

[chongz, 2016/12/02 20:34:44]

Maybe |!frame.editor().canEditRichly()|? e.g. We don't need it for "TEXTAREA" as
well.

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

Done.

+    return false;
+
+  // No need to apply incremental insertion if the old text (text to be
+  // replaced) or the new text (text to be inserted) is empty.
+  if (frame.selectedText().isEmpty() || newText.isEmpty())
+    return false;
+
+  return true;
+}
+
 // Used to insert/replace text during composition update and confirm
 // composition.
 // Procedure:
 //   1. Fire 'beforeinput' event for (TODO(chongz): deleted composed text) and
 //      inserted text
 //   2. Fire 'compositionupdate' event
 //   3. Fire TextEvent and modify DOM
 //   TODO(chongz): 4. Fire 'input' event
 void insertTextDuringCompositionWithEvents(
     LocalFrame& frame,
@@ skipping 34 matching lines @@
 
   // 'beforeinput' event handler may destroy document.
   if (!frame.document())
     return;
 
   dispatchCompositionUpdateEvent(frame, text);
   // 'compositionupdate' event handler may destroy document.
   if (!frame.document())
     return;
 
+  // TODO(xiaochengh): The use of updateStyleAndLayoutIgnorePendingStylesheets
+  // needs to be audited. see http://crbug.com/590369 for more details.
+  frame.document()->updateStyleAndLayoutIgnorePendingStylesheets();
+
+  const bool isIncrementalInsertion = needsIncrementalInsertion(frame, text);
+
   switch (compositionType) {
     case TypingCommand::TextCompositionType::TextCompositionUpdate:
+    case TypingCommand::TextCompositionType::TextCompositionConfirm:

[chongz, 2016/12/02 20:34:44]

Sorry for the confusion from https://crbug.com/620549#c30 to
https://crbug.com/620549#c34, I wasn't suggesting calling |TypingCommand| for
|TextCompositionConfirm|.

My suggestions are:
1. For |TextCompositionUpdate|: Keep using |TypingCommand| (or
|TypingForCompositionCommand| as suggested by yosion@), which should then call
|InsertIncrementalTextCommand| (Already done)
2. For |TextCompositionConfirm/Cancel|: Don't switch to |TypingCommand| until
|TextEvent| has been fully removed (Should at least be in another patch)
For now I'd suggest something like:
```
case |TextCompositionConfirm/Cancel|:
    const bool textEventCanceled =
frame.eventHandler().handleTextInputEvent(..., TextEventInputComposition)
    // Check if |frame| was destroyed maybe
    If (!textEventCanceled)
        InsertIncrementalTextCommand::create(...)->apply()
```

Also in |Editor::handleTextEvent()| we should return false if
|event->isComposition()|

Does that make sense?

Also see previous comment:
https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour...

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

Sorry I still don't understand why we can't switch to TypingCommand for
TextCompositionConfirm since TextInputEvent is kept in this patch. If we decide
to remove TextInputEvent in the future, we can simply stop calling
dispatchTextInputEvent().

I'm not sure if my understanding is correct, but
frame.eventHandler().handleTextInputEvent() deletes the entire composing text
and inserts new text. So we will lose the style before calling
InsertIncrementalTextCommand::create(...)->apply().
Also, it will fire duplicate 'input' event if we call both.
So we need stop calling frame.eventHandler().handleTextInputEvent() for
TextCompositionConfirm case.

[chongz, 2016/12/05 09:01:46]

My concerns are:
1. Moving |TextEvent| into |TypingCommand| might make it even more complex;
2. The hierarchy doesn't look very straightforward to me as |TextEvent| also has
the ability to trigger |TypingCommand|.

But I won't block on this as long as yosin@ is happy and |TextEvent| keeps the
original behavior.

       TypingCommand::insertText(*frame.document(), text, options,
-                                compositionType);
+                                compositionType, isIncrementalInsertion);
       break;
-    case TypingCommand::TextCompositionType::TextCompositionConfirm:
     case TypingCommand::TextCompositionType::TextCompositionCancel:
       // TODO(chongz): Use TypingCommand::insertText after TextEvent was
       // removed. (Removed from spec since 2012)
       // See TextEvent.idl.
       frame.eventHandler().handleTextInputEvent(text, 0,
                                                 TextEventInputComposition);
       break;
     default:
       NOTREACHED();
   }
@@ skipping 23 matching lines @@
   return element->fastGetAttribute(HTMLNames::inputmodeAttr).lower();
 }
 
 }  // anonymous namespace
 
 InputMethodController* InputMethodController::create(LocalFrame& frame) {
   return new InputMethodController(frame);
 }
 
 InputMethodController::InputMethodController(LocalFrame& frame)
-    : m_frame(&frame), m_isDirty(false), m_hasComposition(false) {}
+    : m_frame(&frame), m_hasComposition(false) {}
 
 InputMethodController::~InputMethodController() = default;
 
 bool InputMethodController::isAvailable() const {
   return frame().document();
 }
 
 Document& InputMethodController::document() const {
   DCHECK(isAvailable());
   return *frame().document();
 }
 
 bool InputMethodController::hasComposition() const {
   return m_hasComposition && !m_compositionRange->collapsed() &&
          m_compositionRange->isConnected();
 }
 
 inline Editor& InputMethodController::editor() const {
   return frame().editor();
 }
 
 void InputMethodController::clear() {
   m_hasComposition = false;
   if (m_compositionRange) {
     m_compositionRange->setStart(&document(), 0);
     m_compositionRange->collapse(true);
   }
   document().markers().removeMarkers(DocumentMarker::Composition);
-  m_isDirty = false;
 }
 
 void InputMethodController::contextDestroyed() {
   clear();
   m_compositionRange = nullptr;
 }
 
 void InputMethodController::documentAttached(Document* document) {
   DCHECK(document);
   setContext(document);
@@ skipping 48 matching lines @@
   // duplicate selection change event.
   if (!text.length() && !relativeCaretPosition)
     return false;
   return insertTextAndMoveCaret(text, relativeCaretPosition);
 }
 
 bool InputMethodController::replaceComposition(const String& text) {
   if (!hasComposition())
     return false;
 
-  // 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
-  // may clobber multi-node styled text).
-  if (!m_isDirty && composingText() == text) {
-    clear();
-    return true;
-  }
-
   // Select the text that will be deleted or replaced.
   selectComposition();
 
   if (frame().selection().isNone())
     return false;
 
   if (!isAvailable())
     return false;
 
   // If text is empty, then delete the old composition here. If text is
@@ skipping 114 matching lines @@
   if (!selection.isNone() && !m_compositionRange->collapsed()) {
     if (selection.start().compareTo(m_compositionRange->startPosition()) >= 0 &&
         selection.end().compareTo(m_compositionRange->endPosition()) <= 0)
       return;
   }
 
   cancelComposition();
   frame().chromeClient().didCancelCompositionOnSelectionChange();
 }
 
-static size_t computeCommonPrefixLength(const String& str1,
-                                        const String& str2) {
-  const size_t maxCommonPrefixLength = std::min(str1.length(), str2.length());
-  for (size_t index = 0; index < maxCommonPrefixLength; ++index) {
-    if (str1[index] != str2[index])
-      return index;
-  }
-  return maxCommonPrefixLength;
-}
-
-static size_t computeCommonSuffixLength(const String& str1,
-                                        const String& str2) {
-  const size_t length1 = str1.length();
-  const size_t length2 = str2.length();
-  const size_t maxCommonSuffixLength = std::min(length1, length2);
-  for (size_t index = 0; index < maxCommonSuffixLength; ++index) {
-    if (str1[length1 - index - 1] != str2[length2 - index - 1])
-      return index;
-  }
-  return maxCommonSuffixLength;
-}
-
 // If current position is at grapheme boundary, return 0; otherwise, return the
 // distance to its nearest left grapheme boundary.
 static size_t computeDistanceToLeftGraphemeBoundary(const Position& position) {
   const Position& adjustedPosition = previousPositionOf(
       nextPositionOf(position, PositionMoveType::GraphemeCluster),
       PositionMoveType::GraphemeCluster);
   DCHECK_EQ(position.anchorNode(), adjustedPosition.anchorNode());
   DCHECK_GE(position.computeOffsetInContainerNode(),
             adjustedPosition.computeOffsetInContainerNode());
   return static_cast<size_t>(position.computeOffsetInContainerNode() -
                              adjustedPosition.computeOffsetInContainerNode());
 }
 
-static size_t computeCommonGraphemeClusterPrefixLengthForSetComposition(
-    const String& oldText,
-    const String& newText,
-    const Element* rootEditableElement) {
-  const size_t commonPrefixLength = computeCommonPrefixLength(oldText, newText);
-
-  // For grapheme cluster, we should adjust it for grapheme boundary.
-  const EphemeralRange& range =
-      PlainTextRange(0, commonPrefixLength).createRange(*rootEditableElement);
-  if (range.isNull())
-    return 0;
-  const Position& position = range.endPosition();
-  const size_t diff = computeDistanceToLeftGraphemeBoundary(position);
-  DCHECK_GE(commonPrefixLength, diff);
-  return commonPrefixLength - diff;
-}
-
 // If current position is at grapheme boundary, return 0; otherwise, return the
 // distance to its nearest right grapheme boundary.
 static size_t computeDistanceToRightGraphemeBoundary(const Position& position) {
   const Position& adjustedPosition = nextPositionOf(
       previousPositionOf(position, PositionMoveType::GraphemeCluster),
       PositionMoveType::GraphemeCluster);
   DCHECK_EQ(position.anchorNode(), adjustedPosition.anchorNode());
   DCHECK_GE(adjustedPosition.computeOffsetInContainerNode(),
             position.computeOffsetInContainerNode());
   return static_cast<size_t>(adjustedPosition.computeOffsetInContainerNode() -
                              position.computeOffsetInContainerNode());
 }
 
-static size_t computeCommonGraphemeClusterSuffixLengthForSetComposition(
-    const String& oldText,
-    const String& newText,
-    const Element* rootEditableElement) {
-  const size_t commonSuffixLength = computeCommonSuffixLength(oldText, newText);
-
-  // For grapheme cluster, we should adjust it for grapheme boundary.
-  const EphemeralRange& range =
-      PlainTextRange(0, oldText.length() - commonSuffixLength)
-          .createRange(*rootEditableElement);
-  if (range.isNull())
-    return 0;
-  const Position& position = range.endPosition();
-  const size_t diff = computeDistanceToRightGraphemeBoundary(position);
-  DCHECK_GE(commonSuffixLength, diff);
-  return commonSuffixLength - diff;
-}
-
-void InputMethodController::setCompositionWithIncrementalText(
-    const String& text,
-    const Vector<CompositionUnderline>& underlines,
-    int selectionStart,
-    int selectionEnd) {
-  Element* editable = frame().selection().rootEditableElement();
-  if (!editable)
-    return;
-
-  DCHECK_LE(selectionStart, selectionEnd);
-  String composing = composingText();
-  const size_t commonPrefixLength =
-      computeCommonGraphemeClusterPrefixLengthForSetComposition(composing, text,
-                                                                editable);
-
-  // We should ignore common prefix when finding common suffix.
-  const size_t commonSuffixLength =
-      computeCommonGraphemeClusterSuffixLengthForSetComposition(
-          composing.right(composing.length() - commonPrefixLength),
-          text.right(text.length() - commonPrefixLength), editable);
-
-  const bool inserting =
-      text.length() > commonPrefixLength + commonSuffixLength;
-  const bool deleting =
-      composing.length() > commonPrefixLength + commonSuffixLength;
-
-  if (inserting || deleting) {
-    // Select the text to be deleted.
-    const size_t compositionStart =
-        PlainTextRange::create(*editable, compositionEphemeralRange()).start();
-    const size_t deletionStart = compositionStart + commonPrefixLength;
-    const size_t deletionEnd =
-        compositionStart + composing.length() - commonSuffixLength;
-    const EphemeralRange& deletionRange =
-        PlainTextRange(deletionStart, deletionEnd).createRange(*editable);
-    Document& currentDocument = document();
-    frame().selection().setSelection(
-        SelectionInDOMTree::Builder().setBaseAndExtent(deletionRange).build(),
-        0);
-    clear();
-
-    // FrameSeleciton::setSelection() can change document associate to |frame|.
-    if (!isAvailable() || currentDocument != document())
-      return;
-    if (!currentDocument.focusedElement())
-      return;
-
-    // Insert the incremental text.
-    const size_t insertionLength =
-        text.length() - commonPrefixLength - commonSuffixLength;
-    const String& insertingText =
-        text.substring(commonPrefixLength, insertionLength);
-    insertTextDuringCompositionWithEvents(frame(), insertingText,
-                                          TypingCommand::PreventSpellChecking,
-                                          TypingCommand::TextCompositionUpdate);
-
-    // Event handlers might destroy document.
-    if (!isAvailable() || currentDocument != document())
-      return;
-
-    // TODO(yosin): The use of updateStyleAndLayoutIgnorePendingStylesheets
-    // needs to be audited. see http://crbug.com/590369 for more details.
-    document().updateStyleAndLayoutIgnorePendingStylesheets();
-
-    // Now recreate the composition starting at its original start, and
-    // apply the specified final selection offsets.
-    setCompositionFromExistingText(underlines, compositionStart,
-                                   compositionStart + text.length());
-  }
-
-  selectComposition();
-
-  // TODO(xiaochengh): The use of updateStyleAndLayoutIgnorePendingStylesheets
-  // needs to be audited. see http://crbug.com/590369 for more details.
-  document().updateStyleAndLayoutIgnorePendingStylesheets();
-
-  const PlainTextRange& selectedRange = createSelectionRangeForSetComposition(
-      selectionStart, selectionEnd, text.length());
-  // We shouldn't close typing in the middle of setComposition.
-  setEditableSelectionOffsets(selectedRange, NotUserTriggered);
-  m_isDirty = true;
-}
-
 void InputMethodController::setComposition(
     const String& text,
     const Vector<CompositionUnderline>& underlines,
     int selectionStart,
     int selectionEnd) {
   Editor::RevealSelectionScope revealSelectionScope(&editor());
 
   // Updates styles before setting selection for composition to prevent
   // inserting the previous composition text into text nodes oddly.
   // See https://bugs.webkit.org/show_bug.cgi?id=46868
   document().updateStyleAndLayoutTree();
 
-  // When the IME only wants to change a few characters at the end of the
-  // composition, only touch those characters in order to preserve rich text
-  // substructure.
-  if (hasComposition() && text.length()) {
-    return setCompositionWithIncrementalText(text, underlines, selectionStart,
-                                             selectionEnd);
-  }
-
   selectComposition();
 
   if (frame().selection().isNone())
     return;
 
   Element* target = document().focusedElement();
   if (!target)
     return;
 
   // TODO(xiaochengh): The use of updateStyleAndLayoutIgnorePendingStylesheets
@@ skipping 68 matching lines @@
   document().updateStyleAndLayoutIgnorePendingStylesheets();
 
   // Find out what node has the composition now.
   Position base = mostForwardCaretPosition(frame().selection().base());
   Node* baseNode = base.anchorNode();
   if (!baseNode || !baseNode->isTextNode())
     return;
 
   Position extent = frame().selection().extent();
   Node* extentNode = extent.anchorNode();
-  if (baseNode != extentNode)
-    return;
 
   unsigned extentOffset = extent.computeOffsetInContainerNode();
   unsigned baseOffset = base.computeOffsetInContainerNode();
-  if (baseOffset + text.length() != extentOffset)
-    return;
 
-  m_isDirty = true;
   m_hasComposition = true;
   if (!m_compositionRange)
     m_compositionRange = Range::create(document());
   m_compositionRange->setStart(baseNode, baseOffset);
-  m_compositionRange->setEnd(baseNode, extentOffset);
+  m_compositionRange->setEnd(extentNode, extentOffset);
 
   if (baseNode->layoutObject())
     baseNode->layoutObject()->setShouldDoFullPaintInvalidation();
 
   // TODO(xiaochengh): The use of updateStyleAndLayoutIgnorePendingStylesheets
   // needs to be audited. see http://crbug.com/590369 for more details.
   document().updateStyleAndLayoutIgnorePendingStylesheets();
 
   // We shouldn't close typing in the middle of setComposition.
   setEditableSelectionOffsets(selectedRange, NotUserTriggered);
@@ skipping 481 matching lines @@
   return WebTextInputTypeNone;
 }
 
 DEFINE_TRACE(InputMethodController) {
   visitor->trace(m_frame);
   visitor->trace(m_compositionRange);
   SynchronousMutationObserver::trace(visitor);
 }
 
 }  // namespace blink