[Win] Add tab and keyboard navigation to the new avatar bubble

chrome/browser/ui/views/profiles/profile_chooser_view.cc

Index: chrome/browser/ui/views/profiles/profile_chooser_view.cc
diff --git a/chrome/browser/ui/views/profiles/profile_chooser_view.cc b/chrome/browser/ui/views/profiles/profile_chooser_view.cc
index df8bcaffbe438ee4db98cbb3d1662a92db6e96be..a210ccd6f0857849bf2083c5af204d44de76abe6 100644
--- a/chrome/browser/ui/views/profiles/profile_chooser_view.cc
+++ b/chrome/browser/ui/views/profiles/profile_chooser_view.cc
@@ -141,6 +141,7 @@ class BackgroundColorHoverButton : public views::LabelButton {
     SetMinSize(gfx::Size(0,
         kButtonHeight + views::kRelatedControlVerticalSpacing));
     SetImage(STATE_NORMAL, icon);
+    SetFocusable(true);
   }
 
   virtual ~BackgroundColorHoverButton() {}
@@ -202,17 +203,17 @@ class RightAlignedIconLabelButton : public views::LabelButton {
 // EditableProfilePhoto -------------------------------------------------
 
 // A custom Image control that shows a "change" button when moused over.
-class EditableProfilePhoto : public views::ImageView {
+class EditableProfilePhoto : public views::ImageButton {
  public:
   EditableProfilePhoto(views::ButtonListener* listener,
                        const gfx::Image& icon,
                        bool is_editing_allowed,
                        const gfx::Rect& bounds)
-      : views::ImageView(),
-        change_photo_button_(NULL) {
+      : views::ImageButton(listener),
+        photo_overlay_(NULL) {
     gfx::Image image = profiles::GetSizedAvatarIcon(
         icon, true, kLargeImageSide, kLargeImageSide);
-    SetImage(image.ToImageSkia());
+    SetImage(views::LabelButton::STATE_NORMAL, image.ToImageSkia());
     SetBoundsRect(bounds);
 
     // Calculate the circular mask that will be used to display the photo.
@@ -220,32 +221,32 @@ class EditableProfilePhoto : public views::ImageView {
                              SkIntToScalar(bounds.height() / 2),
                              SkIntToScalar(bounds.width() / 2));
 
-    if (!is_editing_allowed)
+    if (!is_editing_allowed) {
+      SetEnabled(false);
       return;
+    }
 
+    SetFocusable(true);
     set_notify_enter_exit_on_child(true);
 
-    // Button overlay that appears when hovering over the image.
-    change_photo_button_ = new views::LabelButton(listener, base::string16());
-    change_photo_button_->SetHorizontalAlignment(gfx::ALIGN_CENTER);
-    change_photo_button_->SetBorder(views::Border::NullBorder());
+    // Photo overlay that appears when hovering over the button.
+    photo_overlay_ = new views::ImageView();
 
     const SkColor kBackgroundColor = SkColorSetARGB(65, 255, 255, 255);
-    change_photo_button_->set_background(
+    photo_overlay_->set_background(
         views::Background::CreateSolidBackground(kBackgroundColor));
-    change_photo_button_->SetImage(views::LabelButton::STATE_NORMAL,
-        *ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
-            IDR_ICON_PROFILES_EDIT_CAMERA));
+    photo_overlay_->SetImage(*ui::ResourceBundle::GetSharedInstance().
+        GetImageSkiaNamed(IDR_ICON_PROFILES_EDIT_CAMERA));
 
-    change_photo_button_->SetSize(bounds.size());
-    change_photo_button_->SetVisible(false);
-    AddChildView(change_photo_button_);
+    photo_overlay_->SetSize(bounds.size());
+    photo_overlay_->SetVisible(false);
+    AddChildView(photo_overlay_);
   }
 
   virtual void OnPaint(gfx::Canvas* canvas) OVERRIDE {
     // Display the profile picture as a circle.
     canvas->ClipPath(circular_mask_, true);
-    views::ImageView::OnPaint(canvas);
+    views::ImageButton::OnPaint(canvas);
   }
 
   virtual void PaintChildren(gfx::Canvas* canvas,
@@ -255,25 +256,35 @@ class EditableProfilePhoto : public views::ImageView {
     View::PaintChildren(canvas, cull_set);
   }
 
-  views::LabelButton* change_photo_button() { return change_photo_button_; }
-
  private:
   // views::View:
   virtual void OnMouseEntered(const ui::MouseEvent& event) OVERRIDE {
-    if (change_photo_button_)
-      change_photo_button_->SetVisible(true);
+    if (photo_overlay_)
+      photo_overlay_->SetVisible(true);
   }
 
   virtual void OnMouseExited(const ui::MouseEvent& event) OVERRIDE {
-    if (change_photo_button_)
-      change_photo_button_->SetVisible(false);
+    if (photo_overlay_)
+      photo_overlay_->SetVisible(false);
+  }
+
+  virtual void OnFocus() OVERRIDE {
+    views::ImageButton::OnFocus();
+    if (photo_overlay_)
+      photo_overlay_->SetVisible(true);
+  }
+
+  virtual void OnBlur() OVERRIDE {
+    views::ImageButton::OnBlur();
+    if (photo_overlay_)
+      photo_overlay_->SetVisible(false);

[msw, 2014/08/19 22:39:06]

Hmm, does this still work as intended with hovering? (even in edge cases like
hovering the button while cycling through the views?)

[noms (inactive), 2014/08/20 16:43:52]

No, not excellently. I have removed the focus in mouse exited -- i think this
should cover all weird cases. 

On 2014/08/19 22:39:06, msw wrote:

[msw, 2014/08/20 19:29:25]

What? That seems quite bad... if the user focuses some view (not necessarily
this one), then hovers and exits *this* view, the focus will be cleared? I
imagine the right fix is something like:
  if (photo_overlay_ && state() != STATE_HOVERED)
    photo_overlay_->SetVisible(false);
Otherwise, I would rather this was left with some odd edge case behavior when
the user hovers/tabs simultaneously than risk clobbering focus on mouse exit
(ie. just remove the ClearFocus call).

[noms (inactive), 2014/08/20 20:25:32]

Oh, no, it would have just cleared the focus on the photo control, not all of
them (if i understood the focus manager correctly). In any case, StateChanged()
is better.


On 2014/08/20 19:29:25, msw wrote:

   }
 
   gfx::Path circular_mask_;
 
-  // Button that is shown when hovering over the image view. Can be NULL if
+  // Image that is shown when hovering over the image button. Can be NULL if
   // the photo isn't allowed to be edited (e.g. for guest profiles).
-  views::LabelButton* change_photo_button_;
+  views::ImageView* photo_overlay_;
 
   DISALLOW_COPY_AND_ASSIGN(EditableProfilePhoto);
 };
@@ -300,6 +311,7 @@ class EditableProfileName : public RightAlignedIconLabelButton,
       return;
     }
 
+    SetFocusable(true);
     // Show an "edit" pencil icon when hovering over. In the default state,
     // we need to create an empty placeholder of the correct size, so that
     // the text doesn't jump around when the hovered icon appears.
@@ -362,6 +374,16 @@ class EditableProfileName : public RightAlignedIconLabelButton,
     RightAlignedIconLabelButton::Layout();
   }
 
+  virtual void OnFocus() OVERRIDE {
+    RightAlignedIconLabelButton::OnBlur();

[msw, 2014/08/19 22:39:06]

nit: I imagine you mean to call OnFocus?

[noms (inactive), 2014/08/20 16:43:52]

Done.

+    SetState(views::CustomButton::STATE_HOVERED);

[msw, 2014/08/19 22:39:06]

Hmm, does this still work as intended with clicking? (even in edge cases, like
mouse down and drag off the button?)

[noms (inactive), 2014/08/20 16:43:52]

The clicking works fine. The only thing that happens is that if the button is
focused (and shows the pencil icon), if you mouse on it, and mouse away, the
pencil icon will go away. The button is still focused, so pressing enter on it
will still activate it.

Do you think it's worth adding a MouseExisted ClearFocus() like in
EditableProfilePhoto?

On 2014/08/19 22:39:06, msw wrote:

[msw, 2014/08/20 19:29:25]

That sounds fine.
No, I don't think that pattern is good at all.

+  }
+
+  virtual void OnBlur() OVERRIDE {
+    RightAlignedIconLabelButton::OnBlur();
+    SetState(views::CustomButton::STATE_NORMAL);

[msw, 2014/08/19 22:39:05]

Hmm, does this still work as intended with hovering? (even in edge cases like
hovering the button while cycling through the views?)

[noms (inactive), 2014/08/20 16:43:52]

See above.

On 2014/08/19 22:39:05, msw wrote:

+  }
+
   // Textfield that is shown when editing the profile name. Can be NULL if
   // the profile name isn't allowed to be edited (e.g. for guest profiles).
   views::Textfield* profile_name_textfield_;
@@ -386,6 +408,7 @@ class TitleCard : public views::View {
                            rb->GetImageSkiaNamed(IDR_BACK_P));
     back_button_->SetImage(views::ImageButton::STATE_DISABLED,
                            rb->GetImageSkiaNamed(IDR_BACK_D));
+    back_button_->SetFocusable(true);
     *back_button = back_button_;
 
     title_label_ = new views::Label(message);
@@ -569,6 +592,9 @@ void ProfileChooserView::Init() {
     view_mode_ = profiles::BUBBLE_VIEW_MODE_ACCOUNT_MANAGEMENT;
   }
 
+  AddAccelerator(ui::Accelerator(ui::VKEY_DOWN, ui::EF_NONE));
+  AddAccelerator(ui::Accelerator(ui::VKEY_UP, ui::EF_NONE));
+
   ShowView(view_mode_, avatar_menu_.get());
 }
 
@@ -661,6 +687,9 @@ void ProfileChooserView::ShowView(profiles::BubbleViewMode view_to_display,
   Layout();
   if (GetBubbleFrameView())
     SizeToContents();
+
+  if (users_button_)
+    users_button_->RequestFocus();

[noms (inactive), 2014/08/19 20:15:00]

This doesn't do what I think it should do. Ideally, I'd like this button to have
the tab_index=0, and always start tabbing from it. This doesn't happen, though.
Pressing tab will start from the "top" of the bubble, i.e. the
EditablePhotoButton. :/

I've also tried this->GetFocusManager()->SetFocusedView(users_button_) with no
results. I think I am doing this wrong?

[msw, 2014/08/19 22:39:06]

Try overloading GetInitiallyFocusedView.

[noms (inactive), 2014/08/20 16:43:52]

We've decided that's not needed right now (and it might lead to a weird tabbing
paradigm), so taking it out for now.

On 2014/08/19 22:39:06, msw wrote:

 }
 
 void ProfileChooserView::WindowClosing() {
@@ -729,8 +758,7 @@ void ProfileChooserView::ButtonPressed(views::Button* sender,
     ShowView(account_management_available ?
         profiles::BUBBLE_VIEW_MODE_ACCOUNT_MANAGEMENT :
         profiles::BUBBLE_VIEW_MODE_PROFILE_CHOOSER, avatar_menu_.get());
-  } else if (current_profile_photo_ &&
-             sender == current_profile_photo_->change_photo_button()) {
+  } else if (current_profile_photo_ && sender == current_profile_photo_) {

[msw, 2014/08/19 22:39:05]

nit: Is the |current_profile_photo_| NULL check still needed?

[noms (inactive), 2014/08/20 16:43:52]

Done.

     avatar_menu_->EditProfile(avatar_menu_->GetActiveProfileIndex());
     PostActionPerformed(ProfileMetrics::PROFILE_DESKTOP_MENU_EDIT_IMAGE);
   } else if (sender == signin_current_profile_link_) {
@@ -851,6 +879,16 @@ bool ProfileChooserView::HandleKeyEvent(views::Textfield* sender,
   return false;
 }
 
+bool ProfileChooserView::AcceleratorPressed(
+    const ui::Accelerator& accelerator) {
+  if (accelerator.key_code() != ui::VKEY_DOWN &&
+      accelerator.key_code() != ui::VKEY_UP)
+    return BubbleDelegateView::AcceleratorPressed(accelerator);
+  // Move the focus up or down.
+  GetFocusManager()->AdvanceFocus(accelerator.key_code() != ui::VKEY_DOWN);

[msw, 2014/08/19 22:39:06]

The old avatar just used up/down to focus the various profiles (not the add new
user button). It seems odd for up/down to cycle focus among all the bubble
content... was that discussed with PM/UX/Accessibility?

[noms (inactive), 2014/08/20 16:43:52]

Done. I think I was just being too keen on keeping the old functionality, but
you're right that it's weird.

+  return true;
+}
+
 views::View* ProfileChooserView::CreateProfileChooserView(
     AvatarMenu* avatar_menu) {
   views::View* view = new views::View();
@@ -1133,6 +1171,7 @@ views::View* ProfileChooserView::CreateCurrentProfileView(
         auth_error_email_button_->SetTextColor(
             views::LabelButton::STATE_NORMAL,
             views::Link::GetDefaultEnabledColor());
+        auth_error_email_button_->SetFocusable(true);
         layout->AddView(auth_error_email_button_);
       } else {
         views::Label* email_label = new views::Label(avatar_item.sync_state);