Remove VisiblePositions usage during composition.

There are cases where the IME tries to replace the characters that
are in the middle of grapheme cluster and in those cases
|VisiblePositions| adjust the positions to not be at the middle of
grapheme clusters. Thus this ends up replacing/duplicating characters
in many languages such as Devanagari, Tibetan, Burmese etc.
In this patch we are using the |SelectionInDOMTree| instead which has
the correct node and offsets which is used to place the positions in
the middle of grapheme cluster and perform the composition operation
correctly. Also added logic to not remove the existing TypingCommand
when a composition is in-progress.

Bug: 621394, 1020371

Change-Id: I6472ce2e2e34b43ec3e385a2874dee29a158f5ca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2619002
Commit-Queue: Anupam Snigdha <snianu@microsoft.com>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843126}

third_party/blink/renderer/core/editing/commands/typing_command.cc

@@ -42,6 +42,7 @@
 #include "third_party/blink/renderer/core/editing/editor.h"
 #include "third_party/blink/renderer/core/editing/ephemeral_range.h"
 #include "third_party/blink/renderer/core/editing/frame_selection.h"
+#include "third_party/blink/renderer/core/editing/ime/input_method_controller.h"
 #include "third_party/blink/renderer/core/editing/plain_text_range.h"
 #include "third_party/blink/renderer/core/editing/selection_modifier.h"
 #include "third_party/blink/renderer/core/editing/selection_template.h"
@@ -66,9 +67,13 @@ bool IsValidDocument(const Document& document) {
 }
 
 String DispatchBeforeTextInsertedEvent(const String& text,
-                                       const VisibleSelection& selection,
+                                       const SelectionInDOMTree& selection,
                                        EditingState* editing_state) {
-  Node* start_node = selection.Start().ComputeContainerNode();
+  // We use SelectionForUndoStep because it is resilient to DOM
+  // mutation.
+  const SelectionForUndoStep& selection_as_undo_step =
+      SelectionForUndoStep::From(selection);
+  Node* start_node = selection_as_undo_step.Start().ComputeContainerNode();
   if (!start_node || !RootEditableElement(*start_node))
     return text;
 
@@ -77,7 +82,7 @@ String DispatchBeforeTextInsertedEvent(const String& text,
   const Document& document = start_node->GetDocument();
   auto* evt = MakeGarbageCollected<BeforeTextInsertedEvent>(text);
   RootEditableElement(*start_node)->DispatchEvent(*evt);
-  if (IsValidDocument(document) && selection.IsValidFor(document))
+  if (IsValidDocument(document) && selection_as_undo_step.IsValidFor(document))
     return evt->GetText();
   // editing/inserting/webkitBeforeTextInserted-removes-frame.html
   // and
@@ -110,8 +115,7 @@ DispatchEventResult DispatchTextInputEvent(LocalFrame* frame,
 }
 
 PlainTextRange GetSelectionOffsets(const SelectionInDOMTree& selection) {
-  const VisibleSelection visible_selection = CreateVisibleSelection(selection);
-  const EphemeralRange range = FirstEphemeralRangeOf(visible_selection);
+  const EphemeralRange range = selection.ComputeRange();
   if (range.IsNull())
     return PlainTextRange();
   ContainerNode* const editable =
@@ -372,17 +376,19 @@ void TypingCommand::InsertText(
   LocalFrame* frame = document.GetFrame();
   DCHECK(frame);
 
-  const VisibleSelection& current_selection =
-      frame->Selection().ComputeVisibleSelectionInDOMTree();
-  const VisibleSelection& selection_for_insertion =
-      CreateVisibleSelection(passed_selection_for_insertion);
+  // We use SelectionForUndoStep because it is resilient to DOM
+  // mutation.
+  const SelectionForUndoStep& passed_selection_for_insertion_as_undo_step =
+      SelectionForUndoStep::From(passed_selection_for_insertion);
 
   String new_text = text;
   if (composition_type != kTextCompositionUpdate) {
-    new_text = DispatchBeforeTextInsertedEvent(text, selection_for_insertion,
-                                               editing_state);
+    new_text = DispatchBeforeTextInsertedEvent(
+        text, passed_selection_for_insertion, editing_state);
     if (editing_state->IsAborted())
       return;
+    ABORT_EDITING_COMMAND_IF(
+        !passed_selection_for_insertion_as_undo_step.IsValidFor(document));
   }
 
   if (composition_type == kTextCompositionConfirm) {
@@ -394,19 +400,21 @@ void TypingCommand::InsertText(
       return;
     // editing/inserting/insert-text-nodes-disconnect-on-textinput-event.html
     // hits true for ABORT_EDITING_COMMAND_IF macro.
-    ABORT_EDITING_COMMAND_IF(!selection_for_insertion.IsValidFor(document));
+    ABORT_EDITING_COMMAND_IF(
+        !passed_selection_for_insertion_as_undo_step.IsValidFor(document));
   }
 
   // Do nothing if no need to delete and insert.
-  if (selection_for_insertion.IsCaret() && new_text.IsEmpty())
+  if (passed_selection_for_insertion_as_undo_step.IsCaret() &&
+      new_text.IsEmpty())
     return;
 
   // TODO(editing-dev): The use of UpdateStyleAndLayout
   // needs to be audited. see http://crbug.com/590369 for more details.
   document.UpdateStyleAndLayout(DocumentUpdateReason::kEditing);
 
-  const PlainTextRange selection_offsets =
-      GetSelectionOffsets(selection_for_insertion.AsSelection());
+  const PlainTextRange selection_offsets = GetSelectionOffsets(
+      passed_selection_for_insertion_as_undo_step.AsSelection());
   if (selection_offsets.IsNull())
     return;
   const wtf_size_t selection_start = selection_offsets.Start();
@@ -417,14 +425,12 @@ void TypingCommand::InsertText(
   // that can be used by all of the commands.
   if (TypingCommand* last_typing_command =
           LastTypingCommandIfStillOpenForTyping(frame)) {
-    if (last_typing_command->EndingVisibleSelection() !=
-        selection_for_insertion) {
-      const SelectionForUndoStep& selection_for_insertion_as_undo_step =
-          SelectionForUndoStep::From(selection_for_insertion.AsSelection());
+    if (last_typing_command->EndingSelection() !=
+        passed_selection_for_insertion_as_undo_step) {
       last_typing_command->SetStartingSelection(
-          selection_for_insertion_as_undo_step);
+          passed_selection_for_insertion_as_undo_step);
       last_typing_command->SetEndingSelection(
-          selection_for_insertion_as_undo_step);
+          passed_selection_for_insertion_as_undo_step);
     }
 
     last_typing_command->SetCompositionType(composition_type);
@@ -441,12 +447,14 @@ void TypingCommand::InsertText(
   TypingCommand* command = MakeGarbageCollected<TypingCommand>(
       document, kInsertText, new_text, options, TextGranularity::kCharacter,
       composition_type);
-  bool change_selection = selection_for_insertion != current_selection;
+  const SelectionInDOMTree& current_selection =
+      frame->Selection().GetSelectionInDOMTree();
+  bool change_selection =
+      current_selection !=
+      passed_selection_for_insertion_as_undo_step.AsSelection();
   if (change_selection) {
-    const SelectionForUndoStep& selection_for_insertion_as_undo_step =
-        SelectionForUndoStep::From(selection_for_insertion.AsSelection());
-    command->SetStartingSelection(selection_for_insertion_as_undo_step);
-    command->SetEndingSelection(selection_for_insertion_as_undo_step);
+    command->SetStartingSelection(passed_selection_for_insertion_as_undo_step);
+    command->SetEndingSelection(passed_selection_for_insertion_as_undo_step);
   }
   command->is_incremental_insertion_ = is_incremental_insertion;
   command->selection_start_ = selection_start;
@@ -454,9 +462,8 @@ void TypingCommand::InsertText(
   ABORT_EDITING_COMMAND_IF(!command->Apply());
 
   if (change_selection) {
-    ABORT_EDITING_COMMAND_IF(!current_selection.IsValidFor(document));
     const SelectionInDOMTree& current_selection_as_dom =
-        current_selection.AsSelection();
+        frame->Selection().GetSelectionInDOMTree();
     command->SetEndingSelection(
         SelectionForUndoStep::From(current_selection_as_dom));
     frame->Selection().SetSelection(
@@ -529,6 +536,15 @@ void TypingCommand::CloseTyping(LocalFrame* frame) {
     last_typing_command->CloseTyping();
 }
 
+void TypingCommand::CloseTypingIfNeeded(LocalFrame* frame) {
+  if (frame->GetDocument()->IsRunningExecCommand() ||
+      frame->GetInputMethodController().HasComposition())
+    return;
+  if (TypingCommand* last_typing_command =
+          LastTypingCommandIfStillOpenForTyping(frame))
+    last_typing_command->CloseTyping();
+}
+
 void TypingCommand::DoApply(EditingState* editing_state) {
   if (EndingSelection().IsNone() ||
       !EndingSelection().IsValidFor(GetDocument()))

third_party/blink/renderer/core/editing/commands/typing_command.h

@@ -85,6 +85,7 @@ class CORE_EXPORT TypingCommand final : public CompositeEditCommand {
   static bool InsertParagraphSeparator(Document&);
   static bool InsertParagraphSeparatorInQuotedContent(Document&);
   static void CloseTyping(LocalFrame*);
+  static void CloseTypingIfNeeded(LocalFrame*);
 
   static TypingCommand* LastTypingCommandIfStillOpenForTyping(LocalFrame*);
   static void UpdateSelectionIfDifferentFromCurrentSelection(TypingCommand*,

third_party/blink/renderer/core/editing/frame_selection.cc

@@ -363,10 +363,9 @@ void FrameSelection::SetSelectionForAccessibility(
 void FrameSelection::NodeChildrenWillBeRemoved(ContainerNode& container) {
   if (!container.InActiveDocument())
     return;
-  // TODO(yosin): We should move to call |TypingCommand::closeTyping()| to
-  // |Editor| class.
-  if (!GetDocument().IsRunningExecCommand())
-    TypingCommand::CloseTyping(frame_);
+  // TODO(yosin): We should move to call |TypingCommand::CloseTypingIfNeeded()|
+  // to |Editor| class.
+  TypingCommand::CloseTypingIfNeeded(frame_);
 }
 
 void FrameSelection::NodeWillBeRemoved(Node& node) {
@@ -375,10 +374,9 @@ void FrameSelection::NodeWillBeRemoved(Node& node) {
   // needs no adjustment.
   if (!node.InActiveDocument())
     return;
-  // TODO(yosin): We should move to call |TypingCommand::closeTyping()| to
-  // |Editor| class.
-  if (!GetDocument().IsRunningExecCommand())
-    TypingCommand::CloseTyping(frame_);
+  // TODO(yosin): We should move to call |TypingCommand::CloseTypingIfNeeded()|
+  // to |Editor| class.
+  TypingCommand::CloseTypingIfNeeded(frame_);
 }
 
 void FrameSelection::DidChangeFocus() {

third_party/blink/renderer/core/editing/ime/input_method_controller_test.cc

@@ -3411,4 +3411,54 @@ TEST_F(InputMethodControllerTest, VirtualKeyboardPolicyOfFocusedElement) {
             Controller().VirtualKeyboardPolicyOfFocusedElement());
 }
 
+TEST_F(InputMethodControllerTest, SetCompositionInTibetan) {
+  GetFrame().Selection().SetSelectionAndEndTyping(
+      SetSelectionTextToBody(u8"<div id='sample' contenteditable>|</div>"));
+  Element* const div = GetDocument().getElementById("sample");
+  div->focus();
+
+  Vector<ImeTextSpan> ime_text_spans;
+  Controller().SetComposition(String(Vector<UChar>{0xF56}), ime_text_spans, 1,
+                              1);
+  EXPECT_EQ(u8"<div contenteditable id=\"sample\">\u0F56|</div>",
+            GetSelectionTextFromBody());
+
+  Controller().CommitText(String(Vector<UChar>{0xF56}), ime_text_spans, 0);
+  EXPECT_EQ(u8"<div contenteditable id=\"sample\">\u0F56|</div>",
+            GetSelectionTextFromBody());
+
+  Controller().SetComposition(String(Vector<UChar>{0xFB7}), ime_text_spans, 1,
+                              1);
+  EXPECT_EQ(u8"<div contenteditable id=\"sample\">\u0F56\u0FB7|</div>",
+            GetSelectionTextFromBody());
+
+  // Attempt to replace part of grapheme cluster "\u0FB7" in composition
+  Controller().CommitText(String(Vector<UChar>{0xFB7}), ime_text_spans, 0);
+  EXPECT_EQ(u8"<div contenteditable id=\"sample\">\u0F56\u0FB7|</div>",
+            GetSelectionTextFromBody());
+
+  Controller().SetComposition(String(Vector<UChar>{0xF74}), ime_text_spans, 1,
+                              1);
+  EXPECT_EQ(u8"<div contenteditable id=\"sample\">\u0F56\u0FB7\u0F74|</div>",
+            GetSelectionTextFromBody());
+}
+
+TEST_F(InputMethodControllerTest, SetCompositionInDevanagari) {
+  GetFrame().Selection().SetSelectionAndEndTyping(SetSelectionTextToBody(
+      u8"<div id='sample' contenteditable>\u0958|</div>"));
+  Element* const div = GetDocument().getElementById("sample");
+  div->focus();
+
+  Vector<ImeTextSpan> ime_text_spans;
+  Controller().SetComposition(String(Vector<UChar>{0x94D}), ime_text_spans, 1,
+                              1);
+  EXPECT_EQ(u8"<div contenteditable id=\"sample\">\u0958\u094D|</div>",
+            GetSelectionTextFromBody());
+
+  Controller().CommitText(String(Vector<UChar>{0x94D, 0x930}), ime_text_spans,
+                          0);
+  EXPECT_EQ(u8"<div contenteditable id=\"sample\">\u0958\u094D\u0930|</div>",
+            GetSelectionTextFromBody());
+}
+
 }  // namespace blink