Bug 1559994 - Add 'attention'-styled dismissed doorhanger when auto-saving a generated password. r=MattN
authorSam Foster <sfoster@mozilla.com>
Fri, 12 Jul 2019 00:34:08 +0000
changeset 482493 d8f487edf311acbe00e17cd8421549a1e34fb75c
parent 482492 04eacc649af50335484264c5d1cecab40a8503f8
child 482494 805e1d6269d999b8976d7a36417e707b0ca4553f
push id36282
push userdvarga@mozilla.com
push dateFri, 12 Jul 2019 09:56:21 +0000
treeherdermozilla-central@cb2d564879e3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs1559994
milestone70.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1559994 - Add 'attention'-styled dismissed doorhanger when auto-saving a generated password. r=MattN * Add a new optional 'notifySaved' argument to promptToSavePassword * Give the notification an attention style when showing a login doorhanger for an auto-saved login with a generated password Differential Revision: https://phabricator.services.mozilla.com/D37661
browser/themes/shared/notification-icons.inc.css
mobile/android/components/LoginManagerPrompter.js
toolkit/components/passwordmgr/LoginManagerParent.jsm
toolkit/components/passwordmgr/LoginManagerPrompter.jsm
toolkit/components/passwordmgr/nsILoginManagerPrompter.idl
toolkit/components/passwordmgr/test/browser/browser_generated_password_doorhangers.js
toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_onGeneratedPasswordFilled.js
toolkit/modules/PopupNotifications.jsm
--- a/browser/themes/shared/notification-icons.inc.css
+++ b/browser/themes/shared/notification-icons.inc.css
@@ -113,16 +113,21 @@
   list-style-image: url(chrome://browser/skin/notification-icons/indexedDB.svg);
 }
 
 .popup-notification-icon[popupid="password"],
 .login-icon {
   list-style-image: url(chrome://browser/skin/login.svg);
 }
 
+.login-icon[extraAttr="attention"] {
+  fill: var(--toolbarbutton-icon-fill-attention);
+  fill-opacity: 1;
+}
+
 .camera-icon {
   list-style-image: url(chrome://browser/skin/notification-icons/camera.svg);
 }
 
 .camera-icon.in-use {
   list-style-image: url(chrome://browser/skin/notification-icons/camera.svg);
 }
 
--- a/mobile/android/components/LoginManagerPrompter.js
+++ b/mobile/android/components/LoginManagerPrompter.js
@@ -111,17 +111,21 @@ LoginManagerPrompter.prototype = {
   // setting this attribute is ignored because Android does not consider
   // opener windows when displaying login notifications
   set opener(aOpener) {},
 
   /*
    * promptToSavePassword
    *
    */
-  promptToSavePassword: function(aLogin, dismissed) {
+  promptToSavePassword: function(
+    aLogin,
+    dismissed = false,
+    notifySaved = false
+  ) {
     this._showSaveLoginNotification(aLogin, dismissed);
     Services.telemetry
       .getHistogramById("PWMGR_PROMPT_REMEMBER_ACTION")
       .add(PROMPT_DISPLAYED);
     Services.obs.notifyObservers(aLogin, "passwordmgr-prompt-save");
   },
 
   /*
--- a/toolkit/components/passwordmgr/LoginManagerParent.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ -724,28 +724,27 @@ this.LoginManagerParent = {
 
     if (logins.length > 0) {
       log("_onGeneratedPasswordFilled: Login already saved for this site");
       shouldSaveLogin = false;
     }
 
     if (shouldSaveLogin) {
       Services.logins.addLogin(formLogin);
-      log(
-        "TODO: bug 1559994 will get/create dismissed prompt to save/update password and color the icon blue"
-      );
-    } else {
-      // show dismissed "save password" prompt
-      log(
-        "_onGeneratedPasswordFilled, login will not be saved so show dismissed save-password notification"
-      );
-      let browser = browsingContext.top.embedderElement;
-      let prompter = this._getPrompter(browser, openerTopWindowID);
-      prompter.promptToSavePassword(formLogin, true); // dimissed prompt
     }
+    log(
+      "_onGeneratedPasswordFilled: show dismissed save-password notification"
+    );
+    let browser = browsingContext.top.embedderElement;
+    let prompter = this._getPrompter(browser, openerTopWindowID);
+    prompter.promptToSavePassword(
+      formLogin,
+      true, // dimissed prompt
+      shouldSaveLogin // attention
+    );
   },
 
   /**
    * Maps all the <browser> elements for tabs in the parent process to the
    * current state used to display tab-specific UI.
    *
    * This mapping is not updated in case a web page is moved to a different
    * chrome window by the swapDocShells method. In this case, it is possible
--- a/toolkit/components/passwordmgr/LoginManagerPrompter.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerPrompter.jsm
@@ -920,22 +920,24 @@ LoginManagerPrompter.prototype = {
   set browser(aBrowser) {
     this._browser = aBrowser;
   },
 
   set openerBrowser(aOpenerBrowser) {
     this._openerBrowser = aOpenerBrowser;
   },
 
-  promptToSavePassword(aLogin, dismissed) {
+  promptToSavePassword(aLogin, dismissed = false, notifySaved = false) {
     this.log("promptToSavePassword");
     var notifyObj = this._getPopupNote();
     if (notifyObj) {
       this._showLoginCaptureDoorhanger(aLogin, "password-save", {
         dismissed: this._inPrivateBrowsing || dismissed,
+        notifySaved,
+        extraAttr: notifySaved ? "attention" : "",
       });
       Services.obs.notifyObservers(aLogin, "passwordmgr-prompt-save");
     } else {
       this._showSaveLoginDialog(aLogin);
     }
   },
 
   /**
@@ -1255,19 +1257,25 @@ LoginManagerPrompter.prototype = {
                   );
                 }
                 if (this.wasDismissed) {
                   chromeDoc
                     .getElementById("password-notification-visibilityToggle")
                     .setAttribute("hidden", true);
                 }
                 break;
-              case "shown":
+              case "shown": {
                 writeDataToUI();
+                let anchorIcon = this.anchorElement;
+                if (anchorIcon && this.options.extraAttr == "attention") {
+                  anchorIcon.removeAttribute("extraAttr");
+                  delete this.options.extraAttr;
+                }
                 break;
+              }
               case "dismissed":
                 this.wasDismissed = true;
                 readDataFromUI();
               // Fall through.
               case "removed":
                 currentNotification = null;
                 chromeDoc
                   .getElementById("password-notification-username")
@@ -1365,27 +1373,31 @@ LoginManagerPrompter.prototype = {
    *
    * @param {nsILoginInfo} aOldLogin
    *                       The old login we may want to update.
    * @param {nsILoginInfo} aNewLogin
    *                       The new login from the page form.
    * @param dismissed
    *        A boolean indicating if the prompt should be automatically
    *        dismissed on being shown.
+   * @param notifySaved
+   *        A boolean value indicating whether the notification should indicate that
+   *        a login has been saved
    */
-  promptToChangePassword(aOldLogin, aNewLogin, dismissed) {
+  promptToChangePassword(aOldLogin, aNewLogin, dismissed, notifySaved) {
     this.log("promptToChangePassword");
     let notifyObj = this._getPopupNote();
 
     if (notifyObj) {
       this._showChangeLoginNotification(
         notifyObj,
         aOldLogin,
         aNewLogin,
-        dismissed
+        dismissed,
+        notifySaved
       );
     } else {
       this._showChangeLoginDialog(aOldLogin, aNewLogin);
     }
   },
 
   /**
    * Shows the Change Password popup notification.
@@ -1396,29 +1408,35 @@ LoginManagerPrompter.prototype = {
    * @param aOldLogin
    *        The stored login we want to update.
    *
    * @param aNewLogin
    *        The login object with the changes we want to make.
    * @param dismissed
    *        A boolean indicating if the prompt should be automatically
    *        dismissed on being shown.
+   * @param notifySaved
+   *        A boolean value indicating whether the notification should indicate that
+   *        a login has been saved
    */
   _showChangeLoginNotification(
     aNotifyObj,
     aOldLogin,
     aNewLogin,
-    dismissed = false
+    dismissed = false,
+    notifySaved = false
   ) {
     aOldLogin.origin = aNewLogin.origin;
     aOldLogin.formActionOrigin = aNewLogin.formActionOrigin;
     aOldLogin.password = aNewLogin.password;
     aOldLogin.username = aNewLogin.username;
     this._showLoginCaptureDoorhanger(aOldLogin, "password-change", {
       dismissed,
+      notifySaved,
+      extraAttr: notifySaved ? "attention" : "",
     });
 
     let oldGUID = aOldLogin.QueryInterface(Ci.nsILoginMetaInfo).guid;
     Services.obs.notifyObservers(
       aNewLogin,
       "passwordmgr-prompt-change",
       oldGUID
     );
--- a/toolkit/components/passwordmgr/nsILoginManagerPrompter.idl
+++ b/toolkit/components/passwordmgr/nsILoginManagerPrompter.idl
@@ -47,18 +47,23 @@ interface nsILoginManagerPrompter : nsIS
   /**
    * Ask the user if they want to save a login (Yes, Never, Not Now)
    *
    * @param aLogin
    *        The login to be saved.
    * @param dismissed
    *        A boolean value indicating whether the save logins doorhanger should
    *        be dismissed automatically when shown.
+   * @param notifySaved
+   *        A boolean value indicating whether the notification should indicate that
+   *        a login has been saved
    */
-  void promptToSavePassword(in nsILoginInfo aLogin, in boolean dismissed);
+  void promptToSavePassword(in nsILoginInfo aLogin,
+                            [optional] in boolean dismissed,
+                            [optional] in boolean notifySaved);
 
   /**
    * Ask the user if they want to change a login's password or username.
    * If the user consents, modifyLogin() will be called.
    *
    * @param aOldLogin
    *        The existing login (with the old password).
    * @param aNewLogin
--- a/toolkit/components/passwordmgr/test/browser/browser_generated_password_doorhangers.js
+++ b/toolkit/components/passwordmgr/test/browser/browser_generated_password_doorhangers.js
@@ -49,16 +49,49 @@ function openACPopup(popup, browser, inp
     });
 
     let shown = await promiseShown;
     ok(shown, "autocomplete popup shown");
     resolve(shown);
   });
 }
 
+async function fillGeneratedPasswordFromACPopup(
+  browser,
+  passwordInputSelector
+) {
+  let popup = document.getElementById("PopupAutoComplete");
+  ok(popup, "Got popup");
+  await openACPopup(popup, browser, passwordInputSelector);
+
+  let item = popup.querySelector(`[originaltype="generatedPassword"]`);
+  ok(item, "Got generated password richlistitem");
+
+  await TestUtils.waitForCondition(() => {
+    return !EventUtils.isHidden(item);
+  }, "Waiting for item to become visible");
+
+  let inputEventPromise = ContentTask.spawn(
+    browser,
+    [passwordInputSelector],
+    async function waitForInput(inputSelector) {
+      let passwordInput = content.document.querySelector(inputSelector);
+      await ContentTaskUtils.waitForEvent(
+        passwordInput,
+        "input",
+        "Password input value changed"
+      );
+    }
+  );
+  info("Clicking the generated password AC item");
+  EventUtils.synthesizeMouseAtCenter(item, {});
+  info("Waiting for the content input value to change");
+  await inputEventPromise;
+}
+
 async function checkPromptContents(anchorElement, browser) {
   let { panel } = PopupNotifications;
   let promiseShown = BrowserTestUtils.waitForEvent(panel, "popupshown");
   await SimpleTest.promiseFocus(browser);
   info("Clicking on anchor to show popup.");
   anchorElement.click();
   await promiseShown;
   let notificationElement = panel.childNodes[0];
@@ -77,18 +110,92 @@ add_task(async function setup() {
     set: [
       ["signon.generation.available", true],
       ["signon.generation.enabled", true],
     ],
   });
   // assert that there are no logins
   let logins = Services.logins.getAllLogins();
   is(logins.length, 0, "There are no logins");
+});
 
-  // use a single matching login for the following tests
+add_task(async function autocomplete_generated_password_auto_saved() {
+  await BrowserTestUtils.withNewTab(
+    {
+      gBrowser,
+      url: TEST_ORIGIN + FORM_PAGE_PATH,
+    },
+    async function(browser) {
+      await SimpleTest.promiseFocus(browser.ownerGlobal);
+      await ContentTask.spawn(
+        browser,
+        [passwordInputSelector, usernameInputSelector],
+        function prepareAndCheckForm([passwordSelector, usernameSelector]) {
+          let passwordInput = content.document.querySelector(passwordSelector);
+          // We'll reuse the form_basic.html, but ensure we'll get the generated password autocomplete option
+          passwordInput.setAttribute("autocomplete", "new-password");
+          passwordInput.value = "";
+          let usernameInput = content.document.querySelector(usernameSelector);
+          usernameInput.setUserInput("user1");
+        }
+      );
+      await fillGeneratedPasswordFromACPopup(browser, passwordInputSelector);
+      await ContentTask.spawn(
+        browser,
+        [passwordInputSelector],
+        function checkFinalFieldValue(inputSelector) {
+          let passwordInput = content.document.querySelector(inputSelector);
+          is(
+            passwordInput.value.length,
+            15,
+            "Password field was filled with generated password"
+          );
+        }
+      );
+
+      // check a dismissed prompt was shown with extraAttr attribute
+      let notif = getCaptureDoorhanger("password-save");
+      ok(notif && notif.dismissed, "Dismissed notification was created");
+      is(
+        notif.anchorElement.getAttribute("extraAttr"),
+        "attention",
+        "Check if icon has the extraAttr attribute"
+      );
+
+      let { passwordValue, usernameValue } = await checkPromptContents(
+        notif.anchorElement,
+        browser
+      );
+      is(
+        passwordValue.length,
+        15,
+        "Doorhanger password field has generated 15-char value"
+      );
+      is(usernameValue, "user1", "Doorhanger username field was popuplated");
+
+      info("Hiding popup.");
+      let { panel } = PopupNotifications;
+      let promiseHidden = BrowserTestUtils.waitForEvent(panel, "popuphidden");
+      panel.hidePopup();
+      await promiseHidden;
+
+      // confirm the extraAttr attribute is removed after opening & dismissing the doorhanger
+      ok(
+        !notif.anchorElement.hasAttribute("extraAttr"),
+        "Check if the extraAttr attribute was removed"
+      );
+      notif.remove();
+    }
+  );
+});
+
+add_task(async function setup_logins() {
+  // Reset all passwords
+  Services.logins.removeAllLogins();
+  // ..and use a single matching login for the following tests
   await addOneLogin();
 });
 
 add_task(async function contextfill_generated_password_with_matching_logins() {
   // test that we can fill a generated password when there are matching logins
   await BrowserTestUtils.withNewTab(
     {
       gBrowser,
@@ -130,16 +237,20 @@ add_task(async function contextfill_gene
         notif.anchorElement,
         browser
       );
       is(
         passwordValue.length,
         15,
         "Doorhanger password field has generated 15-char value"
       );
+      ok(
+        !notif.anchorElement.hasAttribute("extraAttr"),
+        "Check if icon has the extraAttr attribute"
+      );
       notif.remove();
     }
   );
 });
 
 add_task(async function contextfill_generated_password_with_username() {
   // test that the prompt resulting from filling with a generated password displays the username
   await BrowserTestUtils.withNewTab(
@@ -181,16 +292,20 @@ add_task(async function contextfill_gene
         15,
         "Doorhanger password field has generated 15-char value"
       );
       is(
         usernameValue,
         "user1",
         "Doorhanger username field has the username field value"
       );
+      ok(
+        !notif.anchorElement.hasAttribute("extraAttr"),
+        "Check if icon has the extraAttr attribute"
+      );
       notif.remove();
     }
   );
 });
 
 add_task(async function autocomplete_generated_password() {
   await BrowserTestUtils.withNewTab(
     {
@@ -206,61 +321,37 @@ add_task(async function autocomplete_gen
           let passwordInput = content.document.querySelector(passwordSelector);
           // We'll reuse the form_basic.html, but ensure we'll get the generated password autocomplete option
           passwordInput.setAttribute("autocomplete", "new-password");
           passwordInput.value = "";
           let usernameInput = content.document.querySelector(usernameSelector);
           usernameInput.setUserInput("user1");
         }
       );
-
-      let popup = document.getElementById("PopupAutoComplete");
-      ok(popup, "Got popup");
-      await openACPopup(popup, browser, passwordInputSelector);
-
-      let item = popup.querySelector(`[originaltype="generatedPassword"]`);
-      ok(item, "Got generated password richlistitem");
-
-      await TestUtils.waitForCondition(() => {
-        return !EventUtils.isHidden(item);
-      }, "Waiting for item to become visible");
-
-      let inputEventPromise = ContentTask.spawn(
-        browser,
-        [passwordInputSelector],
-        async function waitForInput(inputSelector) {
-          let passwordInput = content.document.querySelector(inputSelector);
-          await ContentTaskUtils.waitForEvent(
-            passwordInput,
-            "input",
-            "Password input value changed"
-          );
-        }
-      );
-      info("Clicking the generated password AC item");
-      EventUtils.synthesizeMouseAtCenter(item, {});
-      info("Waiting for the content input value to change");
-      await inputEventPromise;
-
+      await fillGeneratedPasswordFromACPopup(browser, passwordInputSelector);
       await ContentTask.spawn(
         browser,
         [passwordInputSelector],
         function checkFinalFieldValue(inputSelector) {
           let passwordInput = content.document.querySelector(inputSelector);
           is(
             passwordInput.value.length,
             15,
             "Password field was filled with generated password"
           );
         }
       );
 
       // check a dismissed prompt was shown
       let notif = getCaptureDoorhanger("password-save");
       ok(notif && notif.dismissed, "Dismissed notification was created");
+      ok(
+        !notif.anchorElement.hasAttribute("extraAttr"),
+        "Check if icon has the extraAttr attribute"
+      );
 
       let { passwordValue, usernameValue } = await checkPromptContents(
         notif.anchorElement,
         browser
       );
       is(
         passwordValue.length,
         15,
--- a/toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_onGeneratedPasswordFilled.js
+++ b/toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_onGeneratedPasswordFilled.js
@@ -106,21 +106,30 @@ add_task(async function test_onGenerated
     "https://www.example.com",
     "https://www.mozilla.org",
     null,
     "",
     password1
   );
 
   ok(login.equals(expected), "Check added login");
-  ok(LMP._getPrompter.notCalled, "Checking _getPrompter was not called");
+  ok(LMP._getPrompter.calledOnce, "Checking _getPrompter was called");
+  ok(
+    fakePromptToSavePassword.calledOnce,
+    "Checking promptToSavePassword was called"
+  );
   ok(
-    fakePromptToSavePassword.notCalled,
-    "Checking promptToSavePassword was not called"
+    fakePromptToSavePassword.getCall(0).args[1],
+    "promptToSavePassword had a truthy 'dismissed' argument"
   );
+  ok(
+    fakePromptToSavePassword.getCall(0).args[2],
+    "promptToSavePassword had a truthy 'notifySaved' argument"
+  );
+
   LMP._getPrompter.resetHistory();
   fakePromptToSavePassword.resetHistory();
 
   Services.logins.removeAllLogins();
 
   info("Disable login saving for the site");
   Services.logins.setLoginSavingEnabled("https://www.example.com", false);
   await LMP._onGeneratedPasswordFilled({
@@ -137,13 +146,17 @@ add_task(async function test_onGenerated
   ok(
     fakePromptToSavePassword.calledOnce,
     "Checking promptToSavePassword was called"
   );
   ok(
     fakePromptToSavePassword.getCall(0).args[1],
     "promptToSavePassword had a truthy 'dismissed' argument"
   );
+  ok(
+    !fakePromptToSavePassword.getCall(0).args[2],
+    "promptToSavePassword had a falsey 'notifySaved' argument"
+  );
   LMP._getPrompter.resetHistory();
   fakePromptToSavePassword.resetHistory();
 
   Services.logins.setLoginSavingEnabled("https://www.example.com", true);
 });
--- a/toolkit/modules/PopupNotifications.jsm
+++ b/toolkit/modules/PopupNotifications.jsm
@@ -515,16 +515,19 @@ PopupNotifications.prototype = {
    *                     where message contains a "<>" and a "{}" placeholder. "<>"
    *                     is considered the first and "{}" is considered the second
    *                     placeholder.
    *        escAction:
    *                     An optional string indicating the action to take when the
    *                     Esc key is pressed. This should be set to the name of the
    *                     command to run. If not provided, "secondarybuttoncommand"
    *                     will be used.
+   *        extraAttr:
+   *                     An optional string value which will be given to the
+   *                     extraAttr attribute on the notification's anchorElement
    * @returns the Notification object corresponding to the added notification.
    */
   show: function PopupNotifications_show(
     browser,
     id,
     message,
     anchorID,
     mainAction,