Fixing undo after dragging text between different elements

Fixing a bug where dragging text from one input element to another, and
then trying to undo this change, makes first input element's value
inaccessible. In order to fix this, the root editable element
associated with a `SelectionForUndoStep` is calculated at construction
time. It is important to do so at construction time as the base
position of the selection might be disconnected from the document, thus
we might not be able to compute the root editable element.

Bug: 864941
Test: Drag text from one input element to another, and then undo this
change. After undoing, try to use the first input element's `value`
property, and verify it has the correct value.

Change-Id: Ie0016f7459bad462e67e508b64e2686c39a66fcf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2574954
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836325}

AUTHORS

@@ -869,6 +869,7 @@ Robert Hogan <robhogan@gmail.com>
 Robert O'Callahan <rocallahan@gmail.com>
 Robert Nagy <robert.nagy@gmail.com>
 Robert Sesek <rsesek@bluestatic.org>
+Roee Kasher <roee91@gmail.com>
 Roger Zanoni <rogerzanoni@gmail.com>
 Roland Takacs <rtakacs.u-szeged@partner.samsung.com>
 Romain Pokrzywka <romain.pokrzywka@gmail.com>

third_party/blink/renderer/core/editing/commands/selection_for_undo_step.cc

@@ -18,6 +18,7 @@ SelectionForUndoStep SelectionForUndoStep::From(
   result.extent_ = selection.Extent();
   result.affinity_ = selection.Affinity();
   result.is_base_first_ = selection.IsBaseFirst();
+  result.root_editable_element_ = RootEditableElementOf(result.base_);
   return result;
 }
 
@@ -81,6 +82,7 @@ bool SelectionForUndoStep::IsValidFor(const Document& document) const {
 void SelectionForUndoStep::Trace(Visitor* visitor) const {
   visitor->Trace(base_);
   visitor->Trace(extent_);
+  visitor->Trace(root_editable_element_);
 }
 
 // ---

third_party/blink/renderer/core/editing/commands/selection_for_undo_step.h

@@ -37,6 +37,7 @@ class SelectionForUndoStep final {
   Position Base() const { return base_; }
   Position Extent() const { return extent_; }
   bool IsBaseFirst() const { return is_base_first_; }
+  Element* RootEditableElement() const { return root_editable_element_.Get(); }
 
   SelectionInDOMTree AsSelection() const;
 
@@ -61,9 +62,12 @@ class SelectionForUndoStep final {
   Position base_;
   Position extent_;
   TextAffinity affinity_ = TextAffinity::kDownstream;
-  // Note: We should compute |is_base_first_| as construction otherwise we
+  // Note: We should compute |is_base_first_| at construction otherwise we
   // fail "backward and forward delete" case in "undo-delete-boundary.html".
   bool is_base_first_ = true;
+  // Since |base_| and |extent_| can be disconnected from document, we have to
+  // calculate the root editable element at construction time
+  Member<Element> root_editable_element_;
 };
 
 // Builds |SelectionForUndoStep| object with disconnected position. You should

third_party/blink/renderer/core/editing/commands/undo_step.cc

@@ -27,10 +27,6 @@ UndoStep::UndoStep(Document* document,
     : document_(document),
       starting_selection_(starting_selection),
       ending_selection_(ending_selection),
-      starting_root_editable_element_(
-          RootEditableElementOf(starting_selection.Base())),
-      ending_root_editable_element_(
-          RootEditableElementOf(ending_selection.Base())),
       input_type_(input_type),
       sequence_number_(++g_current_sequence_number) {}
 
@@ -131,12 +127,10 @@ void UndoStep::Append(UndoStep* undo_step) {
 
 void UndoStep::SetStartingSelection(const SelectionForUndoStep& selection) {
   starting_selection_ = selection;
-  starting_root_editable_element_ = RootEditableElementOf(selection.Base());
 }
 
 void UndoStep::SetEndingSelection(const SelectionForUndoStep& selection) {
   ending_selection_ = selection;
-  ending_root_editable_element_ = RootEditableElementOf(selection.Base());
 }
 
 void UndoStep::Trace(Visitor* visitor) const {
@@ -144,8 +138,6 @@ void UndoStep::Trace(Visitor* visitor) const {
   visitor->Trace(starting_selection_);
   visitor->Trace(ending_selection_);
   visitor->Trace(commands_);
-  visitor->Trace(starting_root_editable_element_);
-  visitor->Trace(ending_root_editable_element_);
 }
 
 }  // namespace blink

third_party/blink/renderer/core/editing/commands/undo_step.h

@@ -65,10 +65,10 @@ class UndoStep final : public GarbageCollected<UndoStep> {
     selection_is_directional_ = is_directional;
   }
   Element* StartingRootEditableElement() const {
-    return starting_root_editable_element_.Get();
+    return starting_selection_.RootEditableElement();
   }
   Element* EndingRootEditableElement() const {
-    return ending_root_editable_element_.Get();
+    return ending_selection_.RootEditableElement();
   }
 
   uint64_t SequenceNumber() const { return sequence_number_; }
@@ -80,8 +80,6 @@ class UndoStep final : public GarbageCollected<UndoStep> {
   SelectionForUndoStep starting_selection_;
   SelectionForUndoStep ending_selection_;
   HeapVector<Member<SimpleEditCommand>> commands_;
-  Member<Element> starting_root_editable_element_;
-  Member<Element> ending_root_editable_element_;
   InputEvent::InputType input_type_;
   const uint64_t sequence_number_;
   bool selection_is_directional_ = false;

third_party/blink/web_tests/editing/undo/undo-after-dragging-text-between-elements.html

+<!DOCTYPE html>
+<body>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+
+<textarea id="textarea1">Drag me to the other textarea</textarea>
+<textarea id="textarea2"></textarea>
+
+<script>
+const textarea1 = document.getElementById('textarea1');
+const textarea2 = document.getElementById('textarea2');
+
+function hoverOverElement(element) {
+    const offsetX = element.offsetLeft;
+    const offsetY = element.offsetTop;
+
+    const centerX = offsetX + (element.offsetWidth / 2);
+    const centerY = offsetY + (element.offsetHeight / 2);
+
+    eventSender.mouseMoveTo(centerX, centerY);
+}
+
+function assertTextareasValues(expectedValueTextarea1, expectedValueTextarea2) {
+    assert_equals(textarea1.value, expectedValueTextarea1, "Wrong value for textarea1");
+    assert_equals(textarea2.value, expectedValueTextarea2, "Wrong value for textarea2");
+}
+
+test(() => {
+    assert_own_property(window, 'eventSender', 'This test requires eventSender.');
+
+    // Make sure the initial values are correct
+    assertTextareasValues("Drag me to the other textarea", "");
+
+    // Select all the text in textarea1
+    textarea1.select();
+
+    // Start dragging the text from textarea1
+    hoverOverElement(textarea1);
+    eventSender.mouseDown();
+
+    // Leap the event time so that mouseMove will start a new drag instead of changing selection.
+    eventSender.leapForward(400);
+
+    // Drop the text on textarea2
+    hoverOverElement(textarea2);
+    eventSender.mouseUp();
+
+    // Make sure the text was dragged and the values were updated
+    assertTextareasValues("", "Drag me to the other textarea");
+
+    // Undo, and make sure the values were also reverted
+    document.execCommand('Undo');
+    assertTextareasValues("Drag me to the other textarea", "");
+
+}, "Undo after dragging text between elements should restore the values");
+</script>
+</body>