Basic keyboard access for recently_closed menu on NTP (re-work).

Basic keyboard access for recently_closed menu on NTP (re-work).

Re-working of http://codereview.chromium.org/10966027/ which addresses the focus ring issue (http://code.google.com/p/chromium/issues/detail?id=153081) by only focusing elements when the keyboard is being used, and the menu-disappearing issue (http://code.google.com/p/chromium/issues/detail?id=152637) by listening for focus events instead of blur events on context menus.

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

[dmazzoni, 2012-10-03 06:53:32]

Just one question about your solution to the focus ring issue: What if someone
wants to click to open the menu, but then use arrow keys to select an item. Does
that work? Does the focus ring show as soon as they press a key?

[aboxhall, 2012-10-03 06:56:31]

On 2012/10/03 06:53:32, Dominic Mazzoni wrote:
> Just one question about your solution to the focus ring issue: What if someone
> wants to click to open the menu, but then use arrow keys to select an item.
Does
> that work? Does the focus ring show as soon as they press a key?

That works; as soon as they press a key the focus is placed on the 'next' item,
as per the selectNextAvailable() method. For example, click + down arrow will
place focus on the first item in the list (i.e. index 0).

[dmazzoni, 2012-10-03 06:59:20]

Thanks!

lgtm for accessibility.

[Dan Beam, 2012-10-03 08:40:45]

is there an easy way to view the difference between your previous CL and this?

http://codereview.chromium.org/11013021/diff/1/chrome/browser/resources/share...
File chrome/browser/resources/shared/js/cr/ui/menu.js (right):

http://codereview.chromium.org/11013021/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/js/cr/ui/menu.js:233: if (item) {
nit: no curlies

[aboxhall, 2012-10-03 23:32:13]

I don't know of any easy way to view the changes. I can paste a diff-of-diffs in
here if that would help.

http://codereview.chromium.org/11013021/diff/1/chrome/browser/resources/share...
File chrome/browser/resources/shared/js/cr/ui/menu.js (right):

http://codereview.chromium.org/11013021/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/js/cr/ui/menu.js:233: if (item) {
On 2012/10/03 08:40:45, Dan Beam wrote:
> nit: no curlies

Done.

[Dan Beam, 2012-10-04 01:56:41]

I just looked at both CLs file by file to see the diff - as long as it works and
doesn't regress anything else this seems fine.  I'll try it out later and then
LG if it seems OK.

[aboxhall, 2012-10-04 23:20:33]

On 2012/10/04 01:56:41, Dan Beam wrote:
> I just looked at both CLs file by file to see the diff - as long as it works
and
> doesn't regress anything else this seems fine.  I'll try it out later and then
> LG if it seems OK.

Did you get a chance to try this out?

[aboxhall, 2012-11-12 22:20:56]

Ping? I just checked and this still works fine. Is someone on the WebUI side
able to take a look?

[Dan Beam, 2012-11-12 22:55:03]

this seems a lot better with the style changes we talked about so long ago :),
just a couple nits / questions

https://codereview.chromium.org/11013021/diff/15002/chrome/browser/resources/...
File chrome/browser/resources/ntp4/footer_menu.css (right):

https://codereview.chromium.org/11013021/diff/15002/chrome/browser/resources/...
chrome/browser/resources/ntp4/footer_menu.css:83: outline: none;
thank you for these changes, this looks a lot better to me

https://codereview.chromium.org/11013021/diff/15002/chrome/browser/resources/...
File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right):

https://codereview.chromium.org/11013021/diff/15002/chrome/browser/resources/...
chrome/browser/resources/shared/js/cr/ui/menu_button.js:103: case 'focus':
is there a functional difference between "focus on a different element" and
"blur on this element"? can there be no current focus (i.e. no
document.activeElement)?

https://codereview.chromium.org/11013021/diff/15002/chrome/browser/resources/...
chrome/browser/resources/shared/js/cr/ui/menu_button.js:117: * @param {boolean}
shouldSetFocus Whether to set focus on the
nit: is this boolean optional to pass? i.e. is there code you want to keep just
as:

  this.showMenu();

? if so, you should make this

  @param {boolean=} opt_shouldSetFocus

but I'd recommend just making all places pass a boolean isntead.

[aboxhall, 2012-11-13 00:49:52]

https://codereview.chromium.org/11013021/diff/15002/chrome/browser/resources/...
File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right):

https://codereview.chromium.org/11013021/diff/15002/chrome/browser/resources/...
chrome/browser/resources/shared/js/cr/ui/menu_button.js:103: case 'focus':
On 2012/11/12 22:55:03, Dan Beam wrote:
> is there a functional difference between "focus on a different element" and
> "blur on this element"? can there be no current focus (i.e. no
> document.activeElement)?

Yep - at the time of blur, the new document.activeElement hasn't yet been set,
so this doesn't work.

https://codereview.chromium.org/11013021/diff/15002/chrome/browser/resources/...
chrome/browser/resources/shared/js/cr/ui/menu_button.js:117: * @param {boolean}
shouldSetFocus Whether to set focus on the
On 2012/11/12 22:55:03, Dan Beam wrote:
> nit: is this boolean optional to pass? i.e. is there code you want to keep
just
> as:
> 
>   this.showMenu();
> 
> ? if so, you should make this
> 
>   @param {boolean=} opt_shouldSetFocus
> 
> but I'd recommend just making all places pass a boolean isntead.

I agree, and I'm pretty sure I changed all call sites.

[Dan Beam, 2012-11-13 03:32:59]

lgtm

[commit-bot: I haz the power, 2012-11-13 03:35:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aboxhall@chromium.org/11013021/15002

[commit-bot: I haz the power, 2012-11-13 10:33:12]

Change committed as 167360

[Dan Beam, 2013-03-14 21:22:01]

https://chromiumcodereview.appspot.com/11013021/diff/15002/chrome/browser/res...
File chrome/browser/resources/ntp4/recently_closed.js (right):

https://chromiumcodereview.appspot.com/11013021/diff/15002/chrome/browser/res...
chrome/browser/resources/ntp4/recently_closed.js:110: e.stopPropagation();
aboxhall@: ^ why did you add this?