Introduce EditablePasswordCombobox class.

This CL moves all password-related functionality from EditableCombobox to a new EditablePasswordCombobox class. This new class also adds an eye icon that is positioned directly in combobox instead of having to draw it separately outside. Screenshot: https://screenshot.googleplex.com/43jqdrcijyDvuex Bug: 1021450 Change-Id: I23b5d661cf3872a46db5c161c14d9aec2677a35a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4188887 Commit-Queue: Jan Keitel <jkeitel@google.com> Reviewed-by: David Roger <droger@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org> Cr-Commit-Position: refs/heads/main@{#1097340}
chromium · Jan 26, 2023 · 483d082 · 483d082
1 parent acb4958
commit 483d082
Show file tree

Hide file tree

Showing 18 changed files with 653 additions and 239 deletions.
diff --git a/chrome/app/vector_icons/BUILD.gn b/chrome/app/vector_icons/BUILD.gn
@@ -49,8 +49,6 @@ aggregate_vector_icons("chrome_vector_icons") {
     "drag_handle.icon",
     "eol.icon",
     "extension_crashed.icon",
-    "eye.icon",
-    "eye_crossed.icon",
     "file_download_shelf.icon",
     "fingerprint.icon",
     "forward_arrow_touch.icon",

diff --git a/chrome/browser/ui/views/location_bar/cookie_controls_icon_view.cc b/chrome/browser/ui/views/location_bar/cookie_controls_icon_view.cc
@@ -7,7 +7,6 @@
 #include <memory>
 
 #include "base/strings/utf_string_conversions.h"
-#include "chrome/app/vector_icons/vector_icons.h"
 #include "chrome/browser/content_settings/cookie_settings_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/view_ids.h"
@@ -20,6 +19,7 @@
 #include "ui/base/metadata/metadata_impl_macros.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/paint_vector_icon.h"
+#include "ui/views/vector_icons.h"
 
 CookieControlsIconView::CookieControlsIconView(
     IconLabelBubbleView::Delegate* icon_label_bubble_delegate,
@@ -116,9 +116,9 @@ views::BubbleDialogDelegate* CookieControlsIconView::GetBubble() const {
 }
 
 const gfx::VectorIcon& CookieControlsIconView::GetVectorIcon() const {
-  if (status_ == CookieControlsStatus::kDisabledForSite)
-    return kEyeIcon;
-  return kEyeCrossedIcon;
+  return status_ == CookieControlsStatus::kDisabledForSite
+             ? views::kEyeIcon
+             : views::kEyeCrossedIcon;
 }
 
 std::u16string CookieControlsIconView::GetTextForTooltipAndAccessibleName()

diff --git a/chrome/browser/ui/views/page_info/page_info_view_factory.cc b/chrome/browser/ui/views/page_info/page_info_view_factory.cc
@@ -481,7 +481,7 @@ const ui::ImageModel PageInfoViewFactory::GetManagedPermissionIcon(
 
 // static
 const ui::ImageModel PageInfoViewFactory::GetBlockingThirdPartyCookiesIcon() {
-  return ui::ImageModel::FromVectorIcon(kEyeCrossedIcon, ui::kColorIcon,
+  return ui::ImageModel::FromVectorIcon(views::kEyeCrossedIcon, ui::kColorIcon,
                                         GetIconSize());
 }
 

diff --git a/chrome/browser/ui/views/passwords/password_save_update_view.cc b/chrome/browser/ui/views/passwords/password_save_update_view.cc
@@ -52,11 +52,10 @@
 #include "ui/gfx/vector_icon_utils.h"
 #include "ui/views/accessibility/view_accessibility.h"
 #include "ui/views/bubble/bubble_frame_view.h"
-#include "ui/views/controls/button/image_button.h"
-#include "ui/views/controls/button/image_button_factory.h"
 #include "ui/views/controls/button/md_text_button.h"
 #include "ui/views/controls/combobox/combobox.h"
 #include "ui/views/controls/editable_combobox/editable_combobox.h"
+#include "ui/views/controls/editable_combobox/editable_password_combobox.h"
 #include "ui/views/controls/styled_label.h"
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/interaction/element_tracker_views.h"
@@ -93,13 +92,10 @@ std::unique_ptr<views::View> CreateRow() {
 
 // Builds a credential row, adds the given elements to the layout.
 // |destination_field| is nullptr if the destination field shouldn't be shown.
-// |password_view_button| is an optional field.
-void BuildCredentialRows(
-    views::View* parent_view,
-    std::unique_ptr<views::View> destination_field,
-    std::unique_ptr<views::View> username_field,
-    std::unique_ptr<views::View> password_field,
-    std::unique_ptr<views::ToggleImageButton> password_view_button) {
+void BuildCredentialRows(views::View* parent_view,
+                         std::unique_ptr<views::View> destination_field,
+                         std::unique_ptr<views::View> username_field,
+                         std::unique_ptr<views::View> password_field) {
   std::unique_ptr<views::Label> username_label(new views::Label(
       l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_USERNAME_LABEL),
       views::style::CONTEXT_LABEL, views::style::STYLE_PRIMARY));
@@ -151,11 +147,6 @@ void BuildCredentialRows(
                                views::MaximumFlexSizeRule::kUnbounded));
   password_row->AddChildView(std::move(password_field));
 
-  // The eye icon is also added to the layout if it was passed.
-  if (password_view_button) {
-    password_row->AddChildView(std::move(password_view_button));
-  }
-
   parent_view->AddChildView(std::move(password_row));
 }
 
@@ -169,22 +160,6 @@ std::vector<std::u16string> ToValues(
   return passwords;
 }
 
-std::unique_ptr<views::ToggleImageButton> CreatePasswordViewButton(
-    views::Button::PressedCallback callback,
-    bool are_passwords_revealed) {
-  auto button = std::make_unique<views::ToggleImageButton>(std::move(callback));
-  button->SetInstallFocusRingOnFocus(true);
-  button->SetRequestFocusOnPress(true);
-  button->SetTooltipText(
-      l10n_util::GetStringUTF16(IDS_MANAGE_PASSWORDS_SHOW_PASSWORD));
-  button->SetToggledTooltipText(
-      l10n_util::GetStringUTF16(IDS_MANAGE_PASSWORDS_HIDE_PASSWORD));
-  button->SetImageHorizontalAlignment(views::ImageButton::ALIGN_CENTER);
-  button->SetImageVerticalAlignment(views::ImageButton::ALIGN_MIDDLE);
-  button->SetToggled(are_passwords_revealed);
-  return button;
-}
-
 // Creates an EditableCombobox from |PasswordForm.all_possible_usernames| or
 // even just |PasswordForm.username_value|.
 std::unique_ptr<views::EditableCombobox> CreateUsernameEditableCombobox(
@@ -204,8 +179,7 @@ std::unique_ptr<views::EditableCombobox> CreateUsernameEditableCombobox(
           std::vector<ui::SimpleComboboxModel::Item>(usernames.begin(),
                                                      usernames.end())),
       /*filter_on_edit=*/false, /*show_on_empty=*/true,
-      views::EditableCombobox::Type::kRegular, views::style::CONTEXT_BUTTON,
-      views::style::STYLE_PRIMARY, display_arrow);
+      views::style::CONTEXT_BUTTON, views::style::STYLE_PRIMARY, display_arrow);
   combobox->SetText(form.username_value);
   combobox->SetAccessibleName(
       l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_USERNAME_LABEL));
@@ -214,11 +188,14 @@ std::unique_ptr<views::EditableCombobox> CreateUsernameEditableCombobox(
   return combobox;
 }
 
-// Creates an EditableCombobox from |PasswordForm.all_possible_passwords| or
-// even just |PasswordForm.password_value|.
-std::unique_ptr<views::EditableCombobox> CreatePasswordEditableCombobox(
+// Creates an EditablePasswordCombobox from
+// `PasswordForm.all_possible_passwords` or even just
+// `PasswordForm.password_value`.
+std::unique_ptr<views::EditablePasswordCombobox> CreateEditablePasswordCombobox(
     const password_manager::PasswordForm& form,
-    bool are_passwords_revealed) {
+    bool are_passwords_revealed,
+    views::EditablePasswordCombobox::IsPasswordRevealPermittedCheck
+        reveal_permitted_check) {
   DCHECK(!form.IsFederatedCredential());
   std::vector<std::u16string> passwords =
       form.all_possible_passwords.empty()
@@ -228,14 +205,16 @@ std::unique_ptr<views::EditableCombobox> CreatePasswordEditableCombobox(
     return password.empty();
   });
   bool display_arrow = !passwords.empty();
-  auto combobox = std::make_unique<views::EditableCombobox>(
+  auto combobox = std::make_unique<views::EditablePasswordCombobox>(
       std::make_unique<ui::SimpleComboboxModel>(
           std::vector<ui::SimpleComboboxModel::Item>(passwords.begin(),
                                                      passwords.end())),
-      /*filter_on_edit=*/false, /*show_on_empty=*/true,
-      views::EditableCombobox::Type::kPassword, views::style::CONTEXT_BUTTON,
-      STYLE_PRIMARY_MONOSPACED, display_arrow);
+      views::style::CONTEXT_BUTTON, STYLE_PRIMARY_MONOSPACED, display_arrow);
   combobox->SetText(form.password_value);
+  combobox->SetPasswordIconTooltips(
+      l10n_util::GetStringUTF16(IDS_MANAGE_PASSWORDS_SHOW_PASSWORD),
+      l10n_util::GetStringUTF16(IDS_MANAGE_PASSWORDS_HIDE_PASSWORD));
+  combobox->SetIsPasswordRevealPermittedCheck(std::move(reveal_permitted_check));
   combobox->RevealPasswords(are_passwords_revealed);
   combobox->SetAccessibleName(
       l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_PASSWORD_LABEL));
@@ -311,9 +290,7 @@ PasswordSaveUpdateView::PasswordSaveUpdateView(
               ? PasswordBubbleControllerBase::DisplayReason::kAutomatic
               : PasswordBubbleControllerBase::DisplayReason::kUserAction),
       is_update_bubble_(controller_.state() ==
-                        password_manager::ui::PENDING_PASSWORD_UPDATE_STATE),
-      are_passwords_revealed_(
-          controller_.are_passwords_revealed_when_bubble_is_opened()) {
+                        password_manager::ui::PENDING_PASSWORD_UPDATE_STATE) {
   DCHECK(controller_.state() == password_manager::ui::PENDING_PASSWORD_STATE ||
          controller_.state() ==
              password_manager::ui::PENDING_PASSWORD_UPDATE_STATE);
@@ -359,16 +336,12 @@ PasswordSaveUpdateView::PasswordSaveUpdateView(
         CreateUsernameEditableCombobox(password_form);
     username_dropdown->SetCallback(base::BindRepeating(
         &PasswordSaveUpdateView::OnContentChanged, base::Unretained(this)));
-    std::unique_ptr<views::EditableCombobox> password_dropdown =
-        CreatePasswordEditableCombobox(password_form, are_passwords_revealed_);
-    password_dropdown->SetCallback(base::BindRepeating(
-        &PasswordSaveUpdateView::OnContentChanged, base::Unretained(this)));
-    std::unique_ptr<views::ToggleImageButton> password_view_button =
-        CreatePasswordViewButton(
-            base::BindRepeating(
-                &PasswordSaveUpdateView::TogglePasswordVisibility,
-                base::Unretained(this)),
-            are_passwords_revealed_);
+    std::unique_ptr<views::EditablePasswordCombobox> password_dropdown =
+        CreateEditablePasswordCombobox(
+            password_form,
+            controller_.are_passwords_revealed_when_bubble_is_opened(),
+            base::BindRepeating(&SaveUpdateBubbleController::RevealPasswords,
+                                base::Unretained(&controller_)));
     // Set up layout:
     SetLayoutManager(std::make_unique<AutoResizingLayout>());
     views::View* root_view = AddChildView(std::make_unique<views::View>());
@@ -393,11 +366,9 @@ PasswordSaveUpdateView::PasswordSaveUpdateView(
 
     username_dropdown_ = username_dropdown.get();
     password_dropdown_ = password_dropdown.get();
-    password_view_button_ = password_view_button.get();
     BuildCredentialRows(root_view, std::move(destination_dropdown),
                         std::move(username_dropdown),
-                        std::move(password_dropdown),
-                        std::move(password_view_button));
+                        std::move(password_dropdown));
 
     // The |username_dropdown_| should observe the animating layout manager to
     // close the dropdown menu when the animation starts.
@@ -522,40 +493,13 @@ void PasswordSaveUpdateView::AddedToWidget() {
     MaybeShowIPH(IPHType::kRegular);
 }
 
-void PasswordSaveUpdateView::OnThemeChanged() {
-  PasswordBubbleViewBase::OnThemeChanged();
-  if (password_view_button_) {
-    const auto* color_provider = GetColorProvider();
-    const SkColor icon_color = color_provider->GetColor(ui::kColorIcon);
-    const SkColor disabled_icon_color =
-        color_provider->GetColor(ui::kColorIconDisabled);
-    views::SetImageFromVectorIconWithColor(password_view_button_, kEyeIcon,
-                                           GetDefaultSizeOfVectorIcon(kEyeIcon),
-                                           icon_color, disabled_icon_color);
-    views::SetToggledImageFromVectorIconWithColor(
-        password_view_button_, kEyeCrossedIcon,
-        GetDefaultSizeOfVectorIcon(kEyeCrossedIcon), icon_color,
-        disabled_icon_color);
-  }
-}
-
 void PasswordSaveUpdateView::OnLayoutIsAnimatingChanged(
     views::AnimatingLayoutManager* source,
     bool is_animating) {
   if (!is_animating)
     MaybeShowIPH(IPHType::kRegular);
 }
 
-void PasswordSaveUpdateView::TogglePasswordVisibility() {
-  if (!are_passwords_revealed_ && !controller_.RevealPasswords())
-    return;
-
-  are_passwords_revealed_ = !are_passwords_revealed_;
-  password_view_button_->SetToggled(are_passwords_revealed_);
-  DCHECK(password_dropdown_);
-  password_dropdown_->RevealPasswords(are_passwords_revealed_);
-}
-
 void PasswordSaveUpdateView::UpdateUsernameAndPasswordInModel() {
   if (!username_dropdown_ && !password_dropdown_)
     return;

diff --git a/chrome/browser/ui/views/passwords/password_save_update_view.h b/chrome/browser/ui/views/passwords/password_save_update_view.h
@@ -19,7 +19,7 @@ namespace views {
 class AnimatingLayoutManager;
 class Combobox;
 class EditableCombobox;
-class ToggleImageButton;
+class EditablePasswordCombobox;
 }  // namespace views
 
 // A view offering the user the ability to save or update credentials (depending
@@ -64,13 +64,10 @@ class PasswordSaveUpdateView : public PasswordBubbleViewBase,
 
   // View:
   void AddedToWidget() override;
-  void OnThemeChanged() override;
 
   // views::AnimatingLayoutManager::Observer:
   void OnLayoutIsAnimatingChanged(views::AnimatingLayoutManager* source,
                                   bool is_animating) override;
-
-  void TogglePasswordVisibility();
   void UpdateUsernameAndPasswordInModel();
   void UpdateBubbleUIElements();
   std::unique_ptr<views::View> CreateFooterView();
@@ -105,12 +102,9 @@ class PasswordSaveUpdateView : public PasswordBubbleViewBase,
 
   raw_ptr<views::Combobox> destination_dropdown_ = nullptr;
 
+  // The views for the username and password dropdown elements.
   raw_ptr<views::EditableCombobox> username_dropdown_ = nullptr;
-  raw_ptr<views::ToggleImageButton> password_view_button_ = nullptr;
-
-  // The view for the password value.
-  raw_ptr<views::EditableCombobox> password_dropdown_ = nullptr;
-  bool are_passwords_revealed_;
+  raw_ptr<views::EditablePasswordCombobox> password_dropdown_ = nullptr;
 
   // When showing kReauthFailure IPH, the promo controller gives back an
   // ID. This is used to close the bubble later.

diff --git a/ui/views/BUILD.gn b/ui/views/BUILD.gn
@@ -27,6 +27,8 @@ aggregate_vector_icons("views_vector_icons") {
     "checkbox_normal.icon",
     "close.icon",
     "drag_general_selection.icon",
+    "eye.icon",
+    "eye_crossed.icon",
     "ic_close.icon",
     "info.icon",
     "launch.icon",
@@ -129,6 +131,7 @@ component("views") {
     "controls/combobox/combobox_util.h",
     "controls/dot_indicator.h",
     "controls/editable_combobox/editable_combobox.h",
+    "controls/editable_combobox/editable_password_combobox.h",
     "controls/focus_ring.h",
     "controls/focusable_border.h",
     "controls/highlight_path_generator.h",
@@ -360,6 +363,7 @@ component("views") {
     "controls/combobox/empty_combobox_model.h",
     "controls/dot_indicator.cc",
     "controls/editable_combobox/editable_combobox.cc",
+    "controls/editable_combobox/editable_password_combobox.cc",
     "controls/focus_ring.cc",
     "controls/focusable_border.cc",
     "controls/highlight_path_generator.cc",
@@ -1171,6 +1175,7 @@ test("views_unittests") {
     "controls/button/toggle_button_unittest.cc",
     "controls/combobox/combobox_unittest.cc",
     "controls/editable_combobox/editable_combobox_unittest.cc",
+    "controls/editable_combobox/editable_password_combobox_unittest.cc",
     "controls/image_view_unittest.cc",
     "controls/label_unittest.cc",
     "controls/link_fragment_unittest.cc",

diff --git a/ui/views/controls/button/image_button_factory.cc b/ui/views/controls/button/image_button_factory.cc
@@ -141,4 +141,19 @@ void SetImageFromVectorIconWithColorId(ImageButton* button,
   InkDrop::Get(button)->SetBaseColorId(icon_color_id);
 }
 
+void SetToggledImageFromVectorIconWithColorId(
+    ToggleImageButton* button,
+    const gfx::VectorIcon& icon,
+    ui::ColorId icon_color_id,
+    ui::ColorId icon_disabled_color_id) {
+  int dip_size = GetDefaultSizeOfVectorIcon(icon);
+  const ui::ImageModel& normal_image =
+      ui::ImageModel::FromVectorIcon(icon, icon_color_id, dip_size);
+  const ui::ImageModel& disabled_image =
+      ui::ImageModel::FromVectorIcon(icon, icon_disabled_color_id, dip_size);
+
+  button->SetToggledImageModel(Button::STATE_NORMAL, normal_image);
+  button->SetToggledImageModel(Button::STATE_DISABLED, disabled_image);
+}
+
 }  // namespace views
diff --git a/ui/views/controls/button/image_button_factory.h b/ui/views/controls/button/image_button_factory.h
@@ -75,6 +75,14 @@ VIEWS_EXPORT void SetImageFromVectorIconWithColorId(
     ui::ColorId icon_color_id,
     ui::ColorId icon_disabled_color_id);
 
+// Sets images on a `ToggleImageButton` |button| for STATE_NORMAL and
+// STATE_DISABLED with the default size from the given vector icon and colors.
+VIEWS_EXPORT void SetToggledImageFromVectorIconWithColorId(
+    ToggleImageButton* button,
+    const gfx::VectorIcon& icon,
+    ui::ColorId icon_color_id,
+    ui::ColorId icon_disabled_color_id);
+
 }  // namespace views
 
 #endif  // UI_VIEWS_CONTROLS_BUTTON_IMAGE_BUTTON_FACTORY_H_