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

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

Index: chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm
diff --git a/chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm b/chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm
index 96af0e4e162c73ee468511ee7c8ab73cc9072d39..2320823b9f57b10bce827b2dfec30fe272bbc08e 100644
--- a/chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm
+++ b/chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm
@@ -84,6 +84,7 @@ 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;
@@ -412,22 +413,32 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
   return buttonSize;
 }
 
-@end
+- (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;
+}
 
-// A custom button that has a transparent backround.
-@interface TransparentBackgroundButton : NSButton
-@end
+- (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];
 
-@implementation TransparentBackgroundButton
-- (id)initWithFrame:(NSRect)frameRect {
-  if ((self = [super initWithFrame:frameRect])) {
-    [self setBordered:NO];
-    [self setFont:[NSFont labelFontOfSize:kTextFontSize]];
-    [self setButtonType:NSMomentaryChangeButton];
+  // 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];
   }
-  return self;
 }
 
+@end
+
+// 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 TransparentBackgroundImageView
 - (void)drawRect:(NSRect)dirtyRect {
   NSColor* backgroundColor = [NSColor colorWithCalibratedWhite:1 alpha:0.6f];
   [backgroundColor setFill];
@@ -436,13 +447,31 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
 }
 @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_;
 }
 
@@ -455,16 +484,6 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
 // 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
@@ -476,24 +495,29 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
   if ((self = [super initWithFrame:frameRect])) {
     avatarMenu_ = avatarMenu;
     controller_ = controller;
-    [self setImage: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 setBordered:NO];
+
+    base::scoped_nsobject<CustomCircleImageCell> cell(
+        [[CustomCircleImageCell alloc] init]);
+    [self setCell:cell.get()];
+
+    [self setDefaultImage:CreateProfileImage(
+        profileIcon, kLargeImageSide).ToNSImage()];
+    [self setImagePosition:NSImageOnly];
 
     NSRect bounds = NSMakeRect(0, 0, kLargeImageSide, kLargeImageSide);
     if (editingAllowed) {
-      changePhotoButton_.reset([self changePhotoButtonWithRect:bounds]);
-      [self addSubview:changePhotoButton_];
-
-      // Hide the button until the image is hovered over.
-      [changePhotoButton_ setHidden:YES];
+      [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 image until the button is hovered over.
+      [changePhotoImage_ setHidden:YES];
     }
 
     // Set the image cell's accessibility strings to be the same as the
@@ -525,36 +549,15 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
   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 {
@@ -574,16 +577,6 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
   [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.
@@ -775,6 +768,21 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
 }
 @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;
@@ -1122,6 +1130,14 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
   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])));
@@ -1637,7 +1653,8 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
 - (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