chrome/browser/ui/webauthn: tweak account selection layout.

Change the layout of the account selection dialog in response to
feedback from UI. See https://screenshot.googleplex.com/d7L6RQC2kEi for
the source of the padding numbers. Also, based on UI feedback, remove
the placeholder avatar icons since we do not currently have plans to
show any non-placeholder icons.

Change-Id: I36f58d4db2a0036a4fb6c38aa11036b6f945b014
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1617098
Commit-Queue: Adam Langley <agl@chromium.org>
Reviewed-by: Nina Satragno <nsatragno@chromium.org>
Auto-Submit: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#661069}

chrome/browser/ui/views/webauthn/hover_list_view.cc

@@ -27,42 +27,87 @@ namespace {
 
 constexpr int kPlaceHolderItemTag = -1;
 
+class WebauthnHoverButton : public HoverButton {
+ public:
+  WebauthnHoverButton(views::ButtonListener* button_listener,
+                      std::unique_ptr<views::View> icon_view,
+                      const base::string16& title,
+                      const base::string16& subtitle,
+                      std::unique_ptr<views::View> secondary_view)
+      : HoverButton(button_listener,
+                    std::move(icon_view),
+                    title,
+                    subtitle,
+                    std::move(secondary_view),
+                    false) {}
+
+  gfx::Insets GetInsets() const override {
+    gfx::Insets ret = HoverButton::GetInsets();
+    if (left_inset_.has_value()) {
+      ret.set_left(*left_inset_);
+    }
+    ret.set_right(8);
+    return ret;
+  }
+
+  void SetInsetForNoIcon() {
+    // When there's no icon, insets within the underlying HoverButton take care
+    // of the padding on the left and we don't want to add any more.
+    left_inset_ = 0;
+  }
+
+ private:
+  base::Optional<int> left_inset_;
+};
+
 std::unique_ptr<HoverButton> CreateHoverButtonForListItem(
     int item_tag,
-    const gfx::VectorIcon& vector_icon,
+    const gfx::VectorIcon* vector_icon,
     base::string16 item_title,
     base::string16 item_description,
     views::ButtonListener* listener,
+    bool is_two_line_item,
     bool is_placeholder_item = false) {
-  // TODO(hongjunchoi): Make HoverListView subclass of HoverButton and listen
-  // for potential native theme color change.
-  //
   // Derive the icon color from the text color of an enabled label.
   auto color_reference_label = std::make_unique<views::Label>(
       base::string16(), CONTEXT_BODY_TEXT_SMALL, views::style::STYLE_PRIMARY);
   const SkColor icon_color = color_utils::DeriveDefaultIconColor(
       color_reference_label->enabled_color());
 
-  constexpr int kIconSize = 20;
   auto item_image = std::make_unique<views::ImageView>();
-  item_image->SetImage(
-      gfx::CreateVectorIcon(vector_icon, kIconSize, icon_color));
+  if (vector_icon) {
+    constexpr int kIconSize = 20;
+    item_image->SetImage(
+        gfx::CreateVectorIcon(*vector_icon, kIconSize, icon_color));
+  }
 
-  constexpr int kChevronSize = 8;
-  constexpr int kChevronPadding = (kIconSize - kChevronSize) / 2;
   std::unique_ptr<views::ImageView> chevron_image = nullptr;
   if (!is_placeholder_item) {
+    constexpr int kChevronSize = 8;
     chevron_image = std::make_unique<views::ImageView>();
     chevron_image->SetImage(gfx::CreateVectorIcon(views::kSubmenuArrowIcon,
                                                   kChevronSize, icon_color));
-    chevron_image->SetBorder(
-        views::CreateEmptyBorder(gfx::Insets(kChevronPadding)));
+
+    int chevron_vert_inset = 0;
+    if (is_two_line_item) {
+      // Items that are sized for two lines use the top and bottom insets of the
+      // chevron image to pad single-line items out to a uniform height of
+      // |kHeight|.
+      constexpr int kHeight = 56;
+      chevron_vert_inset = (kHeight - kChevronSize) / 2;
+    }
+    chevron_image->SetBorder(views::CreateEmptyBorder(
+        gfx::Insets(/*top=*/chevron_vert_inset, /*left=*/12,
+                    /*bottom=*/chevron_vert_inset, /*right=*/0)));
   }
 
-  auto hover_button = std::make_unique<HoverButton>(
+  auto hover_button = std::make_unique<WebauthnHoverButton>(
       listener, std::move(item_image), std::move(item_title),
       std::move(item_description), std::move(chevron_image));
   hover_button->set_tag(item_tag);
+  if (!vector_icon) {
+    hover_button->SetInsetForNoIcon();
+  }
 
   // Because there is an icon on both sides, set a custom border that has only
   // half of the normal padding horizontally.
@@ -98,7 +143,7 @@ views::Separator* AddSeparatorAsChild(views::View* view) {
 // HoverListView ---------------------------------------------------------
 
 HoverListView::HoverListView(std::unique_ptr<HoverListModel> model)
-    : model_(std::move(model)) {
+    : model_(std::move(model)), is_two_line_list_(model_->StyleForTwoLines()) {
   DCHECK(model_);
   SetLayoutManager(std::make_unique<views::FillLayout>());
 
@@ -134,12 +179,12 @@ HoverListView::~HoverListView() {
   model_->RemoveObserver();
 }
 
-void HoverListView::AppendListItemView(const gfx::VectorIcon& icon,
+void HoverListView::AppendListItemView(const gfx::VectorIcon* icon,
                                        base::string16 item_text,
                                        base::string16 description_text,
                                        int item_tag) {
-  auto hover_button = CreateHoverButtonForListItem(item_tag, icon, item_text,
-                                                   description_text, this);
+  auto hover_button = CreateHoverButtonForListItem(
+      item_tag, icon, item_text, description_text, this, is_two_line_list_);
 
   auto* list_item_view_ptr = hover_button.release();
   item_container_->AddChildView(list_item_view_ptr);
@@ -261,8 +306,8 @@ int HoverListView::GetPreferredViewHeight() const {
       model_->GetPreferredItemCount() - tags_to_list_item_views_.size();
   if (reserved_items > 0) {
     auto dummy_hover_button = CreateHoverButtonForListItem(
-        -1 /* tag */, gfx::kNoneIcon, base::string16(), base::string16(),
-        nullptr /* listener */);
+        -1 /* tag */, &gfx::kNoneIcon, base::string16(), base::string16(),
+        nullptr /* listener */, is_two_line_list_);
     const auto list_item_height =
         separator_height + dummy_hover_button->GetPreferredSize().height();
     size += list_item_height * reserved_items;

chrome/browser/ui/views/webauthn/hover_list_view.h

@@ -53,7 +53,7 @@ class HoverListView : public views::View,
     views::Separator* separator_view;
   };
 
-  void AppendListItemView(const gfx::VectorIcon& icon,
+  void AppendListItemView(const gfx::VectorIcon* icon,
                           base::string16 item_text,
                           base::string16 item_description,
                           int item_tag);
@@ -81,6 +81,10 @@ class HoverListView : public views::View,
   base::Optional<ListItemViews> placeholder_list_item_view_;
   views::ScrollView* scroll_view_;
   views::View* item_container_;
+  // is_two_line_list_, if true, indicates that list items should be sized so
+  // that entries with only a single line of text are as tall as entries with
+  // two lines.
+  const bool is_two_line_list_;
 
   DISALLOW_COPY_AND_ASSIGN(HoverListView);
 };

chrome/browser/ui/webauthn/account_hover_list_model.cc

@@ -27,8 +27,8 @@ base::string16 AccountHoverListModel::GetPlaceholderText() const {
   return base::string16();
 }
 
-const gfx::VectorIcon& AccountHoverListModel::GetPlaceholderIcon() const {
-  return kUserAccountAvatarIcon;
+const gfx::VectorIcon* AccountHoverListModel::GetPlaceholderIcon() const {
+  return &kUserAccountAvatarIcon;
 }
 
 std::vector<int> AccountHoverListModel::GetItemTags() const {
@@ -50,9 +50,8 @@ base::string16 AccountHoverListModel::GetDescriptionText(int item_tag) const {
   return base::UTF8ToUTF16(user->name.value_or(""));
 }
 
-const gfx::VectorIcon& AccountHoverListModel::GetItemIcon(int item_tag) const {
-  // TODO(nsatragno): fetch an icon from |user->icon_url|.
-  return GetPlaceholderIcon();
+const gfx::VectorIcon* AccountHoverListModel::GetItemIcon(int item_tag) const {
+  return nullptr;
 }
 
 void AccountHoverListModel::OnListItemSelected(int item_tag) {
@@ -62,3 +61,7 @@ void AccountHoverListModel::OnListItemSelected(int item_tag) {
 size_t AccountHoverListModel::GetPreferredItemCount() const {
   return response_list_->size();
 }
+
+bool AccountHoverListModel::StyleForTwoLines() const {
+  return true;
+}

chrome/browser/ui/webauthn/account_hover_list_model.h

@@ -33,13 +33,14 @@ class AccountHoverListModel : public HoverListModel {
   // HoverListModel:
   bool ShouldShowPlaceholderForEmptyList() const override;
   base::string16 GetPlaceholderText() const override;
-  const gfx::VectorIcon& GetPlaceholderIcon() const override;
+  const gfx::VectorIcon* GetPlaceholderIcon() const override;
   std::vector<int> GetItemTags() const override;
   base::string16 GetItemText(int item_tag) const override;
   base::string16 GetDescriptionText(int item_tag) const override;
-  const gfx::VectorIcon& GetItemIcon(int item_tag) const override;
+  const gfx::VectorIcon* GetItemIcon(int item_tag) const override;
   void OnListItemSelected(int item_tag) override;
   size_t GetPreferredItemCount() const override;
+  bool StyleForTwoLines() const override;
 
  private:
   const std::vector<device::AuthenticatorGetAssertionResponse>* response_list_;

chrome/browser/ui/webauthn/authenticator_dialog_browsertest.cc

@@ -124,6 +124,9 @@ class AuthenticatorDialogTest : public DialogBrowserTest {
           {"", "Test User 2"},
           {"", ""},
           {"bat@example.com", "Test User 4"},
+          {"verylong@reallylongreallylongreallylongreallylongreallylong.com",
+           "Very Long String Very Long String Very Long String Very Long "
+           "String Very Long String Very Long String "},
       };
       std::vector<device::AuthenticatorGetAssertionResponse> responses;
 

chrome/browser/ui/webauthn/ble_device_hover_list_model.cc

@@ -69,7 +69,7 @@ base::string16 BleDeviceHoverListModel::GetPlaceholderText() const {
       IDS_WEBAUTHN_BLE_DEVICE_SELECTION_SEARCHING_LABEL);
 }
 
-const gfx::VectorIcon& BleDeviceHoverListModel::GetPlaceholderIcon() const {
+const gfx::VectorIcon* BleDeviceHoverListModel::GetPlaceholderIcon() const {
   return GetTransportVectorIcon(AuthenticatorTransport::kBluetoothLowEnergy);
 }
 
@@ -81,7 +81,7 @@ base::string16 BleDeviceHoverListModel::GetDescriptionText(int item_tag) const {
   return base::string16();
 }
 
-const gfx::VectorIcon& BleDeviceHoverListModel::GetItemIcon(
+const gfx::VectorIcon* BleDeviceHoverListModel::GetItemIcon(
     int item_tag) const {
   return GetTransportVectorIcon(AuthenticatorTransport::kBluetoothLowEnergy);
 }
@@ -112,6 +112,10 @@ size_t BleDeviceHoverListModel::GetPreferredItemCount() const {
   return kDefaultItemViewCount;
 }
 
+bool BleDeviceHoverListModel::StyleForTwoLines() const {
+  return false;
+}
+
 void BleDeviceHoverListModel::OnAuthenticatorAdded(
     const AuthenticatorReference& authenticator) {
   auto item_tag = authenticator_tags_.empty()

chrome/browser/ui/webauthn/ble_device_hover_list_model.h

@@ -39,13 +39,14 @@ class BleDeviceHoverListModel : public HoverListModel,
   // HoverListModel:
   bool ShouldShowPlaceholderForEmptyList() const override;
   base::string16 GetPlaceholderText() const override;
-  const gfx::VectorIcon& GetPlaceholderIcon() const override;
+  const gfx::VectorIcon* GetPlaceholderIcon() const override;
   base::string16 GetItemText(int item_tag) const override;
   base::string16 GetDescriptionText(int item_tag) const override;
-  const gfx::VectorIcon& GetItemIcon(int item_tag) const override;
+  const gfx::VectorIcon* GetItemIcon(int item_tag) const override;
   std::vector<int> GetItemTags() const override;
   void OnListItemSelected(int item_tag) override;
   size_t GetPreferredItemCount() const override;
+  bool StyleForTwoLines() const override;
 
   // AuthenticatorListObserver:
   void OnAuthenticatorAdded(

chrome/browser/ui/webauthn/hover_list_model.h

@@ -11,6 +11,7 @@
 
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/strings/string16.h"
 #include "ui/gfx/vector_icon_types.h"
 
@@ -37,13 +38,25 @@ class HoverListModel {
 
   virtual bool ShouldShowPlaceholderForEmptyList() const = 0;
   virtual base::string16 GetPlaceholderText() const = 0;
-  virtual const gfx::VectorIcon& GetPlaceholderIcon() const = 0;
+  // GetPlaceholderIcon may return nullptr to indicate that no icon should be
+  // added. This is distinct from using an empty icon as the latter will still
+  // take up as much space as any other icon.
+  virtual const gfx::VectorIcon* GetPlaceholderIcon() const = 0;
   virtual std::vector<int> GetItemTags() const = 0;
   virtual base::string16 GetItemText(int item_tag) const = 0;
   virtual base::string16 GetDescriptionText(int item_tag) const = 0;
-  virtual const gfx::VectorIcon& GetItemIcon(int item_tag) const = 0;
+  // GetItemIcon may return nullptr to indicate that no icon should be added.
+  // This is distinct from using an empty icon as the latter will still take up
+  // as much space as any other icon.
+  virtual const gfx::VectorIcon* GetItemIcon(int item_tag) const = 0;
   virtual void OnListItemSelected(int item_tag) = 0;
   virtual size_t GetPreferredItemCount() const = 0;
+  // StyleForTwoLines returns true if the items in the list should lay out
+  // with the assumption that there will be both item and description text.
+  // This causes items with no description text to be larger than strictly
+  // necessary so that all items, including those with descriptions, are the
+  // same height.
+  virtual bool StyleForTwoLines() const = 0;
 
   void SetObserver(Observer* observer) {
     DCHECK(!observer_);

chrome/browser/ui/webauthn/other_transports_menu_model.cc

@@ -15,7 +15,7 @@ namespace {
 
 gfx::ImageSkia GetTransportIcon(AuthenticatorTransport transport) {
   constexpr int kTransportIconSize = 16;
-  return gfx::CreateVectorIcon(GetTransportVectorIcon(transport),
+  return gfx::CreateVectorIcon(*GetTransportVectorIcon(transport),
                                kTransportIconSize, gfx::kGoogleGrey700);
 }
 

chrome/browser/ui/webauthn/transport_hover_list_model.cc

@@ -24,8 +24,8 @@ base::string16 TransportHoverListModel::GetPlaceholderText() const {
   return base::string16();
 }
 
-const gfx::VectorIcon& TransportHoverListModel::GetPlaceholderIcon() const {
-  return gfx::kNoneIcon;
+const gfx::VectorIcon* TransportHoverListModel::GetPlaceholderIcon() const {
+  return &gfx::kNoneIcon;
 }
 
 std::vector<int> TransportHoverListModel::GetItemTags() const {
@@ -46,7 +46,7 @@ base::string16 TransportHoverListModel::GetDescriptionText(int item_tag) const {
   return base::string16();
 }
 
-const gfx::VectorIcon& TransportHoverListModel::GetItemIcon(
+const gfx::VectorIcon* TransportHoverListModel::GetItemIcon(
     int item_tag) const {
   return GetTransportVectorIcon(static_cast<AuthenticatorTransport>(item_tag));
 }
@@ -59,3 +59,7 @@ void TransportHoverListModel::OnListItemSelected(int item_tag) {
 size_t TransportHoverListModel::GetPreferredItemCount() const {
   return transport_list_.size();
 }
+
+bool TransportHoverListModel::StyleForTwoLines() const {
+  return false;
+}

chrome/browser/ui/webauthn/transport_hover_list_model.h

@@ -29,13 +29,14 @@ class TransportHoverListModel : public HoverListModel {
   // HoverListModel:
   bool ShouldShowPlaceholderForEmptyList() const override;
   base::string16 GetPlaceholderText() const override;
-  const gfx::VectorIcon& GetPlaceholderIcon() const override;
+  const gfx::VectorIcon* GetPlaceholderIcon() const override;
   std::vector<int> GetItemTags() const override;
   base::string16 GetItemText(int item_tag) const override;
   base::string16 GetDescriptionText(int item_tag) const override;
-  const gfx::VectorIcon& GetItemIcon(int item_tag) const override;
+  const gfx::VectorIcon* GetItemIcon(int item_tag) const override;
   void OnListItemSelected(int item_tag) override;
   size_t GetPreferredItemCount() const override;
+  bool StyleForTwoLines() const override;
 
  private:
   std::vector<AuthenticatorTransport> transport_list_;

chrome/browser/ui/webauthn/transport_utils.cc

@@ -61,20 +61,20 @@ base::string16 GetTransportHumanReadableName(
   return l10n_util::GetStringUTF16(message_id);
 }
 
-const gfx::VectorIcon& GetTransportVectorIcon(
+const gfx::VectorIcon* GetTransportVectorIcon(
     AuthenticatorTransport transport) {
   switch (transport) {
     case AuthenticatorTransport::kBluetoothLowEnergy:
-      return kBluetoothIcon;
+      return &kBluetoothIcon;
     case AuthenticatorTransport::kNearFieldCommunication:
-      return kNfcIcon;
+      return &kNfcIcon;
     case AuthenticatorTransport::kUsbHumanInterfaceDevice:
-      return vector_icons::kUsbIcon;
+      return &vector_icons::kUsbIcon;
     case AuthenticatorTransport::kInternal:
-      return kFingerprintIcon;
+      return &kFingerprintIcon;
     case AuthenticatorTransport::kCloudAssistedBluetoothLowEnergy:
-      return kSmartphoneIcon;
+      return &kSmartphoneIcon;
   }
   NOTREACHED();
-  return kFingerprintIcon;
+  return &kFingerprintIcon;
 }

chrome/browser/ui/webauthn/transport_utils.h

@@ -23,6 +23,6 @@ base::string16 GetTransportHumanReadableName(AuthenticatorTransport transport,
 
 // Returns the vector icon to show next to the |transport| in the manual
 // transport selection list.
-const gfx::VectorIcon& GetTransportVectorIcon(AuthenticatorTransport transport);
+const gfx::VectorIcon* GetTransportVectorIcon(AuthenticatorTransport transport);
 
 #endif  // CHROME_BROWSER_UI_WEBAUTHN_TRANSPORT_UTILS_H_