Bug 1551730 - Use hasBeenTypePassword to determine if we're on a password field for autocomplete. r=sfoster

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

--HG--
rename : toolkit/components/passwordmgr/test/mochitest/test_autocomplete_basic_form_subdomain.html => toolkit/components/passwordmgr/test/mochitest/test_autocomplete_hasBeenTypePassword.html
extra : moz-landing-system : lando

toolkit/components/passwordmgr/LoginAutoComplete.jsm

@@ -159,7 +159,7 @@ class InsecureLoginFormAutocompleteItem extends AutocompleteItem {
 class LoginAutocompleteItem extends AutocompleteItem {
   constructor(
     login,
-    isPasswordField,
+    hasBeenTypePassword,
     duplicateUsernames,
     actor,
     isOriginMatched
@@ -184,7 +184,7 @@ class LoginAutocompleteItem extends AutocompleteItem {
     });
 
     XPCOMUtils.defineLazyGetter(this, "value", () => {
-      return isPasswordField ? login.password : login.username;
+      return hasBeenTypePassword ? login.password : login.username;
     });
 
     XPCOMUtils.defineLazyGetter(this, "comment", () => {
@@ -257,7 +257,7 @@ function LoginAutoCompleteResult(
     willAutoSaveGeneratedPassword,
     isSecure,
     actor,
-    isPasswordField,
+    hasBeenTypePassword,
     hostname,
     telemetryEventData,
   }
@@ -272,7 +272,7 @@ function LoginAutoCompleteResult(
 
     // Don't show the footer on non-empty password fields as it's not providing
     // value and only adding noise since a password was already filled.
-    if (isPasswordField && aSearchString && !generatedPassword) {
+    if (hasBeenTypePassword && aSearchString && !generatedPassword) {
       log.debug("Hiding footer: non-empty password field");
       return false;
     }
@@ -280,7 +280,7 @@ function LoginAutoCompleteResult(
     if (
       !matchingLogins.length &&
       !generatedPassword &&
-      isPasswordField &&
+      hasBeenTypePassword &&
       formFillController.passwordPopupAutomaticallyOpened
     ) {
       hidingFooterOnPWFieldAutoOpened = true;
@@ -311,7 +311,7 @@ function LoginAutoCompleteResult(
   for (let login of logins) {
     let item = new LoginAutocompleteItem(
       login,
-      isPasswordField,
+      hasBeenTypePassword,
       duplicateUsernames,
       actor,
       LoginHelper.isOriginMatching(login.origin, formOrigin, {
@@ -483,7 +483,7 @@ LoginAutoComplete.prototype = {
     if (isSecure) {
       isSecure = InsecurePasswordUtils.isFormSecure(form);
     }
-    let isPasswordField = aElement.type == "password";
+    let { hasBeenTypePassword } = aElement;
     let hostname = aElement.ownerDocument.documentURIObject.host;
     let formOrigin = LoginHelper.getLoginOrigin(
       aElement.ownerDocument.documentURI
@@ -529,7 +529,7 @@ LoginAutoComplete.prototype = {
           willAutoSaveGeneratedPassword,
           actor: loginManagerActor,
           isSecure,
-          isPasswordField,
+          hasBeenTypePassword,
           hostname,
           telemetryEventData,
         }
@@ -545,7 +545,7 @@ LoginAutoComplete.prototype = {
     }
 
     if (
-      isPasswordField &&
+      hasBeenTypePassword &&
       aSearchString &&
       !loginManagerActor.isPasswordGenerationForcedOn(aElement)
     ) {
@@ -578,7 +578,7 @@ LoginAutoComplete.prototype = {
       inputElement: aElement,
       form,
       formOrigin,
-      isPasswordField,
+      hasBeenTypePassword,
     });
     completeSearch(acLookupPromise).catch(log.error.bind(log));
   },
@@ -593,7 +593,7 @@ LoginAutoComplete.prototype = {
     inputElement,
     form,
     formOrigin,
-    isPasswordField,
+    hasBeenTypePassword,
   }) {
     let actionOrigin = LoginHelper.getFormActionOrigin(form);
     let autocompleteInfo = inputElement.getAutocompleteInfo();
@@ -603,7 +603,7 @@ LoginAutoComplete.prototype = {
     );
     let forcePasswordGeneration = false;
     let isProbablyANewPasswordField = false;
-    if (isPasswordField) {
+    if (hasBeenTypePassword) {
       forcePasswordGeneration = loginManagerActor.isPasswordGenerationForcedOn(
         inputElement
       );
@@ -620,8 +620,8 @@ LoginAutoComplete.prototype = {
       searchString,
       previousResult,
       forcePasswordGeneration,
+      hasBeenTypePassword,
       isSecure: InsecurePasswordUtils.isFormSecure(form),
-      isPasswordField,
       isProbablyANewPasswordField,
     };
 
@@ -632,9 +632,9 @@ LoginAutoComplete.prototype = {
     log.debug("LoginAutoComplete search:", {
       forcePasswordGeneration,
       isSecure: messageData.isSecure,
-      isPasswordField,
+      hasBeenTypePassword,
       isProbablyANewPasswordField,
-      searchString: isPasswordField
+      searchString: hasBeenTypePassword
         ? "*".repeat(searchString.length)
         : searchString,
     });

toolkit/components/passwordmgr/LoginManagerParent.jsm

@@ -417,8 +417,8 @@ class LoginManagerParent extends JSWindowActorParent {
     searchString,
     previousResult,
     forcePasswordGeneration,
+    hasBeenTypePassword,
     isSecure,
-    isPasswordField,
     isProbablyANewPasswordField,
   }) {
     // Note: previousResult is a regular object, not an
@@ -477,7 +477,7 @@ class LoginManagerParent extends JSWindowActorParent {
       // Remove results that are too short, or have different prefix.
       // Also don't offer empty usernames as possible results except
       // for on password fields.
-      if (isPasswordField) {
+      if (hasBeenTypePassword) {
         return true;
       }
       return match && match.toLowerCase().startsWith(searchStringLower);

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

@@ -100,8 +100,8 @@ add_task(async function test_mpAutocompleteUIBusy() {
     actionOrigin: "",
     searchString: "",
     previousResult: null,
+    hasBeenTypePassword: true,
     isSecure: false,
-    isPasswordField: true,
     isProbablyANewPasswordField: true,
   };
 

toolkit/components/passwordmgr/test/mochitest/mochitest.ini

@@ -37,6 +37,9 @@ scheme = https
 [test_autocomplete_basic_form_subdomain.html]
 skip-if = toolkit == 'android' # android:autocomplete.
 scheme = https
+[test_autocomplete_hasBeenTypePassword.html]
+scheme = https
+skip-if = toolkit == 'android' # autocomplete
 [test_autocomplete_highlight.html]
 scheme = https
 skip-if = toolkit == 'android' # autocomplete

toolkit/components/passwordmgr/test/mochitest/test_autocomplete_hasBeenTypePassword.html

@@ -0,0 +1,117 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Test that passwords are autocompleted into fields that were previously type=password</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/AddTask.js"></script>
+  <script type="text/javascript" src="../../../satchel/test/satchel_common.js"></script>
+  <script type="text/javascript" src="pwmgr_common.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+Login Manager test: Test that passwords are autocompleted into fields that were previously type=password
+
+<script>
+var setupScript = runInParent(function setup() {
+  const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
+
+  // Add two logins to prevent autofill on page load.
+  let nsLoginInfo = Components.Constructor("@mozilla.org/login-manager/loginInfo;1",
+                                           Ci.nsILoginInfo, "init");
+  assert.ok(nsLoginInfo != null, "nsLoginInfo constructor");
+
+  let login1 = new nsLoginInfo("https://example.com", "https://example.com", null,
+                               "user1", "pass1");
+  let login2 = new nsLoginInfo("https://example.com", "https://example.com", null,
+                               "user2", "pass2");
+
+  Services.logins.addLogin(login1);
+  Services.logins.addLogin(login2);
+});
+</script>
+<p id="display"></p>
+
+<!-- we presumably can't hide the content for this test. -->
+<div id="content">
+
+  <!-- form1 tests multiple matching logins -->
+  <form id="form1" action="https://example.com/formtest.js" onsubmit="return false;">
+    <input  type="text"       name="uname">
+    <input  type="password"   name="pword">
+    <button type="submit">Submit</button>
+  </form>
+</div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+
+var uname = $_(1, "uname");
+var pword = $_(1, "pword");
+
+// Restore the form to the default state.
+function restoreForm() {
+  uname.value = "";
+  pword.value = "";
+  uname.focus();
+}
+
+function spinEventLoop() {
+  return Promise.resolve();
+}
+
+add_task(async function setup() {
+  listenForUnexpectedPopupShown();
+});
+
+add_task(async function test_form1_initial_empty() {
+  await SimpleTest.promiseFocus(window);
+
+  // Make sure initial form is empty.
+  checkLoginForm(uname, "", pword, "");
+  let popupState = await getPopupState();
+  is(popupState.open, false, "Check popup is initially closed");
+});
+
+add_task(async function test_form1_password_to_type_text() {
+  await SimpleTest.promiseFocus(window);
+  info("Setting the password field type to text");
+  // This is similar to a site implementing their own password visibility/unmasking toggle
+  pword.type = "text";
+
+  // Trigger autocomplete popup
+  restoreForm();
+  let shownPromise = promiseACShown();
+  synthesizeKey("KEY_ArrowDown"); // open
+  let results = await shownPromise;
+
+  let popupState = await getPopupState();
+  is(popupState.selectedIndex, -1, "Check no entries are selected upon opening");
+
+  checkAutoCompleteResults(results, ["user1", "user2"], "example.com", "Check all menuitems are displayed correctly.");
+
+  synthesizeKey("KEY_ArrowDown"); // first item
+  checkLoginForm(uname, "", pword, ""); // value shouldn't update just by selecting
+
+  synthesizeKey("KEY_Enter");
+
+  is(uname.value, "user1", "username should match the login, not the password");
+
+  restoreForm();
+  info("Focusing the password field");
+  pword.focus();
+
+  shownPromise = promiseACShown();
+  synthesizeKey("KEY_ArrowDown"); // open
+  await shownPromise;
+
+  synthesizeKey("KEY_ArrowDown"); // first item
+  checkLoginForm(uname, "", pword, ""); // value shouldn't update just by selecting
+
+  synthesizeKey("KEY_Enter");
+  is(pword.value, "pass1", "Password should match the login that was selected");
+});
+</script>
+</pre>
+</body>
+</html>

toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_doAutocompleteSearch.js

@@ -16,8 +16,8 @@ const NEW_PASSWORD_TEMPLATE_ARG = {
   searchString: "",
   previousResult: null,
   requestId: "foo",
+  hasBeenTypePassword: true,
   isSecure: true,
-  isPasswordField: true,
   isProbablyANewPasswordField: true,
 };
 
@@ -72,7 +72,7 @@ add_task(async function test_generated_noLogins() {
   let result3 = await LMP.doAutocompleteSearch({
     ...NEW_PASSWORD_TEMPLATE_ARG,
     ...{
-      isPasswordField: false,
+      hasBeenTypePassword: false,
       isProbablyANewPasswordField: false,
     },
   });

toolkit/components/passwordmgr/test/unit/test_login_autocomplete_result.js

@@ -104,7 +104,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: true,
       isSecure: true,
-      isPasswordField: false,
+      hasBeenTypePassword: false,
       matchingLogins,
       items: [
         {
@@ -154,7 +154,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: true,
       isSecure: false,
-      isPasswordField: false,
+      hasBeenTypePassword: false,
       matchingLogins: [],
       items: [
         {
@@ -175,7 +175,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: true,
       isSecure: false,
-      isPasswordField: false,
+      hasBeenTypePassword: false,
       matchingLogins,
       items: [
         {
@@ -232,7 +232,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: true,
       isSecure: true,
-      isPasswordField: true,
+      hasBeenTypePassword: true,
       matchingLogins,
       items: [
         {
@@ -282,7 +282,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: true,
       isSecure: false,
-      isPasswordField: true,
+      hasBeenTypePassword: true,
       matchingLogins,
       items: [
         {
@@ -339,7 +339,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: false,
       isSecure: true,
-      isPasswordField: false,
+      hasBeenTypePassword: false,
       matchingLogins,
       items: [
         {
@@ -389,7 +389,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: false,
       isSecure: false,
-      isPasswordField: false,
+      hasBeenTypePassword: false,
       matchingLogins,
       items: [
         {
@@ -439,7 +439,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: false,
       isSecure: true,
-      isPasswordField: true,
+      hasBeenTypePassword: true,
       matchingLogins,
       items: [
         {
@@ -489,7 +489,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: false,
       isSecure: false,
-      isPasswordField: true,
+      hasBeenTypePassword: true,
       matchingLogins,
       items: [
         {
@@ -539,7 +539,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: true,
       isSecure: true,
-      isPasswordField: false,
+      hasBeenTypePassword: false,
       matchingLogins,
       items: [
         {
@@ -589,7 +589,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: true,
       isSecure: false,
-      isPasswordField: false,
+      hasBeenTypePassword: false,
       matchingLogins,
       items: [
         {
@@ -646,7 +646,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: true,
       isSecure: true,
-      isPasswordField: true,
+      hasBeenTypePassword: true,
       matchingLogins,
       items: [
         {
@@ -696,7 +696,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: true,
       isSecure: false,
-      isPasswordField: true,
+      hasBeenTypePassword: true,
       matchingLogins,
       items: [
         {
@@ -753,7 +753,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: false,
       isSecure: true,
-      isPasswordField: false,
+      hasBeenTypePassword: false,
       matchingLogins,
       items: [
         {
@@ -803,7 +803,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: false,
       isSecure: false,
-      isPasswordField: false,
+      hasBeenTypePassword: false,
       matchingLogins: [],
       items: [
         {
@@ -817,7 +817,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: false,
       isSecure: false,
-      isPasswordField: false,
+      hasBeenTypePassword: false,
       matchingLogins: [],
       searchString: "foo",
       items: [
@@ -832,7 +832,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: false,
       isSecure: false,
-      isPasswordField: false,
+      hasBeenTypePassword: false,
       matchingLogins,
       items: [
         {
@@ -882,7 +882,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: false,
       isSecure: true,
-      isPasswordField: true,
+      hasBeenTypePassword: true,
       matchingLogins,
       items: [
         {
@@ -932,7 +932,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: false,
       isSecure: false,
-      isPasswordField: true,
+      hasBeenTypePassword: true,
       matchingLogins,
       items: [
         {
@@ -982,7 +982,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: true,
       isSecure: true,
-      isPasswordField: true,
+      hasBeenTypePassword: true,
       matchingLogins: [],
       items: [
         {
@@ -996,7 +996,7 @@ add_task(async function test_all_patterns() {
     {
       insecureFieldWarningEnabled: true,
       isSecure: true,
-      isPasswordField: true,
+      hasBeenTypePassword: true,
       matchingLogins: [],
       searchString: "foo",
       items: [],
@@ -1005,7 +1005,7 @@ add_task(async function test_all_patterns() {
       generatedPassword: "9ljgfd4shyktb45",
       insecureFieldWarningEnabled: true,
       isSecure: true,
-      isPasswordField: true,
+      hasBeenTypePassword: true,
       matchingLogins: [],
       items: [
         {
@@ -1032,7 +1032,7 @@ add_task(async function test_all_patterns() {
       willAutoSaveGeneratedPassword: true,
       insecureFieldWarningEnabled: true,
       isSecure: true,
-      isPasswordField: true,
+      hasBeenTypePassword: true,
       matchingLogins: [],
       items: [
         {
@@ -1058,7 +1058,7 @@ add_task(async function test_all_patterns() {
       generatedPassword: "9ljgfd4shyktb45",
       insecureFieldWarningEnabled: true,
       isSecure: true,
-      isPasswordField: true,
+      hasBeenTypePassword: true,
       matchingLogins: [],
       searchString: "9ljgfd4shyktb45",
       items: [
@@ -1084,7 +1084,7 @@ add_task(async function test_all_patterns() {
       formOrigin: "https://sub.mochi.test:8888",
       insecureFieldWarningEnabled: true,
       isSecure: true,
-      isPasswordField: false,
+      hasBeenTypePassword: false,
       matchingLogins,
       items: [
         {
@@ -1136,7 +1136,7 @@ add_task(async function test_all_patterns() {
       formOrigin: "https://sub.mochi.test:8888",
       insecureFieldWarningEnabled: true,
       isSecure: true,
-      isPasswordField: true,
+      hasBeenTypePassword: true,
       matchingLogins,
       items: [
         {
@@ -1188,7 +1188,7 @@ add_task(async function test_all_patterns() {
       formOrigin: "https://mochi.test:8888",
       schemeUpgrades: false,
       isSecure: true,
-      isPasswordField: false,
+      hasBeenTypePassword: false,
       matchingLogins,
       items: [
         {
@@ -1261,7 +1261,7 @@ add_task(async function test_all_patterns() {
         generatedPassword: pattern.generatedPassword,
         willAutoSaveGeneratedPassword: !!pattern.willAutoSaveGeneratedPassword,
         isSecure: pattern.isSecure,
-        isPasswordField: pattern.isPasswordField,
+        hasBeenTypePassword: pattern.hasBeenTypePassword,
         telemetryEventData: { searchStartTimeMS: testIndex },
       }
     );

toolkit/components/satchel/nsFormFillController.cpp

@@ -647,9 +647,8 @@ nsFormFillController::GetNoRollupOnCaretMove(bool* aNoRollupOnCaretMove) {
 
 NS_IMETHODIMP
 nsFormFillController::GetNoRollupOnEmptySearch(bool* aNoRollupOnEmptySearch) {
-  if (mFocusedInput &&
-      (mPwmgrInputs.Get(mFocusedInput) ||
-       mFocusedInput->ControlType() == NS_FORM_INPUT_PASSWORD)) {
+  if (mFocusedInput && (mPwmgrInputs.Get(mFocusedInput) ||
+                        mFocusedInput->HasBeenTypePassword())) {
     // Don't close the login popup when the field is cleared (bug 1534896).
     *aNoRollupOnEmptySearch = true;
   } else {
@@ -680,9 +679,8 @@ nsFormFillController::StartSearch(const nsAString& aSearchString,
   // handle the autocomplete. Otherwise, handle with form history.
   // This method is sometimes called in unit tests and from XUL without a
   // focused node.
-  if (mFocusedInput &&
-      (mPwmgrInputs.Get(mFocusedInput) ||
-       mFocusedInput->ControlType() == NS_FORM_INPUT_PASSWORD)) {
+  if (mFocusedInput && (mPwmgrInputs.Get(mFocusedInput) ||
+                        mFocusedInput->HasBeenTypePassword())) {
     MOZ_LOG(sLogger, LogLevel::Debug, ("StartSearch: login field"));
 
     // Handle the case where a password field is focused but
@@ -941,8 +939,7 @@ void nsFormFillController::MaybeStartControllingInput(
   bool hasList = !!aInput->GetList();
 
   bool isPwmgrInput = false;
-  if (mPwmgrInputs.Get(aInput) ||
-      aInput->ControlType() == NS_FORM_INPUT_PASSWORD) {
+  if (mPwmgrInputs.Get(aInput) || aInput->HasBeenTypePassword()) {
     isPwmgrInput = true;
   }
 
@@ -958,7 +955,7 @@ void nsFormFillController::MaybeStartControllingInput(
 #ifdef NIGHTLY_BUILD
   // Trigger an asynchronous login reputation query when user focuses on the
   // password field.
-  if (aInput->ControlType() == NS_FORM_INPUT_PASSWORD) {
+  if (aInput->HasBeenTypePassword()) {
     StartQueryLoginReputation(aInput);
   }
 #endif
@@ -981,7 +978,7 @@ nsresult nsFormFillController::HandleFocus(HTMLInputElement* aInput) {
   // multiple input forms and the fact that a mousedown into an already focused
   // field does not trigger another focus.
 
-  if (mFocusedInput->ControlType() != NS_FORM_INPUT_PASSWORD) {
+  if (!mFocusedInput->HasBeenTypePassword()) {
     return NS_OK;
   }