[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..02972570a3c256af368925acb50fe92b4710f4ba 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,37 @@ 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 {

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

nit: maybe overload StateChanged instead of OnMouseEntered/Exited? 
(then OnFocus could SetState(views::CustomButton::STATE_HOVERED)).
You might not even need the OnBlur (and maybe OnFocus?) overrides.

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

Still need OnFocus/OnBlur because focusing on/away doesn't trigger a state
change. But nuked the mice and went to StateChanged() for less hackiness.

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

-    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_) {
+      GetFocusManager()->ClearFocus();

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

This seems very wrong (see my other comment).

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

Done.

+      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);
   }
 
   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 +313,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 +376,16 @@ class EditableProfileName : public RightAlignedIconLabelButton,
     RightAlignedIconLabelButton::Layout();
   }
 
+  virtual void OnFocus() OVERRIDE {
+    RightAlignedIconLabelButton::OnFocus();
+    SetState(views::CustomButton::STATE_HOVERED);

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

nit: This might be done for you, but I'm not sure.

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

Sadly, no. OnFocus/Blur doesn't trigger a state change. It probably should, but
I'd rather not do it in this CL as it will get merged to M38 and break
everything. :)

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

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

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

nit: This may already be accomplished by CustomButton::OnBlur.

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

Same as above.
On 2014/08/20 19:29:25, 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 +410,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);
@@ -729,8 +754,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 (sender == current_profile_photo_) {
     avatar_menu_->EditProfile(avatar_menu_->GetActiveProfileIndex());
     PostActionPerformed(ProfileMetrics::PROFILE_DESKTOP_MENU_EDIT_IMAGE);
   } else if (sender == signin_current_profile_link_) {
@@ -1133,6 +1157,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);