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

chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm

 // 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.
 
 #import <Cocoa/Cocoa.h>
 
 #import "chrome/browser/ui/cocoa/profiles/profile_chooser_controller.h"
 
 #include "base/mac/bundle_locations.h"
 #include "base/prefs/pref_service.h"
@@ skipping 66 matching lines @@
 
 const CGFloat kVerticalSpacing = 16.0;
 const CGFloat kSmallVerticalSpacing = 10.0;
 const CGFloat kHorizontalSpacing = 16.0;
 const CGFloat kTitleFontSize = 15.0;
 const CGFloat kTextFontSize = 12.0;
 const CGFloat kProfileButtonHeight = 30;
 const int kBezelThickness = 3;  // Width of the bezel on an NSButton.
 const int kImageTitleSpacing = 10;
 const int kBlueButtonHeight = 30;
+const CGFloat kFocusRingLineWidth = 2;
 
 // Fixed size for embedded sign in pages as defined in Gaia.
 const CGFloat kFixedGaiaViewWidth = 360;
 const CGFloat kFixedGaiaViewHeight = 440;
 
 // Fixed size for the account removal view.
 const CGFloat kFixedAccountRemovalViewWidth = 280;
 
 // Fixed size for the switch user view.
 const int kFixedSwitchUserViewWidth = 280;
@@ skipping 308 matching lines @@
 }
 
 - (NSSize)cellSize {
   NSSize buttonSize = [super cellSize];
   buttonSize.width += leftMarginSpacing_;
   if ([self imagePosition] == NSImageLeft)
     buttonSize.width += imageTitleSpacing_;
   return buttonSize;
 }
 
+- (NSFocusRingType)focusRingType {
+  // This is taken care of by the custom drawing code in the cell.

[groby-ooo-7-16, 2014/08/21 05:06:02]

Isn't "in the cell" implicit?

[noms (inactive), 2014/08/25 14:54:43]

Done.

+  return NSFocusRingTypeNone;
+}
+
+- (void)drawWithFrame:(NSRect)frame inView:(NSView *)controlView {

[groby-ooo-7-16, 2014/08/21 05:06:02]

I'm still not entirely clear why the normal focus ring is not enough...

[noms (inactive), 2014/08/21 14:32:05]

In the case of this control, the default focus ring is just around the image
(not around the image and the text). I thought it looked strange, but maybe
that's the Cocoa way?

On 2014/08/21 05:06:02, groby wrote:

[groby-ooo-7-16, 2014/08/21 17:59:09]

That's exceedingly odd. If I whip up a quick demo with a normal NSButton, the
focus ring does the whole button, not just the image.

Does the same happen if it's an NSButton instead of a HoverImageButton? Can you
share a screenshot of the weirdness?

[noms (inactive), 2014/08/21 18:23:28]

I think this is a HoverImageButton deal. 
From the current Canary:
Screenshot 1:
https://drive.google.com/open?id=0B1B1Up4p2NRMWmdVSGcwSWJZZlU&authuser=1
Screenshot 2:
https://drive.google.com/open?id=0B1B1Up4p2NRMYm83QjN5WTBYVVU&authuser=1

In both the focus ring is just around the image.

With my fix:
https://drive.google.com/open?id=0B1B1Up4p2NRMZDhPczROMHNGZVk&authuser=1


On 2014/08/21 17:59:09, groby wrote:

[groby-ooo-7-16, 2014/08/21 19:07:30]

It's a CustomPaddingButtonCell deal :( -drawTitle:/-drawImage: should not mess
with the rectangle, I think.

I'd bet that if in -drawImage: you change the frame, you'll get a different
focus. And if that's true, you can just replace -drawImage: with
(
  NSRect imageRect = frame;
  if ([self imagePosition] == NSImageLeft)
    imageRect.origin.x += imageTitleSpacing_;
  [image drawInRect:imageRect];
  [super drawImage:emptyImage_ withFrame:frame inView:controlView];
}

and kill drawWithFrame:
  
(Obvs, you need an emptyImage_ :)

[noms (inactive), 2014/08/22 15:48:21]

So doing this draws the image fine, but then there's literally no focus ring at
all. I set [self setFocusRingType:NSFocusRingTypeExterior] in the
CustomPaddingImageButtonCell -init, and the focus ring still doesn't appear.

Focus rings: predictable 0% of the time.

On 2014/08/21 19:07:30, groby wrote:

+  [super drawInteriorWithFrame:frame inView:controlView];
+
+  // Focus ring.
+  if ([self showsFirstResponder]) {

[groby-ooo-7-16, 2014/08/21 05:06:02]

You might want to leave a TODO to fix this with 10.7... that has the nifty
drawFocusRingMaskWithFrame, so there's finally a hook to change focus ring
style.

[noms (inactive), 2014/08/25 14:54:43]

Done.

+    NSRect focusRingRect = NSInsetRect(frame,
+        kFocusRingLineWidth, kFocusRingLineWidth);
+    [[[NSColor keyboardFocusIndicatorColor] colorWithAlphaComponent:1] set];
+    NSBezierPath* path = [NSBezierPath bezierPathWithRect:focusRingRect];
+    [path setLineWidth:kFocusRingLineWidth];
+    [path stroke];
+  }
+}
+
 @end
 
-// A custom button that has a transparent backround.
-@interface TransparentBackgroundButton : NSButton
+// A custom image view that has a transparent backround.
+@interface TransparentBackgroundImageView : NSImageView

[groby-ooo-7-16, 2014/08/21 05:06:02]

It's technically translucent, not transparent :)

Also: Do you still need this, or will the images always be the size of the view
anyways?

[noms (inactive), 2014/08/21 14:32:05]

Still needed: the image is the size of the view, but it needs to sit on a
beautiful opaque, semi transparent white (which is being set in drawRect). 


On 2014/08/21 05:06:02, groby wrote:

[groby-ooo-7-16, 2014/08/21 17:59:09]

So the image has alpha, then? (Because otherwise, it'd obscure the beautiful
white)

[noms (inactive), 2014/08/21 18:23:28]

Yup, the image is just the center icon, and is transparent. This works exactly
like it did before. It's just changing form a button to a view.

Proof: https://drive.google.com/open?id=0B1B1Up4p2NRMYm83QjN5WTBYVVU&authuser=1

On 2014/08/21 17:59:09, groby wrote:

 @end
 
-@implementation TransparentBackgroundButton
-- (id)initWithFrame:(NSRect)frameRect {
-  if ((self = [super initWithFrame:frameRect])) {
-    [self setBordered:NO];
-    [self setFont:[NSFont labelFontOfSize:kTextFontSize]];
-    [self setButtonType:NSMomentaryChangeButton];
-  }
-  return self;
-}
-
+@implementation TransparentBackgroundImageView
 - (void)drawRect:(NSRect)dirtyRect {
   NSColor* backgroundColor = [NSColor colorWithCalibratedWhite:1 alpha:0.6f];
   [backgroundColor setFill];
   NSRectFillUsingOperation(dirtyRect, NSCompositeSourceAtop);
   [super drawRect:dirtyRect];
 }
 @end
 
+@interface CustomCircleImageCell : NSButtonCell
+@end
+
+@implementation CustomCircleImageCell
+- (void)drawWithFrame:(NSRect)frame inView:(NSView *)controlView {
+  // Display everything as a circle that spans the entire control.
+  NSBezierPath* path = [NSBezierPath bezierPathWithOvalInRect:frame];
+  [path addClip];
+
+  [super drawImage:[self image] withFrame:frame inView:controlView];
+
+  // Focus ring.
+  if ([self showsFirstResponder]) {
+    [[[NSColor keyboardFocusIndicatorColor] colorWithAlphaComponent:1] set];
+    [path setLineWidth:kFocusRingLineWidth];
+    [path stroke];
+  }
+}
+@end
+
 // A custom image control that shows a "Change" button when moused over.
-@interface EditableProfilePhoto : NSImageView {
+@interface EditableProfilePhoto : HoverImageButton {
  @private
   AvatarMenu* avatarMenu_;  // Weak; Owned by ProfileChooserController.
-  base::scoped_nsobject<TransparentBackgroundButton> changePhotoButton_;
-  // Used to display the "Change" button on hover.
-  ui::ScopedCrTrackingArea trackingArea_;
+  base::scoped_nsobject<TransparentBackgroundImageView> changePhotoImage_;
   ProfileChooserController* controller_;
 }
 
 - (id)initWithFrame:(NSRect)frameRect
          avatarMenu:(AvatarMenu*)avatarMenu
         profileIcon:(const gfx::Image&)profileIcon
      editingAllowed:(BOOL)editingAllowed
      withController:(ProfileChooserController*)controller;
 
 // Called when the "Change" button is clicked.
 - (void)editPhoto:(id)sender;
 
-// When hovering over the profile photo, show the "Change" button.
-- (void)mouseEntered:(NSEvent*)event;
-
-// When hovering away from the profile photo, hide the "Change" button.
-- (void)mouseExited:(NSEvent*)event;
-@end
-
-@interface EditableProfilePhoto (Private)
-// Create the "Change" avatar photo button.
-- (TransparentBackgroundButton*)changePhotoButtonWithRect:(NSRect)rect;
 @end
 
 @implementation EditableProfilePhoto
 - (id)initWithFrame:(NSRect)frameRect
          avatarMenu:(AvatarMenu*)avatarMenu
         profileIcon:(const gfx::Image&)profileIcon
      editingAllowed:(BOOL)editingAllowed
      withController:(ProfileChooserController*)controller {
   if ((self = [super initWithFrame:frameRect])) {
     avatarMenu_ = avatarMenu;
     controller_ = controller;
-    [self setImage:CreateProfileImage(
+
+    [self setBordered:NO];
+
+    base::scoped_nsobject<CustomCircleImageCell> cell(
+        [[CustomCircleImageCell alloc] init]);
+    [self setCell:cell.get()];
+
+    [self setDefaultImage:CreateProfileImage(
         profileIcon, kLargeImageSide).ToNSImage()];
-
-    // Add a tracking area so that we can show/hide the button when hovering.
-    trackingArea_.reset([[CrTrackingArea alloc]
-        initWithRect:[self bounds]
-             options:NSTrackingMouseEnteredAndExited | NSTrackingActiveAlways
-               owner:self
-            userInfo:nil]);
-    [self addTrackingArea:trackingArea_.get()];
+    [self setImagePosition:NSImageOnly];
 
     NSRect bounds = NSMakeRect(0, 0, kLargeImageSide, kLargeImageSide);
     if (editingAllowed) {
-      changePhotoButton_.reset([self changePhotoButtonWithRect:bounds]);
-      [self addSubview:changePhotoButton_];
+      [self setTarget:self];
+      [self setAction:@selector(editPhoto:)];
+      changePhotoImage_.reset([[TransparentBackgroundImageView alloc]
+          initWithFrame:bounds]);
+      [changePhotoImage_ setImage:ui::ResourceBundle::GetSharedInstance().
+          GetNativeImageNamed(IDR_ICON_PROFILES_EDIT_CAMERA).AsNSImage()];
+      [self addSubview:changePhotoImage_];
 
-      // Hide the button until the image is hovered over.
-      [changePhotoButton_ setHidden:YES];
+      // Hide the image until the button is hovered over.
+      [changePhotoImage_ setHidden:YES];
     }
 
     // Set the image cell's accessibility strings to be the same as the
     // button's strings.
     [[self cell] accessibilitySetOverrideValue:l10n_util::GetNSString(
         editingAllowed ?
         IDS_PROFILES_NEW_AVATAR_MENU_CHANGE_PHOTO_ACCESSIBLE_NAME :
         IDS_PROFILES_NEW_AVATAR_MENU_PHOTO_ACCESSIBLE_NAME)
                                   forAttribute:NSAccessibilityTitleAttribute];
     [[self cell] accessibilitySetOverrideValue:
@@ skipping 11 matching lines @@
                                   forAttribute:NSAccessibilityTitleAttribute];
     [self accessibilitySetOverrideValue:NSAccessibilityButtonRole
                                   forAttribute:NSAccessibilityRoleAttribute];
     [self accessibilitySetOverrideValue:
         NSAccessibilityRoleDescription(NSAccessibilityButtonRole, nil)
             forAttribute:NSAccessibilityRoleDescriptionAttribute];
   }
   return self;
 }
 
-- (void)drawRect:(NSRect)dirtyRect {
-  NSRect bounds = [self bounds];
-
-  // Display the profile picture as a circle.
-  NSBezierPath* path = [NSBezierPath bezierPathWithOvalInRect:bounds];
-  [path addClip];
-  [self.image drawAtPoint:bounds.origin
-                 fromRect:bounds
-                operation:NSCompositeSourceOver
-                 fraction:1.0];
-
-}
-
 - (void)editPhoto:(id)sender {
   avatarMenu_->EditProfile(avatarMenu_->GetActiveProfileIndex());
   [controller_
       postActionPerformed:ProfileMetrics::PROFILE_DESKTOP_MENU_EDIT_IMAGE];
 }
 
-- (void)mouseEntered:(NSEvent*)event {
-  [changePhotoButton_ setHidden:NO];
-}
-
-- (void)mouseExited:(NSEvent*)event {
-  [changePhotoButton_ setHidden:YES];
-}
-
-// Make sure the element is focusable for accessibility.
-- (BOOL)canBecomeKeyView {
-  return YES;
+- (void)setHoverState:(HoverState)state {
+  [super setHoverState:state];
+  [changePhotoImage_ setHidden:([self hoverState] == kHoverStateNone)];
 }
 
 - (BOOL)accessibilityIsIgnored {
   return NO;
 }
 
 - (NSArray*)accessibilityActionNames {
   NSArray* parentActions = [super accessibilityActionNames];
   return [parentActions arrayByAddingObject:NSAccessibilityPressAction];
 }
 
 - (void)accessibilityPerformAction:(NSString*)action {
   if ([action isEqualToString:NSAccessibilityPressAction]) {
     avatarMenu_->EditProfile(avatarMenu_->GetActiveProfileIndex());
   }
 
   [super accessibilityPerformAction:action];
 }
 
-- (TransparentBackgroundButton*)changePhotoButtonWithRect:(NSRect)rect {
-  TransparentBackgroundButton* button =
-      [[TransparentBackgroundButton alloc] initWithFrame:rect];
-  [button setImage:ui::ResourceBundle::GetSharedInstance().GetNativeImageNamed(
-      IDR_ICON_PROFILES_EDIT_CAMERA).AsNSImage()];
-  [button setImagePosition:NSImageOnly];
-  [button setTarget:self];
-  [button setAction:@selector(editPhoto:)];
-  return button;
-}
 @end
 
 // A custom text control that turns into a textfield for editing when clicked.
 @interface EditableProfileNameButton : HoverImageButton {
  @private
   base::scoped_nsobject<NSTextField> profileNameTextField_;
   Profile* profile_;  // Weak.
   ProfileChooserController* controller_;
 }
 
@@ skipping 171 matching lines @@
   return self;
 }
 
 - (void)drawRect:(NSRect)dirtyRect {
   [backgroundColor_ setFill];
   NSRectFill(dirtyRect);
   [super drawRect:dirtyRect];
 }
 @end
 
+// A custom dummy button that is used to clear focus from the bubble's controls.
+@interface DummyWindowFocusButton : NSButton
+@end
+
+@implementation DummyWindowFocusButton
+// Ignore accessibility, as this is a placeholder button.
+- (BOOL)accessibilityIsIgnored {
+  return YES;
+}
+- (id)accessibilityAttributeValue:(NSString*)attribute {
+  return @[];
+}
+
+@end
+
 @interface ProfileChooserController ()
 // Builds the profile chooser view.
 - (NSView*)buildProfileChooserView;
 
 // Builds a tutorial card with a title label using |titleMessage|, a content
 // label using |contentMessage|, a link using |linkMessage|, and a button using
 // |buttonMessage|. If |stackButton| is YES, places the button above the link.
 // Otherwise places both on the same row with the link left aligned and button
 // right aligned. On click, the link would execute |linkAction|, and the button
 // would execute |buttonAction|. It sets |tutorialMode_| to the given |mode|.
@@ skipping 327 matching lines @@
     case profiles::BUBBLE_VIEW_MODE_PROFILE_CHOOSER:
     case profiles::BUBBLE_VIEW_MODE_ACCOUNT_MANAGEMENT:
       subView = [self buildProfileChooserView];
       break;
   }
 
   // Clears tutorial mode for all non-profile-chooser views.
   if (viewMode_ != profiles::BUBBLE_VIEW_MODE_PROFILE_CHOOSER)
     tutorialMode_ = profiles::TUTORIAL_MODE_NONE;
 
+  // Add a dummy, empty element so that we don't initially display any
+  // focus rings.
+  NSButton* dummyFocusButton =

[noms (inactive), 2014/08/20 17:17:38]

This is a giant hack. I don't know how to get around "default focus on
something" without a giant hack. Sigh. Is there even a way (internet says
"remove the focus rings", which, lol)

[groby-ooo-7-16, 2014/08/21 05:06:02]

That's a really bad idea. If there is no focus at all, there is no UI element
that takes keyboard input. That's probably not the indication we want to give
users. Is there a mock to look at?

Also, if you do it this way, the dummy button still participates in the key view
chain. That's probably not the intended outcome :)

[noms (inactive), 2014/08/21 14:32:05]

I don't have mocks for tabbing, but always showing the bubble with a blue circle
around the photo avatar is terrible. For that matter anything that has a blue
circle around it looks pretty terrible, given that this bubble isn't like a
standard form where there's a textbox that can have focus...

For local profiles, I can focus on the "Sign in" button, since that looks the
least jarring, but I don't know what could be similarly focused for signed in
profiles...

There focus, technically, on this dummy button, and once you start tabbing, it
tabs through everything else. It does participate in the key view chain, but I
can't seem to skip it after it's been used once.

On 2014/08/21 05:06:02, groby wrote:

[groby-ooo-7-16, 2014/08/21 17:59:09]

Isn't that pretty bad in terms of a11y? You have no idea the thing popped up,
and you don't know tabbing will get you anywhere?
Well, a "Sign Out" button would work, but we don't have that. What do the UI
gods have to say? (I.e. ping ui-review?)

[noms (inactive), 2014/08/21 18:23:28]

Oh, no no no, it's not that bad. So when the bubble comes up, the bubble says
"Switch person window". Normally it would also add "x button has focus", but
since this button is invisible, the "x button" bit is not read. I would assume
users would tab, but maybe that's incorrect.

I have fired off an email to Travis and Alan and see what they say. As a side
note, with a default focus, the bubble would look like this. Isn't that blue
circle weird?
https://drive.google.com/open?id=0B1B1Up4p2NRMZDhPczROMHNGZVk&authuser=1

[groby-ooo-7-16, 2014/08/21 19:07:30]

I'd assume the blue circle is an issue for everybody using a BlueLabelButton.
Maybe our fearless ui leaders would like to fix that :)

As for what a11y users would do - dmazzoni might know. While I use VoiceOver
from time to time just to know what it's like, I'm not doing it very often, so I
don't know behavior patterns.

[noms (inactive), 2014/08/21 19:53:39]

The blue circle is because the control is round. If it was a text button, it
would be a blue square -- I am referring to the blue focus ring around the
photo. The blue signin button is just blue :)

For a11y, I can make the hidden dummy button read the same thing as the window
("Switch person window), so that it isn't completely silent when it gets to it.

I double checked with Travis/Alan, and we'd *really* like to do our best to not
have anything look focused initially, as it looks better from a visual
standpoint...:/

[groby-ooo-7-16, 2014/08/21 20:53:34]

OK. 

So, [[self window] makeFirstResponder:nil];?

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

That was my first try, but that still focuses the first element. 

+      [[[DummyWindowFocusButton alloc] initWithFrame:NSZeroRect] autorelease];
+  [subView addSubview:dummyFocusButton];
+  [dummyFocusButton setNextKeyView:subView];
+  [[self window] makeFirstResponder:dummyFocusButton];
+
   [contentView addSubview:subView];
   SetWindowSize([self window],
       NSMakeSize(NSWidth([subView frame]), NSHeight([subView frame])));
 }
 
 - (NSView*)buildProfileChooserView {
   base::scoped_nsobject<NSView> container(
       [[NSView alloc] initWithFrame:NSZeroRect]);
 
   NSView* tutorialView = nil;
@@ skipping 495 matching lines @@
   guestItem.active = true;
   guestItem.name = base::SysNSStringToUTF16(
       l10n_util::GetNSString(IDS_PROFILES_GUEST_PROFILE_NAME));
 
   return [self createCurrentProfileView:guestItem];
 }
 
 - (NSButton*)createOtherProfileView:(int)itemIndex {
   const AvatarMenu::Item& item = avatarMenu_->GetItemAt(itemIndex);
 
-  NSRect rect = NSMakeRect(0, 0, kFixedMenuWidth, kBlueButtonHeight);
+  NSRect rect = NSMakeRect(
+      0, 0, kFixedMenuWidth, kBlueButtonHeight + kSmallVerticalSpacing);
   base::scoped_nsobject<BackgroundColorHoverButton> profileButton(
       [[BackgroundColorHoverButton alloc]
           initWithFrame:rect
       imageTitleSpacing:kImageTitleSpacing
         backgroundColor:GetDialogBackgroundColor()]);
   [profileButton setTitle:base::SysUTF16ToNSString(item.name)];
   [profileButton setDefaultImage:CreateProfileImage(
       item.icon, kSmallImageSide).ToNSImage()];
   [profileButton setImagePosition:NSImageLeft];
   [profileButton setAlignment:NSLeftTextAlignment];
@@ skipping 468 matching lines @@
 }
 
 - (bool)shouldShowGoIncognito {
   bool incognitoAvailable =
       IncognitoModePrefs::GetAvailability(browser_->profile()->GetPrefs()) !=
           IncognitoModePrefs::DISABLED;
   return incognitoAvailable && !browser_->profile()->IsGuestSession();
 }
 
 @end