Commit 2bffd11a authored by Jordy Greenblatt's avatar Jordy Greenblatt Committed by Commit Bot

Settings: When saved password check closes, focus on export dialog

The bug in refers to the fact that before this change, pressing 'ENTER'
with the exportPasswords dialog up would cause a password check
initially, however if the user cancelled the password check prompt,
they will again see the exportPasswords but this time 'ENTER' does
nothing.

hcarmona@ pointed out that this is part of a broader accessibility
issue on this page, namely that the password prompts' closing doesn't
put focus back on the element that triggered it to open. This can lead
to a confusing and difficult UX for users who are using tab to navigate
through the interactive elements on the page.

I ran through a wide range of use cases affected by this change and
recorded them, mostly captured in [1]. Note that the transition at
0:29-0:30 in [1] resulted from pressing enter at the
passwords-export-dialog.

I recorded successful password entries ([2], [3], [4]) separately
because if they are done in close succession, the auth token is
reused and the password prompt is skipped, which negates defeats the
purpose of the recording.

[1] https://drive.google.com/file/d/13naHaZSf7hG3pC09XaO7WW4bmDmmhYW6/view
[2] Successful password prompt from passwords-export-dialog:
https://drive.google.com/file/d/1LLN0dJ8NY54zSTOoZL6F9AcPSFHF0EFy/view
[3] Successful password prompt from password-list-item:
https://drive.google.com/file/d/1hIDOFFrEMRClFlT0VfVTXtyw-YweIfsp/view
[4] Successful password entry from password-edit-dialog
https://drive.google.com/file/d/1XJaGTrmDm4dOnc6a1tQyKnNxaHWU_NzR/view

Bug: 989449
Change-Id: Ic5a363d217cb70e76aa6acb5104ec2fea3a00b4c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1731733
Commit-Queue: Jordy Greenblatt <jordynass@chromium.org>
Reviewed-by: default avatarHector Carmona <hcarmona@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683788}
parent 6c0ed629
...@@ -156,11 +156,14 @@ Polymer({ ...@@ -156,11 +156,14 @@ Polymer({
}, },
/** /**
* The element to return focus to, when the currently active dialog is * A stack of the elements that triggered dialog to open and should therefore
* closed. * receive focus when that dialog is closed. The bottom of the stack is the
* @private {?HTMLElement} * element that triggered the earliest open dialog and top of the stack is the
* element that triggered the most recent (i.e. active) dialog. If no dialog
* is open, the stack is empty.
* @private {!Array<Element>}
*/ */
activeDialogAnchor_: null, activeDialogAnchorStack_: [],
/** /**
* @type {PasswordManagerProxy} * @type {PasswordManagerProxy}
...@@ -210,7 +213,7 @@ Polymer({ ...@@ -210,7 +213,7 @@ Polymer({
this.tokenRequestManager_ = new settings.BlockingRequestManager(); this.tokenRequestManager_ = new settings.BlockingRequestManager();
} else { } else {
this.tokenRequestManager_ = new settings.BlockingRequestManager( this.tokenRequestManager_ = new settings.BlockingRequestManager(
() => this.showPasswordPromptDialog_ = true); this.openPasswordPromptDialog_.bind(this));
} }
// </if> // </if>
...@@ -273,6 +276,12 @@ Polymer({ ...@@ -273,6 +276,12 @@ Polymer({
onPasswordPromptClosed_: function() { onPasswordPromptClosed_: function() {
this.showPasswordPromptDialog_ = false; this.showPasswordPromptDialog_ = false;
cr.ui.focusWithoutInk(assert(this.activeDialogAnchorStack_.pop()));
},
openPasswordPromptDialog_: function() {
this.activeDialogAnchorStack_.push(getDeepActiveElement());
this.showPasswordPromptDialog_ = true;
}, },
// </if> // </if>
...@@ -290,8 +299,7 @@ Polymer({ ...@@ -290,8 +299,7 @@ Polymer({
/** @private */ /** @private */
onPasswordEditDialogClosed_: function() { onPasswordEditDialogClosed_: function() {
this.showPasswordEditDialog_ = false; this.showPasswordEditDialog_ = false;
cr.ui.focusWithoutInk(assert(this.activeDialogAnchor_)); cr.ui.focusWithoutInk(assert(this.activeDialogAnchorStack_.pop()));
this.activeDialogAnchor_ = null;
// Trigger a re-evaluation of the activePassword as the visibility state of // Trigger a re-evaluation of the activePassword as the visibility state of
// the password might have changed. // the password might have changed.
...@@ -376,7 +384,7 @@ Polymer({ ...@@ -376,7 +384,7 @@ Polymer({
this.activePassword = this.activePassword =
/** @type {!PasswordListItemElement} */ (event.detail.listItem); /** @type {!PasswordListItemElement} */ (event.detail.listItem);
menu.showAt(target); menu.showAt(target);
this.activeDialogAnchor_ = target; this.activeDialogAnchorStack_.push(target);
}, },
/** /**
...@@ -389,7 +397,7 @@ Polymer({ ...@@ -389,7 +397,7 @@ Polymer({
/** @type {!HTMLElement} */ (this.$$('#exportImportMenuButton')); /** @type {!HTMLElement} */ (this.$$('#exportImportMenuButton'));
menu.showAt(target); menu.showAt(target);
this.activeDialogAnchor_ = target; this.activeDialogAnchorStack_.push(target);
}, },
/** /**
...@@ -413,8 +421,7 @@ Polymer({ ...@@ -413,8 +421,7 @@ Polymer({
/** @private */ /** @private */
onPasswordsExportDialogClosed_: function() { onPasswordsExportDialogClosed_: function() {
this.showPasswordsExportDialog_ = false; this.showPasswordsExportDialog_ = false;
cr.ui.focusWithoutInk(assert(this.activeDialogAnchor_)); cr.ui.focusWithoutInk(assert(this.activeDialogAnchorStack_.pop()));
this.activeDialogAnchor_ = null;
}, },
/** /**
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment