Add new style to LoginPinView

Define two PIN keyboard styles and update view layout accordingly:
* Alphanumeric
Keys display digit and a part of the alphabet below. This is the
exact style of existing LoginPinView.
* Numeric
Keys only display digit, therefore keys height is different. This
is the new style.

Use Alphanumeric PIN style for login PIN input and Numeric PIN style
for parent access code input.

Bug: 911326
Test: LoginPinViewTest
Change-Id: Id164d8580cd64db6891348b5de4638be52ab5c0d
Reviewed-on: https://chromium-review.googlesource.com/c/1483675Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Commit-Queue: Aga Wronska <agawronska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636683}

ash/login/ui/login_auth_user_view.cc

@@ -504,7 +504,8 @@ LoginAuthUserView::LoginAuthUserView(const mojom::LoginUserInfoPtr& user,
   password_view_->UpdateForUser(user);
 
   pin_view_ =
-      new LoginPinView(base::BindRepeating(&LoginPasswordView::InsertNumber,
+      new LoginPinView(LoginPinView::Style::kAlphanumeric,
+                       base::BindRepeating(&LoginPasswordView::InsertNumber,
                                            base::Unretained(password_view_)),
                        base::BindRepeating(&LoginPasswordView::Backspace,
                                            base::Unretained(password_view_)));

ash/login/ui/login_pin_view.cc

@@ -19,6 +19,7 @@
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/events/keycodes/dom/dom_code.h"
+#include "ui/gfx/geometry/size.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/animation/flood_fill_ink_drop_ripple.h"
 #include "ui/views/animation/ink_drop_highlight.h"
@@ -59,6 +60,12 @@ constexpr int kRepeatingBackspaceDelayMs = 150;
 // Size of the md-ripple when a PIN button is tapped.
 constexpr int kRippleSizeDp = 54;
 
+// Button sizes. Button height varies per keyboard style, while button width is
+// the same for both styles.
+constexpr int kAlphanumericButtonHeightDp = 78;
+constexpr int kNumericButtonHeightDp = 70;
+constexpr int kButtonWidthDp = 78;
+
 base::string16 GetButtonLabelForNumber(int value) {
   DCHECK(value >= 0 && value < int{base::size(kPinLabels)});
   return base::ASCIIToUTF16(std::to_string(value));
@@ -79,12 +86,13 @@ int GetViewIdForPinNumber(int number) {
 // A base class for pin button in the pin keyboard.
 class BasePinButton : public views::InkDropHostView {
  public:
-  explicit BasePinButton(const base::RepeatingClosure& on_press,
-                         const base::string16& accessible_name)
+  explicit BasePinButton(const gfx::Size& size,
+                         const base::string16& accessible_name,
+                         const base::RepeatingClosure& on_press)
       : on_press_(on_press), accessible_name_(accessible_name) {
     SetFocusBehavior(FocusBehavior::ALWAYS);
-    SetPreferredSize(
-        gfx::Size(LoginPinView::kButtonSizeDp, LoginPinView::kButtonSizeDp));
+    SetPreferredSize(size);
+
     auto layout =
         std::make_unique<views::BoxLayout>(views::BoxLayout::kVertical);
     layout->set_main_axis_alignment(
@@ -195,31 +203,38 @@ class BasePinButton : public views::InkDropHostView {
 // A PIN button that displays a digit number and corresponding letter mapping.
 class DigitPinButton : public BasePinButton {
  public:
-  DigitPinButton(int value, const LoginPinView::OnPinKey& on_key)
-      : BasePinButton(base::BindRepeating(on_key, value),
-                      GetButtonLabelForNumber(value)) {
+  DigitPinButton(int value,
+                 bool show_sub_label,
+                 const gfx::Size& size,
+                 const LoginPinView::OnPinKey& on_key)
+      : BasePinButton(size,
+                      GetButtonLabelForNumber(value),
+                      base::BindRepeating(on_key, value)) {
     set_id(GetViewIdForPinNumber(value));
     const gfx::FontList& base_font_list = views::Label::GetDefaultFontList();
     views::Label* label = new views::Label(GetButtonLabelForNumber(value),
                                            views::style::CONTEXT_BUTTON,
                                            views::style::STYLE_PRIMARY);
-    views::Label* sub_label = new views::Label(
-        GetButtonSubLabelForNumber(value), views::style::CONTEXT_BUTTON,
-        views::style::STYLE_PRIMARY);
     label->SetEnabledColor(login_constants::kButtonEnabledColor);
-    sub_label->SetEnabledColor(
-        SkColorSetA(login_constants::kButtonEnabledColor,
-                    login_constants::kButtonDisabledAlpha));
     label->SetAutoColorReadabilityEnabled(false);
-    sub_label->SetAutoColorReadabilityEnabled(false);
     label->SetSubpixelRenderingEnabled(false);
-    sub_label->SetSubpixelRenderingEnabled(false);
     label->SetFontList(base_font_list.Derive(8, gfx::Font::FontStyle::NORMAL,
                                              gfx::Font::Weight::LIGHT));
-    sub_label->SetFontList(base_font_list.Derive(
-        -3, gfx::Font::FontStyle::NORMAL, gfx::Font::Weight::NORMAL));
     AddChildView(label);
-    AddChildView(sub_label);
+
+    if (show_sub_label) {
+      views::Label* sub_label = new views::Label(
+          GetButtonSubLabelForNumber(value), views::style::CONTEXT_BUTTON,
+          views::style::STYLE_PRIMARY);
+      sub_label->SetEnabledColor(
+          SkColorSetA(login_constants::kButtonEnabledColor,
+                      login_constants::kButtonDisabledAlpha));
+      sub_label->SetAutoColorReadabilityEnabled(false);
+      sub_label->SetSubpixelRenderingEnabled(false);
+      sub_label->SetFontList(base_font_list.Derive(
+          -3, gfx::Font::FontStyle::NORMAL, gfx::Font::Weight::NORMAL));
+      AddChildView(sub_label);
+    }
   }
 
   ~DigitPinButton() override = default;
@@ -230,16 +245,15 @@ class DigitPinButton : public BasePinButton {
 
 }  // namespace
 
-// static
-const int LoginPinView::kButtonSizeDp = 78;
-
 // A PIN button that displays backspace icon.
 class LoginPinView::BackspacePinButton : public BasePinButton {
  public:
-  BackspacePinButton(const base::RepeatingClosure& on_press)
-      : BasePinButton(on_press,
+  BackspacePinButton(const gfx::Size& size,
+                     const base::RepeatingClosure& on_press)
+      : BasePinButton(size,
                       l10n_util::GetStringUTF16(
-                          IDS_ASH_PIN_KEYBOARD_DELETE_ACCESSIBLE_NAME)),
+                          IDS_ASH_PIN_KEYBOARD_DELETE_ACCESSIBLE_NAME),
+                      on_press),
         delay_timer_(std::make_unique<base::OneShotTimer>()),
         repeat_timer_(std::make_unique<base::RepeatingTimer>()) {
     image_ = new views::ImageView();
@@ -351,6 +365,13 @@ class LoginPinView::BackspacePinButton : public BasePinButton {
   DISALLOW_COPY_AND_ASSIGN(BackspacePinButton);
 };
 
+// static
+gfx::Size LoginPinView::TestApi::GetButtonSize(Style style) {
+  return gfx::Size(kButtonWidthDp, style == Style::kNumeric
+                                       ? kNumericButtonHeightDp
+                                       : kAlphanumericButtonHeightDp);
+}
+
 LoginPinView::TestApi::TestApi(LoginPinView* view) : view_(view) {}
 
 LoginPinView::TestApi::~TestApi() = default;
@@ -370,7 +391,8 @@ void LoginPinView::TestApi::SetBackspaceTimers(
                                          std::move(repeat_timer));
 }
 
-LoginPinView::LoginPinView(const OnPinKey& on_key,
+LoginPinView::LoginPinView(Style keyboard_style,
+                           const OnPinKey& on_key,
                            const OnPinBackspace& on_backspace)
     : NonAccessibleView(kLoginPinViewClassName),
       on_key_(on_key),
@@ -393,31 +415,41 @@ LoginPinView::LoginPinView(const OnPinKey& on_key,
   SetLayoutManager(
       std::make_unique<views::BoxLayout>(views::BoxLayout::kVertical));
 
+  bool show_letters = keyboard_style == Style::kAlphanumeric;
+  const gfx::Size button_size =
+      gfx::Size(kButtonWidthDp, show_letters ? kAlphanumericButtonHeightDp
+                                             : kNumericButtonHeightDp);
+
+  auto add_digit_button = [&](View* row, int value) {
+    row->AddChildView(
+        new DigitPinButton(value, show_letters, button_size, on_key_));
+  };
+
   // 1-2-3
   auto* row = build_and_add_row();
-  row->AddChildView(new DigitPinButton(1, on_key_));
-  row->AddChildView(new DigitPinButton(2, on_key_));
-  row->AddChildView(new DigitPinButton(3, on_key_));
+  add_digit_button(row, 1);
+  add_digit_button(row, 2);
+  add_digit_button(row, 3);
 
   // 4-5-6
   row = build_and_add_row();
-  row->AddChildView(new DigitPinButton(4, on_key_));
-  row->AddChildView(new DigitPinButton(5, on_key_));
-  row->AddChildView(new DigitPinButton(6, on_key_));
+  add_digit_button(row, 4);
+  add_digit_button(row, 5);
+  add_digit_button(row, 6);
 
   // 7-8-9
   row = build_and_add_row();
-  row->AddChildView(new DigitPinButton(7, on_key_));
-  row->AddChildView(new DigitPinButton(8, on_key_));
-  row->AddChildView(new DigitPinButton(9, on_key_));
+  add_digit_button(row, 7);
+  add_digit_button(row, 8);
+  add_digit_button(row, 9);
 
   // 0-backspace
   row = build_and_add_row();
   auto* spacer = new NonAccessibleView();
-  spacer->SetPreferredSize(gfx::Size(kButtonSizeDp, kButtonSizeDp));
+  spacer->SetPreferredSize(button_size);
   row->AddChildView(spacer);
-  row->AddChildView(new DigitPinButton(0, on_key_));
-  backspace_ = new BackspacePinButton(on_backspace_);
+  add_digit_button(row, 0);
+  backspace_ = new BackspacePinButton(button_size, on_backspace_);
   row->AddChildView(backspace_);
 }
 

ash/login/ui/login_pin_view.h

@@ -46,16 +46,27 @@ namespace ash {
 //
 class ASH_EXPORT LoginPinView : public NonAccessibleView {
  public:
-  // Size of each button.
-  static const int kButtonSizeDp;
+  // Visual style of PIN keyboard.
+  enum class Style {
+    // Has a layout and look of a telephone keypad. Keys display a combination
+    // of a digit and letters. The letters can be used for mnemonic techniques.
+    kAlphanumeric,
+    // Has a layout of a telephone keypad, but keys display only digits, no
+    // letters.
+    kNumeric,
+  };
 
   class ASH_EXPORT TestApi {
    public:
+    // Returns expected button size for the given PIN keyboard |style|.
+    static gfx::Size GetButtonSize(Style style);
+
     explicit TestApi(LoginPinView* view);
     ~TestApi();
 
     views::View* GetButton(int number) const;
     views::View* GetBackspaceButton() const;
+
     // Sets the timers that are used for backspace auto-submit. |delay_timer| is
     // the initial delay before an auto-submit, and |repeat_timer| fires
     // whenever a new backspace event should run after the initial delay.
@@ -69,10 +80,12 @@ class ASH_EXPORT LoginPinView : public NonAccessibleView {
   using OnPinKey = base::RepeatingCallback<void(int value)>;
   using OnPinBackspace = base::RepeatingClosure;
 
+  // Creates PIN view with the specified |keyboard_style|.
   // |on_key| is called whenever the user taps one of the pin buttons.
   // |on_backspace| is called when the user wants to erase the most recently
   // tapped key. Neither callback can be null.
-  explicit LoginPinView(const OnPinKey& on_key,
+  explicit LoginPinView(Style keyboard_style,
+                        const OnPinKey& on_key,
                         const OnPinBackspace& on_backspace);
   ~LoginPinView() override;
 

ash/login/ui/login_pin_view_unittest.cc

@@ -4,7 +4,10 @@
 
 #include "ash/login/ui/login_password_view.h"
 
+#include <algorithm>
 #include <memory>
+#include <set>
+#include <vector>
 
 #include "ash/login/ui/login_pin_view.h"
 #include "ash/login/ui/login_test_base.h"
@@ -22,13 +25,15 @@ class LoginPinViewTest : public LoginTestBase {
   LoginPinViewTest() = default;
   ~LoginPinViewTest() override = default;
 
-  // LoginScreenTest:
-  void SetUp() override {
-    LoginTestBase::SetUp();
-
-    view_ = new LoginPinView(
-        base::Bind(&LoginPinViewTest::OnPinKey, base::Unretained(this)),
-        base::Bind(&LoginPinViewTest::OnPinBackspace, base::Unretained(this)));
+  // Creates login pin view with the specified keyboard |style| and sets it up
+  // in a widget.
+  void CreateLoginPinViewWithStyle(LoginPinView::Style style) {
+    view_ =
+        new LoginPinView(style,
+                         base::BindRepeating(&LoginPinViewTest::OnPinKey,
+                                             base::Unretained(this)),
+                         base::BindRepeating(&LoginPinViewTest::OnPinBackspace,
+                                             base::Unretained(this)));
 
     SetWidget(CreateWidgetWithContent(view_));
   }
@@ -50,6 +55,7 @@ class LoginPinViewTest : public LoginTestBase {
 
 // Verifies that PIN submit works with 'Enter'.
 TEST_F(LoginPinViewTest, ButtonsFireEvents) {
+  CreateLoginPinViewWithStyle(LoginPinView::Style::kAlphanumeric);
   ui::test::EventGenerator* generator = GetEventGenerator();
   LoginPinView::TestApi test_api(view_);
 
@@ -70,23 +76,78 @@ TEST_F(LoginPinViewTest, ButtonsFireEvents) {
   EXPECT_EQ(1, backspace_);
 }
 
-// Validates buttons have the correct spacing.
-TEST_F(LoginPinViewTest, ButtonSpacingAndSize) {
+// Validates buttons have the correct spacing for alphanumeric PIN keyboard
+// style.
+TEST_F(LoginPinViewTest, AlphanumericKeyboardButtonSpacingAndSize) {
+  CreateLoginPinViewWithStyle(LoginPinView::Style::kAlphanumeric);
   LoginPinView::TestApi test_api(view_);
 
+  const gfx::Size expected_button_size =
+      LoginPinView::TestApi::GetButtonSize(LoginPinView::Style::kAlphanumeric);
+
+  // Validate pin button size.
+  for (int i = 0; i <= 9; ++i) {
+    DCHECK_EQ(test_api.GetButton(i)->size().width(),
+              expected_button_size.width());
+    DCHECK_EQ(test_api.GetButton(i)->size().height(),
+              expected_button_size.height());
+  }
+
+  // Validate backspace button size.
+  DCHECK_EQ(test_api.GetBackspaceButton()->size().width(),
+            expected_button_size.width());
+  DCHECK_EQ(test_api.GetBackspaceButton()->size().height(),
+            expected_button_size.height());
+
+  // Record all the x/y coordinates of the buttons.
+  std::set<int> seen_x;
+  std::set<int> seen_y;
+  for (int i = 0; i <= 9; ++i) {
+    gfx::Rect screen_bounds = test_api.GetButton(i)->GetBoundsInScreen();
+    seen_x.insert(screen_bounds.x());
+    seen_y.insert(screen_bounds.y());
+  }
+  seen_x.insert(test_api.GetBackspaceButton()->GetBoundsInScreen().x());
+  seen_y.insert(test_api.GetBackspaceButton()->GetBoundsInScreen().y());
+
+  // Sort the coordinates so we can easily check the distance between them.
+  std::vector<int> sorted_x(seen_x.begin(), seen_x.end());
+  std::vector<int> sorted_y(seen_y.begin(), seen_y.end());
+  std::sort(sorted_x.begin(), sorted_x.end());
+  std::sort(sorted_y.begin(), sorted_y.end());
+
+  // Validate each x or y coordinate has the correct distance between it and the
+  // next one. This is correct because we have already validated button size.
+  EXPECT_EQ(3u, sorted_x.size());
+  for (size_t i = 0; i < sorted_x.size() - 1; ++i)
+    EXPECT_EQ(sorted_x[i] + expected_button_size.width(), sorted_x[i + 1]);
+
+  EXPECT_EQ(4u, sorted_y.size());
+  for (size_t i = 0; i < sorted_y.size() - 1; ++i)
+    EXPECT_EQ(sorted_y[i] + expected_button_size.height(), sorted_y[i + 1]);
+}
+
+// Validates buttons have the correct spacing for numeric PIN keyboard style.
+TEST_F(LoginPinViewTest, NumericKeyboardButtonSpacingAndSize) {
+  CreateLoginPinViewWithStyle(LoginPinView::Style::kNumeric);
+  LoginPinView::TestApi test_api(view_);
+
+  const gfx::Size expected_button_size =
+      LoginPinView::TestApi::GetButtonSize(LoginPinView::Style::kNumeric);
+
   // Validate pin button size.
   for (int i = 0; i <= 9; ++i) {
     DCHECK_EQ(test_api.GetButton(i)->size().width(),
-              LoginPinView::kButtonSizeDp);
+              expected_button_size.width());
     DCHECK_EQ(test_api.GetButton(i)->size().height(),
-              LoginPinView::kButtonSizeDp);
+              expected_button_size.height());
   }
 
   // Validate backspace button size.
   DCHECK_EQ(test_api.GetBackspaceButton()->size().width(),
-            LoginPinView::kButtonSizeDp);
+            expected_button_size.width());
   DCHECK_EQ(test_api.GetBackspaceButton()->size().height(),
-            LoginPinView::kButtonSizeDp);
+            expected_button_size.height());
 
   // Record all the x/y coordinates of the buttons.
   std::set<int> seen_x;
@@ -109,16 +170,17 @@ TEST_F(LoginPinViewTest, ButtonSpacingAndSize) {
   // next one. This is correct because we have already validated button size.
   EXPECT_EQ(3u, sorted_x.size());
   for (size_t i = 0; i < sorted_x.size() - 1; ++i)
-    EXPECT_EQ(sorted_x[i] + LoginPinView::kButtonSizeDp, sorted_x[i + 1]);
+    EXPECT_EQ(sorted_x[i] + expected_button_size.width(), sorted_x[i + 1]);
 
   EXPECT_EQ(4u, sorted_y.size());
   for (size_t i = 0; i < sorted_y.size() - 1; ++i)
-    EXPECT_EQ(sorted_y[i] + LoginPinView::kButtonSizeDp, sorted_y[i + 1]);
+    EXPECT_EQ(sorted_y[i] + expected_button_size.height(), sorted_y[i + 1]);
 }
 
 // Verifies that holding the backspace button automatically triggers and begins
 // repeating if it is held down.
 TEST_F(LoginPinViewTest, BackspaceAutoSubmitsAndRepeats) {
+  CreateLoginPinViewWithStyle(LoginPinView::Style::kAlphanumeric);
   ui::test::EventGenerator* generator = GetEventGenerator();
   LoginPinView::TestApi test_api(view_);
 

ash/login/ui/parent_access_view.cc

@@ -60,8 +60,9 @@ constexpr int kLockIconSizeDp = 24;
 
 constexpr int kIconToTitleDistanceDp = 28;
 constexpr int kTitleToDescriptionDistanceDp = 14;
-constexpr int kDescriptionToAccessCodeDistanceDp = 38;
-constexpr int kAccessCodeToPinKeyboardDistanceDp = 20;
+constexpr int kDescriptionToAccessCodeDistanceDp = 28;
+constexpr int kAccessCodeToPinKeyboardDistanceDp = 5;
+constexpr int kSubmitButtonBottomMarginDp = 8;
 
 constexpr int kTitleFontSizeDeltaDp = 3;
 constexpr int kDescriptionFontSizeDeltaDp = -1;
@@ -416,6 +417,7 @@ ParentAccessView::ParentAccessView(const AccountId& account_id,
 
   // Pin keyboard.
   pin_keyboard_view_ = new LoginPinView(
+      LoginPinView::Style::kNumeric,
       base::BindRepeating(&AccessCodeInput::InsertDigit,
                           base::Unretained(access_code_view_)),
       base::BindRepeating(&AccessCodeInput::Backspace,
@@ -461,6 +463,8 @@ ParentAccessView::ParentAccessView(const AccountId& account_id,
   submit_button_->SetEnabled(false);
   footer->AddChildView(submit_button_);
 
+  add_spacer(kSubmitButtonBottomMarginDp);
+
   // Pin keyboard is only shown in tablet mode.
   pin_keyboard_view_->SetVisible(IsTabletMode());