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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 cr.define('cr.ui', function() {
   /** @const */
   var Menu = cr.ui.Menu;
   /** @const */
   var positionPopupAroundElement = cr.ui.positionPopupAroundElement;
 
@@ skipping 82 matching lines @@
           this.handleKeyDown(e);
           // If the menu is visible we let it handle all the keyboard events.
           if (this.isMenuShown() && e.currentTarget == this.ownerDocument) {
             this.menu.handleKeyDown(e);
             e.preventDefault();
             e.stopPropagation();
           }
           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;
       }
     },
 
     /**
      * Shows the menu.
      */
     showMenu: function() {
       this.hideMenu();
 
       var event = document.createEvent('UIEvents');
       event.initUIEvent('menushow', true, true, window, null);
 
       if (this.dispatchEvent(event)) {
         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;
         var win = doc.defaultView;
         this.showingEvents_.add(doc, 'keydown', this, true);
         this.showingEvents_.add(doc, 'mousedown', this, true);
         this.showingEvents_.add(doc, 'blur', this, true);
         this.showingEvents_.add(win, 'resize', this);
         this.showingEvents_.add(this.menu, 'activate', this);
         this.positionMenu_();
       }
     },
 
     /**
      * Hides the menu. If your menu can go out of scope, make sure to call this
      * first.
      */
     hideMenu: function() {
       if (!this.isMenuShown())
         return;
 
       this.removeAttribute('menu-shown');
       this.menu.hidden = true;
 
       this.showingEvents_.removeAll();
-      this.menu.selectedIndex = -1;
+      this.focus();
     },
 
     /**
      * Whether the menu is shown.
      */
     isMenuShown: function() {
       return this.hasAttribute('menu-shown');
     },
 
     /**
@@ skipping 14 matching lines @@
         case 'Down':
         case 'Up':
         case 'Enter':
         case 'U+0020': // Space
           if (!this.isMenuShown())
             this.showMenu();
           e.preventDefault();
           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;
       }
     }
   };
 
   /**
    * Helper for styling a menu button with a drop-down arrow indicator.
    * Creates a new 2D canvas context and draws a downward-facing arrow into it.
    * @param {string} canvasName The name of the canvas. The canvas can be
@@ skipping 37 matching lines @@
         'drop-down-arrow-hover', ARROW_WIDTH, ARROW_HEIGHT, hoverColor);
     createDropDownArrowCanvas(
         'drop-down-arrow-active', ARROW_WIDTH, ARROW_HEIGHT, activeColor);
   };
 
   // Export
   return {
     MenuButton: MenuButton
   };
 });