Fix filtering bug where it wouldn't take the selected item into account.

After selecting an item, the menu disappears. Then if we refocus on the
EditableCombobox we expect the filtering to be based on the selected
item. Before this CL it would still be based on the last thing we typed
(a substring of the selected item), so it could show more items than
necessary.

This CL fixes the issue, adds a unit test, and refactors a little bit.
It moves the ShowDropDownMenu call out of HandleNewContent, which is
called inside OnItemSelected to fix the bug.

Bug: 923660
Change-Id: Iaf6a713763d9d5dd5b2c96186fe2ea0898eec978
Reviewed-on: https://chromium-review.googlesource.com/c/1493516Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Edin Kadric <edinkadric@google.com>
Cr-Commit-Position: refs/heads/master@{#636826}

ui/views/controls/editable_combobox/editable_combobox.cc

@@ -66,6 +66,8 @@ class EditableCombobox::EditableComboboxMenuModel
   }
 
   void UpdateItemsShown(const base::string16& text) {
+    if (!update_items_shown_enabled_)
+      return;
     text_ = text;
     items_shown_.clear();
     items_shown_enabled_.clear();
@@ -83,19 +85,32 @@ class EditableCombobox::EditableComboboxMenuModel
       menu_model_delegate()->OnMenuStructureChanged();
   }
 
+  void DisableUpdateItemsShown() { update_items_shown_enabled_ = false; }
+
+  void EnableUpdateItemsShown() { update_items_shown_enabled_ = true; }
+
   //////////////////////////////////////////////////////////////////////////////
   // Overridden from ComboboxModelObserver:
   void OnComboboxModelChanged(ui::ComboboxModel* model) override {
     UpdateItemsShown(text_);
   }
 
- private:
   //////////////////////////////////////////////////////////////////////////////
   // Overridden from MenuModel:
-  bool HasIcons() const override { return false; }
 
   int GetItemCount() const override { return items_shown_.size(); }
 
+  base::string16 GetLabelAt(int index) const override {
+    // Inserting the Unicode formatting characters if necessary so that the text
+    // is displayed correctly in right-to-left UIs.
+    base::string16 text = items_shown_[index];
+    base::i18n::AdjustStringForLocaleDirection(&text);
+    return text;
+  }
+
+ private:
+  bool HasIcons() const override { return false; }
+
   ItemType GetTypeAt(int index) const override { return TYPE_COMMAND; }
 
   ui::MenuSeparatorType GetSeparatorTypeAt(int index) const override {
@@ -107,14 +122,6 @@ class EditableCombobox::EditableComboboxMenuModel
     return index + kFirstMenuItemId;
   }
 
-  base::string16 GetLabelAt(int index) const override {
-    // Inserting the Unicode formatting characters if necessary so that the text
-    // is displayed correctly in right-to-left UIs.
-    base::string16 text = items_shown_[index];
-    base::i18n::AdjustStringForLocaleDirection(&text);
-    return text;
-  }
-
   bool IsItemDynamicAt(int index) const override { return false; }
 
   const gfx::FontList* GetLabelFontListAt(int index) const override {
@@ -160,6 +167,9 @@ class EditableCombobox::EditableComboboxMenuModel
   // The current content of |owner_|'s textfield.
   base::string16 text_;
 
+  // When false, UpdateItemsShown doesn't do anything.
+  bool update_items_shown_enabled_ = true;
+
   DISALLOW_COPY_AND_ASSIGN(EditableComboboxMenuModel);
 };
 
@@ -196,13 +206,6 @@ const base::string16& EditableCombobox::GetText() const {
   return textfield_->text();
 }
 
-void EditableCombobox::SetText(const base::string16& text) {
-  textfield_->SetText(text);
-  // SetText does not actually notify the TextfieldController, so we call the
-  // handling code directly.
-  HandleNewContent(text);
-}
-
 const gfx::FontList& EditableCombobox::GetFontList() const {
   return style::GetFont(text_context_, text_style_);
 }
@@ -215,6 +218,22 @@ void EditableCombobox::SetAssociatedLabel(View* labelling_view) {
   textfield_->SetAssociatedLabel(labelling_view);
 }
 
+int EditableCombobox::GetItemCountForTest() {
+  return menu_model_->GetItemCount();
+}
+
+base::string16 EditableCombobox::GetItemForTest(int index) {
+  return menu_model_->GetLabelAt(index);
+}
+
+void EditableCombobox::SetTextForTest(const base::string16& text) {
+  textfield_->SetText(text);
+  // SetText does not actually notify the TextfieldController, so we call the
+  // handling code directly.
+  HandleNewContent(text);
+  ShowDropDownMenu();
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // EditableCombobox, View overrides:
 
@@ -232,6 +251,7 @@ void EditableCombobox::OnNativeThemeChanged(const ui::NativeTheme* theme) {
 void EditableCombobox::ContentsChanged(Textfield* sender,
                                        const base::string16& new_contents) {
   HandleNewContent(new_contents);
+  ShowDropDownMenu(ui::MENU_SOURCE_KEYBOARD);
 }
 
 void EditableCombobox::OnViewBlurred(View* observed_view) {
@@ -247,22 +267,27 @@ void EditableCombobox::OnViewFocused(View* observed_view) {
 
 void EditableCombobox::OnItemSelected(int index) {
   textfield_->SetText(menu_model_->GetLabelAt(index));
+  // SetText does not actually notify the TextfieldController, so we call the
+  // handling code directly.
+  HandleNewContent(menu_model_->GetLabelAt(index));
   NotifyAccessibilityEvent(ax::mojom::Event::kValueChanged,
                            /*xsend_native_event=*/true);
 }
 
 void EditableCombobox::HandleNewContent(const base::string16& new_content) {
   DCHECK(GetText() == new_content);
-  static_cast<EditableComboboxMenuModel*>(menu_model_.get())
-      ->UpdateItemsShown(new_content);
-
-  if (!menu_model_->GetItemCount())
-    menu_runner_.reset();
-  if (textfield_->HasFocus() && !(menu_runner_ && menu_runner_->IsRunning()))
-    ShowDropDownMenu(ui::MENU_SOURCE_KEYBOARD);
-
-  if (listener_)
+  // We notify |listener_| before updating |menu_model_|'s items shown. This
+  // gives the user a chance to modify the ComboboxModel beforehand if they wish
+  // to do so.
+  // We disable UpdateItemsShown while we notify the listener in case it
+  // modifies the ComboboxModel, then calls OnComboboxModelChanged and thus
+  // UpdateItemsShown. That way UpdateItemsShown doesn't do its work twice.
+  if (listener_) {
+    menu_model_->DisableUpdateItemsShown();
     listener_->OnContentChanged(this);
+    menu_model_->EnableUpdateItemsShown();
+  }
+  menu_model_->UpdateItemsShown(new_content);
 }
 
 void EditableCombobox::OnMenuClosed() {
@@ -278,6 +303,8 @@ void EditableCombobox::ShowDropDownMenu(ui::MenuSourceType source_type) {
     menu_runner_.reset();
     return;
   }
+  if (!textfield_->HasFocus() || (menu_runner_ && menu_runner_->IsRunning()))
+    return;
 
   gfx::Rect local_bounds = textfield_->GetLocalBounds();
   gfx::Point menu_position(local_bounds.origin());

ui/views/controls/editable_combobox/editable_combobox.h

@@ -23,7 +23,6 @@ class FontList;
 
 namespace ui {
 class ComboboxModel;
-class MenuModel;
 class NativeTheme;
 }  // namespace ui
 
@@ -60,9 +59,6 @@ class VIEWS_EXPORT EditableCombobox : public View,
   // Gets the text currently in the textfield.
   const base::string16& GetText() const;
 
-  // Sets the text in the textfield.
-  void SetText(const base::string16& text);
-
   const gfx::FontList& GetFontList() const;
 
   // Sets the listener that we will call when a selection is made.
@@ -80,9 +76,11 @@ class VIEWS_EXPORT EditableCombobox : public View,
 
   // Accessors of private members for tests.
   ui::ComboboxModel* GetComboboxModelForTest() { return combobox_model_.get(); }
-  ui::MenuModel* GetMenuModelForTest() { return menu_model_.get(); }
+  int GetItemCountForTest();
+  base::string16 GetItemForTest(int index);
   MenuRunner* GetMenuRunnerForTest() { return menu_runner_.get(); }
   Textfield* GetTextfieldForTest() { return textfield_; }
+  void SetTextForTest(const base::string16& text);
 
  private:
   class EditableComboboxMenuModel;
@@ -90,7 +88,7 @@ class VIEWS_EXPORT EditableCombobox : public View,
   // Called when an item is selected from the menu.
   void OnItemSelected(int index);
 
-  // Close/Open/Update the menu based on the new textfield content.
+  // Notifies listener of new content and updates the menu items to show.
   void HandleNewContent(const base::string16& new_content);
 
   // Cleans up after the menu is closed.
@@ -114,7 +112,7 @@ class VIEWS_EXPORT EditableCombobox : public View,
   std::unique_ptr<ui::ComboboxModel> combobox_model_;
 
   // The EditableComboboxMenuModel used by |menu_runner_|.
-  std::unique_ptr<ui::MenuModel> menu_model_;
+  std::unique_ptr<EditableComboboxMenuModel> menu_model_;
 
   // Typography context for the text written in the textfield and the options
   // shown in the drop-down menu.

ui/views/controls/editable_combobox/editable_combobox_unittest.cc

@@ -417,17 +417,40 @@ TEST_F(EditableComboboxTest, MenuCanAdaptToContentChange) {
   EXPECT_EQ(menu_runner1, menu_runner2);
 }
 
+TEST_F(EditableComboboxTest, RefocusingReopensMenuBasedOnLatestContent) {
+  std::vector<base::string16> items = {ASCIIToUTF16("abc"), ASCIIToUTF16("abd"),
+                                       ASCIIToUTF16("bac"), ASCIIToUTF16("bad"),
+                                       ASCIIToUTF16("bac2")};
+  InitEditableCombobox(items, /*filter_on_edit=*/true);
+  combobox_->GetTextfieldForTest()->RequestFocus();
+
+  SendKeyEvent(ui::VKEY_B);
+  ASSERT_EQ(3, combobox_->GetItemCountForTest());
+
+  SendKeyEvent(ui::VKEY_DOWN);
+  SendKeyEvent(ui::VKEY_RETURN);
+  WaitForMenuClosureAnimation();
+  EXPECT_FALSE(IsMenuOpen());
+  EXPECT_EQ(ASCIIToUTF16("bac"), combobox_->GetText());
+
+  // Blur then focus to make the menu reopen. It should only show completions of
+  // "bac", the selected item, instead of showing completions of "b", what we
+  // had typed.
+  dummy_focusable_view_->RequestFocus();
+  combobox_->GetTextfieldForTest()->RequestFocus();
+  EXPECT_TRUE(IsMenuOpen());
+  ASSERT_EQ(2, combobox_->GetItemCountForTest());
+}
+
 TEST_F(EditableComboboxTest, GetItemsWithoutFiltering) {
   std::vector<base::string16> items = {ASCIIToUTF16("item0"),
                                        ASCIIToUTF16("item1")};
   InitEditableCombobox(items, /*filter_on_edit=*/false, /*show_on_empty=*/true);
 
-  combobox_->SetText(ASCIIToUTF16("z"));
-  ASSERT_EQ(2, combobox_->GetMenuModelForTest()->GetItemCount());
-  ASSERT_EQ(ASCIIToUTF16("item0"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(0));
-  ASSERT_EQ(ASCIIToUTF16("item1"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(1));
+  combobox_->SetTextForTest(ASCIIToUTF16("z"));
+  ASSERT_EQ(2, combobox_->GetItemCountForTest());
+  ASSERT_EQ(ASCIIToUTF16("item0"), combobox_->GetItemForTest(0));
+  ASSERT_EQ(ASCIIToUTF16("item1"), combobox_->GetItemForTest(1));
 }
 
 TEST_F(EditableComboboxTest, FilteringEffectOnGetItems) {
@@ -436,36 +459,26 @@ TEST_F(EditableComboboxTest, FilteringEffectOnGetItems) {
                                        ASCIIToUTF16("bad")};
   InitEditableCombobox(items, /*filter_on_edit=*/true, /*show_on_empty=*/true);
 
-  ASSERT_EQ(4, combobox_->GetMenuModelForTest()->GetItemCount());
-  ASSERT_EQ(ASCIIToUTF16("abc"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(0));
-  ASSERT_EQ(ASCIIToUTF16("abd"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(1));
-  ASSERT_EQ(ASCIIToUTF16("bac"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(2));
-  ASSERT_EQ(ASCIIToUTF16("bad"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(3));
-
-  combobox_->SetText(ASCIIToUTF16("b"));
-  ASSERT_EQ(2, combobox_->GetMenuModelForTest()->GetItemCount());
-  ASSERT_EQ(ASCIIToUTF16("bac"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(0));
-  ASSERT_EQ(ASCIIToUTF16("bad"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(1));
-
-  combobox_->SetText(ASCIIToUTF16("bc"));
-  ASSERT_EQ(0, combobox_->GetMenuModelForTest()->GetItemCount());
-
-  combobox_->SetText(base::string16());
-  ASSERT_EQ(4, combobox_->GetMenuModelForTest()->GetItemCount());
-  ASSERT_EQ(ASCIIToUTF16("abc"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(0));
-  ASSERT_EQ(ASCIIToUTF16("abd"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(1));
-  ASSERT_EQ(ASCIIToUTF16("bac"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(2));
-  ASSERT_EQ(ASCIIToUTF16("bad"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(3));
+  ASSERT_EQ(4, combobox_->GetItemCountForTest());
+  ASSERT_EQ(ASCIIToUTF16("abc"), combobox_->GetItemForTest(0));
+  ASSERT_EQ(ASCIIToUTF16("abd"), combobox_->GetItemForTest(1));
+  ASSERT_EQ(ASCIIToUTF16("bac"), combobox_->GetItemForTest(2));
+  ASSERT_EQ(ASCIIToUTF16("bad"), combobox_->GetItemForTest(3));
+
+  combobox_->SetTextForTest(ASCIIToUTF16("b"));
+  ASSERT_EQ(2, combobox_->GetItemCountForTest());
+  ASSERT_EQ(ASCIIToUTF16("bac"), combobox_->GetItemForTest(0));
+  ASSERT_EQ(ASCIIToUTF16("bad"), combobox_->GetItemForTest(1));
+
+  combobox_->SetTextForTest(ASCIIToUTF16("bc"));
+  ASSERT_EQ(0, combobox_->GetItemCountForTest());
+
+  combobox_->SetTextForTest(base::string16());
+  ASSERT_EQ(4, combobox_->GetItemCountForTest());
+  ASSERT_EQ(ASCIIToUTF16("abc"), combobox_->GetItemForTest(0));
+  ASSERT_EQ(ASCIIToUTF16("abd"), combobox_->GetItemForTest(1));
+  ASSERT_EQ(ASCIIToUTF16("bac"), combobox_->GetItemForTest(2));
+  ASSERT_EQ(ASCIIToUTF16("bad"), combobox_->GetItemForTest(3));
 }
 
 TEST_F(EditableComboboxTest, FilteringWithMismatchedCase) {
@@ -473,27 +486,20 @@ TEST_F(EditableComboboxTest, FilteringWithMismatchedCase) {
       ASCIIToUTF16("AbCd"), ASCIIToUTF16("aBcD"), ASCIIToUTF16("xyz")};
   InitEditableCombobox(items, /*filter_on_edit=*/true, /*show_on_empty=*/true);
 
-  ASSERT_EQ(3, combobox_->GetMenuModelForTest()->GetItemCount());
-  ASSERT_EQ(ASCIIToUTF16("AbCd"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(0));
-  ASSERT_EQ(ASCIIToUTF16("aBcD"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(1));
-  ASSERT_EQ(ASCIIToUTF16("xyz"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(2));
-
-  combobox_->SetText(ASCIIToUTF16("abcd"));
-  ASSERT_EQ(2, combobox_->GetMenuModelForTest()->GetItemCount());
-  ASSERT_EQ(ASCIIToUTF16("AbCd"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(0));
-  ASSERT_EQ(ASCIIToUTF16("aBcD"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(1));
-
-  combobox_->SetText(ASCIIToUTF16("ABCD"));
-  ASSERT_EQ(2, combobox_->GetMenuModelForTest()->GetItemCount());
-  ASSERT_EQ(ASCIIToUTF16("AbCd"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(0));
-  ASSERT_EQ(ASCIIToUTF16("aBcD"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(1));
+  ASSERT_EQ(3, combobox_->GetItemCountForTest());
+  ASSERT_EQ(ASCIIToUTF16("AbCd"), combobox_->GetItemForTest(0));
+  ASSERT_EQ(ASCIIToUTF16("aBcD"), combobox_->GetItemForTest(1));
+  ASSERT_EQ(ASCIIToUTF16("xyz"), combobox_->GetItemForTest(2));
+
+  combobox_->SetTextForTest(ASCIIToUTF16("abcd"));
+  ASSERT_EQ(2, combobox_->GetItemCountForTest());
+  ASSERT_EQ(ASCIIToUTF16("AbCd"), combobox_->GetItemForTest(0));
+  ASSERT_EQ(ASCIIToUTF16("aBcD"), combobox_->GetItemForTest(1));
+
+  combobox_->SetTextForTest(ASCIIToUTF16("ABCD"));
+  ASSERT_EQ(2, combobox_->GetItemCountForTest());
+  ASSERT_EQ(ASCIIToUTF16("AbCd"), combobox_->GetItemForTest(0));
+  ASSERT_EQ(ASCIIToUTF16("aBcD"), combobox_->GetItemForTest(1));
 }
 
 TEST_F(EditableComboboxTest, DontShowOnEmpty) {
@@ -502,13 +508,11 @@ TEST_F(EditableComboboxTest, DontShowOnEmpty) {
   InitEditableCombobox(items, /*filter_on_edit=*/false,
                        /*show_on_empty=*/false);
 
-  ASSERT_EQ(0, combobox_->GetMenuModelForTest()->GetItemCount());
-  combobox_->SetText(ASCIIToUTF16("a"));
-  ASSERT_EQ(2, combobox_->GetMenuModelForTest()->GetItemCount());
-  ASSERT_EQ(ASCIIToUTF16("item0"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(0));
-  ASSERT_EQ(ASCIIToUTF16("item1"),
-            combobox_->GetMenuModelForTest()->GetLabelAt(1));
+  ASSERT_EQ(0, combobox_->GetItemCountForTest());
+  combobox_->SetTextForTest(ASCIIToUTF16("a"));
+  ASSERT_EQ(2, combobox_->GetItemCountForTest());
+  ASSERT_EQ(ASCIIToUTF16("item0"), combobox_->GetItemForTest(0));
+  ASSERT_EQ(ASCIIToUTF16("item1"), combobox_->GetItemForTest(1));
 }
 
 TEST_F(EditableComboboxTest, NoFilteringNotifiesListener) {
@@ -517,9 +521,9 @@ TEST_F(EditableComboboxTest, NoFilteringNotifiesListener) {
   InitEditableCombobox(items, /*filter_on_edit=*/false, /*show_on_empty=*/true);
 
   ASSERT_EQ(0, listener_->change_count());
-  combobox_->SetText(ASCIIToUTF16("a"));
+  combobox_->SetTextForTest(ASCIIToUTF16("a"));
   ASSERT_EQ(1, listener_->change_count());
-  combobox_->SetText(ASCIIToUTF16("ab"));
+  combobox_->SetTextForTest(ASCIIToUTF16("ab"));
   ASSERT_EQ(2, listener_->change_count());
 }
 
@@ -529,11 +533,11 @@ TEST_F(EditableComboboxTest, FilteringNotifiesListener) {
   InitEditableCombobox(items, /*filter_on_edit=*/true, /*show_on_empty=*/true);
 
   ASSERT_EQ(0, listener_->change_count());
-  combobox_->SetText(ASCIIToUTF16("i"));
+  combobox_->SetTextForTest(ASCIIToUTF16("i"));
   ASSERT_EQ(1, listener_->change_count());
-  combobox_->SetText(ASCIIToUTF16("ix"));
+  combobox_->SetTextForTest(ASCIIToUTF16("ix"));
   ASSERT_EQ(2, listener_->change_count());
-  combobox_->SetText(ASCIIToUTF16("ixy"));
+  combobox_->SetTextForTest(ASCIIToUTF16("ixy"));
   ASSERT_EQ(3, listener_->change_count());
 }