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 ca051a0c662ea21fe8e1dd82ca3db819b1c1d4e9..c8630caf54f642c69bcb1ae38354f93452b2ab69 100644
--- a/third_party/WebKit/Source/core/editing/InputMethodController.cpp
+++ b/third_party/WebKit/Source/core/editing/InputMethodController.cpp
@@ -472,6 +472,46 @@ void InputMethodController::extendSelectionAndDelete(int before, int after)
     TypingCommand::deleteSelection(*frame().document());
 }
 
+void InputMethodController::deleteSurroundingText(size_t before, size_t after)

[yosin_UTC9, 2016/04/26 08:18:54]

Can you guarantee |before| is at end of grapheme and |after| is at start of
grapheme?
Note: I assume |before| and |after| is code unit (16-bit).

+{
+    if (!editor().canEdit())
+        return;
+    PlainTextRange selectionOffsets(getSelectionOffsets());
+    if (selectionOffsets.isNull())
+        return;
+
+    size_t selectionStart = selectionOffsets.start();
+    size_t selectionEnd = selectionOffsets.end();
+
+    if (before > 0u) {
+        before = std::min(selectionStart, before);

[aelias_OOO_until_Jul13, 2016/04/26 07:29:32]

What is this line for?  I don't understand it.

[yabinh, 2016/04/26 08:05:15]

It's for boundary checking. In case that "before" is too large to exceed the
left boundary of the text.

[yosin_UTC9, 2016/04/27 08:21:16]

When |selectionStart|=0 and |before|=2, |before| goes to |0|.

+
+        // Select the text to be deleted before selectionStart.
+        // For multi-code text, we can select it successfully if we only select
+        // the left half of it, but we can't select it if we only select the
+        // right half of it. For the latter case, we need to adjust the start
+        // of selection.
+        Position basePosition(frame().selection().start().anchorNode(), selectionStart - before + 1);

[aelias_OOO_until_Jul13, 2016/04/26 07:29:32]

Why + 1?

[yabinh, 2016/04/26 08:05:16]

Because previousPositionOf(basePosition, PositionMoveType::GraphemeCluster) is
used to move to left by 1 user-visible character.

For example, if the text is "hello", previousPositionOf is equivalent to minus
1.

+        Position adjustedPosition = previousPositionOf(basePosition, PositionMoveType::GraphemeCluster);
+        int adjustedStart = adjustedPosition.computeOffsetInContainerNode();
+
+        if (!setSelectionOffsets(PlainTextRange(adjustedStart, static_cast<int>(selectionStart))))
+            return;
+        TypingCommand::deleteSelection(*frame().document());

[aelias_OOO_until_Jul13, 2016/04/26 07:29:32]

Hmm, I still don't like the two deletion calls too much because it may fire
duplicate events to JS.  I had in mind that this could be done using a
CompositeEditCommand although looking at that file, it's not too obvious to me
how to do so.  yosin@, any thoughts on if this is the best way?

[yabinh, 2016/04/26 08:05:16]

I've verified that this patch doesn't trigger any onselect or onchange event. Do
I need to check other events?

[yosin_UTC9, 2016/04/26 08:18:54]

Better way is using Range::delteContent(). It dispatches only DOM mutation
events.
You could write:
frame().firstRange()->deleteContent().

[yosin_UTC9, 2016/04/26 09:12:29]

How did you check these events? Did you check with event loop?
"selectionchange" event should be dispatched whenever selection is changed. But,
it is dispatched in event loop since it is async event.

+
+        selectionEnd = selectionEnd - (selectionStart - adjustedStart);
+        selectionStart = adjustedStart;
+    }
+
+    if (after > 0u) {
+        if (!setSelectionOffsets(PlainTextRange(static_cast<int>(selectionEnd), static_cast<int>(selectionEnd + after))))

[yosin_UTC9, 2016/04/26 09:12:29]

We may want to |selectionEnd + after| is at end of grapheme. Could you call
nextPositionOf or previousPositionOf with |PositionMoveType::GraphemeCluster|
like adjusting position for |before|?

+            return;
+        TypingCommand::deleteSelection(*frame().document());
+    }
+
+    setSelectionOffsets(PlainTextRange(selectionStart, selectionEnd));
+}
+
 DEFINE_TRACE(InputMethodController)
 {
     visitor->trace(m_frame);