ESC button closes Add Supervised User menu

chrome/browser/resources/chromeos/login/header_bar.js

Index: chrome/browser/resources/chromeos/login/header_bar.js
diff --git a/chrome/browser/resources/chromeos/login/header_bar.js b/chrome/browser/resources/chromeos/login/header_bar.js
index 524e5ac4b89f868f49efd850601363c5fd688ddb..e9579ef1529e7a206ba3c8f954483d40489e49a3 100644
--- a/chrome/browser/resources/chromeos/login/header_bar.js
+++ b/chrome/browser/resources/chromeos/login/header_bar.js
@@ -64,6 +64,8 @@ cr.define('login', function() {
           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)
@@ -118,8 +120,10 @@ cr.define('login', function() {
         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;
       }
     },
 
@@ -147,11 +151,6 @@ cr.define('login', function() {
       this.isMoreSettingsActive = false;
     },
 
-    handleAddSupervisedUserClick_: function(e) {
-      chrome.send('showSupervisedUserCreationScreen');
-      e.preventDefault();
-    },
-
     /**
      * Cancel add user button click handler.
      *
@@ -211,6 +210,28 @@ cr.define('login', function() {
     },
 
     /**
+     * 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}