Fix InputConnection.deleteSurroundingText()

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

 /*
  * Copyright (C) 2006, 2007, 2008, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 454 matching lines @@
             return;
         if (before == 0)
             break;
         ++before;
     } while (frame().selection().start() == frame().selection().end() && before <= static_cast<int>(selectionOffsets.start()));
     // TODO(chongz): According to spec |data| should be "forward" or "backward".
     dispatchBeforeInputEditorCommand(frame().document()->focusedElement(), InputEvent::InputType::DeleteContent);
     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);
     visitor->trace(m_compositionRange);
 }
 
 } // namespace blink