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

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/profiles/profile_chooser_view.h"
 
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
@@ skipping 123 matching lines @@
   BackgroundColorHoverButton(views::ButtonListener* listener,
                              const base::string16& text,
                              const gfx::ImageSkia& icon)
       : views::LabelButton(listener, text) {
     SetImageLabelSpacing(views::kItemLabelSpacing);
     SetBorder(views::Border::CreateEmptyBorder(
         0, views::kButtonHEdgeMarginNew, 0, views::kButtonHEdgeMarginNew));
     SetMinSize(gfx::Size(0,
         kButtonHeight + views::kRelatedControlVerticalSpacing));
     SetImage(STATE_NORMAL, icon);
+    SetFocusable(true);
   }
 
   virtual ~BackgroundColorHoverButton() {}
 
  private:
   // views::LabelButton:
   virtual void OnPaint(gfx::Canvas* canvas) OVERRIDE {
     if ((state() == STATE_PRESSED) ||
         (state() == STATE_HOVERED) ||
         HasFocus()) {
@@ skipping 41 matching lines @@
     views::LabelButton::Layout();
     label()->SetHorizontalAlignment(gfx::ALIGN_CENTER);
   }
 
   DISALLOW_COPY_AND_ASSIGN(RightAlignedIconLabelButton);
 };
 
 // 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.
     circular_mask_.addCircle(SkIntToScalar(bounds.width() / 2),
                              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,
                              const views::CullSet& cull_set) OVERRIDE {
     // Display any children (the "change photo" overlay) as a circle.
     canvas->ClipPath(circular_mask_, true);
     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);
 };
 
 // EditableProfileName -------------------------------------------------
 
 // A custom text control that turns into a textfield for editing when clicked.
 class EditableProfileName : public RightAlignedIconLabelButton,
                             public views::ButtonListener {
  public:
   EditableProfileName(views::TextfieldController* controller,
                       const base::string16& text,
                       bool is_editing_allowed)
       : RightAlignedIconLabelButton(this, text),
         profile_name_textfield_(NULL) {
     ui::ResourceBundle* rb = &ui::ResourceBundle::GetSharedInstance();
     const gfx::FontList& medium_font_list =
         rb->GetFontList(ui::ResourceBundle::MediumFont);
     SetFontList(medium_font_list);
     SetHorizontalAlignment(gfx::ALIGN_CENTER);
 
     if (!is_editing_allowed) {
       SetBorder(views::Border::CreateEmptyBorder(2, 0, 2, 0));
       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.
     gfx::ImageSkia hover_image =
         *rb->GetImageSkiaNamed(IDR_ICON_PROFILES_EDIT_HOVER);
     SetImage(STATE_NORMAL, CreateSquarePlaceholderImage(hover_image.width()));
     SetImage(STATE_HOVERED, hover_image);
     SetImage(STATE_PRESSED,
              *rb->GetImageSkiaNamed(IDR_ICON_PROFILES_EDIT_PRESSED));
     // To center the text, we need to offest it by the width of the icon we
@@ skipping 42 matching lines @@
     // part of the new profile name typed in the textfield.
     return false;
   }
 
   virtual void Layout() OVERRIDE {
     if (profile_name_textfield_)
       profile_name_textfield_->SetBounds(0, 0, width(), height());
     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_;
 
   DISALLOW_COPY_AND_ASSIGN(EditableProfileName);
 };
 
 // A title card with one back button right aligned and one label center aligned.
 class TitleCard : public views::View {
  public:
   TitleCard(const base::string16& message, views::ButtonListener* listener,
             views::ImageButton** back_button) {
     back_button_ = new views::ImageButton(listener);
     back_button_->SetImageAlignment(views::ImageButton::ALIGN_LEFT,
                                     views::ImageButton::ALIGN_MIDDLE);
     ui::ResourceBundle* rb = &ui::ResourceBundle::GetSharedInstance();
     back_button_->SetImage(views::ImageButton::STATE_NORMAL,
                            rb->GetImageSkiaNamed(IDR_BACK));
     back_button_->SetImage(views::ImageButton::STATE_HOVERED,
                            rb->GetImageSkiaNamed(IDR_BACK_H));
     back_button_->SetImage(views::ImageButton::STATE_PRESSED,
                            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);
     title_label_->SetHorizontalAlignment(gfx::ALIGN_CENTER);
     const gfx::FontList& medium_font_list =
         rb->GetFontList(ui::ResourceBundle::MediumFont);
     title_label_->SetFontList(medium_font_list);
 
     AddChildView(back_button_);
     AddChildView(title_label_);
@@ skipping 163 matching lines @@
   // If view mode is PROFILE_CHOOSER but there is an auth error, force
   // ACCOUNT_MANAGEMENT mode.
   if (view_mode_ == profiles::BUBBLE_VIEW_MODE_PROFILE_CHOOSER &&
       HasAuthError(browser_->profile()) &&
       switches::IsEnableAccountConsistency() &&
       avatar_menu_->GetItemAt(avatar_menu_->GetActiveProfileIndex()).
           signed_in) {
     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());
 }
 
 void ProfileChooserView::OnAvatarMenuChanged(
     AvatarMenu* avatar_menu) {
   // Do not refresh the avatar menu if the user is on a signin related view.
   if (view_mode_ == profiles::BUBBLE_VIEW_MODE_GAIA_SIGNIN ||
       view_mode_ == profiles::BUBBLE_VIEW_MODE_GAIA_ADD_ACCOUNT ||
       view_mode_ == profiles::BUBBLE_VIEW_MODE_GAIA_REAUTH) {
     return;
@@ skipping 72 matching lines @@
   }
   // Clears tutorial mode for all non-profile-chooser views.
   if (view_mode_ != profiles::BUBBLE_VIEW_MODE_PROFILE_CHOOSER)
     tutorial_mode_ = profiles::TUTORIAL_MODE_NONE;
 
   layout->StartRow(1, 0);
   layout->AddView(sub_view);
   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() {
   DCHECK_EQ(profile_bubble_, this);
   profile_bubble_ = NULL;
 
   if (tutorial_mode_ == profiles::TUTORIAL_MODE_CONFIRM_SIGNIN) {
     LoginUIServiceFactory::GetForProfile(browser_->profile())->
         SyncConfirmationUIClosed(false /* configure_sync_first */);
   }
@@ skipping 48 matching lines @@
     std::string primary_account =
         SigninManagerFactory::GetForProfile(browser_->profile())->
         GetAuthenticatedUsername();
     // The account management view is only available with the
     // --enable-account-consistency flag.
     bool account_management_available = !primary_account.empty() &&
         switches::IsEnableAccountConsistency();
     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_) {
     ShowView(profiles::BUBBLE_VIEW_MODE_GAIA_SIGNIN, avatar_menu_.get());
   } else if (sender == add_person_button_) {
     ProfileMetrics::LogProfileNewAvatarMenuNotYou(
         ProfileMetrics::PROFILE_AVATAR_MENU_NOT_YOU_ADD_PERSON);
     chrome::ShowUserManager(browser_->profile()->GetPath());
   } else if (sender == disconnect_button_) {
     ProfileMetrics::LogProfileNewAvatarMenuNotYou(
@@ skipping 100 matching lines @@
       return true;
 
     profiles::UpdateProfileName(profile, new_profile_name);
     PostActionPerformed(ProfileMetrics::PROFILE_DESKTOP_MENU_EDIT_NAME);
     current_profile_name_->ShowReadOnlyView();
     return true;
   }
   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();
   views::GridLayout* layout = CreateSingleColumnLayout(view, kFixedMenuWidth);
   // Separate items into active and alternatives.
   Indexes other_profiles;
   views::View* tutorial_view = NULL;
   views::View* current_profile_view = NULL;
   views::View* current_profile_accounts = NULL;
   views::View* option_buttons_view = NULL;
@@ skipping 262 matching lines @@
             IDR_ICON_PROFILES_ACCOUNT_BUTTON_ERROR).ToImageSkia();
         auth_error_email_button_ =
             new RightAlignedIconLabelButton(this, avatar_item.sync_state);
         auth_error_email_button_->SetElideBehavior(gfx::ELIDE_EMAIL);
         auth_error_email_button_->SetBorder(views::Border::NullBorder());
         auth_error_email_button_->SetImage(
             views::LabelButton::STATE_NORMAL, warning_image);
         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);
         email_label->SetElideBehavior(gfx::ELIDE_EMAIL);
         email_label->SetEnabled(false);
         layout->AddView(email_label);
       }
     }
   } else {
     SigninManagerBase* signin_manager = SigninManagerFactory::GetForProfile(
@@ skipping 440 matching lines @@
       IncognitoModePrefs::GetAvailability(browser_->profile()->GetPrefs()) !=
           IncognitoModePrefs::DISABLED;
   return incognito_available && !browser_->profile()->IsGuestSession();
 }
 
 void ProfileChooserView::PostActionPerformed(
     ProfileMetrics::ProfileDesktopMenu action_performed) {
   ProfileMetrics::LogProfileDesktopMenu(action_performed, gaia_service_type_);
   gaia_service_type_ = signin::GAIA_SERVICE_TYPE_NONE;
 }