[WIP] Add support for Android SuggestionSpans when editing text

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 50c4d95d121410f20ccfe36e4d0942ff890e6f28..e27f8d173d98456de4961f563753da2f5b4e7c33 100644
--- a/third_party/WebKit/Source/core/editing/InputMethodController.cpp
+++ b/third_party/WebKit/Source/core/editing/InputMethodController.cpp
@@ -33,6 +33,7 @@
 #include "core/dom/Text.h"
 #include "core/editing/EditingUtilities.h"
 #include "core/editing/Editor.h"
+#include "core/editing/TextSuggestionList.h"
 #include "core/editing/commands/TypingCommand.h"
 #include "core/editing/markers/DocumentMarkerController.h"
 #include "core/events/CompositionEvent.h"
@@ -48,6 +49,9 @@ namespace blink {
 
 namespace {
 
+const int kMaxNumberSuggestions = 5;
+const double kSuggestionUnderlineAlphaMultiplier = 0.4;
+
 void dispatchCompositionUpdateEvent(LocalFrame& frame, const String& text) {
   Element* target = frame.document()->focusedElement();
   if (!target)
@@ -69,10 +73,6 @@ void dispatchCompositionEndEvent(LocalFrame& frame, const String& text) {
 }
 
 bool needsIncrementalInsertion(const LocalFrame& frame, const String& newText) {
-  // No need to apply incremental insertion if it doesn't support formated text.
-  if (!frame.editor().canEditRichly())
-    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())
@@ -284,6 +284,167 @@ bool InputMethodController::commitText(
   return insertTextAndMoveCaret(text, relativeCaretPosition, underlines);
 }
 
+void InputMethodController::applySuggestionReplacement(int documentMarkerID,
+                                                       int suggestionIndex) {
+  closeSuggestionMenu();
+
+  Element* rootEditableElement = frame().selection().rootEditableElement();
+  DocumentMarkerVector suggestionMarkers =
+      rootEditableElement->document().markers().markersInRange(
+          firstEphemeralRangeOf(m_frame->selection().selection()),
+          DocumentMarker::Suggestion);
+
+  DocumentMarker* markerPtr = nullptr;
+  for (const Member<DocumentMarker>& marker : suggestionMarkers) {
+    if (marker->suggestionMarkerID() == documentMarkerID) {
+      markerPtr = marker.get();
+      break;
+    }
+  }
+
+  if (markerPtr == nullptr)
+    return;
+
+  // Remove markers that contain part, but not all, of the text range to be
+  // replaced.
+  Vector<int> markerIdsToRemove;
+  HeapVector<Member<DocumentMarker>> markersToExpand;
+  for (const Member<DocumentMarker>& marker : suggestionMarkers) {
+    if ((marker->startOffset() > markerPtr->startOffset() &&
+         marker->startOffset() <= markerPtr->endOffset()) ||

[aelias_OOO_until_Jul13, 2017/01/25 03:34:27]

Isn't it equivalent to say "marker->startOffset() > markerPtr->startOffset() &&
marker->endOffset() < markerPtr->endOffset()"?  You can DCHECK_LT(startOffset,
endOffset); at some point to make that assumption guaranteed.

[rlanday, 2017/01/25 18:48:10]

I don't think so, because your check doesn't handle the case where the markers
share either a start or end position?

+        (marker->endOffset() < markerPtr->endOffset() &&
+         marker->endOffset() >= markerPtr->startOffset())) {
+      markerIdsToRemove.push_back(marker->suggestionMarkerID());
+    }
+
+    if (marker->startOffset() == markerPtr->startOffset() &&
+        marker->endOffset() == markerPtr->endOffset()) {
+      // Special case: DocumentMarkerController handles shifting most of the
+      // markers around, but if we replace exactly the range of text spanned by
+      // a marker, in some cases, the range will get collapsed to a single
+      // position when the old text is removed and will not be re-expanded. So
+      // we have to re-expand those markers here after the replacement.
+
+      // We actually have to remove and re-insert these markers because in the
+      // case
+      // where all the text is replaced, it's possible for the text node to get
+      // removed and re-added, thereby dropping the markers.
+      markerIdsToRemove.push_back(marker->suggestionMarkerID());
+      markersToExpand.push_back(marker);

[aelias_OOO_until_Jul13, 2017/01/25 03:34:26]

Seems a bit weird, though I don't understand the scenario well enough, perhaps
we can discuss it more later.

I notice SpellChecker::updateMarkersForWordsAffectedByEditing has a very general
approach to solve the same problem.  I'd suggest harmonizing the approach
(ideally using the same code, perhaps by refactoring out the code in
SpellChecker).  And note that performance is pretty negligible here, I'd rather
keep the logic simple and general.

[rlanday, 2017/01/25 18:48:10]

I'll look into that code

[rlanday, 2017/01/25 18:48:10]

I'll look into that code

[rlanday, 2017/01/28 02:35:28]

Ok, there are actually two methods involved with updating the spell check
markers:

- SpellChecker::updateMarkersForWordsAffectedByEditing() clears spellcheck
markers from words that are being edited
- SpellChecker::markMisspellingsAfterApplyingCommand() adds them back once
you're finished typing a word

We can't reuse markMisspellingsAfterApplyingCommand() at all because the stuff
we need to do to keep the suggestion markers up to date is totally different
from spell check markers (we can't just clear on every edit operation and re-add
because we can't re-generate the suggestion markers from the text the same way
we re-generate spellcheck markers).

I'm not sure we can re-use updateMarkersForWordsAffectedByEditing() because it's
overly aggressive (e.g. it seems that just tapping around on different words and
making the suggestion menu sometimes clears the suggestion span). It might be
that we need to call it from a different place,
updateMarkersForWordsAffectedByEditing() is called rather aggressively, from all
of these places:

- Editor::respondToChangedContents()
- Editor::insertTextWithoutSendingTextEvent()
- Editor::cut()
- Editor::paste()
- Editor::pasteAsPlainText()
- TypingCommand::insertText()
- TypingCommand::deleteKeyPressed()
- TypingCommand::forwardDeleteKeyPressed()

I'm not quite sure why this needs to be called from so many different places; I
think we need to clear the suggestion markers only in response to user-generated
edits, and not stuff like tapping around in the text box and the IME sending
stuff as a response, and we need to have custom logic when we actually perform a
replacement/delete from the suggestion menu. What I currently have works for the
most common cases, but there might be stuff like pasting text right after a
suggestion range, or the text being updated by JavaScript that doesn't work
quite as expected. I will have to think about this some more, but I think what I
currently have is pretty close to being correct in most cases. Ideally I think
we'll be able to pull some more of this code out of InputMethodController
though.

+    }
+  }
+
+  document().markers().removeSuggestionMarkersByID(markerIdsToRemove);
+
+  setCompositionFromExistingText(Vector<CompositionUnderline>(),
+                                 markerPtr->startOffset(),
+                                 markerPtr->endOffset());
+
+  // The entry in the suggestion drop-down that the user tapped on is replaced
+  // by the text they just overwrote
+  String replacement =
+      String::fromUTF8(markerPtr->suggestions()[suggestionIndex].c_str());
+  int replacementLength = replacement.length();
+  String newSuggestion = composingText();
+  replaceComposition(replacement);
+  markerPtr->replaceSuggestion(suggestionIndex, newSuggestion.utf8().data());
+
+  // Could have changed when we replaced the composition, if we deleted all the
+  // text
+  rootEditableElement = frame().selection().rootEditableElement();
+  for (const Member<DocumentMarker>& marker : markersToExpand) {
+    const EphemeralRange& range =
+        PlainTextRange(marker->startOffset(),
+                       marker->startOffset() + replacementLength)
+            .createRange(*rootEditableElement);
+    document().markers().addCompositionMarker(
+        range.startPosition(), range.endPosition(), marker->underlineColor(),
+        marker->thick(), marker->backgroundColor(), marker->suggestions());
+  }
+}
+
+void InputMethodController::deleteSuggestionHighlight() {
+  Element* rootEditableElement = frame().selection().rootEditableElement();
+  DocumentMarkerVector suggestionHighlightMarkers =
+      rootEditableElement->document().markers().markersInRange(
+          firstEphemeralRangeOf(m_frame->selection().selection()),
+          DocumentMarker::SuggestionBackgroundHighlight);
+
+  if (suggestionHighlightMarkers.size() == 0)

[aelias_OOO_until_Jul13, 2017/01/25 03:34:26]

isEmpty()

+    return;
+
+  const DocumentMarker* suggestionHighlightMarkerPtr =
+      suggestionHighlightMarkers[0].get();
+
+  // Remove markers that contain part, but not all, of the text range to be
+  // deleted.
+  Vector<int> markerIdsToRemove;
+  HeapVector<Member<DocumentMarker>> markersToExpand;

[aelias_OOO_until_Jul13, 2017/01/25 03:34:27]

unused

+  for (const Member<DocumentMarker>& marker : suggestionHighlightMarkers) {
+    if ((marker->startOffset() > suggestionHighlightMarkerPtr->startOffset() &&
+         marker->startOffset() <= suggestionHighlightMarkerPtr->endOffset()) ||
+        (marker->endOffset() < suggestionHighlightMarkerPtr->endOffset() &&
+         marker->endOffset() >= suggestionHighlightMarkerPtr->startOffset())) {
+      markerIdsToRemove.push_back(marker->suggestionMarkerID());
+    }
+  }
+
+  document().markers().removeSuggestionMarkersByID(markerIdsToRemove);
+
+  // If the character immediately following the range to be deleted is a space,
+  // delete it if either of these conditions holds:
+  // - We're deleting at the beginning of the editable text (to avoid ending up
+  //   with a space at the beginning)
+  // - The character immediately before the range being deleted is also a space
+  //   (to avoid ending up with two adjacent spaces)
+
+  bool deleteNextChar = false;
+
+  const PlainTextRange nextCharacterRange(
+      suggestionHighlightMarkerPtr->endOffset(),
+      suggestionHighlightMarkerPtr->endOffset() + 1);
+  const EphemeralRange& nextCharacterEphemeralRange =
+      nextCharacterRange.createRange(*rootEditableElement);
+  if (!nextCharacterEphemeralRange.isNull()) {
+    String nextCharacterStr =
+        plainText(nextCharacterEphemeralRange, TextIteratorEmitsOriginalText);

[aelias_OOO_until_Jul13, 2017/01/25 03:34:26]

EmitsOriginalText is for password fields not to be reported as dot symbols, not
really needed here.

[rlanday, 2017/01/26 18:28:52]

I copied this from the composingText() method, are you sure it's not needed?
What happens if you're editing text inside a password field?

[aelias_OOO_until_Jul13, 2017/01/27 19:56:36]

It doesn't really make sense for password fields to have voice input and even
less so voice suggestions.  If we are want to write code specific to password
fields it should probably be along the lines of disabling SuggestionSpans on
purpose for them instead of trying to make bits of our implementations
compatible.

+
+    if (WTF::isASCIISpace(nextCharacterStr[0])) {

[aelias_OOO_until_Jul13, 2017/01/25 03:34:26]

Probably want to consider non-breaking spaces as well.  The text editing code
likes to use them in many cases in contenteditable elements.

[rlanday, 2017/01/25 18:48:10]

I was having trouble finding a function that seemed to include just spaces and
non-breaking spaces, I'll check again

+      if (suggestionHighlightMarkerPtr->startOffset() == 0) {

[aelias_OOO_until_Jul13, 2017/01/25 03:34:27]

The range code works even if this is 0, right?  Could you delete this
special-case branch to simplify the logic, then?  Performance is a negligible
concern here.

[rlanday, 2017/01/25 18:48:10]

This is not for performance, this is so if you have a word (or a series of
words) highlighted at the beginning of the text box, and hit the "Delete" menu
option, we also delete the space following the word (which is not part of the
range). This is behavior I'm copying over from the native Android
implementation.

+        deleteNextChar = true;
+      } else {
+        const PlainTextRange prevCharacterRange(
+            suggestionHighlightMarkerPtr->startOffset() - 1,
+            suggestionHighlightMarkerPtr->startOffset());
+        const EphemeralRange prevCharacterEphemeralRange =
+            prevCharacterRange.createRange(*rootEditableElement);
+        if (!prevCharacterEphemeralRange.isNull()) {
+          String prevCharacterStr = plainText(prevCharacterEphemeralRange,
+                                              TextIteratorEmitsOriginalText);
+          if (WTF::isASCIISpace(prevCharacterStr[0])) {
+            deleteNextChar = true;
+          }
+        }
+      }
+    }
+  }
+
+  setCompositionFromExistingText(

[aelias_OOO_until_Jul13, 2017/01/25 03:34:27]

We don't want to send composition events to Javascript here, so I'd prefer
setting the selection and doing TypingCommand directly (likewise above).

Note
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/s...
does frame().editor().replaceSelectionWithText

+      Vector<CompositionUnderline>(),
+      suggestionHighlightMarkerPtr->startOffset(),
+      suggestionHighlightMarkerPtr->endOffset() + deleteNextChar);
+  // send msg to IME
+  replaceComposition("");
+
+  closeSuggestionMenu();
+}
+
+void InputMethodController::closeSuggestionMenu() {
+  document().markers().removeMarkers(
+      DocumentMarker::SuggestionBackgroundHighlight);
+  frame().selection().setCaretVisible(true);
+}
+
 bool InputMethodController::replaceComposition(const String& text) {
   if (!hasComposition())
     return false;
@@ -341,10 +502,30 @@ void InputMethodController::addCompositionUnderlines(
 
     document().markers().addCompositionMarker(
         ephemeralLineRange.startPosition(), ephemeralLineRange.endPosition(),
-        underline.color(), underline.thick(), underline.backgroundColor());
+        underline.color(), underline.thick(), underline.backgroundColor(),
+        underline.suggestions());
   }
 }
 
+void InputMethodController::clearSuggestionMarkersTouchingSelection() {
+  Element* rootEditableElement = frame().selection().rootEditableElement();
+
+  DocumentMarkerVector suggestionMarkers =
+      rootEditableElement->document().markers().markersInRange(
+          firstEphemeralRangeOf(m_frame->selection().selection()),
+          DocumentMarker::Suggestion);
+
+  // We can't use DocumentMarkerController::removeMarkers() to remove markers
+  // overlapping with the selection range because we also want to remove markers
+  // merely touching, not overlapping, the selection
+  Vector<int> markerIdsToRemove;
+  for (const Member<DocumentMarker>& marker : suggestionMarkers) {
+    markerIdsToRemove.push_back(marker->suggestionMarkerID());
+  }
+
+  document().markers().removeSuggestionMarkersByID(markerIdsToRemove);
+}
+
 bool InputMethodController::replaceCompositionAndMoveCaret(
     const String& text,
     int relativeCaretPosition,
@@ -357,6 +538,7 @@ bool InputMethodController::replaceCompositionAndMoveCaret(
       PlainTextRange::create(*rootEditableElement, *m_compositionRange);
   if (compositionRange.isNull())
     return false;
+
   int textStart = compositionRange.start();
 
   if (!replaceComposition(text))
@@ -388,6 +570,9 @@ bool InputMethodController::insertTextAndMoveCaret(
   PlainTextRange selectionRange = getSelectionOffsets();
   if (selectionRange.isNull())
     return false;
+
+  clearSuggestionMarkersTouchingSelection();
+
   int textStart = selectionRange.start();
 
   if (text.length()) {
@@ -500,13 +685,14 @@ void InputMethodController::setComposition(
   if (!target)
     return;
 
+  clearSuggestionMarkersTouchingSelection();
+
   // TODO(xiaochengh): The use of updateStyleAndLayoutIgnorePendingStylesheets
   // needs to be audited. see http://crbug.com/590369 for more details.
   document().updateStyleAndLayoutIgnorePendingStylesheets();
 
   PlainTextRange selectedRange = createSelectionRangeForSetComposition(
       selectionStart, selectionEnd, text.length());
-
   // 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:
@@ -601,7 +787,8 @@ void InputMethodController::setComposition(
     document().markers().addCompositionMarker(
         m_compositionRange->startPosition(), m_compositionRange->endPosition(),
         Color::black, false,
-        LayoutTheme::theme().platformDefaultCompositionBackgroundColor());
+        LayoutTheme::theme().platformDefaultCompositionBackgroundColor(),
+        std::vector<std::string>());
     return;
   }
 
@@ -1061,6 +1248,123 @@ WebTextInputType InputMethodController::textInputType() const {
   return WebTextInputTypeNone;
 }
 
+WebVector<blink::WebTextSuggestionInfo>
+InputMethodController::getTextSuggestionInfosUnderCaret() const {
+  Element* rootEditableElement = frame().selection().rootEditableElement();
+  if (!rootEditableElement)
+    return WebVector<blink::WebTextSuggestionInfo>();
+
+  DocumentMarkerVector suggestionMarkers =
+      rootEditableElement->document().markers().markersInRange(
+          firstEphemeralRangeOf(m_frame->selection().selection()),
+          DocumentMarker::Suggestion);
+
+  Vector<TextSuggestionList> suggestionSpans;
+  for (const Member<DocumentMarker>& marker : suggestionMarkers) {
+    // ignore ranges that have been collapsed as a result of editing
+    // operations
+    if (marker->endOffset() == marker->startOffset())
+      continue;
+
+    TextSuggestionList suggestionList;
+    suggestionList.documentMarkerID = marker->suggestionMarkerID();
+    suggestionList.start = marker->startOffset();
+    suggestionList.end = marker->endOffset();
+    suggestionList.suggestions = marker->suggestions();
+    suggestionSpans.push_back(suggestionList);
+  }
+
+  std::sort(suggestionSpans.begin(), suggestionSpans.end());
+
+  std::vector<WebTextSuggestionInfo> suggestionInfos;
+
+  for (const TextSuggestionList& suggestionList : suggestionSpans) {
+    if (suggestionInfos.size() == kMaxNumberSuggestions)
+      break;
+
+    for (size_t suggestionIndex = 0;
+         suggestionIndex < suggestionList.suggestions.size();
+         suggestionIndex++) {
+      const std::string& suggestion =
+          suggestionList.suggestions[suggestionIndex];
+      bool isDupe = false;
+      for (size_t i = 0; i < suggestionInfos.size(); i++) {
+        const WebTextSuggestionInfo& otherSuggestionInfo = suggestionInfos[i];
+        if (otherSuggestionInfo.suggestion == suggestion) {
+          if (suggestionList.start == otherSuggestionInfo.spanStart &&
+              suggestionList.end == otherSuggestionInfo.spanEnd) {
+            isDupe = true;
+            break;
+          }
+        }
+      }
+
+      if (isDupe)
+        continue;
+
+      WebTextSuggestionInfo suggestionInfo;
+      suggestionInfo.documentMarkerID = suggestionList.documentMarkerID;
+      suggestionInfo.suggestionStart = 0;
+      suggestionInfo.suggestionEnd = suggestion.length();
+      suggestionInfo.spanStart = suggestionList.start;
+      suggestionInfo.spanEnd = suggestionList.end;
+      suggestionInfo.suggestionIndex = suggestionIndex;
+      suggestionInfo.suggestion = suggestion;
+      suggestionInfos.push_back(suggestionInfo);
+      if (suggestionInfos.size() == kMaxNumberSuggestions)
+        break;
+    }
+  }
+
+  if (suggestionInfos.size() == 0)
+    return WebVector<blink::WebTextSuggestionInfo>();
+
+  int spanUnionStart = suggestionInfos[0].spanStart;
+  int spanUnionEnd = suggestionInfos[0].spanEnd;
+
+  for (size_t i = 1; i < suggestionInfos.size(); i++) {
+    spanUnionStart = std::min(spanUnionStart, suggestionInfos[i].spanStart);
+    spanUnionEnd = std::max(spanUnionEnd, suggestionInfos[i].spanEnd);
+  }
+
+  for (WebTextSuggestionInfo& info : suggestionInfos) {
+    const EphemeralRange& prefixRange =
+        PlainTextRange(spanUnionStart, info.spanStart)
+            .createRange(*rootEditableElement);
+    String prefix = plainText(prefixRange, TextIteratorEmitsOriginalText);
+
+    const EphemeralRange& suffixRange =
+        PlainTextRange(info.spanEnd, spanUnionEnd)
+            .createRange(*rootEditableElement);
+    String suffix = plainText(suffixRange, TextIteratorEmitsOriginalText);
+
+    info.prefix = prefix.utf8().data();
+    info.suffix = suffix.utf8().data();
+  }
+
+  Color highlightColor;
+  if (suggestionMarkers[0]->underlineColor() != 0) {
+    highlightColor = suggestionMarkers[0]->underlineColor().combineWithAlpha(
+        kSuggestionUnderlineAlphaMultiplier);
+  } else {
+    highlightColor = LayoutTheme::tapHighlightColor();
+  }
+
+  EphemeralRange backgroundHighlightRange =
+      PlainTextRange(spanUnionStart, spanUnionEnd)
+          .createRange(*rootEditableElement);
+  document().markers().addSuggestionBackgroundHighlightMarker(
+      backgroundHighlightRange.startPosition(),
+      backgroundHighlightRange.endPosition(), Color::black, false,
+      highlightColor);
+
+  return suggestionInfos;
+}
+
+void InputMethodController::prepareForTextSuggestionMenuToBeShown() {
+  frame().selection().setCaretVisible(false);
+}
+
 void InputMethodController::willChangeFocus() {
   if (!finishComposingText(DoNotKeepSelection))
     return;