MD Settings: Restore focus after closing dialogs, for certificate page.

chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js

Index: chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js
diff --git a/chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js b/chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js
index afa70a9b5d8bd4549374686e448f9242bbc98772..e1db657a3aca3977e8e7f538ceb45dcaac901764 100644
--- a/chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js
+++ b/chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js
@@ -82,6 +82,13 @@ Polymer({
      * @private {null|!CertificatesError|!CertificatesImportError}
      */
     errorDialogModel_: Object,
+
+    /**
+     * The element to return focus to, when the currently shown dialog is
+     * closed.
+     * @private {?HTMLElement}
+     */
+    activeDialogAnchor_: Object,
   },
 
   /** @override */
@@ -110,25 +117,28 @@ Polymer({
         if (event.detail.certificateType == CertificateType.PERSONAL) {
           this.openDialog_(
               'settings-certificate-password-decryption-dialog',
-              'showPasswordDecryptionDialog_');
+              'showPasswordDecryptionDialog_',
+              event.detail.anchor);
         } else if (event.detail.certificateType ==
             CertificateType.CA) {
           this.openDialog_(
-              'settings-ca-trust-edit-dialog', 'showCaTrustEditDialog_');
+              'settings-ca-trust-edit-dialog', 'showCaTrustEditDialog_',
+              event.detail.anchor);
         }
       } else {
         if (event.detail.action == CertificateAction.EDIT) {
           this.openDialog_(
-              'settings-ca-trust-edit-dialog', 'showCaTrustEditDialog_');
+              'settings-ca-trust-edit-dialog', 'showCaTrustEditDialog_',
+              event.detail.anchor);
         } else if (event.detail.action == CertificateAction.DELETE) {
           this.openDialog_(
               'settings-certificate-delete-confirmation-dialog',
-              'showDeleteConfirmationDialog_');
+              'showDeleteConfirmationDialog_', event.detail.anchor);
         } else if (event.detail.action ==
             CertificateAction.EXPORT_PERSONAL) {
           this.openDialog_(
               'settings-certificate-password-encryption-dialog',
-              'showPasswordEncryptionDialog_');
+              'showPasswordEncryptionDialog_', event.detail.anchor);
         }
       }
 
@@ -136,10 +146,12 @@ Polymer({
     }.bind(this));
 
     this.addEventListener('certificates-error', function(event) {
-      this.errorDialogModel_ = event.detail;
+      var detail = /** @type {!CertificatesErrorEventDetail} */ (event.detail);
+      this.errorDialogModel_ = detail.error;
       this.openDialog_(
           'settings-certificates-error-dialog',
-          'showErrorDialog_');
+          'showErrorDialog_',
+          detail.anchor);
       event.stopPropagation();
     }.bind(this));
   },
@@ -152,14 +164,19 @@ Polymer({
    * @param {string} dialogTagName The tag name of the dialog to be shown.
    * @param {string} domIfBooleanName The name of the boolean variable
    *     corresponding to the dialog.
+   * @param {?HTMLElement} anchor The element to focus when the dialog is
+   *     closed. If null, the previous anchor element should be reused.
    * @private
    */
-  openDialog_: function(dialogTagName, domIfBooleanName) {
+  openDialog_: function(dialogTagName, domIfBooleanName, anchor) {
+    if (anchor)
+      this.activeDialogAnchor_ = anchor;
     this.set(domIfBooleanName, true);
     this.async(function() {
       var dialog = this.$$(dialogTagName);
       dialog.addEventListener('close', function() {
         this.set(domIfBooleanName, false);
+        this.activeDialogAnchor_.focus();

[tommycli, 2017/04/04 22:42:29]

It looks like you don't actually need to store activeDialogAnchor_ as a
property... since you can access the anchor parameter in the inner function too
right?

[Dan Beam, 2017/04/04 22:46:00]

also, why only set it if (anchor)?

[Dan Beam, 2017/04/04 22:47:21]

tommycli@: the difference is that openDialog_ (in theory) could be called
multiple times and only the last |anchor| it's called with would be focused. 
but I'm not sure if that's intended.

[tommycli, 2017/04/04 22:56:49]

Ah yes, that behavior is different.

Actually now I think using the parameter directly is more desirable, since then
the last-closed dialog controls which element gets refocused afterwards.

[dpapad, 2017/04/04 23:15:42]

Certificates manager page is a pretty interesting case in terms of dialogs. I
hope this will explain the questions above

- 1st dialog opens, anchorElement_ is set.
- An error occurs which causes the 1st dialog to be closed and to immediately
request the error dialog to be shown, (example
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/certif...).
- The close handler of the 1st dialog is executing, which focuses the
|anchorElement_|, but we intentionally never set |anchorElement_| back to null.
- The error-dialog is shown, and the |anchor| parameter in this call is null
intentionally, which forces it to reuse the previously set |anchorElement_|,
when the error dialog is closed.

anchorElement_ needs to be stored on |this| so that it can be accessed across
different calls of openDialog_(). Let me know if that addresses all the
questions above.

[tommycli, 2017/04/04 23:21:26]

Wow that's really tricky. If the above is the intended behavior I recommend copy
pasting that example scenario into the comment of this.activeDialogAnchor_.

still lgtm.

[dpapad, 2017/04/05 00:03:18]

I was hoping that the explanation given at the JSDoc comment for the possible
null value of the |anchor| parameter would be sufficient. Can you verify that
you still think a longer explanation would add value, on top of what is already
stated in that comment?

[Dan Beam, 2017/04/05 03:31:36]

ping ^

[tommycli, 2017/04/05 16:11:44]

dbeam: I'm assuming it's because if anchor is null, we want to preserve the old
activeDialogAnchor_ rather than overwriting it...

dpapad: Eh... I would say the desired behavior wasn't obvious to me, as a code
reader. I think the behavior here is pretty subtle, and I'm not 100% sure it's
worth it. But you guys think it's worth it, I would still support a longer
comment.

I don't see it as a blocking issue though. Just a matter of style / taste -- so
I'll leave it to your discretion.

       }.bind(this));
     }.bind(this));
   },