ESC button closes Add Supervised User menu

ESC button closes Add Supervised User menu

BUG=525651
TEST=On login screen, press More options [...] button, TAB focus onto
Add supervised user button/menu, pres ESC.  Menu should close.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2609393003
Cr-Commit-Position: refs/heads/master@{#448379}
Committed: https://chromium.googlesource.com/chromium/src/+/3d0dd655bbf5a976d0df8a7c3c9c15253c2d32f6

[Greg Levin, 2017-01-04 16:38:26]

Description was changed from

==========
ESC button closes Add Supervised User menu

BUG=525651
TEST=On login screen, press More options [...] button, TAB focus onto
Add supervised user button/menu, pres ESC.  Menu should close.
==========

to

==========
ESC button closes Add Supervised User menu

BUG=525651
TEST=On login screen, press More options [...] button, TAB focus onto
Add supervised user button/menu, pres ESC.  Menu should close.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Greg Levin, 2017-01-04 16:39:02]

The CQ bit was checked by glevin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 16:39:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 17:42:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 17:42:57]

Dry run: This issue passed the CQ dry run.

[Greg Levin, 2017-01-04 21:35:36]

glevin@chromium.org changed reviewers:
+ alemate@chromium.org, estade@chromium.org

[Greg Levin, 2017-01-04 21:35:37]

Please have a look!

NOTE: This CL also removes the [...] button as a TAB stop while the menu is
open, and places focus on the [...] button when the menu gets closed.  Also,
while not actually dependent, this CL goes along with
  https://codereview.chromium.org/2610193002/
which adds a visible focus style to the [Add supervised user] button.

[Evan Stade, 2017-01-04 23:47:48]

lgtm

[Greg Levin, 2017-01-09 21:44:09]

glevin@chromium.org changed reviewers:
+ xiyuan@chromium.org
- alemate@chromium.org

[Greg Levin, 2017-01-09 21:44:09]

-alemate (seems unavailable)
+xiyuan (please have a look!)

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

https://codereview.chromium.org/2609393003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/header_bar.js (right):

https://codereview.chromium.org/2609393003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/header_bar.js:122:
this.getMoreSettingsMenu.classList.add('active');
nit: maybe change this and line 125 to

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

https://codereview.chromium.org/2609393003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/header_bar.js:123:
$('more-settings-button').tabIndex = -1;
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]

https://codereview.chromium.org/2609393003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/header_bar.js (right):

https://codereview.chromium.org/2609393003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/header_bar.js:122:
this.getMoreSettingsMenu.classList.add('active');
On 2017/01/09 21:55:43, xiyuan wrote:
> nit: maybe change this and line 125 to
> 
>   this.getMoreSettingsMenu.classList.toggle('active', active);

Done.

https://codereview.chromium.org/2609393003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/header_bar.js:123:
$('more-settings-button').tabIndex = -1;
On 2017/01/09 21:55:43, xiyuan wrote:
> 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".

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]

lgtm

https://codereview.chromium.org/2609393003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/header_bar.js (right):

https://codereview.chromium.org/2609393003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/header_bar.js:123:
$('more-settings-button').tabIndex = -1;
On 2017/01/09 22:34:40, Greg Levin wrote:
> On 2017/01/09 21:55:43, xiyuan wrote:
> > 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".
> 
> 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.

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

[Greg Levin, 2017-02-06 19:31:51]

The CQ bit was checked by glevin@chromium.org

[Greg Levin, 2017-02-06 19:31:51]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2609393003/#ps20001
(title: "Simplify if/else block, as per review")

[commit-bot: I haz the power, 2017-02-06 19:32:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 20:39:26]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486409511349600,
"parent_rev": "dbaa71fb0ffc6eb3b122e98c69f1e63ff4f77639", "commit_rev":
"3d0dd655bbf5a976d0df8a7c3c9c15253c2d32f6"}

[commit-bot: I haz the power, 2017-02-06 20:40:01]

Description was changed from

==========
ESC button closes Add Supervised User menu

BUG=525651
TEST=On login screen, press More options [...] button, TAB focus onto
Add supervised user button/menu, pres ESC.  Menu should close.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
ESC button closes Add Supervised User menu

BUG=525651
TEST=On login screen, press More options [...] button, TAB focus onto
Add supervised user button/menu, pres ESC.  Menu should close.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2609393003
Cr-Commit-Position: refs/heads/master@{#448379}
Committed:
https://chromium.googlesource.com/chromium/src/+/3d0dd655bbf5a976d0df8a7c3c9c...
==========

[commit-bot: I haz the power, 2017-02-06 20:40:02]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/3d0dd655bbf5a976d0df8a7c3c9c...