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

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

Tabbing should navigate through all the visible controls.

I've changed the EditablePhotoButton to actually be a button and
not just an image, so that tabbing and pressing enter actually
works.

BUG=405056
Committed: https://crrev.com/aaacc3030b8b46e4d0463d6d51f24341bf51ddfa
Cr-Commit-Position: refs/heads/master@{#292455}

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

Hiya Rachel!

The new avatar bubble gets focus rings: now at a Rietveld near you! There is a
giant hack so that we don't show any focus rings unless you start tabbing
(because they look weird) which is so hacky I'm pretty sure it's wrong. Cocoa <3

(the unit tests currently fail because of said hack, but I'll wait until your
first review pass before I fix them)

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

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1135: NSButton*
dummyFocusButton =
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-20 18:01:02]

In meeting mania, will address later. Please ping me if it's urgent.

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

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

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:417: // This is
taken care of by the custom drawing code in the cell.
Isn't "in the cell" implicit?

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:421: -
(void)drawWithFrame:(NSRect)frame inView:(NSView *)controlView {
I'm still not entirely clear why the normal focus ring is not enough...

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:425: if ([self
showsFirstResponder]) {
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.

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:438: @interface
TransparentBackgroundImageView : NSImageView
It's technically translucent, not transparent :)

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

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1135: NSButton*
dummyFocusButton =
On 2014/08/20 17:17:38, Monica Dinculescu wrote:
> 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)

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]

(no code, just questions)

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

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:421: -
(void)drawWithFrame:(NSRect)frame inView:(NSView *)controlView {
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:
> I'm still not entirely clear why the normal focus ring is not enough...

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:438: @interface
TransparentBackgroundImageView : NSImageView
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:
> It's technically translucent, not transparent :)
> 
> Also: Do you still need this, or will the images always be the size of the
view
> anyways?

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1135: NSButton*
dummyFocusButton =
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:
> On 2014/08/20 17:17:38, Monica Dinculescu wrote:
> > 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)
> 
> 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 :)

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

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

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:421: -
(void)drawWithFrame:(NSRect)frame inView:(NSView *)controlView {
On 2014/08/21 14:32:05, Monica Dinculescu wrote:
> 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:
> > I'm still not entirely clear why the normal focus ring is not enough...
> 

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?

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:438: @interface
TransparentBackgroundImageView : NSImageView
On 2014/08/21 14:32:05, Monica Dinculescu wrote:
> 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:
> > It's technically translucent, not transparent :)
> > 
> > Also: Do you still need this, or will the images always be the size of the
> view
> > anyways?
> 
So the image has alpha, then? (Because otherwise, it'd obscure the beautiful
white)

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1135: NSButton*
dummyFocusButton =
On 2014/08/21 14:32:05, Monica Dinculescu wrote:
> 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...
> 

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?

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

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

> 
> 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:
> > On 2014/08/20 17:17:38, Monica Dinculescu wrote:
> > > 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)
> > 
> > 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 18:23:28]

Screenshots for all!

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

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:421: -
(void)drawWithFrame:(NSRect)frame inView:(NSView *)controlView {
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:
> On 2014/08/21 14:32:05, Monica Dinculescu wrote:
> > 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:
> > > I'm still not entirely clear why the normal focus ring is not enough...
> > 
> 
> 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?

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:438: @interface
TransparentBackgroundImageView : NSImageView
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:
> On 2014/08/21 14:32:05, Monica Dinculescu wrote:
> > 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:
> > > It's technically translucent, not transparent :)
> > > 
> > > Also: Do you still need this, or will the images always be the size of the
> > view
> > > anyways?
> > 
> So the image has alpha, then? (Because otherwise, it'd obscure the beautiful
> white)

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1135: NSButton*
dummyFocusButton =
> 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?

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]

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

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:421: -
(void)drawWithFrame:(NSRect)frame inView:(NSView *)controlView {
On 2014/08/21 18:23:28, Monica Dinculescu wrote:
> I think this is a HoverImageButton deal. 

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

> 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:
> > On 2014/08/21 14:32:05, Monica Dinculescu wrote:
> > > 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:
> > > > I'm still not entirely clear why the normal focus ring is not enough...
> > > 
> > 
> > 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?
>

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1135: NSButton*
dummyFocusButton =
On 2014/08/21 18:23:28, Monica Dinculescu wrote:
> > 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?
> 
> 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
> 

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]

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

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1135: NSButton*
dummyFocusButton =
> 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.

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:35]

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

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1135: NSButton*
dummyFocusButton =
On 2014/08/21 19:53:39, Monica Dinculescu wrote:
> > 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.
> 
> 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...:/

OK. 

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

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

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

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1135: NSButton*
dummyFocusButton =
On 2014/08/21 20:53:34, groby wrote:
> On 2014/08/21 19:53:39, Monica Dinculescu wrote:
> > > 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.
> > 
> > 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...:/
> 
> OK. 
> 
> So, [[self window] makeFirstResponder:nil];?

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

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

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

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:421: -
(void)drawWithFrame:(NSRect)frame inView:(NSView *)controlView {
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:
> On 2014/08/21 18:23:28, Monica Dinculescu wrote:
> > I think this is a HoverImageButton deal. 
> 
> 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_ :)
> 
> > 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:
> > > On 2014/08/21 14:32:05, Monica Dinculescu wrote:
> > > > 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:
> > > > > I'm still not entirely clear why the normal focus ring is not
enough...
> > > > 
> > > 
> > > 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-25 14:54:43]

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

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:417: // This is
taken care of by the custom drawing code in the cell.
On 2014/08/21 05:06:02, groby wrote:
> Isn't "in the cell" implicit?

Done.

https://codereview.chromium.org/489143002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:425: if ([self
showsFirstResponder]) {
On 2014/08/21 05:06:02, groby wrote:
> 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.

Done.

[groby-ooo-7-16, 2014-08-25 20:56:04]

LGTM w/ nit

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

https://codereview.chromium.org/489143002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:423: NSRect
focusRingRect = NSInsetRect(frame,
nit: style guide wants arguments aligned at parentheses.

I'd move the NSInsetRect call to the next line instead

[noms (inactive), 2014-08-25 21:48:27]

Patchset #4 (id:80001) has been deleted

[noms (inactive), 2014-08-25 21:49:54]

I ended up adding the dummy button as a sibling to the subview containing all
the subviews (rather than as a child of that subview). This way the tests stayed
mostly the same, and didn't get confusing when actually testing the contents of
the subview.

Let me know if you disagree! :)

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

https://codereview.chromium.org/489143002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:423: NSRect
focusRingRect = NSInsetRect(frame,
On 2014/08/25 20:56:04, groby wrote:
> nit: style guide wants arguments aligned at parentheses.
> 
> I'd move the NSInsetRect call to the next line instead

Done.

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

LGTM % KeyViewLoop issue

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

https://codereview.chromium.org/489143002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1144: [[self
window] makeFirstResponder:dummyFocusButton];

Wondering: Did you test you can't cycle back to the dummy window?

I'd think you need 
[[self window] setAutorecalculateKeyViewLoop:NO];
[[self window] recalculateKeyViewLoop];

before you add the dummybutton subview? (And the contentView needs to be set
before that)

[noms (inactive), 2014-08-26 15:08:17]

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

https://codereview.chromium.org/489143002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1144: [[self
window] makeFirstResponder:dummyFocusButton];
Hmm, I can cycle back for sure. Adding the two lines doesn't change that . Also,
my understanding is that calling "setAutorecalculatesKeyViewLoop:NO" means that
calling "recalculateKeyViewLoop" has no effect.


On 2014/08/25 22:06:01, groby wrote:
> 
> Wondering: Did you test you can't cycle back to the dummy window?
> 
> I'd think you need 
> [[self window] setAutorecalculateKeyViewLoop:NO];
> [[self window] recalculateKeyViewLoop];
> 
> before you add the dummybutton subview? (And the contentView needs to be set
> before that)

[groby-ooo-7-16, 2014-08-26 17:49:25]

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

https://codereview.chromium.org/489143002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1144: [[self
window] makeFirstResponder:dummyFocusButton];
On 2014/08/26 15:08:17, Monica Dinculescu wrote:
> Hmm, I can cycle back for sure. Adding the two lines doesn't change that .
Also,
> my understanding is that calling "setAutorecalculatesKeyViewLoop:NO" means
that
> calling "recalculateKeyViewLoop" has no effect.

No, autorecalculate means recalculate is called whenever you add/remove a view.
Problem with calling recalculate is that it only sets a dirty bit, and then
recalculates when you request the next/prev view. (Sigh)

So, what you need to do is _just_ set autorecalculate to NO once you've added
the last view, before the dummy button. Sorry for misleading. At that point, you
shouldn't be able to cycle to the dummy button any more. (Hope and pray)
> 
> 
> On 2014/08/25 22:06:01, groby wrote:
> > 
> > Wondering: Did you test you can't cycle back to the dummy window?
> > 
> > I'd think you need 
> > [[self window] setAutorecalculateKeyViewLoop:NO];
> > [[self window] recalculateKeyViewLoop];
> > 
> > before you add the dummybutton subview? (And the contentView needs to be set
> > before that)
>

[noms (inactive), 2014-08-28 15:23:58]

ヽ(^o^)ノ KeyView loop has been conquered. Apparently you can refuse to be a
KeyView and still be the first responder, because Cocoa.

[groby-ooo-7-16, 2014-08-28 19:39:15]

lgtm

[noms (inactive), 2014-08-28 19:43:43]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-08-28 19:44:51]

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

[commit-bot: I haz the power, 2014-08-28 20:41:13]

Committed patchset #7 (id:160001) as 6db7b1daf607adc28ee100a716b2ceebe0579674

[commit-bot: I haz the power, 2014-09-10 03:02:00]

Patchset 7 (id:??) landed as
https://crrev.com/aaacc3030b8b46e4d0463d6d51f24341bf51ddfa
Cr-Commit-Position: refs/heads/master@{#292455}