ESC button closes Add Supervised User menu

chrome/browser/resources/chromeos/login/header_bar.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.
 
 /**
  * @fileoverview Login UI header bar implementation.
  */
 
 cr.define('login', function() {
   /**
@@ skipping 46 matching lines @@
       $('guest-user-header-bar-item').addEventListener('click',
           this.handleGuestClick_);
       $('guest-user-button').addEventListener('click',
           this.handleGuestClick_);
       $('sign-out-user-button').addEventListener('click',
           this.handleSignoutClick_);
       $('cancel-multiple-sign-in-button').addEventListener('click',
           this.handleCancelMultipleSignInClick_);
       this.addSupervisedUserMenu.addEventListener('click',
           this.handleAddSupervisedUserClick_.bind(this));
+      this.addSupervisedUserMenu.addEventListener('keydown',
+          this.handleAddSupervisedUserKeyDown_.bind(this));
       if (Oobe.getInstance().displayType == DISPLAY_TYPE.LOGIN ||
           Oobe.getInstance().displayType == DISPLAY_TYPE.OOBE) {
         if (Oobe.getInstance().newKioskUI)
           chrome.send('initializeKioskApps');
         else
           login.AppsMenuButton.decorate($('show-apps-button'));
       }
       this.updateUI_();
     },
 
@@ skipping 33 matching lines @@
      * Whether action box button is in active state.
      * @type {boolean}
      */
     get isMoreSettingsActive() {
       return this.getMoreSettingsMenu.classList.contains('active');
     },
     set isMoreSettingsActive(active) {
       if (active == this.isMoreSettingsActive)
         return;
       if (active) {
         this.getMoreSettingsMenu.classList.add('active');

[xiyuan, 2017/01/09 21:55:43]

nit: maybe change this and line 125 to

  this.getMoreSettingsMenu.classList.toggle('active', active);

[Greg Levin, 2017/01/09 22:34:40]

Done.

+        $('more-settings-button').tabIndex = -1;

[xiyuan, 2017/01/09 21:55:43]

Is changing tabIndex necessary?

If the intention is to prevent user tab away, it would not work since there
would other elements that can get focus in addition to "more-settings-button".

[Greg Levin, 2017/01/09 22:34:40]

The intent is only to remove the [...] button from the TAB order when [Add
supervised user] is visible.  The only reason to leave it in the TAB order in
this case is to allow a user to focus and activate it in order to close [Add
supervised user] (as is currently the case).  This is of dubious utility to
begin with, and with this CL, there's a more direct way of closing [Add
supervised user].
(I don't know that there's ever any reason to close [Add supervised user], which
is why this issue is a P3; I just figured ESC should close the menu as a matter
of general UI consistency.)

Finally, as per https://codereview.chromium.org/2610193002/ and the comments in
that issue, we're moving [Add supervised user] so that it completely covers
[...].  So the user couldn't see that [...] was focused in this case.

[xiyuan, 2017/01/09 23:17:43]

I understand what we are trying to do and agree that adding ESC is a UX
improvement. Just not sure whether it is necessary to take the [...] button from
the TAB order. I don't see much benefit of doing it. 

Anyway, it is a trivial request and I would not push on it. :p

       } else {
         this.getMoreSettingsMenu.classList.remove('active');
+        $('more-settings-button').tabIndex = 0;
       }
     },
 
 
     /**
      * Add user button click handler.
      *
      * @private
      */
     handleAddUserClick_: function(e) {
       Oobe.showSigninUI();
       // Prevent further propagation of click event. Otherwise, the click event
       // handler of document object will set wallpaper to user's wallpaper when
       // there is only one existing user. See http://crbug.com/166477
       e.stopPropagation();
     },
 
     handleMoreSettingsClick_: function(e) {
       this.isMoreSettingsActive = !this.isMoreSettingsActive;
       this.addSupervisedUserMenu.focus();
       e.stopPropagation();
     },
 
     handleClick_: function(e) {
       this.isMoreSettingsActive = false;
     },
 
-    handleAddSupervisedUserClick_: function(e) {
-      chrome.send('showSupervisedUserCreationScreen');
-      e.preventDefault();
-    },
-
     /**
      * Cancel add user button click handler.
      *
      * @private
      */
     handleCancelAddUserClick_: function(e) {
       // Let screen handle cancel itself if that is capable of doing so.
       if (Oobe.getInstance().currentScreen &&
           Oobe.getInstance().currentScreen.cancel) {
         Oobe.getInstance().currentScreen.cancel();
@@ skipping 39 matching lines @@
      * Cancel user adding button handler.
      *
      * @private
      */
     handleCancelMultipleSignInClick_: function(e) {
       chrome.send('cancelUserAdding');
       e.stopPropagation();
     },
 
     /**
+     * Add supervised user button handler.
+     *
+     * @private
+     */
+    handleAddSupervisedUserClick_: function(e) {
+      chrome.send('showSupervisedUserCreationScreen');
+      e.preventDefault();
+    },
+
+    /**
+     * Add supervised user key handler, ESC closes menu.
+     *
+     * @private
+     */
+    handleAddSupervisedUserKeyDown_: function(e) {
+      if (e.key == 'Escape' && this.isMoreSettingsActive) {
+        this.isMoreSettingsActive = false;
+        $('more-settings-button').focus();
+      }
+    },
+
+    /**
      * If true then "Browse as Guest" button is shown.
      *
      * @type {boolean}
      */
     set showGuestButton(value) {
       this.showGuest_ = value;
       this.updateUI_();
     },
 
     set showCreateSupervisedButton(value) {
@@ skipping 170 matching lines @@
    * Convenience wrapper of animateIn.
    */
   HeaderBar.animateIn = function(fast, callback) {
     $('login-header-bar').animateIn(fast, callback);
   };
 
   return {
     HeaderBar: HeaderBar
   };
 });