Simplify/clarify Textfield APIs.

In addition to avoiding multiple versions of the same-named function,
this is shorter and generally less-overlapping without loss of
functionality.

Bug: none
Change-Id: Ic7252c960d3ca18ea78351ea3386993e9eae91bb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2363716Reviewed-by: manuk hovanesian <manukh@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801074}

chrome/browser/ui/views/omnibox/omnibox_view_views.cc

@@ -884,16 +884,14 @@ void OmniboxViewViews::SetTextAndSelectedRanges(
   static const uint32_t kPadTrailing = 30;
   static const uint32_t kPadLeading = 10;
 
-  // We use SetTextAndScrollAndSelectRange instead of SetText and
-  // SetSelectedRange in order to avoid triggering accessibility events multiple
-  // times.
-  SetTextAndScrollAndSelectRange(
-      text, ranges[0].end(),
-      {0, std::min<size_t>(ranges[0].end() + kPadTrailing, text.size()),
-       ranges[0].end() - std::min(kPadLeading, ranges[0].end())},
-      ranges[0]);
-  for (size_t i = 1; i < ranges.size(); i++)
-    SetSelectedRange(ranges[i], false);
+  // We use SetTextWithoutCaretBoundsChangeNotification() in order to avoid
+  // triggering accessibility events multiple times.
+  SetTextWithoutCaretBoundsChangeNotification(text, ranges[0].end());
+  Scroll({0, std::min<size_t>(ranges[0].end() + kPadTrailing, text.size()),
+          ranges[0].end() - std::min(kPadLeading, ranges[0].end())});
+  // Setting the primary selected range will also fire an appropriate final
+  // accessibility event after the changes above.
+  SetSelectedRanges(ranges);
 
   // Clear the additional text.
   SetAdditionalText(base::string16());
@@ -907,7 +905,7 @@ void OmniboxViewViews::SetSelectedRanges(
 
   SetSelectedRange(ranges[0]);
   for (size_t i = 1; i < ranges.size(); i++)
-    SetSelectedRange(ranges[i], false);
+    AddSecondarySelectedRange(ranges[i]);
 }
 
 base::string16 OmniboxViewViews::GetSelectedText() const {

ui/views/controls/textfield/textfield.h

@@ -113,31 +113,28 @@ class VIEWS_EXPORT Textfield : public View,
   // textfield.
   const base::string16& GetText() const;
 
-  // Sets the text currently displayed in the Textfield and the cursor position.
-  // Calls to |SetText| are often followed by updating the selection or cursor,
-  // which does not update the edit history. I.e. the cursor position after
-  // redoing this change will be determined by |cursor_position| here and not by
-  // any subsequent calls to e.g. |SetSelectedRange|. Selections are not
-  // explicitly set here since redo's clear the selection anyways.
+  // Sets the text currently displayed in the Textfield.
   void SetText(const base::string16& new_text);
-  void SetText(const base::string16& new_text, size_t cursor_position);
-
-  // Similar to calling SetText followed by SetSelectedRange calls, but will
-  // dedupe some calls. Notably, this prevents OnCaretBoundsChanged from being
-  // called multiple times for a single change which can cause issues for
-  // accessibility tools.
-  // - |text| and |cursor_position| see SetText() comment above.
-  // - |scroll_positions| a vector of positions to scroll into view. This can
-  // contain multiple positions which can be useful to e.g. ensure multiple
-  // positions are in view (if possible). Scrolls are applied in the order of
-  // |scroll_positions|; i.e., later positions will have priority over earlier
-  // positions.
-  // - |range| is used to invoke SetSelectedRange and will also be scrolled to.
-  void SetTextAndScrollAndSelectRange(
-      const base::string16& text,
-      const size_t cursor_position,
-      const std::vector<size_t>& scroll_positions,
-      const gfx::Range range);
+
+  // Sets the text currently displayed in the Textfield and the cursor position.
+  // Does not fire notifications about the caret bounds changing. This is
+  // intended for low-level use, where callers need precise control over what
+  // notifications are fired when, e.g. to avoid firing duplicate accessibility
+  // notifications, which can cause issues for accessibility tools. Updating the
+  // selection or cursor separately afterwards does not update the edit history,
+  // i.e. the cursor position after redoing this change will be determined by
+  // |cursor_position| and not by subsequent calls to e.g. SetSelectedRange().
+  void SetTextWithoutCaretBoundsChangeNotification(const base::string16& text,
+                                                   size_t cursor_position);
+
+  // Scrolls all of |scroll_positions| into view, if possible. For each
+  // position, the minimum scrolling change necessary to just bring the position
+  // into view is applied. |scroll_positions| are applied in order, so later
+  // positions will have priority over earlier positions if not all can be
+  // visible simultaneously.
+  // NOTE: Unlike MoveCursorTo(), this will not fire any accessibility
+  // notifications.
+  void Scroll(const std::vector<size_t>& scroll_positions);
 
   // Appends the given string to the previously-existing text in the field.
   void AppendText(const base::string16& new_text);
@@ -235,7 +232,12 @@ class VIEWS_EXPORT Textfield : public View,
 
   // Selects the specified logical text range.
   void SetSelectedRange(const gfx::Range& range);
-  void SetSelectedRange(const gfx::Range& range, bool primary);
+
+  // Without clearing the current selected range, adds |range| as an additional
+  // selection.
+  // NOTE: Unlike SetSelectedRange(), this will not fire any accessibility
+  // notifications.
+  void AddSecondarySelectedRange(const gfx::Range& range);
 
   // Gets the text selection model.
   const gfx::SelectionModel& GetSelectionModel() const;
@@ -469,6 +471,8 @@ class VIEWS_EXPORT Textfield : public View,
  private:
   friend class TextfieldTestApi;
 
+  enum class TextChangeType { kNone, kInternal, kUserTriggered };
+
   // View overrides:
   // Declared final since overriding by subclasses would interfere with the
   // accounting related to the scheduled text edit command. Subclasses should
@@ -504,7 +508,12 @@ class VIEWS_EXPORT Textfield : public View,
   void UpdateSelectionBackgroundColor();
 
   // Does necessary updates when the text and/or cursor position changes.
-  void UpdateAfterChange(bool text_changed, bool cursor_changed);
+  // If |notify_caret_bounds_changed| is not explicitly set, it will be computed
+  // based on whether either of the other arguments is set.
+  void UpdateAfterChange(
+      TextChangeType text_change_type,
+      bool cursor_changed,
+      base::Optional<bool> notify_caret_bounds_changed = base::nullopt);
 
   // Updates cursor visibility and blinks the cursor if needed.
   void ShowCursor();

ui/views/controls/textfield/textfield_unittest.cc

@@ -845,98 +845,91 @@ TEST_F(TextfieldTest, ModelChangesTest) {
   EXPECT_STR_EQ("this is a test", textfield_->GetSelectedText());
   EXPECT_TRUE(last_contents_.empty());
 
-  textfield_->SetTextAndScrollAndSelectRange(ASCIIToUTF16("another test"), 3,
-                                             {}, {4, 5});
+  textfield_->SetTextWithoutCaretBoundsChangeNotification(
+      ASCIIToUTF16("another test"), 3);
   EXPECT_STR_EQ("another test", model_->text());
   EXPECT_STR_EQ("another test", textfield_->GetText());
-  EXPECT_STR_EQ("h", textfield_->GetSelectedText());
+  EXPECT_EQ(textfield_->GetCursorPosition(), 3u);
   EXPECT_TRUE(last_contents_.empty());
 }
 
-TEST_F(TextfieldTest, SetTextAndScrollAndSelectRange_Scrolling) {
+TEST_F(TextfieldTest, Scroll) {
   InitTextfield();
 
   // Size the textfield wide enough to hold 10 characters.
   gfx::test::RenderTextTestApi render_text_test_api(test_api_->GetRenderText());
-  render_text_test_api.SetGlyphWidth(10);
-  // 10px/char * 10chars + 1px for cursor width
-  test_api_->GetRenderText()->SetDisplayRect(gfx::Rect(0, 0, 101, 20));
-
-  // Should scroll cursor into view.
+  constexpr int kGlyphWidth = 10;
+  render_text_test_api.SetGlyphWidth(kGlyphWidth);
+  constexpr int kCursorWidth = 1;
+  test_api_->GetRenderText()->SetDisplayRect(
+      gfx::Rect(0, 0, kGlyphWidth * 10 + kCursorWidth, 20));
+  textfield_->SetTextWithoutCaretBoundsChangeNotification(
+      ASCIIToUTF16("0123456789_123456789_123456789"), 0);
   test_api_->SetDisplayOffsetX(0);
-  textfield_->SetTextAndScrollAndSelectRange(
-      ASCIIToUTF16("0123456789_123456789_123456789"), 0, {}, {0, 20});
+
+  // Empty Scroll() call should have no effect.
+  textfield_->Scroll({});
+  EXPECT_EQ(test_api_->GetDisplayOffsetX(), 0);
+
+  // Selected range should scroll cursor into view.
+  textfield_->SetSelectedRange({0, 20});
   EXPECT_EQ(test_api_->GetDisplayOffsetX(), -100);
-  EXPECT_EQ(textfield_->GetSelectedRange(), gfx::Range(0, 20));
 
-  // Cursor position should not affect scroll.
+  // Selected range should override new cursor position.
   test_api_->SetDisplayOffsetX(0);
-  textfield_->SetTextAndScrollAndSelectRange(
-      ASCIIToUTF16("0123456789_123456789_123456789"), 30, {}, {20, 20});
+  textfield_->SetTextWithoutCaretBoundsChangeNotification(
+      ASCIIToUTF16("0123456789_123456789_123456789"), 30);
   EXPECT_EQ(test_api_->GetDisplayOffsetX(), -100);
-  EXPECT_EQ(textfield_->GetSelectedRange(), gfx::Range(20));
 
   // Scroll positions should affect scroll.
+  textfield_->SetSelectedRange(gfx::Range());
   test_api_->SetDisplayOffsetX(0);
-  textfield_->SetTextAndScrollAndSelectRange(
-      ASCIIToUTF16("0123456789_123456789_123456789"), 0, {30}, {20, 20});
+  textfield_->Scroll({30});
   EXPECT_EQ(test_api_->GetDisplayOffsetX(), -200);
-  EXPECT_EQ(textfield_->GetSelectedRange(), gfx::Range(20));
 
-  // Should scroll no more than necessary; e.g., scrolling right should put the
-  // cursor at the right edge.
+  // Should scroll right no more than necessary.
   test_api_->SetDisplayOffsetX(0);
-  textfield_->SetTextAndScrollAndSelectRange(
-      ASCIIToUTF16("0123456789_123456789_123456789"), 0, {}, {15, 15});
+  textfield_->Scroll({15});
   EXPECT_EQ(test_api_->GetDisplayOffsetX(), -50);
-  EXPECT_EQ(textfield_->GetSelectedRange(), gfx::Range(15));
 
-  // Should scroll no more than necessary; e.g., scrolling left should put the
-  // cursor at the left edge.
+  // Should scroll left no more than necessary.
   test_api_->SetDisplayOffsetX(-200);  // Scroll all the way right.
-  textfield_->SetTextAndScrollAndSelectRange(
-      ASCIIToUTF16("0123456789_123456789_123456789"), 0, {}, {15, 15});
+  textfield_->Scroll({15});
   EXPECT_EQ(test_api_->GetDisplayOffsetX(), -150);
-  EXPECT_EQ(textfield_->GetSelectedRange(), gfx::Range(15));
 
-  // Should scroll no more than necessary; e.g., scrolling to a position already
-  // in view should not change the offset.
+  // Should not scroll if position is already in view.
   test_api_->SetDisplayOffsetX(-100);  // Scroll the middle 10 chars into view.
-  textfield_->SetTextAndScrollAndSelectRange(
-      ASCIIToUTF16("0123456789_123456789_123456789"), 0, {}, {15, 15});
+  textfield_->Scroll({15});
   EXPECT_EQ(test_api_->GetDisplayOffsetX(), -100);
-  EXPECT_EQ(textfield_->GetSelectedRange(), gfx::Range(15));
 
-  // With multiple scroll positions, the Last scroll position should be scrolled
-  // to after previous scroll positions.
+  // With multiple scroll positions, the Last scroll position takes priority.
   test_api_->SetDisplayOffsetX(0);
-  textfield_->SetTextAndScrollAndSelectRange(
-      ASCIIToUTF16("0123456789_123456789_123456789"), 0, {30, 0}, {20, 20});
+  textfield_->Scroll({30, 0});
+  EXPECT_EQ(test_api_->GetDisplayOffsetX(), 0);
+  textfield_->Scroll({30, 0, 20});
   EXPECT_EQ(test_api_->GetDisplayOffsetX(), -100);
-  EXPECT_EQ(textfield_->GetSelectedRange(), gfx::Range(20));
 
   // With multiple scroll positions, the previous scroll positions should be
   // scrolled to anyways.
   test_api_->SetDisplayOffsetX(0);
-  textfield_->SetTextAndScrollAndSelectRange(
-      ASCIIToUTF16("0123456789_123456789_123456789"), 0, {30, 20}, {20, 20});
+  textfield_->Scroll({30, 20});
   EXPECT_EQ(test_api_->GetDisplayOffsetX(), -200);
-  EXPECT_EQ(textfield_->GetSelectedRange(), gfx::Range(20));
 
-  // With a non empty selection, only the selection end should affect scrolling.
+  // Only the selection end should affect scrolling.
   test_api_->SetDisplayOffsetX(0);
-  textfield_->SetTextAndScrollAndSelectRange(
-      ASCIIToUTF16("0123456789_123456789_123456789"), 0, {0}, {30, 0});
-  EXPECT_EQ(test_api_->GetDisplayOffsetX(), 0);
-  EXPECT_EQ(textfield_->GetSelectedRange(), gfx::Range(30, 0));
+  textfield_->Scroll({20});
+  textfield_->SetSelectedRange({30, 20});
+  EXPECT_EQ(test_api_->GetDisplayOffsetX(), -100);
 }
 
-TEST_F(TextfieldTest, SetTextAndScrollAndSelectRange_ModelEditHistory) {
+TEST_F(TextfieldTest,
+       SetTextWithoutCaretBoundsChangeNotification_ModelEditHistory) {
   InitTextfield();
 
-  // The cursor and selected range should reflect the |range| parameter.
-  textfield_->SetTextAndScrollAndSelectRange(
-      ASCIIToUTF16("0123456789_123456789_123456789"), 20, {}, {10, 15});
+  // The cursor and selected range should reflect the selected range.
+  textfield_->SetTextWithoutCaretBoundsChangeNotification(
+      ASCIIToUTF16("0123456789_123456789_123456789"), 20);
+  textfield_->SetSelectedRange({10, 15});
   EXPECT_EQ(textfield_->GetCursorPosition(), 15u);
   EXPECT_EQ(textfield_->GetSelectedRange(), gfx::Range(10, 15));
 
@@ -1292,7 +1285,7 @@ TEST_F(TextfieldTest, ModifySelectionWithMultipleSelections) {
   InitTextfield();
   textfield_->SetText(ASCIIToUTF16("0123456 89"));
   textfield_->SetSelectedRange(gfx::Range(3, 5));
-  textfield_->SetSelectedRange(gfx::Range(8, 9), false);
+  textfield_->AddSecondarySelectedRange(gfx::Range(8, 9));
 
   test_api_->ExecuteTextEditCommand(
       ui::TextEditCommand::MOVE_RIGHT_AND_MODIFY_SELECTION);
@@ -1411,8 +1404,8 @@ TEST_F(TextfieldTest, DeletionWithMultipleSelections) {
     textfield_->SetText(ASCIIToUTF16("one two three"));
     // Select: o[ne] [two] th[re]e
     textfield_->SetSelectedRange(gfx::Range(4, 7));
-    textfield_->SetSelectedRange(gfx::Range(10, 12), false);
-    textfield_->SetSelectedRange(gfx::Range(1, 3), false);
+    textfield_->AddSecondarySelectedRange(gfx::Range(10, 12));
+    textfield_->AddSecondarySelectedRange(gfx::Range(1, 3));
     SendWordEvent(cases[i].key, cases[i].shift);
     EXPECT_STR_EQ("o  the", textfield_->GetText());
     EXPECT_EQ(gfx::Range(2), textfield_->GetSelectedRange());
@@ -1709,14 +1702,14 @@ TEST_F(TextfieldTest, CursorMovementWithMultipleSelections) {
   textfield_->SetText(ASCIIToUTF16("012 456 890 234 678"));
   //                                    [p]     [s]
   textfield_->SetSelectedRange({4, 7});
-  textfield_->SetSelectedRange({12, 15}, false);
+  textfield_->AddSecondarySelectedRange({12, 15});
 
   test_api_->ExecuteTextEditCommand(ui::TextEditCommand::MOVE_LEFT);
   EXPECT_EQ(gfx::Range(4, 4), textfield_->GetSelectedRange());
   EXPECT_EQ(0U, textfield_->GetSelectionModel().secondary_selections().size());
 
   textfield_->SetSelectedRange({4, 7});
-  textfield_->SetSelectedRange({12, 15}, false);
+  textfield_->AddSecondarySelectedRange({12, 15});
 
   test_api_->ExecuteTextEditCommand(ui::TextEditCommand::MOVE_RIGHT);
   EXPECT_EQ(gfx::Range(7, 7), textfield_->GetSelectedRange());
@@ -1730,13 +1723,13 @@ TEST_F(TextfieldTest, ShouldShowCursor) {
   // should show cursor when there's no primary selection
   textfield_->SetSelectedRange({4, 4});
   EXPECT_TRUE(test_api_->ShouldShowCursor());
-  textfield_->SetSelectedRange({1, 3}, false);
+  textfield_->AddSecondarySelectedRange({1, 3});
   EXPECT_TRUE(test_api_->ShouldShowCursor());
 
   // should not show cursor when there's a primary selection
   textfield_->SetSelectedRange({4, 7});
   EXPECT_FALSE(test_api_->ShouldShowCursor());
-  textfield_->SetSelectedRange({1, 3}, false);
+  textfield_->AddSecondarySelectedRange({1, 3});
   EXPECT_FALSE(test_api_->ShouldShowCursor());
 }