Linux GTK: Support keyboard navigation in avatar menu.

Linux GTK: Support keyboard navigation in avatar menu.

BUG=101399
TEST= Navigate inside the avatar menu using keyboard.
Tab key and down arrow key to navigate forward.
Shift-Tab key and up arrow key to navigate backward.
Enter key to open/edit a profile.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113577

[SanjoyPal, 2011-12-07 11:08:53]

PTAL. Thanks

[sail, 2011-12-07 16:36:13]

Did you change the look of the avatar menu? If so can you state that in the
change description and add screenshots? Thanks!

[sail, 2011-12-07 16:42:26]

I didn't see any code to change the look of the UI. I'm not sure this CL will
fix bug 105998.

Also, in your change description could you add the list of keyboard shortcuts
that you now support. For example, up / down arrow, enter, etc...

http://codereview.chromium.org/8834010/diff/1/chrome/browser/ui/gtk/avatar_me...
File chrome/browser/ui/gtk/avatar_menu_item_gtk.cc (right):

http://codereview.chromium.org/8834010/diff/1/chrome/browser/ui/gtk/avatar_me...
chrome/browser/ui/gtk/avatar_menu_item_gtk.cc:112: GdkEventFocus* event) {
indentation looks wrong here

http://codereview.chromium.org/8834010/diff/1/chrome/browser/ui/gtk/avatar_me...
chrome/browser/ui/gtk/avatar_menu_item_gtk.cc:114: if (item_.active) {
can you refactor this with :OnProfileEnter

http://codereview.chromium.org/8834010/diff/1/chrome/browser/ui/gtk/avatar_me...
chrome/browser/ui/gtk/avatar_menu_item_gtk.cc:125: if (item_.active) {
can you refactor this with onprofileleave

[Elliot Glaysher, 2011-12-07 21:15:39]

Patching this in, there's a visual difference between the highlight of the
individual items and the link. You should fix that.

[SanjoyPal, 2011-12-08 06:38:54]

I removed the BUG=105998 from CL description, will submit a separate CL to fix
that. Uploaded Patch set 2 after addressing other review comments.

http://codereview.chromium.org/8834010/diff/1/chrome/browser/ui/gtk/avatar_me...
File chrome/browser/ui/gtk/avatar_menu_item_gtk.cc (right):

http://codereview.chromium.org/8834010/diff/1/chrome/browser/ui/gtk/avatar_me...
chrome/browser/ui/gtk/avatar_menu_item_gtk.cc:112: GdkEventFocus* event) {
On 2011/12/07 16:42:26, sail wrote:
> indentation looks wrong here

Done.

http://codereview.chromium.org/8834010/diff/1/chrome/browser/ui/gtk/avatar_me...
chrome/browser/ui/gtk/avatar_menu_item_gtk.cc:114: if (item_.active) {
On 2011/12/07 16:42:26, sail wrote:
> can you refactor this with :OnProfileEnter

Done.

http://codereview.chromium.org/8834010/diff/1/chrome/browser/ui/gtk/avatar_me...
chrome/browser/ui/gtk/avatar_menu_item_gtk.cc:125: if (item_.active) {
On 2011/12/07 16:42:26, sail wrote:
> can you refactor this with onprofileleave

Done.

[sail, 2011-12-08 06:43:37]

lgtm

[commit-bot: I haz the power, 2011-12-08 06:49:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncj674@motorola.com/8834010/10001

[commit-bot: I haz the power, 2011-12-08 08:51:53]

Change committed as 113577

[sail, 2011-12-08 17:46:59]

On 2011/12/08 06:38:54, SanjoyPal wrote:
> I removed the BUG=105998 from CL description, will submit a separate CL to fix
> that. Uploaded Patch set 2 after addressing other review comments.

Were you able to address erg's comments about the visual difference between
highlight of individual items vs link? If not could you send out a 2nd CL to fix
that or file a bug?

Thanks!

[SanjoyPal, 2011-12-09 07:02:38]

On 2011/12/08 17:46:59, sail wrote:
> On 2011/12/08 06:38:54, SanjoyPal wrote:
> > I removed the BUG=105998 from CL description, will submit a separate CL to
fix
> > that. Uploaded Patch set 2 after addressing other review comments.
> 
> Were you able to address erg's comments about the visual difference between
> highlight of individual items vs link? If not could you send out a 2nd CL to
fix
> that or file a bug?
> 
> Thanks!

Elliot, Sail,
Actually i dint understand erg's comment. Are you talking about the focus-ring
around the "New user" link? Should I draw a focus-ring for individual items as
it is in windows?

[Elliot Glaysher, 2011-12-09 18:33:21]

On 2011/12/09 07:02:38, SanjoyPal wrote:
> On 2011/12/08 17:46:59, sail wrote:
> > On 2011/12/08 06:38:54, SanjoyPal wrote:
> > > I removed the BUG=105998 from CL description, will submit a separate CL to
> fix
> > > that. Uploaded Patch set 2 after addressing other review comments.
> > 
> > Were you able to address erg's comments about the visual difference between
> > highlight of individual items vs link? If not could you send out a 2nd CL to
> fix
> > that or file a bug?
> > 
> > Thanks!
> 
> Elliot, Sail,
> Actually i dint understand erg's comment. Are you talking about the focus-ring
> around the "New user" link? Should I draw a focus-ring for individual items as
> it is in windows?

Correct. You were drawing a highlight when cycling through with the keyboard
instead of the focus ring.