[InputEvent] Don't modify selection when transpose was canceled

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

Index: third_party/WebKit/Source/core/editing/Editor.cpp
diff --git a/third_party/WebKit/Source/core/editing/Editor.cpp b/third_party/WebKit/Source/core/editing/Editor.cpp
index 5d0c85686a345b6edbb41d7c3b46bb40eaa3ee0f..4d3e869fc31306f16374516549334d89f25ce1eb 100644
--- a/third_party/WebKit/Source/core/editing/Editor.cpp
+++ b/third_party/WebKit/Source/core/editing/Editor.cpp
@@ -145,6 +145,25 @@ bool isInPasswordFieldWithUnrevealedPassword(const Position& position) {
          !input->shouldRevealPassword();
 }
 
+EphemeralRange getTransposeRange(LocalFrame& frame) {

[yosin_UTC9, 2017/04/06 04:32:23]

s/getTransposeRange/computeRangeForTranspose/.
Let's avoid using |get| for less confusion of getter of Editor class.

[chongz, 2017/04/07 02:29:22]

Done.

+  VisibleSelection selection =

[yosin_UTC9, 2017/04/06 04:32:23]

nit: s/VisibleSeleciton/const VisibleSelection&/

[chongz, 2017/04/07 02:29:22]

Done.

+      frame.selection().computeVisibleSelectionInDOMTree();
+  if (!selection.isCaret())
+    return EphemeralRange();
+
+  // Make a selection that goes back one character and forward two characters.
+  VisiblePosition caret = selection.visibleStart();

[yosin_UTC9, 2017/04/06 04:32:23]

nit: s/VisiblePosition/const VisiblePositon&/

[chongz, 2017/04/07 02:29:22]

Done.

+  VisiblePosition next =

[yosin_UTC9, 2017/04/06 04:32:23]

nit: s/VisiblePosition/const VisiblePositon&/

[chongz, 2017/04/07 02:29:22]

Done.

+      isEndOfParagraph(caret) ? caret : nextPositionOf(caret);
+  VisiblePosition previous = previousPositionOf(next);

[yosin_UTC9, 2017/04/06 04:32:23]

nit: s/VisiblePosition/const VisiblePositon&/

[chongz, 2017/04/07 02:29:22]

Done.

+  if (next.deepEquivalent() == previous.deepEquivalent())
+    return EphemeralRange();
+  previous = previousPositionOf(previous);

[yosin_UTC9, 2017/04/06 04:32:23]

Could you introduce new variable rather than re-using |previous|?

[chongz, 2017/04/07 02:29:22]

Done.

+  if (!inSameParagraph(next, previous))
+    return EphemeralRange();
+  return makeRange(previous, next);
+}
+
 }  // anonymous namespace
 
 Editor::RevealSelectionScope::RevealSelectionScope(Editor* editor)
@@ -1380,26 +1399,13 @@ void Editor::transpose() {
   if (!canEdit())
     return;
 
-  VisibleSelection selection =
-      frame().selection().computeVisibleSelectionInDOMTreeDeprecated();
-  if (!selection.isCaret())
-    return;
+  // TODO(editing-dev): The use of updateStyleAndLayoutIgnorePendingStylesheets
+  // needs to be audited. see http://crbug.com/590369 for more details.
+  frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
 
-  // Make a selection that goes back one character and forward two characters.
-  VisiblePosition caret = selection.visibleStart();
-  VisiblePosition next =
-      isEndOfParagraph(caret) ? caret : nextPositionOf(caret);
-  VisiblePosition previous = previousPositionOf(next);
-  if (next.deepEquivalent() == previous.deepEquivalent())
-    return;
-  previous = previousPositionOf(previous);
-  if (!inSameParagraph(next, previous))
-    return;
-  const EphemeralRange range = makeRange(previous, next);
+  EphemeralRange range = getTransposeRange(frame());

[yosin_UTC9, 2017/04/06 04:32:24]

nit: s/EphemeralRange/const EphemeralRange&/

[chongz, 2017/04/07 02:29:22]

Done.

   if (range.isNull())
     return;
-  const SelectionInDOMTree newSelection =
-      SelectionInDOMTree::Builder().setBaseAndExtent(range).build();
 
   // Transpose the two characters.
   String text = plainText(range);
@@ -1407,14 +1413,10 @@ void Editor::transpose() {
     return;
   String transposed = text.right(1) + text.left(1);
 
-  // Select the two characters.
-  if (createVisibleSelection(newSelection) !=
-      frame().selection().computeVisibleSelectionInDOMTreeDeprecated())
-    frame().selection().setSelection(newSelection);
-
   if (dispatchBeforeInputInsertText(
           eventTargetNodeForDocument(frame().document()), transposed,
-          InputEvent::InputType::InsertTranspose) !=
+          InputEvent::InputType::InsertTranspose,
+          new StaticRangeVector(1, StaticRange::create(range))) !=
       DispatchEventResult::NotCanceled)
     return;
 
@@ -1426,6 +1428,25 @@ void Editor::transpose() {
   // needs to be audited. see http://crbug.com/590369 for more details.
   frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
 
+  // 'beforeinput' event handler may change selection, we need to re-calculate
+  // range.
+  range = getTransposeRange(frame());
+  if (range.isNull())
+    return;
+
+  text = plainText(range);

[yosin_UTC9, 2017/04/06 04:32:23]

Could you introduce new variable to avoid re-using |text|?

[chongz, 2017/04/07 02:29:22]

Done.

+  if (text.length() != 2)
+    return;
+  transposed = text.right(1) + text.left(1);

[yosin_UTC9, 2017/04/06 04:32:24]

Could you introduce new variable?

[chongz, 2017/04/07 02:29:22]

Done.

+
+  const SelectionInDOMTree newSelection =

[yosin_UTC9, 2017/04/06 04:32:24]

nit: s/const SelectionInDOMTree/const SelectionInDOMTree&/

[chongz, 2017/04/07 02:29:22]

Done.

+      SelectionInDOMTree::Builder().setBaseAndExtent(range).build();
+
+  // Select the two characters.
+  if (createVisibleSelection(newSelection) !=
+      frame().selection().computeVisibleSelectionInDOMTree())
+    frame().selection().setSelection(newSelection);
+
   // Insert the transposed characters.
   replaceSelectionWithText(transposed, false, false,
                            InputEvent::InputType::InsertTranspose);