Bug 1538952 - Don't automatically open the password autocomplete popup when we only have the footer to show. r=jaws,mak

Normally autocomplete results are cached based upon the search string but to get the desired behaviour we want two different sets of results for the same search string depending on how the autocomplete search was started:
a) Via automatically focusing a password field.
b) Every other method of starting an autocomplete search.

In order to not have cached results used, the result code for case (a) [an empty result] will be `RESULT_FAILURE` and I've updated the autocomplete code to not re-use an error result.

In the coming months we may be rewriting our content autocomplete code but that would be too risky to uplift to 67 so for now I'm tracking when satchel automatically opens the popup upon focus and then using that state in the autocomplete result creation code to know whether to include the footer.

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

--HG--
extra : moz-landing-system : lando

toolkit/components/autocomplete/nsAutoCompleteController.cpp

@@ -509,9 +509,13 @@ nsAutoCompleteController::HandleKeyNavigation(uint32_t aKey, bool *_retval) {
 #endif
       if (*_retval) {
         nsAutoString oldSearchString;
-        // Open the popup if there has been a previous search, or else kick off
-        // a new search
+        uint16_t oldResult = 0;
+
+        // Open the popup if there has been a previous non-errored search, or
+        // else kick off a new search
         if (!mResults.IsEmpty() &&
+            NS_SUCCEEDED(mResults[0]->GetSearchResult(&oldResult)) &&
+            oldResult != nsIAutoCompleteResult::RESULT_FAILURE &&
             NS_SUCCEEDED(mResults[0]->GetSearchString(oldSearchString)) &&
             oldSearchString.Equals(mSearchString,
                                    nsCaseInsensitiveStringComparator())) {

toolkit/components/passwordmgr/LoginAutoCompleteResult.jsm

@@ -16,9 +16,12 @@ const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
 ChromeUtils.defineModuleGetter(this, "LoginHelper",
                                "resource://gre/modules/LoginHelper.jsm");
 
+XPCOMUtils.defineLazyServiceGetter(this, "formFillController",
+                                   "@mozilla.org/satchel/form-fill-controller;1",
+                                   Ci.nsIFormFillController);
+
 XPCOMUtils.defineLazyGetter(this, "log", () => {
-  let logger = LoginHelper.createLogger("LoginAutoCompleteResult");
-  return logger.log.bind(logger);
+  return LoginHelper.createLogger("LoginAutoCompleteResult");
 });
 
 // nsIAutoCompleteResult implementation
@@ -50,15 +53,32 @@ function LoginAutoCompleteResult(aSearchString, matchingLogins, {isSecure, messa
     return duplicates;
   }
 
-  this._showInsecureFieldWarning = (!isSecure && LoginHelper.showInsecureFieldWarning) ? 1 : 0;
-  this._showAutoCompleteFooter = 0;
-  // We need to check LoginHelper.enabled here since the insecure warning should
-  // appear even if pwmgr is disabled but the footer should never appear in that case.
-  if (LoginHelper.showAutoCompleteFooter && LoginHelper.enabled) {
+  let hidingFooterOnPWFieldAutoOpened = false;
+  function isFooterEnabled() {
+    // We need to check LoginHelper.enabled here since the insecure warning should
+    // appear even if pwmgr is disabled but the footer should never appear in that case.
+    if (!LoginHelper.showAutoCompleteFooter || !LoginHelper.enabled) {
+      return false;
+    }
+
     // 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.
-    this._showAutoCompleteFooter = (!isPasswordField || !aSearchString) ? 1 : 0;
+    if (isPasswordField && aSearchString) {
+      log.debug("Hiding footer: non-empty password field");
+      return false;
+    }
+
+    if (!matchingLogins.length && isPasswordField && formFillController.passwordPopupAutomaticallyOpened) {
+      hidingFooterOnPWFieldAutoOpened = true;
+      log.debug("Hiding footer: no logins and the popup was opened upon focus of the pw. field");
+      return false;
+    }
+
+    return true;
   }
+
+  this._showInsecureFieldWarning = (!isSecure && LoginHelper.showInsecureFieldWarning) ? 1 : 0;
+  this._showAutoCompleteFooter = isFooterEnabled() ? 1 : 0;
   this.searchString = aSearchString;
   this.logins = matchingLogins.sort(loginSort);
   this.matchCount = matchingLogins.length + this._showInsecureFieldWarning + this._showAutoCompleteFooter;
@@ -74,6 +94,11 @@ function LoginAutoCompleteResult(aSearchString, matchingLogins, {isSecure, messa
   if (this.matchCount > 0) {
     this.searchResult = Ci.nsIAutoCompleteResult.RESULT_SUCCESS;
     this.defaultIndex = 0;
+  } else if (hidingFooterOnPWFieldAutoOpened) {
+    // We use a failure result so that the empty results aren't re-used for when
+    // the user tries to manually open the popup (we want the footer in that case).
+    this.searchResult = Ci.nsIAutoCompleteResult.RESULT_FAILURE;
+    this.defaultIndex = -1;
   }
 }
 

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

@@ -29,6 +29,9 @@ scheme = https
 skip-if = toolkit == 'android' # autocomplete
 [test_autocomplete_https_upgrade.html]
 skip-if = toolkit == 'android' # autocomplete
+[test_autocomplete_password_open.html]
+scheme = https
+skip-if = toolkit == 'android' # autocomplete
 [test_autocomplete_sandboxed.html]
 scheme = https
 skip-if = toolkit == 'android' # autocomplete

toolkit/components/passwordmgr/test/mochitest/pwmgr_common.js

@@ -43,10 +43,17 @@ function $_(formNum, name) {
  * values are being shown correctly as items in the popup.
  */
 function checkAutoCompleteResults(actualValues, expectedValues, hostname, msg) {
-  // Check the footer first.
-  let footerResult = actualValues[actualValues.length - 1];
-  ok(footerResult.includes("View Saved Logins"), "the footer text is shown correctly");
-  ok(footerResult.includes(hostname), "the footer has the correct hostname attribute");
+  if (hostname !== null) {
+    // Check the footer first.
+    let footerResult = actualValues[actualValues.length - 1];
+    ok(footerResult.includes("View Saved Logins"), "the footer text is shown correctly");
+    ok(footerResult.includes(hostname), "the footer has the correct hostname attribute");
+  }
+
+  if (hostname === null) {
+    checkArrayValues(actualValues, expectedValues, msg);
+    return;
+  }
 
   if (actualValues.length == 0) {
     info("Only the footer is present in the popup");

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

@@ -0,0 +1,118 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Test password field autocomplete footer with and without logins</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>
+
+<script>
+var chromeScript = runChecksAfterCommonInit();
+
+var setupScript = runInParent(function setup() {
+  const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
+
+  var nsLoginInfo = Components.Constructor("@mozilla.org/login-manager/loginInfo;1",
+                                           Ci.nsILoginInfo, "init");
+  assert.ok(nsLoginInfo != null, "nsLoginInfo constructor");
+
+  var login1 = new nsLoginInfo("https://example.com", "", null,
+                               "tempuser1", "temppass1", "uname", "pword");
+  var login2 = new nsLoginInfo("https://example.com", "", null,
+                               "tempuser2", "temppass2", "uname", "pword");
+
+  // try/catch in case someone runs the tests manually, twice.
+  try {
+    Services.logins.addLogin(login1);
+    Services.logins.addLogin(login2);
+  } catch (e) {
+    assert.ok(false, "addLogin threw: " + e);
+  }
+});
+</script>
+<p id="display"></p>
+
+<div id="content">
+  <input id="uname"/>
+  <input id="pword" type="password"/>
+</div>
+
+<pre id="test">
+<script class="testbody" type="text/javascript">
+
+/** Test for Login Manager: Test password field autocomplete footer with and without logins **/
+
+let uname = document.getElementById("uname");
+let pword = document.getElementById("pword");
+
+// Check for expected username/password in form.
+function checkACForm(expectedUsername, expectedPassword) {
+  var formID = uname.parentNode.id;
+  is(uname.value, expectedUsername, "Checking " + formID + " username is: " + expectedUsername);
+  is(pword.value, expectedPassword, "Checking " + formID + " password is: " + expectedPassword);
+}
+
+add_task(async function test_no_autofill() {
+  // Make sure initial form is empty as autofill shouldn't happen in the sandboxed frame.
+  checkACForm("", "");
+  let popupState = await getPopupState();
+  is(popupState.open, false, "Check popup is initially closed");
+});
+
+add_task(async function test_two_logins() {
+  let shownPromise = promiseACShown();
+  uname.focus();
+  await shownPromise;
+
+  shownPromise = promiseACShown();
+  synthesizeKey("KEY_Tab"); // popup on the password field should open upon focus
+  let results = await shownPromise;
+
+  let popupState = await getPopupState();
+  is(popupState.selectedIndex, -1, "Check no entries are selected upon opening");
+
+  let expectedMenuItems = [
+    "tempuser1",
+    "tempuser2",
+  ];
+  checkAutoCompleteResults(results, expectedMenuItems, "example.com", "Check all menuitems are displayed correctly.");
+
+  checkACForm("", "");
+  let removedPromise = promiseStorageChanged(["removeAllLogins"]);
+  LoginManager.removeAllLogins();
+  await removedPromise;
+});
+
+add_task(async function test_zero_logins() {
+  uname.focus();
+
+  let shownPromise = promiseACShown().then(() => ok(false, "Should not have shown"));
+  // Popup on the password field should NOT automatically open upon focus when there are no saved logins.
+  synthesizeKey("KEY_Tab"); // focus the password field
+  SimpleTest.requestFlakyTimeout("Giving a chance for the unexpected popup to show");
+  let results = await Promise.race([
+    shownPromise,
+    new Promise(resolve => setTimeout(resolve, 2000)), // Wait 2s for the popup to appear
+  ]);
+
+  let popupState = await getPopupState();
+  is(popupState.open, false, "Check popup is still closed");
+
+  checkACForm("", "");
+  shownPromise = promiseACShown();
+  info("arrow down should still open the popup");
+  synthesizeKey("KEY_ArrowDown");
+  results = await shownPromise;
+  checkAutoCompleteResults(results, [], "example.com", "Check only footer is displayed.");
+  checkACForm("", "");
+});
+</script>
+</pre>
+</body>
+</html>

toolkit/components/satchel/nsFormFillController.cpp

@@ -97,7 +97,8 @@ nsFormFillController::nsFormFillController()
       mCompleteDefaultIndex(false),
       mCompleteSelectedIndex(false),
       mForceComplete(false),
-      mSuppressOnInput(false) {
+      mSuppressOnInput(false),
+      mPasswordPopupAutomaticallyOpened(false) {
   mController = do_GetService("@mozilla.org/autocomplete/controller;1");
   MOZ_ASSERT(mController);
 }
@@ -393,6 +394,7 @@ nsFormFillController::SetPopupOpen(bool aPopupOpen) {
       }
     } else {
       mFocusedPopup->ClosePopup();
+      mPasswordPopupAutomaticallyOpened = false;
     }
   }
 
@@ -628,7 +630,10 @@ nsFormFillController::OnTextEntered(Event* aEvent, bool itemWasSelected,
 }
 
 NS_IMETHODIMP
-nsFormFillController::OnTextReverted(bool* _retval) { return NS_OK; }
+nsFormFillController::OnTextReverted(bool* _retval) {
+  mPasswordPopupAutomaticallyOpened = false;
+  return NS_OK;
+}
 
 NS_IMETHODIMP
 nsFormFillController::GetConsumeRollupEvent(bool* aConsumeRollupEvent) {
@@ -997,6 +1002,7 @@ nsresult nsFormFillController::HandleFocus(HTMLInputElement* aInput) {
 
   // If we have not seen a right click yet, just show the popup.
   if (mLastRightClickTimeStamp.IsNull()) {
+    mPasswordPopupAutomaticallyOpened = true;
     ShowPopup();
     return NS_OK;
   }
@@ -1004,6 +1010,7 @@ nsresult nsFormFillController::HandleFocus(HTMLInputElement* aInput) {
   uint64_t timeDiff =
       (TimeStamp::Now() - mLastRightClickTimeStamp).ToMilliseconds();
   if (timeDiff > mFocusAfterRightClickThreshold) {
+    mPasswordPopupAutomaticallyOpened = true;
     ShowPopup();
   }
 #endif
@@ -1018,6 +1025,9 @@ nsresult nsFormFillController::Focus(Event* aEvent) {
 
 nsresult nsFormFillController::KeyDown(Event* aEvent) {
   NS_ASSERTION(mController, "should have a controller!");
+
+  mPasswordPopupAutomaticallyOpened = false;
+
   if (!mFocusedInput || !mController) {
     return NS_OK;
   }
@@ -1051,6 +1061,9 @@ nsresult nsFormFillController::KeyDown(Event* aEvent) {
 
 nsresult nsFormFillController::KeyPress(Event* aEvent) {
   NS_ASSERTION(mController, "should have a controller!");
+
+  mPasswordPopupAutomaticallyOpened = false;
+
   if (!mFocusedInput || !mController) {
     return NS_OK;
   }
@@ -1200,6 +1213,12 @@ nsFormFillController::ShowPopup() {
   return NS_OK;
 }
 
+NS_IMETHODIMP nsFormFillController::GetPasswordPopupAutomaticallyOpened(
+    bool* _retval) {
+  *_retval = mPasswordPopupAutomaticallyOpened;
+  return NS_OK;
+}
+
 ////////////////////////////////////////////////////////////////////////
 //// nsFormFillController
 
@@ -1323,6 +1342,8 @@ void nsFormFillController::StartControllingInput(HTMLInputElement* aInput) {
 }
 
 void nsFormFillController::StopControllingInput() {
+  mPasswordPopupAutomaticallyOpened = false;
+
   if (mListNode) {
     mListNode->RemoveMutationObserver(this);
     mListNode = nullptr;

toolkit/components/satchel/nsFormFillController.h

@@ -138,6 +138,7 @@ class nsFormFillController final : public nsIFormFillController,
   bool mCompleteSelectedIndex;
   bool mForceComplete;
   bool mSuppressOnInput;
+  bool mPasswordPopupAutomaticallyOpened;
 };
 
 #endif  // __nsFormFillController__

toolkit/components/satchel/nsIFormFillController.idl

@@ -6,8 +6,8 @@
 
 interface nsIDocShell;
 interface nsIAutoCompletePopup;
-webidl Element;
 
+webidl Element;
 webidl HTMLInputElement;
 
 /*
@@ -27,6 +27,12 @@ interface nsIFormFillController : nsISupports
    */
   readonly attribute HTMLInputElement focusedInput;
 
+  /*
+   * Whether the autocomplete popup on a password field was automatically opened
+   * by the form fill controller (upon focus).
+   */
+  readonly attribute boolean passwordPopupAutomaticallyOpened;
+
   /*
    * Start controlling form fill behavior for the given browser
    *