Basic keyboard access for recently_closed menu on NTP. Allows using up and down arrows to navigate,…

chrome/browser/resources/shared/js/cr/ui/menu_button.js

Index: chrome/browser/resources/shared/js/cr/ui/menu_button.js
diff --git a/chrome/browser/resources/shared/js/cr/ui/menu_button.js b/chrome/browser/resources/shared/js/cr/ui/menu_button.js
index 3363babb65b5bd3f2ffa490243c1da3f9aed5566..ff81258728967d5a72272819189e15d5a334e16e 100644
--- a/chrome/browser/resources/shared/js/cr/ui/menu_button.js
+++ b/chrome/browser/resources/shared/js/cr/ui/menu_button.js
@@ -100,7 +100,6 @@ cr.define('cr.ui', function() {
           break;
 
         case 'activate':
-        case 'blur':

[dmazzoni, 2012/09/24 17:49:06]

Just checking, do we want to handle 'blur' still but maybe handle it more
intelligently?

For example, we shouldn't hide the menu if someone moves focus from the menu
button to one of the items. But if someone moves focus to another control on the
page, should we hide the menu then? After some delay?

[aboxhall, 2012/09/24 22:39:38]

This makes sense, but as I describe below, it actually already works.

         case 'resize':
           this.hideMenu();
           break;
@@ -120,6 +119,7 @@ cr.define('cr.ui', function() {
         this.menu.hidden = false;
 
         this.setAttribute('menu-shown', '');
+        this.menu.focusSelectedItem();
 
         // when the menu is shown we steal all keyboard events.
         var doc = this.ownerDocument;
@@ -145,7 +145,7 @@ cr.define('cr.ui', function() {
       this.menu.hidden = true;
 
       this.showingEvents_.removeAll();
-      this.menu.selectedIndex = -1;
+      this.focus();
     },
 
     /**
@@ -180,6 +180,7 @@ cr.define('cr.ui', function() {
           break;
         case 'Esc':
         case 'U+001B': // Maybe this is remote desktop playing a prank?
+        case 'U+0009': // Tab

[dmazzoni, 2012/09/24 17:49:06]

Specifically trapping the tab key worries me a bit.

I wonder if a slightly smarter 'blur' handler would be the least brittle
solution? If the user focuses something on the page outside of the menu button
or menu items, we should close the menu. But just pressing Tab might not catch
everything, and it's a pretty low-level thing to check for.

Things to test:
* Clicking on another element on the page, or on the page background, to blur
the menu
* Jumping to another focusable element with a screen reader.

[aboxhall, 2012/09/24 22:39:38]

So, I did have the same thought while I was working on this, and tried to
implement this by checking the focused item during the blur event, but it wasn't
yet set correctly at that time (it was telling me the focused element was
document.body).

However, when I tested it out as you suggest above (at least, with the mouse; I
didn't actually check with the screen reader as I just used the 'esc' menu close
shortcut), it did in fact work. I will double check when I'm in the office
today, but I'm pretty sure it already behaves sensibly as you describe. To be
honest, I'm not sure _how_ it works, but I could try and figure it out if you
like.

The Tab key is necessary here as the menu traps all keyboard events, so you
can't tab out of the menu by using the Tab key unless we look for it here.

[dmazzoni, 2012/09/24 22:48:56]

OK. If it isn't needed, that's fine. I'm still curious as to why it works -
there must be code somewhere that's triggering it, can you set a breakpoint in
hideMenu and figure out where it's called?
So does it work to just check for Tab (and maybe some other keys) and bail out
of handleKeyDown without doing anything? That makes more sense to me, rather
than hiding the menu bar. Trapping most keys is useful, trapping ALL keys is
sometimes a bad idea.

[aboxhall, 2012/09/25 04:31:30]

Ok, it's triggered by a mouse event at line 78 of this file. So it doesn't
actually close when you navigate away using a screen reader.

I've fixed it by listening for focusin on the document and checking whether the
target is related to the button or menu.
I agree that trapping all keys seems a bit excessive (for one thing it means you
can't easily open a new tab or devtools using a keyboard shortcut while the menu
is open). I changed it to only preventDefault and stopPropagation if the menu
does something with the keyboard event (the menu's handleKeyDown() method
returns a boolean value which seems to be intended for roughly this purpose).

I left the handler for Tab here, because without it Tab navigates between menu
items rather than going to the next menu, and I thought it was better to have
Tab move to the next button (even if the tab order of the buttons is a little
counter-intuitive for now).

           this.hideMenu();
           break;
       }