[Mac] Misc fixes for 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 c1b84666aebe8bc76e466633013286a66a890d66..6b21c1a36e5b4313898fff8619b0d27047b0f4bf 100644
--- a/chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm
+++ b/chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm
@@ -332,6 +332,88 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
   DISALLOW_COPY_AND_ASSIGN(ActiveProfileObserverBridge);
 };
 
+// Custom button cell that adds a left padding to a button.

[groby-ooo-7-16, 2014/06/10 19:53:23]

If you want to add padding, -drawInteriorWithFrame: is a better choice. As it
stands, drawImage for this button would ignore the padding for example.

[noms (inactive), 2014/06/11 15:09:00]

Hmm, I might have done it wrong, but -drawInteriorWithFrame adds a margin (so
indents the whole control), whereas drawTitle just indents the title, but keeps
the same control frame.

This is relevant because CustomPaddingImageButtonCell (which inherits from this)
is used in a button that, on hover, sets the background color of the button, so
if the entire button is indented, and not just the text, it looks weird. I'm not
sure I've explained this clearly... :(

On 2014/06/10 19:53:23, groby wrote:

[groby-ooo-7-16, 2014/06/11 18:11:21]

I'm not sure I follow - how is the button indented? From what? (Sorry. I'm slow.
It's easiest if people draw me pictures :)

[noms (inactive), 2014/06/11 19:29:57]

Yeah, my bad. This entire CL is "really annoying and minor UI fixes that are a
pain to explain and fix".
Let's give this another try. Here are some drawings:
https://docs.google.com/drawings/d/1gvNMEs0AJpI0ZTa2x6wv24qGh0uJPdkjN3kJe2aVu...

The problem we are trying to fix here is that "monica" isn't centered (see the
"status quo" images) -- it's off to the left by the width of the pencil. If you
look at the "pressed" state, the grey background covers essentially the entire
bubble, minus some edge padding.

If I add the left padding in drawTitle, then the button stays the same width (so
the grey on pressed state looks the same), but the button title gets shifted to
the right and gets centered correctly.

If i add the left padding in drawInteriorWithFrame (with essentially the same
code as in drawTitle), the frame.origin.x += padding call will add the padding
to the _entire_ button -- so it will end up looking like the "after
drawInteriorWithFrame" image. The text is centered correctly, but that's because
the whole button moved, which is bad news bears.

Am I doing this wrong? I bet I'm doing this wrong.

On 2014/06/11 18:11:21, groby wrote:

[groby-ooo-7-16, 2014/06/12 01:48:12]

No, you're not. I misunderstood what you want padding to do. The picture helped
:)

But you can still do it via drawInteriorWithFrame - you just need to take care
of the background yourself.

- (void)drawInteriorWithFrame:(NSRect)cellFrame inView:(NSView *)controlView {
  NSRect marginRect;
  NSRect interiorRect;

  NSDivideRect(cellFrame, &marginRect, &interiorRect, leftMarginSpacing_,
NSMinXEdge);

   // You might possibly need to fill the entire cellFrame instead if the border
shows aliasing issues.
   if(![self isBordered]){
     [[self backgroundColor] setFill];
     NSRectFill(marginRect);
  }

  [super drawInteriorWithFrame:interiorRect];
}
  

[noms (inactive), 2014/06/16 19:17:10]

Ok. The -drawInterior was almost perfect, but because you can edit the profile
name while the bubble is open, it needs to redraw while you're looking at it,
and the original background flashes before the NSRectFill() call is made.

However, not all is lost: I managed to merge the two classes together and make
it work for both cases. Better?

+@interface LeftMarginButtonCell : NSButtonCell {
+ @protected
+  // Padding added to the left margin of the button.
+  int leftMarginSpacing_;
+}
+
+- (id)initWithLeftMarginSpacing:(int)leftMarginSpacing;
+@end
+
+@implementation LeftMarginButtonCell
+- (id)initWithLeftMarginSpacing:(int)leftMarginSpacing {
+  if ((self = [super init])) {
+    leftMarginSpacing_ = leftMarginSpacing;
+  }
+  return self;
+}
+
+- (NSRect)drawTitle:(NSAttributedString*)title
+          withFrame:(NSRect)frame
+             inView:(NSView*)controlView {
+  // The title frame origin isn't aware of the left margin spacing added
+  // in -drawImage, so it must be added when drawing the title as well.
+  frame.origin.x += leftMarginSpacing_;
+  frame.size.width -= leftMarginSpacing_;
+  return [super drawTitle:title withFrame:frame inView:controlView];
+}
+
+- (NSSize)cellSize {
+  NSSize buttonSize = [super cellSize];
+  buttonSize.width += leftMarginSpacing_;
+  return buttonSize;
+}
+
+@end
+
+// Custom button cell that adds a left padding before the button image, and
+// a custom spacing between the button image and title.
+@interface CustomPaddingImageButtonCell : LeftMarginButtonCell {

[groby-ooo-7-16, 2014/06/10 19:53:23]

You might want to _just_ keep CustomPaddingImageButtonCell.

LeftMarginImageButtonCell is just this with an |imageTitleSpacing_| of 0, no?

[noms (inactive), 2014/06/11 15:09:00]

Hmm, it's a little worse than that (I'm noticing this is a poorly named
control). CustomPaddingImageButtonCell is hardcoded to always place the image on
the left. LeftMarginButtonCell just adds a padding to the left of the title, but
you can have the image position be on the right (which is how it's used in the
profile name control). I should rename these, shouldn't I?

On 2014/06/10 19:53:23, groby wrote:

[groby-ooo-7-16, 2014/06/11 18:11:22]

You might want to. Also, with the image forced to the left, RTL is going to
be... fun. If you could make a quick sketch what you want to pad, exactly,
because I think I misunderstand things?

[noms (inactive), 2014/06/11 19:29:57]

Does the sketch above explain the padding? I was talking about the "space I need
to shift the button title to the right so that it's centered". 

On 2014/06/11 18:11:22, groby wrote:

[groby-ooo-7-16, 2014/06/12 01:48:11]

If you use the drawInteriorWithFrame approach, you don't have to make
assumptions about the image alignment any more. Or I'm still missing the point.
It's 50/50 ;)

+ @private
+  // Spacing between the cell image and title.
+  int imageTitleSpacing_;
+}
+
+- (id)initWithLeftMarginSpacing:(int)leftMarginSpacing
+              imageTitleSpacing:(int)imageTitleSpacing;
+@end
+
+@implementation CustomPaddingImageButtonCell
+- (id)initWithLeftMarginSpacing:(int)leftMarginSpacing
+              imageTitleSpacing:(int)imageTitleSpacing {
+  if ((self = [super initWithLeftMarginSpacing:leftMarginSpacing])) {
+    imageTitleSpacing_ = imageTitleSpacing;
+  }
+  return self;
+}
+
+- (NSRect)drawTitle:(NSAttributedString*)title
+          withFrame:(NSRect)frame
+             inView:(NSView*)controlView {
+  // The title frame origin isn't aware of the left margin spacing added
+  // in -drawImage, so it must be added when drawing the title as well.
+  frame.origin.x += imageTitleSpacing_;
+  frame.size.width -= imageTitleSpacing_;
+  return [super drawTitle:title withFrame:frame inView:controlView];
+}
+
+- (void)drawImage:(NSImage*)image
+       withFrame:(NSRect)frame
+          inView:(NSView*)controlView {
+  frame.origin.x = leftMarginSpacing_;
+  [super drawImage:image withFrame:frame inView:controlView];
+}
+
+- (NSSize)cellSize {
+  NSSize buttonSize = [super cellSize];
+  buttonSize.width += imageTitleSpacing_;
+  return buttonSize;
+}
+
+@end
+
 // A custom button that has a transparent backround.
 @interface TransparentBackgroundButton : NSButton
 @end
@@ -347,7 +429,7 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
 }
 
 - (void)drawRect:(NSRect)dirtyRect {
-  NSColor* backgroundColor = [NSColor colorWithCalibratedWhite:1 alpha:0.4f];
+  NSColor* backgroundColor = [NSColor colorWithCalibratedWhite:1 alpha:0.6f];
   [backgroundColor setFill];
   NSRectFillUsingOperation(dirtyRect, NSCompositeSourceAtop);
   [super drawRect:dirtyRect];
@@ -487,12 +569,6 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
     profile_ = profile;
     controller_ = controller;
 
-    [self setBordered:NO];
-    [self setFont:[NSFont labelFontOfSize:kTitleFontSize]];
-    [self setAlignment:NSCenterTextAlignment];
-    [[self cell] setLineBreakMode:NSLineBreakByTruncatingTail];
-    [self setTitle:profileName];
-
     if (editingAllowed) {
       // 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
@@ -500,6 +576,14 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
       ui::ResourceBundle* rb = &ui::ResourceBundle::GetSharedInstance();
       NSImage* hoverImage = rb->GetNativeImageNamed(
           IDR_ICON_PROFILES_EDIT_HOVER).AsNSImage();
+
+      // In order to center the button title, we need to add a left padding of
+      // the same width as the pencil icon.
+      base::scoped_nsobject<LeftMarginButtonCell> cell(
+          [[LeftMarginButtonCell alloc]
+              initWithLeftMarginSpacing:[hoverImage size].width]);
+      [self setCell:cell.get()];
+
       NSImage* placeholder = [[NSImage alloc] initWithSize:[hoverImage size]];
       [self setDefaultImage:placeholder];
       [self setHoverImage:hoverImage];
@@ -530,6 +614,12 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
       // Hide the textfield until the user clicks on the button.
       [profileNameTextField_ setHidden:YES];
     }
+
+    [self setBordered:NO];
+    [self setFont:[NSFont labelFontOfSize:kTitleFontSize]];
+    [self setAlignment:NSCenterTextAlignment];
+    [[self cell] setLineBreakMode:NSLineBreakByTruncatingTail];
+    [self setTitle:profileName];
   }
   return self;
 }
@@ -545,62 +635,16 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
         postActionPerformed:ProfileMetrics::PROFILE_DESKTOP_MENU_EDIT_NAME];
     [self setTitle:text];
   }
+
+  [[self window] makeFirstResponder:nil];

[groby-ooo-7-16, 2014/06/10 19:53:23]

Are you sure you want the window to be first responder, not the next UI element?

(If the latter, [[self window] selectNextKeyView:self] makes more sense.

[noms (inactive), 2014/06/11 15:09:00]

Giant incoming comment:

If I do what you suggested, what ends up happening (crbug.com/343688) is that
you can end up typing into the textfield even though it's hidden. To repro it,
all you have to do is try to edit the profile name (press the
EditableProfileButton), but without changing the contents, press enter and
commit the changes. Next time you try to press the button a) the text field
won't become visible but b) you can type things, press enter, and they will be
committed.

Disabling the text field fixes it, hence my original change. What also fixes it
is this line, but this adds a blue focus ring around the pencil image of the
NsButton.
[[self window] makeFirstResponder:[profileNameTextField_ nextResponder]];

I can remove the focus ring with [self setFocusRingType:NSFocusRingTypeNone],
but I don't know if that's poor form.

Do you have a preference between the two evils?

On 2014/06/10 19:53:23, groby wrote:

[noms (inactive), 2014/06/11 15:09:00]

Setting the next UI element to be the first responder adds a blue focus border
around the pencil icon, which is awful looking. :(

On 2014/06/10 19:53:23, groby wrote:

[groby-ooo-7-16, 2014/06/11 18:11:21]

You know that you can actually specify the key view loop order, right? ;)

[groby-ooo-7-16, 2014/06/11 18:11:21]

That's odd. Pressing the button should make the text field visible again, if I
read showEditableView correctly.

Also, hidden views really don't get key events. Quoth the doc: " hidden view
disappears from its window and does not receive input events.". So it looks like
there's a draw issue, not an input event issue. 

What happens if you take out _all_ firstResponder/keyview handling, and the
setEnabled code? (That _should_ do the entire trick)

Removing the firstResponder/keyView code has the added benefit that you don't
interfere with focus behavior if the user tabs through instead of pressing
enter.
That is _almost_ the equivalent of [[self window] selectNextKeyView:self].
(Except selectNextKeyView skips hidden fields)

You don't need either, though. setHidden automatically selects the next valid
key view.
That's poor form :)
I'd prefer we figure out why setHidden alone is not enough. (I may well be wrong
in assuming it's enough, but if so, I'd like to know why.)

[noms (inactive), 2014/06/11 19:29:57]

I completely agree it should (which is why the code is like that), but it leads
to the following crazy bug:
1. Start Canary with --new-profile-management
2. Press on the avatar button, which should show you the avatar bubble.
3. Press on the profile name in the bubble. It will go into edit mode and
display the text field. 
Change the name from whatever it is to (say) "boom". Press enter. Name has been
saved, everyone is 
happy, text field is hidden.
4. Repeat step 3, and change the name to "bork". Everyone is still happy.
5. Repeat step 3, only instead of changing the name, press enter as soon as the
text field is displayed.
6. Try to press on the button again. It won't go into editable mode until you
reopen the bubble, 
and you are now a sad panda.
7. It's even worse! Press on the button (even though the text field isn't
visible, i promise you it's enabled), type something and press enter. Your
hidden, typed name should be saved.
8. Halp.
Taking out all the firstResponder stuff makes everything work normally (bug
isn't reproducible), but
then the text doesn't get auto-selected when you go in edit mode (that's why the
firstResponder stuff 
is there in the first place).
If i do [[self window] selectNextKeyView:self], then the bug is still
reproducible :(

[groby-ooo-7-16, 2014/06/12 01:48:11]

So there's something different if you don't change it and only hit enter.
Hmmmm.....
This sounds as if the text field is still active and never relinquished control.
So you'll never get the button delegate to fire.

And finally, my brain switches on - you _so_ don't want to do this in
controlTextDidEndEditing. It's doing things to firstResponder internally, and
calling makeFirstResponder here confuses it. So does calling setHidden. It
probably also interacts in unholy ways with the embedded field editor.
(NSTextView is a pain...)

All you want is change the text if the user is done editing, no? Just set an
action on the profileNameTextField_.

 In that action, extract the text and then only do a setHidden on the text
field.
In showEditableView, only do setHidden:NO, followed by makeFirstResponder. That
should do the trick.
If it's invisible but taking input, it got activated via makeFirstResponder, not
selectNextValidKeyView. (firstResponder can be invisible. KeyViews can't)
The above should halp. I hope. I hate NSTextView.
OK, you _do_ need the makeFirstResponder in showEditableView. But you shouldn't
need to do it in controlTextDidEndEditing. setHidden should jump you to the next
view as if by magic.
You want setNextValidKeyView, sorry. I'm really curious what it thinks the
nextKeyView is, though. Maybe you can log that in the buggy version? (Ignore
this. See above. You don't need to do anything with key views)

[noms (inactive), 2014/06/16 19:17:10]

DONE! \o/

   [profileNameTextField_ setHidden:YES];
-  [profileNameTextField_ resignFirstResponder];
+  [profileNameTextField_ setEnabled:NO];

[groby-ooo-7-16, 2014/06/10 19:53:23]

You shouldn't need to disable if it's hidden.

 }
 
 - (void)showEditableView:(id)sender {
   [profileNameTextField_ setHidden:NO];
-  [profileNameTextField_ becomeFirstResponder];
-}
-
-@end
-
-// Custom button cell that adds a left padding before the button image, and
-// a custom spacing between the button image and title.
-@interface CustomPaddingImageButtonCell : NSButtonCell {
- @private
-  // Padding between the left margin of the button and the cell image.
-  int leftMarginSpacing_;
-  // Spacing between the cell image and title.
-  int imageTitleSpacing_;
-}
-
-- (id)initWithLeftMarginSpacing:(int)leftMarginSpacing
-              imageTitleSpacing:(int)imageTitleSpacing;
-@end
-
-@implementation CustomPaddingImageButtonCell
-- (id)initWithLeftMarginSpacing:(int)leftMarginSpacing
-              imageTitleSpacing:(int)imageTitleSpacing {
-  if ((self = [super init])) {
-    leftMarginSpacing_ = leftMarginSpacing;
-    imageTitleSpacing_ = imageTitleSpacing;
-  }
-  return self;
-}
-
-- (NSRect)drawTitle:(NSAttributedString*)title
-          withFrame:(NSRect)frame
-             inView:(NSView*)controlView {
-  // The title frame origin isn't aware of the left margin spacing added
-  // in -drawImage, so it must be added when drawing the title as well.
-  frame.origin.x += leftMarginSpacing_ + imageTitleSpacing_;
-  frame.size.width -= (imageTitleSpacing_ + leftMarginSpacing_);
-  return [super drawTitle:title withFrame:frame inView:controlView];
-}
-
-- (void)drawImage:(NSImage*)image
-       withFrame:(NSRect)frame
-          inView:(NSView*)controlView {
-  frame.origin.x = leftMarginSpacing_;
-  [super drawImage:image withFrame:frame inView:controlView];
-}
-
-- (NSSize)cellSize {
-  NSSize buttonSize = [super cellSize];
-  buttonSize.width += leftMarginSpacing_ + imageTitleSpacing_;
-  return buttonSize;
+  [profileNameTextField_ setEnabled:YES];
+  [[self window] makeFirstResponder:profileNameTextField_];
 }
 
 @end
@@ -985,8 +1029,9 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
     currentProfileView = [self createGuestProfileView];
 
   // |yOffset| is the next position at which to draw in |container|
-  // coordinates.
-  CGFloat yOffset = 0;
+  // coordinates. Add a pixel offset so that the bottom option buttons don't
+  // overlap the bubble's rounded corners.
+  CGFloat yOffset = 1;
 
   // Option buttons. Only available with the new profile management flag.
   if (switches::IsNewProfileManagement()) {
@@ -996,7 +1041,7 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
     [container addSubview:optionsView];
     rect.origin.y = NSMaxY([optionsView frame]);
 
-    NSBox* separator = [self separatorWithFrame:rect];
+    NSBox* separator = [self horizontalSeparatorWithFrame:rect];
     [container addSubview:separator];
     yOffset = NSMaxY([separator frame]);
   }
@@ -1010,8 +1055,8 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
       [container addSubview:otherProfileView];
       yOffset = NSMaxY([otherProfileView frame]);
 
-      NSBox* separator =
-          [self separatorWithFrame:NSMakeRect(0, yOffset, kFixedMenuWidth, 0)];
+      NSBox* separator = [self horizontalSeparatorWithFrame:NSMakeRect(
+          0, yOffset, kFixedMenuWidth, 0)];
       [container addSubview:separator];
       yOffset = NSMaxY([separator frame]);
     }
@@ -1021,7 +1066,7 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
     [container addSubview:currentProfileAccountsView];
     yOffset = NSMaxY([currentProfileAccountsView frame]);
 
-    NSBox* accountsSeparator = [self separatorWithFrame:
+    NSBox* accountsSeparator = [self horizontalSeparatorWithFrame:
         NSMakeRect(0, yOffset, kFixedMenuWidth, 0)];
     [container addSubview:accountsSeparator];
     yOffset = NSMaxY([accountsSeparator frame]);
@@ -1036,8 +1081,8 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
     yOffset = NSMaxY([disclaimerContainer frame]);
     yOffset += kSmallVerticalSpacing;
 
-    NSBox* separator =
-        [self separatorWithFrame:NSMakeRect(0, yOffset, kFixedMenuWidth, 0)];
+    NSBox* separator = [self horizontalSeparatorWithFrame:NSMakeRect(
+        0, yOffset, kFixedMenuWidth, 0)];
     [container addSubview:separator];
     yOffset = NSMaxY([separator frame]);
   }
@@ -1422,7 +1467,7 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
 
 - (NSView*)createOptionsViewWithRect:(NSRect)rect
                           enableLock:(BOOL)enableLock {
-  int widthOfLockButton = enableLock? 2 * kHorizontalSpacing + 12 : 0;
+  int widthOfLockButton = enableLock ? 2 * kHorizontalSpacing + 14 : 0;
   NSRect viewRect = NSMakeRect(0, 0,
                                rect.size.width - widthOfLockButton,
                                kBlueButtonHeight + kVerticalSpacing);
@@ -1444,6 +1489,10 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
 
   if (enableLock) {
     viewRect.origin.x = NSMaxX([notYouButton frame]);
+    NSBox* separator = [self verticalSeparatorWithFrame:viewRect];
+    [container addSubview:separator];
+
+    viewRect.origin.x = NSMaxX([separator frame]);
     viewRect.size.width = widthOfLockButton;
     NSButton* lockButton =
         [self hoverButtonWithRect:viewRect
@@ -1551,7 +1600,7 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
   yOffset = NSMaxY([webview frame]);
 
   // Adds the title card.
-  NSBox* separator = [self separatorWithFrame:
+  NSBox* separator = [self horizontalSeparatorWithFrame:
       NSMakeRect(0, yOffset, kFixedGaiaViewWidth, 0)];
   [container addSubview:separator];
   yOffset = NSMaxY([separator frame]) + kSmallVerticalSpacing;
@@ -1623,7 +1672,7 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
   yOffset = NSMaxY([contentView frame]) + kVerticalSpacing;
 
   // Adds the title card.
-  NSBox* separator = [self separatorWithFrame:
+  NSBox* separator = [self horizontalSeparatorWithFrame:
       NSMakeRect(0, yOffset, kFixedAccountRemovalViewWidth, 0)];
   [container addSubview:separator];
   yOffset = NSMaxY([separator frame]) + kSmallVerticalSpacing;
@@ -1674,7 +1723,7 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
   yOffset = NSMaxY([contentLabel frame]) + kVerticalSpacing;
 
   // Adds the title card.
-  NSBox* separator = [self separatorWithFrame:
+  NSBox* separator = [self horizontalSeparatorWithFrame:
       NSMakeRect(0, yOffset, kFixedEndPreviewViewWidth, 0)];
   [container addSubview:separator];
   yOffset = NSMaxY([separator frame]) + kSmallVerticalSpacing;