Bug 1650941 - expand pmgr doorhanger telemetry to include autocomplete suggestions;r=MattN

Differential Revision: https://phabricator.services.mozilla.com/D85101

toolkit/components/passwordmgr/LoginManagerPrompter.jsm

@@ -66,6 +66,34 @@ const NOTIFICATION_TIMEOUT_MS = 10 * 1000; // 10 seconds
  */
 const ATTENTION_NOTIFICATION_TIMEOUT_MS = 60 * 1000; // 1 minute
 
+function autocompleteSelected(popup) {
+  let doc = popup.ownerDocument;
+  let nameField = doc.getElementById("password-notification-username");
+  let passwordField = doc.getElementById("password-notification-password");
+
+  let activeElement = nameField.ownerDocument.activeElement;
+  if (activeElement == nameField) {
+    popup.onUsernameSelect();
+  } else if (activeElement == passwordField) {
+    popup.onPasswordSelect();
+  }
+}
+
+const observer = {
+  QueryInterface: ChromeUtils.generateQI(["nsIObserver"]),
+
+  // nsIObserver
+  observe(subject, topic, data) {
+    switch (topic) {
+      case "autocomplete-did-enter-text": {
+        let input = subject.QueryInterface(Ci.nsIAutoCompleteInput);
+        autocompleteSelected(input.popupElement);
+        break;
+      }
+    }
+  },
+};
+
 /**
  * Implements interfaces for prompting the user to enter/save/change login info
  * found in HTML forms.
@@ -218,7 +246,9 @@ class LoginManagerPrompter {
     let wasModifiedEvent = {
       // Values are mutated
       did_edit_un: "false",
+      did_select_un: "false",
       did_edit_pw: "false",
+      did_select_pw: "false",
     };
 
     let updateButtonStatus = element => {
@@ -312,14 +342,26 @@ class LoginManagerPrompter {
 
     let onUsernameInput = () => {
       wasModifiedEvent.did_edit_un = "true";
+      wasModifiedEvent.did_select_un = "false";
       onInput();
     };
 
+    let onUsernameSelect = () => {
+      wasModifiedEvent.did_edit_un = "false";
+      wasModifiedEvent.did_select_un = "true";
+    };
+
     let onPasswordInput = () => {
       wasModifiedEvent.did_edit_pw = "true";
+      wasModifiedEvent.did_select_pw = "false";
       onInput();
     };
 
+    let onPasswordSelect = () => {
+      wasModifiedEvent.did_edit_pw = "false";
+      wasModifiedEvent.did_select_pw = "true";
+    };
+
     let onKeyUp = e => {
       if (e.key == "Enter") {
         e.target.closest("popupnotification").button.doCommand();
@@ -677,6 +719,10 @@ class LoginManagerPrompter {
                 toggleBtn.setAttribute("hidden", hideToggle);
               }
 
+              let popup = chromeDoc.getElementById("PopupAutoComplete");
+              popup.onUsernameSelect = onUsernameSelect;
+              popup.onPasswordSelect = onPasswordSelect;
+
               LoginManagerPrompter._setUsernameAutocomplete(
                 login,
                 possibleValues?.usernames
@@ -1045,6 +1091,9 @@ class LoginManagerPrompter {
   }
 }
 
+// Add this observer once for the process.
+Services.obs.addObserver(observer, "autocomplete-did-enter-text");
+
 XPCOMUtils.defineLazyGetter(this, "log", () => {
   return LoginHelper.createLogger("LoginManagerPrompter");
 });

toolkit/components/passwordmgr/test/browser/browser.ini

@@ -80,6 +80,7 @@ support-files =
   subtst_notifications_change_p.html
 [browser_doorhanger_save_password.js]
 [browser_doorhanger_submit_telemetry.js]
+skip-if = os == "linux" && debug # Bug 1658056
 [browser_doorhanger_target_blank.js]
 support-files =
   subtst_notifications_12_target_blank.html

toolkit/components/passwordmgr/test/browser/browser_doorhanger_submit_telemetry.js

@@ -24,7 +24,9 @@ const TEST_CASES = [
         type: "save",
         ping: {
           did_edit_un: "false",
+          did_select_un: "false",
           did_edit_pw: "false",
+          did_select_pw: "false",
         },
       },
     ],
@@ -35,15 +37,19 @@ const TEST_CASES = [
     userActions: [
       {
         pageChanges: { password: "pagePw" },
-        doorhangerChanges: {
-          username: "doorhangerUn",
-        },
+        doorhangerChanges: [
+          {
+            typedUsername: "doorhangerUn",
+          },
+        ],
       },
       {
         pageChanges: { password: "pagePw2" },
-        doorhangerChanges: {
-          password: "doorhangerPw",
-        },
+        doorhangerChanges: [
+          {
+            typedPassword: "doorhangerPw",
+          },
+        ],
       },
     ],
     expectedEvents: [
@@ -51,14 +57,18 @@ const TEST_CASES = [
         type: "save",
         ping: {
           did_edit_un: "true",
+          did_select_un: "false",
           did_edit_pw: "false",
+          did_select_pw: "false",
         },
       },
       {
         type: "update",
         ping: {
           did_edit_un: "false",
+          did_select_un: "false",
           did_edit_pw: "true",
+          did_select_pw: "false",
         },
       },
     ],
@@ -73,9 +83,11 @@ const TEST_CASES = [
     userActions: [
       {
         pageChanges: { password: "pagePw" },
-        doorhangerChanges: {
-          password: "doorhangerPw",
-        },
+        doorhangerChanges: [
+          {
+            typedPassword: "doorhangerPw",
+          },
+        ],
       },
     ],
     expectedEvents: [
@@ -83,7 +95,9 @@ const TEST_CASES = [
         type: "update",
         ping: {
           did_edit_un: "false",
+          did_select_un: "false",
           did_edit_pw: "true",
+          did_select_pw: "false",
         },
       },
     ],
@@ -99,9 +113,11 @@ const TEST_CASES = [
     userActions: [
       {
         pageChanges: { password: "pagePw" },
-        doorhangerChanges: {
-          username: "doorhangerUn",
-        },
+        doorhangerChanges: [
+          {
+            typedUsername: "doorhangerUn",
+          },
+        ],
       },
     ],
     expectedEvents: [
@@ -109,7 +125,104 @@ const TEST_CASES = [
         type: "update",
         ping: {
           did_edit_un: "true",
+          did_select_un: "false",
           did_edit_pw: "false",
+          did_select_pw: "false",
+        },
+      },
+    ],
+  },
+  ///////////////
+  {
+    description: "selecting a saved username sends a 'not edited' event",
+    savedLogin: {
+      username: "savedUn",
+      password: "savedPw",
+    },
+    userActions: [
+      {
+        pageChanges: { password: "pagePw" },
+        doorhangerChanges: [
+          {
+            selectUsername: "savedUn",
+          },
+        ],
+      },
+    ],
+    expectedEvents: [
+      {
+        type: "update",
+        ping: {
+          did_edit_un: "false",
+          did_select_un: "true",
+          did_edit_pw: "false",
+          did_select_pw: "false",
+        },
+      },
+    ],
+  },
+  /////////////////
+  {
+    description:
+      "typing a new username then selecting a saved username sends a 'not edited' event",
+    savedLogin: {
+      username: "savedUn",
+      password: "savedPw",
+    },
+    userActions: [
+      {
+        pageChanges: { password: "pagePw" },
+        doorhangerChanges: [
+          {
+            typedUsername: "doorhangerTypedUn",
+          },
+          {
+            selectUsername: "savedUn",
+          },
+        ],
+      },
+    ],
+    expectedEvents: [
+      {
+        type: "update",
+        ping: {
+          did_edit_un: "false",
+          did_select_un: "true",
+          did_edit_pw: "false",
+          did_select_pw: "false",
+        },
+      },
+    ],
+  },
+  /////////////////
+  {
+    description:
+      "selecting a saved username then typing a new username sends an 'edited' event",
+    savedLogin: {
+      username: "savedUn",
+      password: "savedPw",
+    },
+    userActions: [
+      {
+        pageChanges: { password: "pagePw" },
+        doorhangerChanges: [
+          {
+            selectUsername: "savedUn",
+          },
+          {
+            typedUsername: "doorhangerTypedUn",
+          },
+        ],
+      },
+    ],
+    expectedEvents: [
+      {
+        type: "update",
+        ping: {
+          did_edit_un: "true",
+          did_select_un: "false",
+          did_edit_pw: "false",
+          did_select_pw: "false",
         },
       },
     ],
@@ -127,6 +240,19 @@ for (let testData of TEST_CASES) {
   add_task(tmp[testData.description]);
 }
 
+function _validateTestCase(tc) {
+  for (let event of tc.expectedEvents) {
+    ok(
+      !(event.ping.did_edit_un && event.ping.did_select_un),
+      "'did_edit_un' and 'did_select_un' can never be true at the same time"
+    );
+    ok(
+      !(event.ping.did_edit_pw && event.ping.did_select_pw),
+      "'did_edit_pw' and 'did_select_pw' can never be true at the same time"
+    );
+  }
+}
+
 async function test_submit_telemetry(tc) {
   if (tc.savedLogin) {
     Services.logins.addLogin(
@@ -168,6 +294,7 @@ async function test_submit_telemetry(tc) {
           await changeContentFormValues(browser, changeTo);
         }
 
+        info("Submitting form");
         let formSubmittedPromise = listenForTestNotification("FormSubmit");
         await SpecialPowers.spawn(browser, [], async function() {
           let doc = this.content.document;
@@ -177,11 +304,37 @@ async function test_submit_telemetry(tc) {
 
         let saveDoorhanger = waitForDoorhanger(browser, "password-save");
         let updateDoorhanger = waitForDoorhanger(browser, "password-change");
-        info("Waiting for doorhanger");
         notif = await Promise.race([saveDoorhanger, updateDoorhanger]);
 
-        await updateDoorhangerInputValues(userAction.doorhangerChanges);
+        if (PopupNotifications.panel.state !== "open") {
+          await BrowserTestUtils.waitForEvent(
+            PopupNotifications.panel,
+            "popupshown"
+          );
+        }
 
+        if (userAction.doorhangerChanges) {
+          for (let doorhangerChange of userAction.doorhangerChanges) {
+            if (
+              doorhangerChange.typedUsername ||
+              doorhangerChange.typedPassword
+            ) {
+              await updateDoorhangerInputValues({
+                username: doorhangerChange.typedUsername,
+                password: doorhangerChange.typedPassword,
+              });
+            }
+
+            if (doorhangerChange.selectUsername) {
+              await selectDoorhangerUsername(doorhangerChange.selectUsername);
+            }
+            if (doorhangerChange.selectPassword) {
+              await selectDoorhangerPassword(doorhangerChange.selectPassword);
+            }
+          }
+        }
+
+        info("Waiting for doorhanger");
         await clickDoorhangerButton(notif, REMEMBER_BUTTON);
       }
     );

toolkit/components/passwordmgr/test/browser/head.js

@@ -470,10 +470,10 @@ async function updateDoorhangerInputValues(
     info(`setInputValue: on target: ${target.id}, value: ${value}`);
     target.focus();
     target.select();
-    await EventUtils.synthesizeKey("KEY_Backspace");
     info(
-      `setInputValue: target.value: ${target.value}, sending new value string`
+      `setInputValue: current value: '${target.value}', setting new value '${value}'`
     );
+    await EventUtils.synthesizeKey("KEY_Backspace");
     await EventUtils.sendString(value);
     await EventUtils.synthesizeKey("KEY_Tab");
     return Promise.resolve();
@@ -498,6 +498,79 @@ async function updateDoorhangerInputValues(
   }
 }
 
+/**
+ * Open doorhanger autocomplete popup and select a username value.
+ *
+ * @param {string} text the text value of the username that should be selected.
+ *                 Noop if `text` is falsy.
+ */
+async function selectDoorhangerUsername(text) {
+  await _selectDoorhanger(
+    text,
+    "#password-notification-username",
+    "#password-notification-username-dropmarker"
+  );
+}
+
+/**
+ * Open doorhanger autocomplete popup and select a password value.
+ *
+ * @param {string} text the text value of the password that should be selected.
+ *                 Noop if `text` is falsy.
+ */
+async function selectDoorhangerPassword(text) {
+  await _selectDoorhanger(
+    text,
+    "#password-notification-password",
+    "#password-notification-password-dropmarker"
+  );
+}
+
+async function _selectDoorhanger(text, inputSelector, dropmarkerSelector) {
+  if (!text) {
+    return;
+  }
+
+  info("Opening doorhanger suggestion popup");
+
+  let doorhangerPopup = document.getElementById("password-notification");
+  let dropmarker = doorhangerPopup.querySelector(dropmarkerSelector);
+
+  let autocompletePopup = document.getElementById("PopupAutoComplete");
+  let popupShown = BrowserTestUtils.waitForEvent(
+    autocompletePopup,
+    "popupshown"
+  );
+
+  EventUtils.synthesizeMouseAtCenter(dropmarker, {});
+
+  await popupShown;
+
+  let suggestions = [
+    ...document
+      .getElementById("PopupAutoComplete")
+      .getElementsByTagName("richlistitem"),
+  ].filter(richlistitem => !richlistitem.collapsed);
+
+  let suggestionText = suggestions.map(
+    richlistitem => richlistitem.querySelector(".ac-title-text").innerHTML
+  );
+
+  let targetIndex = suggestionText.indexOf(text);
+  ok(targetIndex != -1, "Suggestions include expected text");
+
+  let promiseHidden = BrowserTestUtils.waitForEvent(
+    autocompletePopup,
+    "popuphidden"
+  );
+
+  info("Selecting doorhanger suggestion");
+
+  EventUtils.synthesizeMouseAtCenter(suggestions[targetIndex], {});
+
+  await promiseHidden;
+}
+
 // End popup notification (doorhanger) functions //
 
 async function openPasswordManager(openingFunc, waitForFilter) {

toolkit/components/telemetry/Events.yaml

@@ -880,13 +880,15 @@ pwmgr:
   doorhanger_submitted:
     description: >
       A login is saved or updated via the capture doorhanger. Carries information about whether 
-      or not the username and password were modified by the user. 
+      the username and password _that were saved/updated by the user_ were modified in the 
+      doorhanger, selected from the suggestion autocomplete, or neither. `did_edit_X` and 
+      `did_select_X` will never both be true in the same event.
       
       The `object` describes the type of doorhanger when it was originally created. Note that user 
       updates to the doorhanger may change whether a login is actually saved or updated, but will 
       not impact the sent object.
     objects: ["save", "update"]
-    bug_numbers: [1650929]
+    bug_numbers: [1650929, 1650941]
     expiry_version: "86"
     release_channel_collection: opt-out
     products: ["firefox"]
@@ -894,11 +896,17 @@ pwmgr:
     notification_emails: ["srudie@mozilla.com", "passwords-dev@mozilla.org"]
     extra_keys:
       did_edit_un: >
-        Whether or not the username field of the doorhanger was modified by the user.  
-        This does not count selecting an autocompleted value.
+        Whether or not the saved/updated username was modified by the user typing into the 
+        username field.
+      did_select_un: >
+        Whether or not the saved/updated username was selected by the user choosing a suggested 
+        value from the autocomplete popup.
       did_edit_pw: >
-        Whether or not the password field of the doorhanger was modified by the user.  
-        This does not count selecting an autocompleted value.
+        Whether or not the saved/updated password was modified by the user typing into the 
+        password field.
+      did_select_pw: >
+        Whether or not the saved/updated password was selected by the user choosing a suggested 
+        value from the autocomplete popup.
 
 jsonfile:
   load: