Rewrite DocumentMarkerController to use SynchronousMutationObserver

third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp

Index: third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
diff --git a/third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp b/third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
index 322e8367a663e6d78cfa4130f6ab1f462080a257..c0dfe6484cfade004f9eea9400909f0c5946e7db 100644
--- a/third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
+++ b/third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
@@ -1290,4 +1290,296 @@ TEST_F(InputMethodControllerTest, SelectionWhenFocusChangeFinishesComposition) {
                    .computeOffsetInContainerNode());
 }
 
+static String getMarkedText(DocumentMarkerController& documentMarkerController,
+                            Node* node,
+                            int markerIndex) {
+  DocumentMarker* marker = documentMarkerController.markers()[markerIndex];
+  return node->textContent().substring(
+      marker->startOffset(), marker->endOffset() - marker->startOffset());
+}
+
+TEST_F(InputMethodControllerTest, WhitespaceFixupAroundMarker) {
+  Element* div = insertHTMLElement(
+      "<div id='sample' contenteditable>Initial text blah</div>", "sample");
+
+  // Add marker under "text" (use TextMatch since Composition markers don't
+  // persist across editing operations)
+  EphemeralRange markerRange = PlainTextRange(8, 12).createRange(*div);
+  document().markers().addMarker(markerRange.startPosition(),
+                                 markerRange.endPosition(),
+                                 DocumentMarker::TextMatch);
+  // Delete "Initial"
+  Vector<CompositionUnderline> emptyUnderlines;
+  controller().setCompositionFromExistingText(emptyUnderlines, 0, 7);
+  controller().commitText(String(""), emptyUnderlines, 0);
+
+  // Delete "blah"
+  controller().setCompositionFromExistingText(emptyUnderlines, 5, 9);

[rlanday, 2017/02/24 20:23:45]

I actually did end up changing the offsets here after adding the assertion that
the string is actually " text " at this point; the offsets were previously 6 to
10 and the string was " text b". This doesn't make sense to me because this
string has seven characters and offset 6 means "between the 6th and 7th
characters" (counting from 1). Is the space at the beginning of the string not
getting counted here?

[Changwan Ryu, 2017/02/27 13:55:57]

Hmm... I think something's wrong here.
The text before this line was " text blah", and you're composing and then
deleting " bla",
so I think the final result should be " text h". Normally the range [5,9) means
starting from 5th character where the index starts from 0 and before 9th
character.

Could you check if you get the same test result with partially reverting the
patch - with this test case and without the main logic? (Also additionally
removing the marker parts in the test).
If the test still fails, I don't mind you landing this with a TODO as this is
perpendicular to that issue.

You can also check controller().compositionText() after this line to make sure
you have the correct composition.

[rlanday, 2017/02/27 18:29:09]

This must be a pre-existing issue since this test passes verbatim on master. I
will file a bug.

[Xiaocheng, 2017/02/27 19:31:23]

I think the bug is in IMC::commitText, instead of offset counting.

In the removal of "Initial", IMC::commitText should convert the following space
into an nbsp so that it doesn't get collapsed. It seems that the space didn't
get converted, which is the bug.

[rlanday, 2017/02/28 00:49:24]

I think what's going on is that if you go through DeleteSelectionCommand (looks
like the only way to do this is by going through
Editor::deleteSelectionWithSmartDelete()),
DeleteSelectionCommand::fixupWhitespace() gets called to convert the spaces to
NBSPs if necessary. But if you try to delete text by performing the operations
I'm performing in this test case, that code never gets run.

Is this something we want to fix? I'm not sure what the right level to call
fixupWhitespace() is. Presumably we need some sort of low-level operation that
doesn't change the whitespace (CharacterData::replaceData()?), but we should
probably do it in response to IME methods?

+  controller().commitText(String(""), emptyUnderlines, 0);
+
+  ASSERT_STREQ(div->innerHTML().utf8().data(), " text ");

[Changwan Ryu, 2017/02/27 13:55:58]

Normally the order is: ASSERT_STREQ(expected, actual);

+
+  // Check that the marker is still attached to "text" and doesn't include
+  // either space around it
+  EXPECT_EQ(1u, document().markers().markersFor(div->firstChild()).size());
+  ASSERT_STREQ(
+      "text",
+      getMarkedText(document().markers(), div->firstChild(), 0).utf8().data());
+}
+
+TEST_F(InputMethodControllerTest, WhitespaceFixupAroundMarker2) {
+  Element* div = insertHTMLElement(
+      "<div id='sample' contenteditable>Initial text blah</div>", "sample");
+
+  // Add marker under " text" (use TextMatch since Composition markers don't
+  // persist across editing operations)
+  EphemeralRange markerRange = PlainTextRange(7, 12).createRange(*div);
+  document().markers().addMarker(markerRange.startPosition(),
+                                 markerRange.endPosition(),
+                                 DocumentMarker::TextMatch);
+  // Delete "Initial"
+  Vector<CompositionUnderline> emptyUnderlines;
+  controller().setCompositionFromExistingText(emptyUnderlines, 0, 7);
+  controller().commitText(String(""), emptyUnderlines, 0);
+
+  // Delete "blah"
+  controller().setCompositionFromExistingText(emptyUnderlines, 5, 9);
+  controller().commitText(String(""), emptyUnderlines, 0);
+
+  ASSERT_STREQ(div->innerHTML().utf8().data(), " text ");
+
+  // before "text" but not the space after
+  EXPECT_EQ(1u, document().markers().markers().size());
+  ASSERT_STREQ(
+      " text",
+      getMarkedText(document().markers(), div->firstChild(), 0).utf8().data());
+}
+
+TEST_F(InputMethodControllerTest, WhitespaceFixupAroundMarker3) {
+  Element* div = insertHTMLElement(
+      "<div id='sample' contenteditable>Initial text blah</div>", "sample");
+
+  // Add marker under "text " (use TextMatch since Composition markers don't
+  // persist across editing operations)
+  EphemeralRange markerRange = PlainTextRange(8, 13).createRange(*div);
+  document().markers().addMarker(markerRange.startPosition(),
+                                 markerRange.endPosition(),
+                                 DocumentMarker::TextMatch);
+  // Delete "Initial"
+  Vector<CompositionUnderline> emptyUnderlines;
+  controller().setCompositionFromExistingText(emptyUnderlines, 0, 7);
+  controller().commitText(String(""), emptyUnderlines, 0);
+
+  // Delete "blah"
+  controller().setCompositionFromExistingText(emptyUnderlines, 5, 9);
+  controller().commitText(String(""), emptyUnderlines, 0);
+
+  ASSERT_STREQ(div->innerHTML().utf8().data(), " text ");
+
+  // Check that the marker is still attached to "text " and includes the space
+  // after "text" but not the space before
+  EXPECT_EQ(1u, document().markers().markers().size());
+  ASSERT_STREQ(
+      "text ",
+      getMarkedText(document().markers(), div->firstChild(), 0).utf8().data());
+}
+
+TEST_F(InputMethodControllerTest, WhitespaceFixupAroundMarker4) {
+  Element* div = insertHTMLElement(
+      "<div id='sample' contenteditable>Initial text blah</div>", "sample");
+
+  // Add marker under " text " (use TextMatch since Composition markers don't
+  // persist across editing operations)
+  EphemeralRange markerRange = PlainTextRange(7, 13).createRange(*div);
+  document().markers().addMarker(markerRange.startPosition(),
+                                 markerRange.endPosition(),
+                                 DocumentMarker::TextMatch);
+
+  // Delete "Initial"
+  Vector<CompositionUnderline> emptyUnderlines;
+  controller().setCompositionFromExistingText(emptyUnderlines, 0, 7);
+  controller().commitText(String(""), emptyUnderlines, 0);
+
+  // Delete "blah"
+  controller().setCompositionFromExistingText(emptyUnderlines, 5, 9);
+  controller().commitText(String(""), emptyUnderlines, 0);
+
+  ASSERT_STREQ(div->innerHTML().utf8().data(), " text ");
+
+  // Check that the marker is still attached to " text " and includes both the
+  // space before "text" and the space after
+  EXPECT_EQ(1u, document().markers().markers().size());
+  ASSERT_STREQ(
+      " text ",
+      getMarkedText(document().markers(), div->firstChild(), 0).utf8().data());
+}
+
+TEST_F(InputMethodControllerTest, ReplaceStartOfMarker) {
+  Element* div = insertHTMLElement(
+      "<div id='sample' contenteditable>Initial text</div>", "sample");
+
+  // Add marker under "Initial text"
+  EphemeralRange markerRange = PlainTextRange(0, 12).createRange(*div);
+  document().markers().addMarker(markerRange.startPosition(),
+                                 markerRange.endPosition(),
+                                 DocumentMarker::TextMatch);
+
+  // Replace "Initial" with "Original"
+  Vector<CompositionUnderline> emptyUnderlines;
+  controller().setCompositionFromExistingText(emptyUnderlines, 0, 7);
+  controller().commitText(String("Original"), emptyUnderlines, 0);
+
+  ASSERT_STREQ(div->innerHTML().utf8().data(), "Original text");
+
+  // Verify marker is under "Original text"
+  EXPECT_EQ(1u, document().markers().markers().size());
+  ASSERT_STREQ(
+      "Original text",
+      getMarkedText(document().markers(), div->firstChild(), 0).utf8().data());
+}
+
+TEST_F(InputMethodControllerTest, ReplaceBeforeAndAfterStartOfMarker) {
+  Element* div = insertHTMLElement(
+      "<div id='sample' contenteditable>This is some initial text</div>",
+      "sample");
+
+  // Add marker under "initial text"
+  EphemeralRange markerRange = PlainTextRange(13, 25).createRange(*div);
+  document().markers().addMarker(markerRange.startPosition(),
+                                 markerRange.endPosition(),
+                                 DocumentMarker::TextMatch);
+
+  // Replace "some initial" with "boring"
+  Vector<CompositionUnderline> emptyUnderlines;
+  controller().setCompositionFromExistingText(emptyUnderlines, 8, 20);
+  controller().commitText(String("boring"), emptyUnderlines, 0);
+
+  ASSERT_STREQ(div->innerHTML().utf8().data(), "This is boring text");
+
+  // Verify marker is under " text"
+  EXPECT_EQ(1u, document().markers().markers().size());
+  ASSERT_STREQ(
+      " text",
+      getMarkedText(document().markers(), div->firstChild(), 0).utf8().data());
+}
+
+TEST_F(InputMethodControllerTest, ReplaceEndOfMarker) {
+  Element* div = insertHTMLElement(
+      "<div id='sample' contenteditable>Initial text</div>", "sample");
+
+  // Add marker under "Initial text"
+  EphemeralRange markerRange = PlainTextRange(0, 12).createRange(*div);
+  document().markers().addMarker(markerRange.startPosition(),
+                                 markerRange.endPosition(),
+                                 DocumentMarker::TextMatch);
+
+  // Replace "text" with "string"
+  Vector<CompositionUnderline> emptyUnderlines;
+  controller().setCompositionFromExistingText(emptyUnderlines, 8, 12);
+  controller().commitText(String("string"), emptyUnderlines, 0);
+
+  ASSERT_STREQ(div->innerHTML().utf8().data(), "Initial string");
+
+  // Verify marker is under "Initial string"
+  EXPECT_EQ(1u, document().markers().markers().size());
+  ASSERT_STREQ(
+      "Initial string",
+      getMarkedText(document().markers(), div->firstChild(), 0).utf8().data());
+}
+
+TEST_F(InputMethodControllerTest, ReplaceBeforeAndAfterEndOfMarker) {
+  Element* div = insertHTMLElement(
+      "<div id='sample' contenteditable>This is some initial text</div>",
+      "sample");
+
+  // Add marker under "some initial"
+  EphemeralRange markerRange = PlainTextRange(8, 20).createRange(*div);
+  document().markers().addMarker(markerRange.startPosition(),
+                                 markerRange.endPosition(),
+                                 DocumentMarker::TextMatch);
+
+  // Replace "initial text" with "content"
+  Vector<CompositionUnderline> emptyUnderlines;
+  controller().setCompositionFromExistingText(emptyUnderlines, 13, 25);
+  controller().commitText(String("content"), emptyUnderlines, 0);
+
+  ASSERT_STREQ(div->innerHTML().utf8().data(), "This is some content");
+
+  // Verify marker is under "some "
+  EXPECT_EQ(1u, document().markers().markers().size());
+  ASSERT_STREQ(
+      "some ",
+      getMarkedText(document().markers(), div->firstChild(), 0).utf8().data());
+}
+
+TEST_F(InputMethodControllerTest, ReplaceEntireMarker) {
+  Element* div = insertHTMLElement(
+      "<div id='sample' contenteditable>Initial text</div>", "sample");
+
+  // Add marker under "text"
+  EphemeralRange markerRange = PlainTextRange(8, 12).createRange(*div);
+  document().markers().addMarker(markerRange.startPosition(),
+                                 markerRange.endPosition(),
+                                 DocumentMarker::TextMatch);
+
+  // Replace "text" with "string"
+  Vector<CompositionUnderline> emptyUnderlines;
+  controller().setCompositionFromExistingText(emptyUnderlines, 8, 12);
+  controller().commitText(String("string"), emptyUnderlines, 0);
+
+  ASSERT_STREQ(div->innerHTML().utf8().data(), "Initial string");
+
+  // Verify marker is under "string"
+  EXPECT_EQ(1u, document().markers().markers().size());
+  ASSERT_STREQ(
+      "string",
+      getMarkedText(document().markers(), div->firstChild(), 0).utf8().data());
+}
+
+TEST_F(InputMethodControllerTest, ReplaceTextWithMarkerAtBeginning) {
+  Element* div = insertHTMLElement(
+      "<div id='sample' contenteditable>Initial text</div>", "sample");
+
+  // Add marker under "Initial"
+  EphemeralRange markerRange = PlainTextRange(0, 7).createRange(*div);
+  document().markers().addMarker(markerRange.startPosition(),
+                                 markerRange.endPosition(),
+                                 DocumentMarker::TextMatch);
+
+  EXPECT_EQ(1u, document().markers().markers().size());
+
+  // Replace "Initial text" with "New string"
+  Vector<CompositionUnderline> emptyUnderlines;
+  controller().setCompositionFromExistingText(emptyUnderlines, 0, 12);
+  controller().commitText(String("New string"), emptyUnderlines, 0);
+
+  ASSERT_STREQ(div->innerHTML().utf8().data(), "New string");
+
+  // Verify marker was removed
+  EXPECT_EQ(0u, document().markers().markers().size());
+}
+
+TEST_F(InputMethodControllerTest, ReplaceTextWithMarkerAtEnd) {
+  Element* div = insertHTMLElement(
+      "<div id='sample' contenteditable>Initial text</div>", "sample");
+
+  // Add marker under "text"
+  EphemeralRange markerRange = PlainTextRange(8, 12).createRange(*div);
+  document().markers().addMarker(markerRange.startPosition(),
+                                 markerRange.endPosition(),
+                                 DocumentMarker::TextMatch);
+
+  EXPECT_EQ(1u, document().markers().markers().size());
+
+  // Replace "Initial text" with "New string"
+  Vector<CompositionUnderline> emptyUnderlines;
+  controller().setCompositionFromExistingText(emptyUnderlines, 0, 12);
+  controller().commitText(String("New string"), emptyUnderlines, 0);
+
+  ASSERT_STREQ(div->innerHTML().utf8().data(), "New string");
+
+  // Verify marker was removed
+  EXPECT_EQ(0u, document().markers().markers().size());
+}
+
 }  // namespace blink