Bug 1576276 - Persist sort direction in about:logins. r=sfoster

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

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

browser/app/profile/firefox.js

@@ -1784,6 +1784,7 @@ pref("signon.privateBrowsingCapture.enabled", true);
 pref("signon.showAutoCompleteFooter", true);
 pref("signon.management.page.enabled", true);
 pref("signon.management.page.breach-alerts.enabled", true);
+pref("signon.management.page.sort", "name");
 pref("signon.management.overrideURI", "about:logins?filter=%DOMAIN%");
 #ifdef NIGHTLY_BUILD
   // Bug 1563330 tracks shipping this by default.

browser/components/BrowserGlue.jsm

@@ -155,6 +155,7 @@ let LEGACY_ACTORS = {
         AboutLoginsOpenPreferences: { wantUntrusted: true },
         AboutLoginsOpenSite: { wantUntrusted: true },
         AboutLoginsRecordTelemetryEvent: { wantUntrusted: true },
+        AboutLoginsSortChanged: { wantUntrusted: true },
         AboutLoginsSyncEnable: { wantUntrusted: true },
         AboutLoginsSyncOptions: { wantUntrusted: true },
         AboutLoginsUpdateLogin: { wantUntrusted: true },
@@ -623,6 +624,7 @@ const listeners = {
     "AboutLogins:OpenMobileAndroid": ["AboutLoginsParent"],
     "AboutLogins:OpenMobileIos": ["AboutLoginsParent"],
     "AboutLogins:OpenSite": ["AboutLoginsParent"],
+    "AboutLogins:SortChanged": ["AboutLoginsParent"],
     "AboutLogins:Subscribe": ["AboutLoginsParent"],
     "AboutLogins:SyncEnable": ["AboutLoginsParent"],
     "AboutLogins:SyncOptions": ["AboutLoginsParent"],

browser/components/aboutlogins/AboutLoginsChild.jsm

@@ -109,6 +109,10 @@ class AboutLoginsChild extends ActorChild {
         });
         break;
       }
+      case "AboutLoginsGetHelp": {
+        this.mm.sendAsyncMessage("AboutLogins:GetHelp");
+        break;
+      }
       case "AboutLoginsHideFooter": {
         this.mm.sendAsyncMessage("AboutLogins:HideFooter");
         break;
@@ -129,10 +133,6 @@ class AboutLoginsChild extends ActorChild {
         });
         break;
       }
-      case "AboutLoginsGetHelp": {
-        this.mm.sendAsyncMessage("AboutLogins:GetHelp");
-        break;
-      }
       case "AboutLoginsOpenPreferences": {
         this.mm.sendAsyncMessage("AboutLogins:OpenPreferences");
         break;
@@ -180,6 +180,10 @@ class AboutLoginsChild extends ActorChild {
         }
         break;
       }
+      case "AboutLoginsSortChanged": {
+        this.mm.sendAsyncMessage("AboutLogins:SortChanged", event.detail);
+        break;
+      }
       case "AboutLoginsSyncEnable": {
         this.mm.sendAsyncMessage("AboutLogins:SyncEnable");
         break;

browser/components/aboutlogins/AboutLoginsParent.jsm

@@ -251,6 +251,10 @@ var AboutLoginsParent = {
         Services.prefs.setBoolPref(HIDE_MOBILE_FOOTER_PREF, true);
         break;
       }
+      case "AboutLogins:SortChanged": {
+        Services.prefs.setCharPref("signon.management.page.sort", message.data);
+        break;
+      }
       case "AboutLogins:SyncEnable": {
         message.target.ownerGlobal.gSync.openFxAEmailFirstPage(
           "password-manager"
@@ -408,6 +412,10 @@ var AboutLoginsParent = {
 
           messageManager.sendAsyncMessage("AboutLogins:Setup", {
             logins,
+            selectedSort: Services.prefs.getCharPref(
+              "signon.management.page.sort",
+              "name"
+            ),
             syncState,
             selectedBadgeLanguages,
             masterPasswordEnabled: LoginHelper.isMasterPasswordSet(),

browser/components/aboutlogins/content/aboutLogins.html

@@ -85,10 +85,10 @@
         <label for="login-sort">
           <span data-l10n-id="login-list-sort-label-text"></span>
           <select id="login-sort">
-            <option name="name" data-l10n-id="login-list-name-option" value="name"/>
-            <option name="last-user" data-l10n-id="login-list-last-used-option" value="last-used"/>
-            <option name="last-changed" data-l10n-id="login-list-last-changed-option" value="last-changed"/>
-            <option name="breached" data-l10n-id="login-list-breached-option" value="breached" hidden/>
+            <option name="name" data-l10n-id="login-list-name-option" value="name">
+            <option name="last-used" data-l10n-id="login-list-last-used-option" value="last-used">
+            <option name="last-changed" data-l10n-id="login-list-last-changed-option" value="last-changed">
+            <option name="breached" data-l10n-id="login-list-breached-option" value="breached" hidden>
           </select>
         </label>
         <span class="count" data-l10n-id="login-list-count" data-l10n-args='{"count": 0}'></span>

browser/components/aboutlogins/content/aboutLogins.js

@@ -81,6 +81,7 @@ window.addEventListener("AboutLoginsChromeToContent", event => {
         event.detail.value.selectedBadgeLanguages
       );
       handleSyncState(event.detail.value.syncState);
+      gElements.loginList.setSortDirection(event.detail.value.selectedSort);
       document.documentElement.classList.add("initialized");
       break;
     }

browser/components/aboutlogins/content/components/login-list.js

@@ -174,6 +174,12 @@ export default class LoginList extends HTMLElement {
         this._applySortAndScrollToTop();
         const extra = { sort_key: this._sortSelect.value };
         recordTelemetryEvent({ object: "list", method: "sort", extra });
+        document.dispatchEvent(
+          new CustomEvent("AboutLoginsSortChanged", {
+            bubbles: true,
+            detail: this._sortSelect.value,
+          })
+        );
         break;
       }
       case "AboutLoginsClearSelection": {
@@ -291,18 +297,7 @@ export default class LoginList extends HTMLElement {
     this.render();
 
     if (!this._selectedGuid || !this._logins[this._selectedGuid]) {
-      // Select the first visible login after any possible filter is applied.
-      let firstVisibleListItem = this._list.querySelector(
-        ".login-list-item[data-guid]:not([hidden])"
-      );
-      if (firstVisibleListItem) {
-        let { login } = this._logins[firstVisibleListItem.dataset.guid];
-        window.dispatchEvent(
-          new CustomEvent("AboutLoginsInitialLoginSelected", {
-            detail: login,
-          })
-        );
-      }
+      this._selectFirstVisibleLogin();
     }
   }
 
@@ -364,6 +359,12 @@ export default class LoginList extends HTMLElement {
     this.setBreaches(this._breachesByLoginGUID);
   }
 
+  setSortDirection(sortDirection) {
+    this._sortSelect.value = sortDirection;
+    this._applySortAndScrollToTop();
+    this._selectFirstVisibleLogin();
+  }
+
   /**
    * @param {login} login A login that was added to storage.
    */
@@ -586,6 +587,25 @@ export default class LoginList extends HTMLElement {
     newlyFocusedItem.scrollIntoView({ block: "nearest" });
   }
 
+  /**
+   * Selects the first visible login as part of the initial load of the page,
+   * which will bypass any focus changes that occur during manual login
+   * selection.
+   */
+  _selectFirstVisibleLogin() {
+    let firstVisibleListItem = this._list.querySelector(
+      ".login-list-item[data-guid]:not([hidden])"
+    );
+    if (firstVisibleListItem) {
+      let { login } = this._logins[firstVisibleListItem.dataset.guid];
+      window.dispatchEvent(
+        new CustomEvent("AboutLoginsInitialLoginSelected", {
+          detail: login,
+        })
+      );
+    }
+  }
+
   _setListItemAsSelected(listItem) {
     let oldSelectedItem = this._list.querySelector(".selected");
     if (oldSelectedItem) {

browser/components/aboutlogins/tests/browser/browser.ini

@@ -21,6 +21,7 @@ skip-if = asan || debug || verify # bug 1574023
 [browser_loginItemErrors.js]
 skip-if = debug # Bug 1577710
 [browser_loginListChanges.js]
+[browser_loginSortOrderRestored.js]
 [browser_masterPassword.js]
 skip-if = (os == 'linux') # bug 1569789
 [browser_noLoginsView.js]

browser/components/aboutlogins/tests/browser/browser_aaa_eventTelemetry_run_first.js

@@ -169,6 +169,9 @@ add_task(async function test_telemetry_events() {
     loginSort.dispatchEvent(new content.Event("change", { bubbles: true }));
   });
   await LoginTestUtils.telemetry.waitForEventCount(14);
+  registerCleanupFunction(() => {
+    Services.prefs.clearUserPref("signon.management.page.sort");
+  });
 
   await ContentTask.spawn(gBrowser.selectedBrowser, null, async function() {
     let loginFilter = content.document.querySelector("login-filter");

browser/components/aboutlogins/tests/browser/browser_loginSortOrderRestored.js

+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+ChromeUtils.import("resource://testing-common/TelemetryTestUtils.jsm", this);
+ChromeUtils.import("resource://testing-common/LoginTestUtils.jsm", this);
+
+add_task(async function setup() {
+  TEST_LOGIN2.QueryInterface(Ci.nsILoginMetaInfo).timePasswordChanged = 1;
+  TEST_LOGIN1 = await addLogin(TEST_LOGIN1);
+  info(`TEST_LOGIN1 added with guid=${TEST_LOGIN1.guid}`);
+  TEST_LOGIN2 = await addLogin(TEST_LOGIN2);
+  info(`TEST_LOGIN2 added with guid=${TEST_LOGIN2.guid}`);
+  registerCleanupFunction(() => {
+    Services.logins.removeAllLogins();
+    Services.prefs.clearUserPref("signon.management.page.sort");
+  });
+});
+
+add_task(async function test_sort_order_persisted() {
+  await BrowserTestUtils.withNewTab(
+    {
+      gBrowser,
+      url: "about:logins",
+    },
+    async function(browser) {
+      await ContentTask.spawn(
+        browser,
+        [TEST_LOGIN1, TEST_LOGIN2],
+        async function([testLogin1, testLogin2]) {
+          let loginList = Cu.waiveXrays(
+            content.document.querySelector("login-list")
+          );
+          is(
+            loginList._sortSelect.value,
+            "name",
+            "default selected sort should be 'name'"
+          );
+          is(
+            loginList._list.querySelector(
+              ".login-list-item[data-guid]:not([hidden])"
+            ).dataset.guid,
+            testLogin2.guid,
+            "the first login should be TEST_LOGIN2 since they are sorted by origin"
+          );
+
+          loginList._sortSelect.value = "last-changed";
+          loginList._sortSelect.dispatchEvent(
+            new content.Event("change", { bubbles: true })
+          );
+          is(
+            loginList._list.querySelector(
+              ".login-list-item[data-guid]:not([hidden])"
+            ).dataset.guid,
+            testLogin1.guid,
+            "the first login should be TEST_LOGIN1 since it has the most recent timePasswordChanged value"
+          );
+        }
+      );
+    }
+  );
+
+  await BrowserTestUtils.withNewTab(
+    {
+      gBrowser,
+      url: "about:logins",
+    },
+    async function(browser) {
+      await ContentTask.spawn(browser, TEST_LOGIN1, async function(testLogin1) {
+        let loginList = Cu.waiveXrays(
+          content.document.querySelector("login-list")
+        );
+        is(
+          loginList._sortSelect.value,
+          "last-changed",
+          "selected sort should be restored to 'last-changed'"
+        );
+        is(
+          loginList._list.querySelector(
+            ".login-list-item[data-guid]:not([hidden])"
+          ).dataset.guid,
+          testLogin1.guid,
+          "the first login should still be TEST_LOGIN1 since it has the most recent timePasswordChanged value"
+        );
+      });
+    }
+  );
+});