[Views] Added Selection on Undo

Previously, after a section of text was removed and the removal was
undone, the cursor was placed after the text. The desired behavior
would be a selection of the re-placed text. This change saves the
current selection range on delete or replace. The selection range is
then restored on undo.

Changed tests:
  - TextfieldModelTest.UndoRedo_CutCopyPasteTest
  - OmniboxViewTest.UndoRedo
Added tests:
  - TextfieldModelTest.Undo_SelectionTest

Bug: 615345
Change-Id: I9ce001099de7175da688a993ed3bfd29d296660c
Reviewed-on: https://chromium-review.googlesource.com/1066675
Commit-Queue: Tessa Nijssen <tnijssen@google.com>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Sarah Chan <spqchan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561338}

chrome/browser/ui/omnibox/omnibox_view_browsertest.cc

@@ -1614,15 +1614,16 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, UndoRedo) {
 
   // Perform an undo.
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_Z, kCtrlOrCmdMask));
-  EXPECT_FALSE(omnibox_view->IsSelectAll());
+  EXPECT_TRUE(omnibox_view->IsSelectAll());
 
-  // The cursor should be at the end.
+  // The text should be selected.
   size_t start, end;
   omnibox_view->GetSelectionBounds(&start, &end);
   EXPECT_EQ(old_text.size(), start);
-  EXPECT_EQ(old_text.size(), end);
+  EXPECT_EQ(0U, end);
 
   // Delete three characters; "about:bl" should not trigger inline autocomplete.
+  ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_END, 0));
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_BACK, 0));
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_BACK, 0));
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_BACK, 0));

ui/views/controls/textfield/textfield_model.cc

@@ -36,16 +36,15 @@ class Edit {
 
   // Revert the change made by this edit in |model|.
   void Undo(TextfieldModel* model) {
-    model->ModifyText(new_text_start_, new_text_end(),
-                      old_text_, old_text_start_,
-                      old_cursor_pos_);
+    model->ModifyText(new_text_start_, new_text_end(), old_text_,
+                      old_text_start_, old_selection_);
   }
 
   // Apply the change of this edit to the |model|.
   void Redo(TextfieldModel* model) {
-    model->ModifyText(old_text_start_, old_text_end(),
-                      new_text_, new_text_start_,
-                      new_cursor_pos_);
+    model->ModifyText(old_text_start_, old_text_end(), new_text_,
+                      new_text_start_,
+                      gfx::Range(new_cursor_pos_, new_cursor_pos_));
   }
 
   // Try to merge the |edit| into this edit and returns true on success. The
@@ -72,23 +71,22 @@ class Edit {
 
   Edit(Type type,
        MergeType merge_type,
-       size_t old_cursor_pos,
        const base::string16& old_text,
        size_t old_text_start,
+       gfx::Range old_selection,
        bool delete_backward,
        size_t new_cursor_pos,
        const base::string16& new_text,
        size_t new_text_start)
       : type_(type),
         merge_type_(merge_type),
-        old_cursor_pos_(old_cursor_pos),
         old_text_(old_text),
         old_text_start_(old_text_start),
+        old_selection_(old_selection),
         delete_backward_(delete_backward),
         new_cursor_pos_(new_cursor_pos),
         new_text_(new_text),
-        new_text_start_(new_text_start) {
-  }
+        new_text_start_(new_text_start) {}
 
   // Each type of edit provides its own specific merge implementation.
   virtual bool DoMerge(const Edit* edit) = 0;
@@ -131,12 +129,12 @@ class Edit {
 
   // The type of merging allowed.
   MergeType merge_type_;
-  // Old cursor position.
-  size_t old_cursor_pos_;
   // Deleted text by this edit.
   base::string16 old_text_;
   // The index of |old_text_|.
   size_t old_text_start_;
+  // The range of the text selection prior to the edit.
+  gfx::Range old_selection_;
   // True if the deletion is made backward.
   bool delete_backward_;
   // New cursor position.
@@ -154,14 +152,13 @@ class InsertEdit : public Edit {
   InsertEdit(bool mergeable, const base::string16& new_text, size_t at)
       : Edit(INSERT_EDIT,
              mergeable ? MERGEABLE : DO_NOT_MERGE,
-             at  /* old cursor */,
              base::string16(),
              at,
-             false  /* N/A */,
-             at + new_text.length()  /* new cursor */,
+             gfx::Range(at, at),
+             false /* N/A */,
+             at + new_text.length() /* new cursor */,
              new_text,
-             at) {
-  }
+             at) {}
 
   // Edit implementation.
   bool DoMerge(const Edit* edit) override {
@@ -180,21 +177,21 @@ class ReplaceEdit : public Edit {
  public:
   ReplaceEdit(MergeType merge_type,
               const base::string16& old_text,
-              size_t old_cursor_pos,
               size_t old_text_start,
+              gfx::Range old_selection,
               bool backward,
               size_t new_cursor_pos,
               const base::string16& new_text,
               size_t new_text_start)
-      : Edit(REPLACE_EDIT, merge_type,
-             old_cursor_pos,
+      : Edit(REPLACE_EDIT,
+             merge_type,
              old_text,
              old_text_start,
+             old_selection,
              backward,
              new_cursor_pos,
              new_text,
-             new_text_start) {
-  }
+             new_text_start) {}
 
   // Edit implementation.
   bool DoMerge(const Edit* edit) override {
@@ -214,17 +211,17 @@ class DeleteEdit : public Edit {
   DeleteEdit(bool mergeable,
              const base::string16& text,
              size_t text_start,
-             bool backward)
+             bool backward,
+             gfx::Range old_selection)
       : Edit(DELETE_EDIT,
              mergeable ? MERGEABLE : DO_NOT_MERGE,
-             (backward ? text_start + text.length() : text_start),
              text,
              text_start,
+             old_selection,
              backward,
              text_start,
              base::string16(),
-             text_start) {
-  }
+             text_start) {}
 
   // Edit implementation.
   bool DoMerge(const Edit* edit) override {
@@ -334,14 +331,13 @@ bool TextfieldModel::SetText(const base::string16& new_text) {
   if (text() != new_text) {
     if (changed)  // No need to remember composition.
       Undo();
-    size_t old_cursor = GetCursorPosition();
     // SetText moves the cursor to the end.
     size_t new_cursor = new_text.length();
-    SelectAll(false);
     // If there is a composition text, don't merge with previous edit.
     // Otherwise, force merge the edits.
     ExecuteAndRecordReplace(changed ? DO_NOT_MERGE : FORCE_MERGE,
-                            old_cursor, new_cursor, new_text, 0U);
+                            gfx::Range(0, text().length()), new_cursor,
+                            new_text, 0U);
     render_text_->SetCursorPosition(new_cursor);
   }
   ClearSelection();
@@ -627,11 +623,8 @@ void TextfieldModel::DeleteSelectionAndInsertTextAt(
     size_t position) {
   if (HasCompositionText())
     CancelCompositionText();
-  ExecuteAndRecordReplace(DO_NOT_MERGE,
-                          GetCursorPosition(),
-                          position + new_text.length(),
-                          new_text,
-                          position);
+  ExecuteAndRecordReplace(DO_NOT_MERGE, render_text_->selection(),
+                          position + new_text.length(), new_text, position);
 }
 
 base::string16 TextfieldModel::GetTextFromRange(const gfx::Range& range) const {
@@ -776,8 +769,9 @@ void TextfieldModel::ExecuteAndRecordDelete(gfx::Range range, bool mergeable) {
   size_t old_text_start = range.GetMin();
   const base::string16 old_text = text().substr(old_text_start, range.length());
   bool backward = range.is_reversed();
+  gfx::Range curr_selection = render_text_->selection();
   auto edit = std::make_unique<DeleteEdit>(mergeable, old_text, old_text_start,
-                                           backward);
+                                           backward, curr_selection);
   edit->Redo(this);
   AddOrMergeEditHistory(std::move(edit));
 }
@@ -787,23 +781,21 @@ void TextfieldModel::ExecuteAndRecordReplaceSelection(
     const base::string16& new_text) {
   size_t new_text_start = render_text_->selection().GetMin();
   size_t new_cursor_pos = new_text_start + new_text.length();
-  ExecuteAndRecordReplace(merge_type,
-                          GetCursorPosition(),
-                          new_cursor_pos,
-                          new_text,
-                          new_text_start);
+  ExecuteAndRecordReplace(merge_type, render_text_->selection(), new_cursor_pos,
+                          new_text, new_text_start);
 }
 
 void TextfieldModel::ExecuteAndRecordReplace(MergeType merge_type,
-                                             size_t old_cursor_pos,
+                                             gfx::Range replacement_range,
                                              size_t new_cursor_pos,
                                              const base::string16& new_text,
                                              size_t new_text_start) {
-  size_t old_text_start = render_text_->selection().GetMin();
-  bool backward = render_text_->selection().is_reversed();
+  size_t old_text_start = replacement_range.GetMin();
+  bool backward = replacement_range.is_reversed();
   auto edit = std::make_unique<ReplaceEdit>(
-      merge_type, GetSelectedText(), old_cursor_pos, old_text_start, backward,
-      new_cursor_pos, new_text, new_text_start);
+      merge_type, GetTextFromRange(replacement_range), old_text_start,
+      render_text_->selection(), backward, new_cursor_pos, new_text,
+      new_text_start);
   edit->Redo(this);
   AddOrMergeEditHistory(std::move(edit));
 }
@@ -840,7 +832,7 @@ void TextfieldModel::ModifyText(size_t delete_from,
                                 size_t delete_to,
                                 const base::string16& new_text,
                                 size_t new_text_insert_at,
-                                size_t new_cursor_pos) {
+                                gfx::Range selection) {
   DCHECK_LE(delete_from, delete_to);
   base::string16 old_text = text();
   ClearComposition();
@@ -848,8 +840,11 @@ void TextfieldModel::ModifyText(size_t delete_from,
     render_text_->SetText(old_text.erase(delete_from, delete_to - delete_from));
   if (!new_text.empty())
     render_text_->SetText(old_text.insert(new_text_insert_at, new_text));
-  render_text_->SetCursorPosition(new_cursor_pos);
-  // TODO(oshima): Select text that was just undone, like Mac (but not GTK).
+  if (selection.start() == selection.end()) {
+    render_text_->SetCursorPosition(selection.start());
+  } else {
+    render_text_->SelectRange(selection);
+  }
 }
 
 // static

ui/views/controls/textfield/textfield_model.h

@@ -260,7 +260,7 @@ class VIEWS_EXPORT TextfieldModel {
   void ExecuteAndRecordReplaceSelection(internal::MergeType merge_type,
                                         const base::string16& new_text);
   void ExecuteAndRecordReplace(internal::MergeType merge_type,
-                               size_t old_cursor_pos,
+                               gfx::Range replacement_range,
                                size_t new_cursor_pos,
                                const base::string16& new_text,
                                size_t new_text_start);
@@ -270,15 +270,15 @@ class VIEWS_EXPORT TextfieldModel {
   void AddOrMergeEditHistory(std::unique_ptr<internal::Edit> edit);
 
   // Modify the text buffer in following way:
-  // 1) Delete the string from |delete_from| to |delte_to|.
+  // 1) Delete the string from |delete_from| to |delete_to|.
   // 2) Insert the |new_text| at the index |new_text_insert_at|.
   //    Note that the index is after deletion.
-  // 3) Move the cursor to |new_cursor_pos|.
+  // 3) Select |selection|.
   void ModifyText(size_t delete_from,
                   size_t delete_to,
                   const base::string16& new_text,
                   size_t new_text_insert_at,
-                  size_t new_cursor_pos);
+                  gfx::Range selection);
 
   void ClearComposition();
 

ui/views/controls/textfield/textfield_model_unittest.cc

@@ -1386,7 +1386,9 @@ TEST_F(TextfieldModelTest, UndoRedo_CutCopyPasteTest) {
   EXPECT_EQ(1U, model.GetCursorPosition());
   EXPECT_TRUE(model.Undo());
   EXPECT_STR_EQ("ABCDE", model.text());
-  EXPECT_EQ(3U, model.GetCursorPosition());
+  EXPECT_EQ(1U, model.GetCursorPosition());
+  EXPECT_TRUE(model.render_text()->selection().EqualsIgnoringDirection(
+      gfx::Range(1, 3)));
   EXPECT_TRUE(model.Undo());
   EXPECT_STR_EQ("", model.text());
   EXPECT_EQ(0U, model.GetCursorPosition());
@@ -1417,7 +1419,9 @@ TEST_F(TextfieldModelTest, UndoRedo_CutCopyPasteTest) {
   EXPECT_EQ(1U, model.GetCursorPosition());
   EXPECT_TRUE(model.Undo());
   EXPECT_STR_EQ("ABCDE", model.text());
-  EXPECT_EQ(3U, model.GetCursorPosition());
+  EXPECT_EQ(1U, model.GetCursorPosition());
+  EXPECT_TRUE(model.render_text()->selection().EqualsIgnoringDirection(
+      gfx::Range(1, 3)));
   EXPECT_TRUE(model.Undo());
   EXPECT_STR_EQ("", model.text());
   EXPECT_EQ(0U, model.GetCursorPosition());
@@ -1459,8 +1463,9 @@ TEST_F(TextfieldModelTest, UndoRedo_CutCopyPasteTest) {
   // An empty cut shouldn't create an edit.
   EXPECT_TRUE(model.Undo());
   EXPECT_STR_EQ("ABCBCBCDE", model.text());
-  EXPECT_EQ(3U, model.GetCursorPosition());
-
+  EXPECT_EQ(1U, model.GetCursorPosition());
+  EXPECT_TRUE(model.render_text()->selection().EqualsIgnoringDirection(
+      gfx::Range(1, 3)));
   // Test Copy.
   ResetModel(&model);
   model.SetText(base::ASCIIToUTF16("12345"));
@@ -1532,6 +1537,47 @@ TEST_F(TextfieldModelTest, UndoRedo_CursorTest) {
   EXPECT_FALSE(model.Redo());
 }
 
+TEST_F(TextfieldModelTest, Undo_SelectionTest) {
+  gfx::Range range = gfx::Range(2, 4);
+  TextfieldModel model(nullptr);
+  model.SetText(base::ASCIIToUTF16("abcdef"));
+  model.SelectRange(range);
+  EXPECT_EQ(model.render_text()->selection(), range);
+
+  // Deleting the selected text should change the text and the range.
+  EXPECT_TRUE(model.Backspace());
+  EXPECT_STR_EQ("abef", model.text());
+  EXPECT_EQ(model.render_text()->selection(), gfx::Range(2, 2));
+
+  // Undoing the deletion should restore the former range.
+  EXPECT_TRUE(model.Undo());
+  EXPECT_STR_EQ("abcdef", model.text());
+  EXPECT_EQ(model.render_text()->selection(), range);
+
+  // When range.start = range.end, nothing is selected and
+  // range.start = range.end = cursor position
+  model.MoveCursor(gfx::CHARACTER_BREAK, gfx::CURSOR_LEFT, gfx::SELECTION_NONE);
+  EXPECT_EQ(model.render_text()->selection(), gfx::Range(2, 2));
+
+  // Deleting a single character should change the text and cursor location.
+  EXPECT_TRUE(model.Backspace());
+  EXPECT_STR_EQ("acdef", model.text());
+  EXPECT_EQ(model.render_text()->selection(), gfx::Range(1, 1));
+
+  // Undoing the deletion should restore the former range.
+  EXPECT_TRUE(model.Undo());
+  EXPECT_STR_EQ("abcdef", model.text());
+  EXPECT_EQ(model.render_text()->selection(), gfx::Range(2, 2));
+
+  MoveCursorTo(model, model.text().length());
+  EXPECT_TRUE(model.Backspace());
+  model.SelectRange(gfx::Range(1, 3));
+  model.SetText(base::ASCIIToUTF16("[set]"));
+  EXPECT_TRUE(model.Undo());
+  EXPECT_STR_EQ("abcde", model.text());
+  EXPECT_EQ(model.render_text()->selection(), gfx::Range(1, 3));
+}
+
 void RunInsertReplaceTest(TextfieldModel& model) {
   const bool reverse = model.render_text()->selection().is_reversed();
   model.InsertChar('1');