[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 7e6b5be0144daac85af6d9449041a6e167da83b3..5c30f79342dc8b5624d8c611d5d6c67538cdad07 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;

[esprehn, 2017/01/31 22:41:34]

Why 5?

[rlanday, 2017/01/31 23:30:09]

5 is what the native text box on Android uses. Blink has to be aware of this
number because once we find 5 suggestions, we stop looking for longer and longer
ranges that the cursor is over to pull more suggestions. E.g. if we have:

The quick brown fox jumped over the lazy dog.

and we have the following suggestion ranges:

"The": 1 suggestion
"The quick": 1 suggestion
"The quick brown": 1 suggestion
"The quick brown fox": 1 suggestion
"The quick brown fox jumped:" 1 suggestion
"The quick brown fox jumped over:" 1 suggestion

and you tap on "The", we highlight up to "The quick brown fox jumped" since
that's how far we have to go to get 5 suggestions.

+const double kSuggestionUnderlineAlphaMultiplier = 0.4;

[rlanday, 2017/01/31 19:50:21]

The suggestion highlight background seems kind of hard to see on the yellow
background of the webpage I've been using for testing (editpad.org), but this is
the multiplier the native Android text box uses...

[esprehn, 2017/01/31 22:41:34]

Did this get a UI review?

[rlanday, 2017/01/31 23:30:09]

This is just existing Android UI we're porting over, so it didn't seem
necessary. Do you disagree?

+
 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())
@@ -305,6 +305,179 @@ bool InputMethodController::commitText(
   return insertTextAndMoveCaret(text, relativeCaretPosition, underlines);
 }
 
+void InputMethodController::applySuggestionReplacement(int documentMarkerID,

[esprehn, 2017/01/31 22:41:34]

This change is so huge, we probably need a lot of tests. Why is this different
than how normal spell check suggestions are applied?

[rlanday, 2017/01/31 23:30:09]

Yeah I should write some tests for this method.

Basically it's different from spell check in that, for spell check, the markers
do not overlap, and can be regenerated whenever we want by re-running the spell
check. For this, the markers are added by the IME, can overlap and/or be nested,
and cannot be regenerated by blink (the IME can rewrite the text but we can't
control that), so we have to carefully persist them, and keep them updated
properly when performing replacements.

+                                                       int suggestionIndex) {
+  suggestionMenuClosed();
+
+  Element* rootEditableElement = frame().selection().rootEditableElement();
+  DocumentMarkerVector suggestionMarkers =
+      rootEditableElement->document().markers().markersInRange(

[rlanday, 2017/01/31 21:17:12]

I introduced a bug here when I was refactoring, this should be
markersInRangeInclusive() (to include markers the cursor is immediately before
or immediately after)

+          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)
+    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()) ||
+        (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

[aelias_OOO_until_Jul13, 2017/02/10 21:43:55]

As a reminder of what we discussed offline, marking marker ranges "inclusive" is
the way to solve this problem, and this special case should be deleted.

+      // 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);
+    }
+  }
+
+  document().markers().removeSuggestionMarkersByID(markerIdsToRemove);
+
+  setCompositionFromExistingText(Vector<CompositionUnderline>(),

[aelias_OOO_until_Jul13, 2017/02/10 21:43:55]

There's no reason to do anything relating to composition here.  Please select
the word directly without going through composition.  Then it will be easier to
separate this method out from InputMethodController.

+                                 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();
+
+  // Don't use replaceComposition() here because we don't want to send
+  // JavaScript composition events
+  selectComposition();
+  frame().editor().replaceSelectionWithText(
+      replacement, false, false, InputEvent::InputType::InsertReplacementText);
+
+  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.isEmpty())
+    return;
+
+  const DocumentMarker* suggestionHighlightMarkerPtr =
+      suggestionHighlightMarkers[0].get();
+
+  // Remove markers that contain part, but not all, of the text range to be

[aelias_OOO_until_Jul13, 2017/02/10 21:43:55]

It can't be correct for this code to live here, since there's no difference
between this situation, and the one where you long-press-select the word and
press backspace.  It should be in a more generic place that covers both cases.

+  // deleted.
+  Vector<int> markerIdsToRemove;
+  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,

[aelias_OOO_until_Jul13, 2017/02/10 21:43:55]

Please split this off into a separate method called
"expandRangeToSurroundingWhitespace"

+  // 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,
+        TextIteratorBehavior::Builder().setEmitsSpaceForNbsp(true).build());
+
+    if (WTF::isASCIISpace(nextCharacterStr[0])) {
+      if (suggestionHighlightMarkerPtr->startOffset() == 0) {
+        deleteNextChar = true;
+      } else {
+        const PlainTextRange prevCharacterRange(
+            suggestionHighlightMarkerPtr->startOffset() - 1,
+            suggestionHighlightMarkerPtr->startOffset());
+        const EphemeralRange prevCharacterEphemeralRange =
+            prevCharacterRange.createRange(*rootEditableElement);
+        if (!prevCharacterEphemeralRange.isNull()) {
+          String prevCharacterStr = plainText(prevCharacterEphemeralRange,
+                                              TextIteratorBehavior::Builder()
+                                                  .setEmitsSpaceForNbsp(true)
+                                                  .build());
+          if (WTF::isASCIISpace(prevCharacterStr[0])) {
+            deleteNextChar = true;
+          }
+        }
+      }
+    }
+  }
+
+  const EphemeralRange highlightRange =
+      PlainTextRange(suggestionHighlightMarkerPtr->startOffset(),
+                     suggestionHighlightMarkerPtr->endOffset() + deleteNextChar)
+          .createRange(*rootEditableElement);
+
+  frame().selection().setSelection(
+      SelectionInDOMTree::Builder().setBaseAndExtent(highlightRange).build(),
+      0);
+  // Don't want to send JavaScript composition events here
+  frame().editor().replaceSelectionWithText(
+      "", false, false, InputEvent::InputType::InsertReplacementText);
+
+  suggestionMenuClosed();
+}
+
+void InputMethodController::suggestionMenuClosed() {
+  document().markers().removeMarkers(
+      DocumentMarker::SuggestionBackgroundHighlight);
+  frame().selection().setCaretVisible(true);
+}
+
 bool InputMethodController::replaceComposition(const String& text) {
   if (!hasComposition())
     return false;
@@ -362,10 +535,16 @@ 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() {
+  document().markers().removeMarkersForWordsAffectedByEditing(
+      DocumentMarker::Suggestion, false);
+}
+
 bool InputMethodController::replaceCompositionAndMoveCaret(
     const String& text,
     int relativeCaretPosition,
@@ -378,6 +557,7 @@ bool InputMethodController::replaceCompositionAndMoveCaret(
       PlainTextRange::create(*rootEditableElement, *m_compositionRange);
   if (compositionRange.isNull())
     return false;
+
   int textStart = compositionRange.start();
 
   if (!replaceComposition(text))
@@ -409,6 +589,9 @@ bool InputMethodController::insertTextAndMoveCaret(
   PlainTextRange selectionRange = getSelectionOffsets();
   if (selectionRange.isNull())
     return false;
+
+  clearSuggestionMarkersTouchingSelection();

[aelias_OOO_until_Jul13, 2017/02/10 21:43:55]

If text is inserted by typing on a physical keyboard or by Javascript method, it
doesn't go through this method (it's only for IME).  So this spot is too
high-level for this, it needs to be in lower-level code.

+
   int textStart = selectionRange.start();
 
   if (text.length()) {
@@ -505,13 +688,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:
@@ -606,7 +790,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;
   }
 
@@ -1073,6 +1258,125 @@ WebTextInputType InputMethodController::textInputType() const {
   return WebTextInputTypeNone;
 }
 
+WebVector<blink::WebTextSuggestionInfo>
+InputMethodController::getTextSuggestionInfosUnderCaret() const {

[aelias_OOO_until_Jul13, 2017/02/10 21:43:55]

Please move this and most other changes in this file into a new file called
something like MobileSuggestions.cpp

+  Element* rootEditableElement = frame().selection().rootEditableElement();
+  if (!rootEditableElement)
+    return WebVector<blink::WebTextSuggestionInfo>();
+
+  DocumentMarkerVector suggestionMarkers =
+      rootEditableElement->document().markers().markersInRangeInclusive(
+          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;

[esprehn, 2017/01/31 22:41:34]

WTF Vector

[rlanday, 2017/01/31 23:30:09]

Ok

+
+  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, TextIteratorBehavior::Builder().build());
+
+    const EphemeralRange& suffixRange =
+        PlainTextRange(info.spanEnd, spanUnionEnd)
+            .createRange(*rootEditableElement);
+    String suffix =
+        plainText(suffixRange, TextIteratorBehavior::Builder().build());
+
+    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(

[aelias_OOO_until_Jul13, 2017/02/10 21:43:55]

I don't think a method called "get...()" should be adding a marker, this should
be split off.

+      backgroundHighlightRange.startPosition(),
+      backgroundHighlightRange.endPosition(), Color::black, false,
+      highlightColor);
+
+  return suggestionInfos;
+}
+
+void InputMethodController::prepareForTextSuggestionMenuToBeShown() {
+  frame().selection().setCaretVisible(false);
+}
+
 void InputMethodController::willChangeFocus() {
   if (!finishComposingText(DoNotKeepSelection))
     return;