Remove EditableCombobox special handling with show_menu_on_next_focus_.

We would set this variable to false to prevent the menu from coming
down if the EditableCombobox gets focus automatically (as soon as the
password bubble is shown for example).
We now remove it, so focusing on the EditableCombobox always brings
down the menu, and instead we make sure that the username
EditableCombobox doesn't get focus automatically when the password
bubble is shown.

When the bubble shows up automatically the new Widget wouldn't get
focus anyway so there is no issue there.

When the bubble shows up due to the user clicking the key icon in the
URL bar, the Widget would become active, and in turn the username
EditableCombobox would get focus.
This CL changes this behavior: The Widget still becomes active, but the
username doesn't get focus.

The only scenario in which the username also gets focus is when there
are no suggested usernames, so the user can start filling it out right
away. This was intended by the current code, but there was a mistake,
where it was checking for
|model()->pending_password().username_value.empty()|
instead of |username_dropdown_->GetText().empty()|.
The |username_value| variable could in fact be empty even when the
control shows text in cases where there are more than one possible
username.

Bug: 965426
Change-Id: I5b843670beca6022f819336d9c9611940eb1e05f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1621395
Commit-Queue: Edin Kadric <edinkadric@google.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662789}

chrome/browser/ui/views/passwords/password_pending_view.cc

@@ -208,7 +208,6 @@ PasswordPendingView::PasswordPendingView(content::WebContents* web_contents,
       sign_in_promo_(nullptr),
       username_dropdown_(nullptr),
       password_view_button_(nullptr),
-      initially_focused_view_(nullptr),
       password_dropdown_(nullptr),
       are_passwords_revealed_(
           model()->are_passwords_revealed_when_bubble_is_opened()) {
@@ -234,7 +233,6 @@ PasswordPendingView::PasswordPendingView(content::WebContents* web_contents,
     username_dropdown_ =
         CreateUsernameEditableCombobox(password_form).release();
     username_dropdown_->set_listener(this);
-    username_dropdown_->set_show_menu_on_next_focus(false);
     password_dropdown_ =
         CreatePasswordEditableCombobox(password_form, are_passwords_revealed_)
             .release();
@@ -248,8 +246,6 @@ PasswordPendingView::PasswordPendingView(content::WebContents* web_contents,
 
     BuildCredentialRows(layout, username_dropdown_, password_dropdown_,
                         password_view_button_);
-    if (model()->pending_password().username_value.empty())
-      initially_focused_view_ = username_dropdown_;
   }
 }
 
@@ -327,9 +323,15 @@ gfx::Size PasswordPendingView::CalculatePreferredSize() const {
 }
 
 views::View* PasswordPendingView::GetInitiallyFocusedView() {
-  if (initially_focused_view_)
-    return initially_focused_view_;
-  return PasswordBubbleViewBase::GetInitiallyFocusedView();
+  if (username_dropdown_ && username_dropdown_->GetText().empty())
+    return username_dropdown_;
+  View* initial_view = PasswordBubbleViewBase::GetInitiallyFocusedView();
+  // |initial_view| will normally be the 'Save' button, but in case it's not
+  // focusable, we return nullptr so the Widget doesn't give focus to the next
+  // focusable View, which would be |username_dropdown_|, and which would bring
+  // up the menu without a user interaction. We only allow initial focus on
+  // |username_dropdown_| above, when the text is empty.
+  return (initial_view && initial_view->IsFocusable()) ? initial_view : nullptr;
 }
 
 int PasswordPendingView::GetDialogButtons() const {
@@ -408,7 +410,6 @@ void PasswordPendingView::UpdateUsernameAndPasswordInModel() {
 
 void PasswordPendingView::ReplaceWithPromo() {
   RemoveAllChildViews(true);
-  initially_focused_view_ = nullptr;
   SetLayoutManager(std::make_unique<views::FillLayout>());
   set_margins(ChromeLayoutProvider::Get()->GetDialogInsetsForContentType(
       views::TEXT, views::TEXT));

chrome/browser/ui/views/passwords/password_pending_view.h

@@ -73,7 +73,6 @@ class PasswordPendingView : public PasswordBubbleViewBase,
 
   views::EditableCombobox* username_dropdown_;
   views::ToggleImageButton* password_view_button_;
-  views::View* initially_focused_view_;
 
   // The view for the password value.
   views::EditableCombobox* password_dropdown_;

ui/views/controls/editable_combobox/editable_combobox.cc

@@ -376,14 +376,12 @@ bool EditableCombobox::HandleMouseEvent(Textfield* sender,
   // We show the menu on mouse release instead of mouse press so that the menu
   // showing up doesn't interrupt a potential text selection operation by the
   // user.
-  // If we don't already have focus when the mouse is pressed, we set
-  // |show_menu_on_next_focus_| to false so that the focus event that will
-  // follow the press event and precede the release event does not end up
-  // showing the menu and interrupting a text selection operation.
-  if (mouse_event.type() == ui::ET_MOUSE_PRESSED && !textfield_->HasFocus())
-    show_menu_on_next_focus_ = false;
-  else if (mouse_event.type() == ui::ET_MOUSE_RELEASED)
+  if (mouse_event.type() == ui::ET_MOUSE_PRESSED) {
+    mouse_pressed_ = true;
+  } else if (mouse_event.type() == ui::ET_MOUSE_RELEASED) {
+    mouse_pressed_ = false;
     ShowDropDownMenu(ui::MENU_SOURCE_MOUSE);
+  }
   return false;
 }
 
@@ -402,9 +400,11 @@ void EditableCombobox::OnViewBlurred(View* observed_view) {
 }
 
 void EditableCombobox::OnViewFocused(View* observed_view) {
-  if (show_menu_on_next_focus_)
+  // We only show the menu if the mouse is not currently pressed to avoid
+  // interrupting a text selection operation. The menu will be shown on mouse
+  // release inside HandleMouseEvent.
+  if (!mouse_pressed_)
     ShowDropDownMenu();
-  show_menu_on_next_focus_ = true;
 }
 
 ////////////////////////////////////////////////////////////////////////////////

ui/views/controls/editable_combobox/editable_combobox.h

@@ -80,10 +80,6 @@ class VIEWS_EXPORT EditableCombobox : public View,
     listener_ = listener;
   }
 
-  void set_show_menu_on_next_focus(bool show_menu_on_next_focus) {
-    show_menu_on_next_focus_ = show_menu_on_next_focus;
-  }
-
   // Selects the specified logical text range for the textfield.
   void SelectRange(const gfx::Range& range);
 
@@ -141,9 +137,7 @@ class VIEWS_EXPORT EditableCombobox : public View,
   void ButtonPressed(Button* sender, const ui::Event& event) override;
 
   Textfield* textfield_;
-
   Button* arrow_ = nullptr;
-
   std::unique_ptr<ui::ComboboxModel> combobox_model_;
 
   // The EditableComboboxMenuModel used by |menu_runner_|.
@@ -159,9 +153,9 @@ class VIEWS_EXPORT EditableCombobox : public View,
 
   const Type type_;
 
-  // If false, then the menu won't be shown the next time the View is focused.
-  // Set false on creation to avoid showing the menu on the first focus event.
-  bool show_menu_on_next_focus_ = true;
+  // True between mouse press and release, used to avoid opening the menu and
+  // interrupting textfield selection interactions.
+  bool mouse_pressed_ = false;
 
   // Set while the drop-down is showing.
   std::unique_ptr<MenuRunner> menu_runner_;

ui/views/controls/editable_combobox/editable_combobox_unittest.cc

@@ -81,6 +81,7 @@ class EditableComboboxTest : public ViewsTestBase {
   void ClickArrow();
   void ClickMenuItem(int index);
   void ClickTextfield();
+  void DragMouseTo(const gfx::Point& location);
   bool IsMenuOpen();
   void PerformMouseEvent(Widget* widget,
                          const gfx::Point& point,
@@ -144,7 +145,6 @@ void EditableComboboxTest::InitEditableCombobox(
   InitWidget();
   combobox_->GetTextfieldForTest()->RequestFocus();
   combobox_->RequestFocus();
-  combobox_->SizeToPreferredSize();
 }
 
 // Initializes the widget where the combobox and the dummy control live.
@@ -152,11 +152,12 @@ void EditableComboboxTest::InitWidget() {
   widget_ = new Widget();
   Widget::InitParams params =
       CreateParams(Widget::InitParams::TYPE_WINDOW_FRAMELESS);
-  params.bounds = gfx::Rect(200, 200, 200, 200);
+  params.bounds = gfx::Rect(0, 0, 1000, 1000);
   widget_->Init(params);
   View* container = new View();
   widget_->SetContentsView(container);
   container->AddChildView(combobox_);
+  combobox_->SetBoundsRect(gfx::Rect(0, 0, 500, 40));
   container->AddChildView(dummy_focusable_view_);
   widget_->Show();
 
@@ -190,6 +191,12 @@ void EditableComboboxTest::ClickTextfield() {
   PerformClick(widget_, textfield);
 }
 
+void EditableComboboxTest::DragMouseTo(const gfx::Point& location) {
+  ui::MouseEvent drag(ui::ET_MOUSE_DRAGGED, location, location,
+                      ui::EventTimeForNow(), ui::EF_LEFT_MOUSE_BUTTON, 0);
+  combobox_->GetTextfieldForTest()->OnMouseDragged(drag);
+}
+
 bool EditableComboboxTest::IsMenuOpen() {
   return combobox_->GetMenuRunnerForTest() &&
          combobox_->GetMenuRunnerForTest()->IsRunning();
@@ -232,6 +239,7 @@ void EditableComboboxTest::SendKeyEvent(ui::KeyboardCode key_code,
 TEST_F(EditableComboboxTest, FocusOnTextfieldOpensMenu) {
   InitEditableCombobox();
   dummy_focusable_view_->RequestFocus();
+  WaitForMenuClosureAnimation();
   EXPECT_FALSE(IsMenuOpen());
   combobox_->GetTextfieldForTest()->RequestFocus();
   EXPECT_TRUE(IsMenuOpen());
@@ -245,6 +253,7 @@ TEST_F(EditableComboboxTest, TabMovesToOtherViewAndClosesMenu) {
   SendKeyEvent(ui::VKEY_TAB);
   EXPECT_FALSE(combobox_->GetTextfieldForTest()->HasFocus());
   EXPECT_TRUE(dummy_focusable_view_->HasFocus());
+  WaitForMenuClosureAnimation();
   EXPECT_FALSE(IsMenuOpen());
 }
 
@@ -417,6 +426,7 @@ TEST_F(EditableComboboxTest, EscClosesMenuWithoutSelectingHighlightedMenuItem) {
   SendKeyEvent(ui::VKEY_DOWN);
   EXPECT_TRUE(IsMenuOpen());
   SendKeyEvent(ui::VKEY_ESCAPE);
+  WaitForMenuClosureAnimation();
   EXPECT_FALSE(IsMenuOpen());
   EXPECT_EQ(ASCIIToUTF16("a"), combobox_->GetText());
 }
@@ -636,6 +646,7 @@ TEST_F(EditableComboboxTest, PasswordCanBeHiddenAndRevealed) {
 TEST_F(EditableComboboxTest, ArrowButtonOpensAndClosesMenu) {
   InitEditableCombobox();
   dummy_focusable_view_->RequestFocus();
+  WaitForMenuClosureAnimation();
   EXPECT_FALSE(IsMenuOpen());
 
   ClickArrow();
@@ -645,38 +656,62 @@ TEST_F(EditableComboboxTest, ArrowButtonOpensAndClosesMenu) {
   EXPECT_FALSE(IsMenuOpen());
 }
 
-TEST_F(EditableComboboxTest, ShowMenuOnNextFocusBehavior) {
+TEST_F(EditableComboboxTest, ShowMenuOnMouseRelease) {
   std::vector<base::string16> items = {ASCIIToUTF16("item0"),
                                        ASCIIToUTF16("item1")};
   InitEditableCombobox(items, /*filter_on_edit=*/false,
                        /*show_on_empty=*/true);
-
   dummy_focusable_view_->RequestFocus();
-  combobox_->set_show_menu_on_next_focus(true);
-  combobox_->GetTextfieldForTest()->RequestFocus();
+  WaitForMenuClosureAnimation();
+  EXPECT_FALSE(IsMenuOpen());
+
+  // Without initial focus.
+  EXPECT_FALSE(combobox_->GetTextfieldForTest()->HasFocus());
+  const gfx::Point textfield_point(combobox_->x() + 1, combobox_->y() + 1);
+  PerformMouseEvent(widget_, textfield_point, ui::ET_MOUSE_PRESSED);
+  EXPECT_FALSE(IsMenuOpen());
+  PerformMouseEvent(widget_, textfield_point, ui::ET_MOUSE_RELEASED);
   EXPECT_TRUE(IsMenuOpen());
 
-  dummy_focusable_view_->RequestFocus();
-  combobox_->set_show_menu_on_next_focus(false);
-  combobox_->GetTextfieldForTest()->RequestFocus();
+  SendKeyEvent(ui::VKEY_ESCAPE);
+  WaitForMenuClosureAnimation();
   EXPECT_FALSE(IsMenuOpen());
-  dummy_focusable_view_->RequestFocus();
-  combobox_->GetTextfieldForTest()->RequestFocus();
+
+  // With initial focus.
+  EXPECT_TRUE(combobox_->GetTextfieldForTest()->HasFocus());
+  PerformMouseEvent(widget_, textfield_point, ui::ET_MOUSE_PRESSED);
+  EXPECT_FALSE(IsMenuOpen());
+  PerformMouseEvent(widget_, textfield_point, ui::ET_MOUSE_RELEASED);
   EXPECT_TRUE(IsMenuOpen());
 }
 
-TEST_F(EditableComboboxTest, ShowMenuOnClickWhenAlreadyFocused) {
+TEST_F(EditableComboboxTest, DragToSelectDoesntOpenTheMenuUntilDone) {
   std::vector<base::string16> items = {ASCIIToUTF16("item0"),
                                        ASCIIToUTF16("item1")};
   InitEditableCombobox(items, /*filter_on_edit=*/false,
                        /*show_on_empty=*/true);
-
+  combobox_->SetText(ASCIIToUTF16("abc"));
   dummy_focusable_view_->RequestFocus();
-  combobox_->set_show_menu_on_next_focus(false);
-  combobox_->GetTextfieldForTest()->RequestFocus();
+  WaitForMenuClosureAnimation();
+  EXPECT_FALSE(IsMenuOpen());
+
+  const int kCursorXStart = 0;
+  const int kCursorXEnd = combobox_->x() + combobox_->width();
+  const int kCursorY = combobox_->y() + 1;
+  gfx::Point start_point(kCursorXStart, kCursorY);
+  gfx::Point end_point(kCursorXEnd, kCursorY);
+
+  PerformMouseEvent(widget_, start_point, ui::ET_MOUSE_PRESSED);
+  EXPECT_TRUE(combobox_->GetTextfieldForTest()->GetSelectedText().empty());
+
+  DragMouseTo(end_point);
+  ASSERT_EQ(ASCIIToUTF16("abc"),
+            combobox_->GetTextfieldForTest()->GetSelectedText());
   EXPECT_FALSE(IsMenuOpen());
 
-  ClickTextfield();
+  PerformMouseEvent(widget_, end_point, ui::ET_MOUSE_RELEASED);
+  ASSERT_EQ(ASCIIToUTF16("abc"),
+            combobox_->GetTextfieldForTest()->GetSelectedText());
   EXPECT_TRUE(IsMenuOpen());
 }