[Mac] Misc fixes for the new avatar bubble

[Mac] Misc fixes for the new avatar bubble

In this CL:
- crbug.com/343688: when editing the profile name but not changing it, the text field would
freeze. this is because it would remain enabled, but hidden, and steal the button click
(because it used to be a first responder)
- add a separator between the "not you" and "lock" bottom option buttons, to
match the mocks. I added this as a vertical separator to the BaseBubbleController itself, 
so that other classes can use it in the future.
- center the profile name correctly. Because of the pencil icon, the name would be shifted
slightly to the left
- the "change photo" overlay should be more opaque
- the bubble bottom corners were overlapped by the option buttons, and didn't appear rounded.

BUG=343688
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278840

[noms (inactive), 2014-06-10 18:46:29]

Welcome back! :)

Here's some minor UI fixes that came up from a recent UI review. Please take a
look. Thanks!

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

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:335: // Custom
button cell that adds a left padding to a button.
If you want to add padding, -drawInteriorWithFrame: is a better choice. As it
stands, drawImage for this button would ignore the padding for example.

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:373: @interface
CustomPaddingImageButtonCell : LeftMarginButtonCell {
You might want to _just_ keep CustomPaddingImageButtonCell.

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

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:639: [[self
window] makeFirstResponder:nil];
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.

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:641:
[profileNameTextField_ setEnabled:NO];
You shouldn't need to disable if it's hidden.

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

No code, just questions, answers, and the occasional fist shaking at Cocoa.

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:335: // Custom
button cell that adds a left padding to a button.
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:
> If you want to add padding, -drawInteriorWithFrame: is a better choice. As it
> stands, drawImage for this button would ignore the padding for example.

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:373: @interface
CustomPaddingImageButtonCell : LeftMarginButtonCell {
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:
> You might want to _just_ keep CustomPaddingImageButtonCell.
> 
> LeftMarginImageButtonCell is just this with an |imageTitleSpacing_| of 0, no?

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:639: [[self
window] makeFirstResponder:nil];
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:
> 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.

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:639: [[self
window] makeFirstResponder:nil];
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:
> 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.

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

Some answers, but mostly, questions  for your questions ;)

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:335: // Custom
button cell that adds a left padding to a button.
On 2014/06/11 15:09:00, Monica Dinculescu wrote:
> 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:
> > If you want to add padding, -drawInteriorWithFrame: is a better choice. As
it
> > stands, drawImage for this button would ignore the padding for example.
> 

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 :)

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:373: @interface
CustomPaddingImageButtonCell : LeftMarginButtonCell {
On 2014/06/11 15:09:00, Monica Dinculescu wrote:
> 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:
> > You might want to _just_ keep CustomPaddingImageButtonCell.
> > 
> > LeftMarginImageButtonCell is just this with an |imageTitleSpacing_| of 0,
no?
> 

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?

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:639: [[self
window] makeFirstResponder:nil];
On 2014/06/11 15:09:00, Monica Dinculescu wrote:
> 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:
> > 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.
> 

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

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:639: [[self
window] makeFirstResponder:nil];
On 2014/06/11 15:09:00, Monica Dinculescu wrote:
> 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.

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.

> 
> 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]];

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.


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

That's poor form :)
> 
> Do you have a preference between the two evils?

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.)

> 
> On 2014/06/10 19:53:23, groby wrote:
> > 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 19:29:58]

Moar answers, but now with diagrams! :)

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:335: // Custom
button cell that adds a left padding to a button.
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:
> On 2014/06/11 15:09:00, Monica Dinculescu wrote:
> > 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:
> > > If you want to add padding, -drawInteriorWithFrame: is a better choice. As
> it
> > > stands, drawImage for this button would ignore the padding for example.
> > 
> 
> 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 :)
>

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:373: @interface
CustomPaddingImageButtonCell : LeftMarginButtonCell {
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:
> On 2014/06/11 15:09:00, Monica Dinculescu wrote:
> > 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:
> > > You might want to _just_ keep CustomPaddingImageButtonCell.
> > > 
> > > LeftMarginImageButtonCell is just this with an |imageTitleSpacing_| of 0,
> no?
> > 
> 
> 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?

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:639: [[self
window] makeFirstResponder:nil];
On 2014/06/11 18:11:21, groby wrote:
> On 2014/06/11 15:09:00, Monica Dinculescu wrote:
> > 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.
> 
> That's odd. Pressing the button should make the text field visible again, if I
> read showEditableView correctly.

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.

> 
> 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)

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).

> 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.


> > 
> > 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]];
> 
> That is _almost_ the equivalent of [[self window] selectNextKeyView:self].
> (Except selectNextKeyView skips hidden fields)

If i do [[self window] selectNextKeyView:self], then the bug is still
reproducible :(

> You don't need either, though. setHidden automatically selects the next valid
> key view.
> 
> 
> > 
> > I can remove the focus ring with [self
setFocusRingType:NSFocusRingTypeNone],
> > but I don't know if that's poor form.
> 
> That's poor form :)
> > 
> > Do you have a preference between the two evils?
> 
> 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.)
> 
> > 
> > On 2014/06/10 19:53:23, groby wrote:
> > > 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.
> > 
>

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

Gah. Head asplode.

I hope this answers your questions. If it doesn't, maybe you should split this
into separate CLs for draw/text view/the rest, so we can go through them
separately?

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:335: // Custom
button cell that adds a left padding to a button.
On 2014/06/11 19:29:57, Monica Dinculescu wrote:
> 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.

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];
}
  
> 
> On 2014/06/11 18:11:21, groby wrote:
> > On 2014/06/11 15:09:00, Monica Dinculescu wrote:
> > > 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:
> > > > If you want to add padding, -drawInteriorWithFrame: is a better choice.
As
> > it
> > > > stands, drawImage for this button would ignore the padding for example.
> > > 
> > 
> > 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 :)
> > 
>

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:373: @interface
CustomPaddingImageButtonCell : LeftMarginButtonCell {
On 2014/06/11 15:09:00, Monica Dinculescu wrote:
> 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?

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 ;)

> 
> On 2014/06/10 19:53:23, groby wrote:
> > You might want to _just_ keep CustomPaddingImageButtonCell.
> > 
> > LeftMarginImageButtonCell is just this with an |imageTitleSpacing_| of 0,
no?
>

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:639: [[self
window] makeFirstResponder:nil];
On 2014/06/11 19:29:57, Monica Dinculescu wrote:
> On 2014/06/11 18:11:21, groby wrote:
> > On 2014/06/11 15:09:00, Monica Dinculescu wrote:
> > > 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.
> > 
> > That's odd. Pressing the button should make the text field visible again, if
I
> > read showEditableView correctly.
> 
> 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.

So there's something different if you don't change it and only hit enter.
Hmmmm.....

> 6. Try to press on the button again. It won't go into editable mode until you
> reopen the bubble, 

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.


> 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), 

If it's invisible but taking input, it got activated via makeFirstResponder, not
selectNextValidKeyView. (firstResponder can be invisible. KeyViews can't)

> type something and press enter. Your
> hidden, typed name should be saved.
> 8. Halp.

The above should halp. I hope. I hate NSTextView.

> 
> > 
> > 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)
> 
> 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).

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.


> 
> > 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.
> 
> 
> > > 
> > > 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]];
> > 
> > That is _almost_ the equivalent of [[self window] selectNextKeyView:self].
> > (Except selectNextKeyView skips hidden fields)
> 
> If i do [[self window] selectNextKeyView:self], then the bug is still
> reproducible :(

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)

> 
> > You don't need either, though. setHidden automatically selects the next
valid
> > key view.
> > 
> > 
> > > 
> > > I can remove the focus ring with [self
> setFocusRingType:NSFocusRingTypeNone],
> > > but I don't know if that's poor form.
> > 
> > That's poor form :)
> > > 
> > > Do you have a preference between the two evils?
> > 
> > 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.)
> > 
> > > 
> > > On 2014/06/10 19:53:23, groby wrote:
> > > > 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.
> > > 
> > 
>

[groby-ooo-7-16, 2014-06-12 01:49:05]

OK, since it's buried in tons of comments, here's the relevant text on
controlTextDidEndEditing:

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.

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

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:335: // Custom
button cell that adds a left padding to a button.
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?

https://codereview.chromium.org/323143004/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:639: [[self
window] makeFirstResponder:nil];
DONE! \o/

[groby-ooo-7-16, 2014-06-16 20:49:38]

lgtm w/ nit

https://codereview.chromium.org/323143004/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/323143004/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:362:
frame.origin.x += leftMarginSpacing_;
nit: Might want to use NSDivideRect for simplicity

https://codereview.chromium.org/323143004/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:368:
frame.origin.x += imageTitleSpacing_;
ditto

[noms (inactive), 2014-06-17 14:11:19]

Thanks for putting up with this review! I will try to not repeat this experience
for at least a month :)

https://codereview.chromium.org/323143004/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/323143004/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:362:
frame.origin.x += leftMarginSpacing_;
NSDivideRect is magical!!! <3 
Done.
On 2014/06/16 20:49:38, groby wrote:
> nit: Might want to use NSDivideRect for simplicity

https://codereview.chromium.org/323143004/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:368:
frame.origin.x += imageTitleSpacing_;
On 2014/06/16 20:49:38, groby wrote:
> ditto

Done.

[noms (inactive), 2014-06-17 14:11:29]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-06-17 14:12:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noms@chromium.org/323143004/100001

[commit-bot: I haz the power, 2014-06-17 16:46:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-17 16:46:48]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[noms (inactive), 2014-06-17 16:48:40]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-06-17 16:50:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noms@chromium.org/323143004/100001

[groby-ooo-7-16, 2014-06-17 18:24:34]

On 2014/06/17 14:11:19, Monica Dinculescu wrote:
> Thanks for putting up with this review! I will try to not repeat this
experience
> for at least a month :)

No worries. It made for an entertaining day or two :)

[commit-bot: I haz the power, 2014-06-17 19:35:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-17 19:35:48]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[noms (inactive), 2014-06-18 13:58:38]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-06-18 13:59:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noms@chromium.org/323143004/100001

[commit-bot: I haz the power, 2014-06-18 16:59:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-18 16:59:28]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[noms (inactive), 2014-06-18 17:56:44]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-06-18 17:58:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noms@chromium.org/323143004/100001

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

If this keeps failing on the win builders: As long as the mac builders pass, I
think you'll be OK to add NOTRY=true to the CL to make this land. There's
nothing non-mac in here :)

[groby-ooo-7-16, 2014-06-18 18:02:30]

NM. The build log finally opened, and it's a patch failure. I don't know why
that'd be only on win, but you might want to rebase to HEAD and try again.

[noms (inactive), 2014-06-18 18:04:19]

I think the bot is sick: crbug.com/383353. 

On 2014/06/18 18:02:30, groby wrote:
> NM. The build log finally opened, and it's a patch failure. I don't know why
> that'd be only on win, but you might want to rebase to HEAD and try again.

[commit-bot: I haz the power, 2014-06-18 23:28:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-18 23:28:55]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[noms (inactive), 2014-06-19 13:46:13]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-06-19 13:47:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noms@chromium.org/323143004/100001

[commit-bot: I haz the power, 2014-06-19 16:56:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-19 16:56:01]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[noms (inactive), 2014-06-19 17:14:26]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-06-19 17:16:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noms@chromium.org/323143004/120001

[commit-bot: I haz the power, 2014-06-19 22:18:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-19 22:18:24]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/19171)

[noms (inactive), 2014-06-20 13:56:28]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-06-20 13:58:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noms@chromium.org/323143004/140001

[commit-bot: I haz the power, 2014-06-20 17:17:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[noms (inactive), 2014-06-20 17:48:36]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-06-20 17:50:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noms@chromium.org/323143004/160001

[commit-bot: I haz the power, 2014-06-20 22:36:19]

Change committed as 278840