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 542 matching lines @@
         if (before == 0)
             break;
         ++before;
     } while (frame().selection().start() == frame().selection().end() && before <= static_cast<int>(selectionOffsets.start()));
     // TODO(chongz): New spec might want to change InputType.
     // https://github.com/w3c/editing/issues/125#issuecomment-213041256
     dispatchBeforeInputEditorCommand(m_frame->document()->focusedElement(), InputEvent::InputType::DeleteContent, emptyString(), new RangeVector(1, m_frame->selection().firstRange()));
     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);
     visitor->trace(m_compositionRange);
 }
 
 } // namespace blink