Fix InputConnection.deleteSurroundingText()

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 532d114c4474dd5d350eec85d64df9090f6be9cd..f6c001c603e9f6a0e8d75dd21837035ce7891ae2 100644
--- a/third_party/WebKit/Source/core/editing/InputMethodController.cpp
+++ b/third_party/WebKit/Source/core/editing/InputMethodController.cpp
@@ -560,6 +560,69 @@ void InputMethodController::extendSelectionAndDelete(int before, int after)
     TypingCommand::deleteSelection(*frame().document());
 }
 
+// TODO(yabinh): We should reduce the number of selectionchange events.
+void InputMethodController::deleteSurroundingText(size_t before, size_t after)
+{
+    if (!editor().canEdit())
+        return;
+    PlainTextRange selectionOffsets(getSelectionOffsets());
+    if (selectionOffsets.isNull())
+        return;
+    Element* rootEditableElement = frame().selection().rootEditableElement();
+    if (!rootEditableElement)
+        return;
+
+    size_t selectionStart = selectionOffsets.start();
+    size_t selectionEnd = selectionOffsets.end();
+
+    if (before > 0u && selectionStart > 0u) {
+        // In case of exceeding the left boundary.
+        int start = std::max(static_cast<int>(selectionStart) - static_cast<int>(before), 0);
+
+        // Select the text to be deleted before selectionStart.
+        // For multi-code text, we can't select it successfully if we only

[yosin_UTC9, 2016/10/04 09:59:54]

What do you mean "multi-code text"? Do you mean a grapheme cluster contains more
than one code point?

[yabinh, 2016/10/11 03:16:57]

Yes. I'll add some comment in the next patch.

+        // select the right half of it. So we need to adjust the start of
+        // selection.
+        const EphemeralRange range = PlainTextRange(0, start).createRange(*rootEditableElement);

[yosin_UTC9, 2016/10/04 09:59:54]

s/const EphemeralRange/const EphemeralRange&/

[yabinh, 2016/10/11 03:16:57]

Done.

+        if (range.isNull())
+            return;
+        Position position = range.endPosition();
+        Position adjustedPosition = previousPositionOf(nextPositionOf(position, PositionMoveType::GraphemeCluster), PositionMoveType::GraphemeCluster);
+        int diff = adjustedPosition.computeOffsetInContainerNode() - position.computeOffsetInContainerNode();

[Changwan Ryu, 2016/07/20 08:37:42]

what should happen if adjustedPosition and position have different anchor nodes?

Also, I'm a bit worried that we're not using PositionMoveType::BackwardDeletion.
Keyboard apps may expect that code point will be deleted, and may do something
weird if the whole grapheme cluster gets deleted. What happens when you're at
the end of a + telugu combining character and try to delete one value?

[yabinh, 2016/07/20 11:49:16]

There is no such case. I'll add assertion here.
There are 2 problems about "\xE0\xB0\x83"(telugu combining character):


1. BackwardDeletion is only available for prevPositionOf(). This will cause
incompatibility.

For example:

"a\xE0\xB0\x83\xE0\xB0\x83*", deleteSurroundingText(2, 0),  * stands for cursor.

That means we want to delete [1, 3].

|position.computeOffsetInContainerNode()|==1.

If we call  nextPosition = nextPositionOf(position,
PositionMoveType::GraphemeCluster), we will get
|nextPosition.computeOffsetInContainerNode()|==3.

Then call adjustedPosition = previousPositionOf(nextPosition,
PositionMoveType::BackwardDeletion), we will get
|adjustedPosition.computeOffsetInContainerNode()|==2.

That means we are trying to delete [2, 3]. It's not correct.


2. TypingCommand::deleteSelection() adjusts selection for grapheme boundary (
like in crbug.com/620690 ), so we can't delete it successfully.

For example:

"a\xE0\xB0\x83*", deleteSurroundingText(1, 0),  * stands for cursor. 
That means we want to delete [1, 2].

Even if we can select [1, 2], we will not delete it. Actually, we will delete
nothing, because TypingCommand::deleteSelection() adjusts selection to [2, 2] 

+        start = start + diff;
+
+        if (!setSelectionOffsets(PlainTextRange(start, static_cast<int>(selectionStart))))
+            return;
+        TypingCommand::deleteSelection(*frame().document());
+
+        selectionEnd = selectionEnd - (selectionStart - start);
+        selectionStart = start;
+    }
+
+    if (after > 0u) {
+        // Adjust the deleted range in case of exceeding the right boundary.
+        PlainTextRange range(0, static_cast<int>(selectionEnd + after));
+        if (range.isNull())
+            return;
+        const EphemeralRange validRange = range.createRange(*rootEditableElement);
+        if (validRange.isNull())
+            return;
+        int end = PlainTextRange::create(*rootEditableElement, validRange).end();
+
+        // We also need to adjust the end of selection for multi-code text.
+        Position position = validRange.endPosition();

[yosin_UTC9, 2016/10/04 09:59:54]

nit: s/Position/const Position&/

[yabinh, 2016/10/11 03:16:57]

Done.

+        Position adjustedPosition = nextPositionOf(previousPositionOf(position, PositionMoveType::GraphemeCluster), PositionMoveType::GraphemeCluster);

[yosin_UTC9, 2016/10/04 09:59:54]

nit: s/Position/const Position&/

[yabinh, 2016/10/11 03:16:57]

Done.

+        int diff = adjustedPosition.computeOffsetInContainerNode() - position.computeOffsetInContainerNode();

[Changwan Ryu, 2016/07/20 08:37:42]

same comment applies here

[yosin_UTC9, 2016/10/04 09:59:54]

We should handle in case of position.computeContainerNode() !=
adjustedPosition().computedContainerNode()

[yabinh, 2016/10/11 03:16:57]

It seems that we couldn't find such a case. As we discussed offline,  I'll just
add assertion in the next patch.

+        end = end + diff;
+
+        if (!setSelectionOffsets(PlainTextRange(static_cast<int>(selectionEnd), end)))
+            return;
+        TypingCommand::deleteSelection(*frame().document());
+    }
+
+    setSelectionOffsets(PlainTextRange(selectionStart, selectionEnd));
+}
+
 DEFINE_TRACE(InputMethodController)
 {
     visitor->trace(m_frame);