Bug 1924349 - Add other sorting options in History sidebar r=sidebar-reviewers,fluent-reviewers,desktop-theme-reviewers,fxview-reviewers,reusable-components-reviewers,bolsson,hjones,nsharpley

- Process history sorted by date and site, which is a nested mapping of (Date) => (Site) => Visit[], as opposed to sorting by date which is (Date) => Visit[], or site which is (Site) => Visit[].
- Also process history sorted by last visited, which is a "flat" array of Visit[], sorted by recency.
- A couple of minor <moz-card> tweaks to bring the "inner" cards closer to spec, but I think more work is still needed here.

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

browser/base/content/main-popupset.inc.xhtml

@@ -319,12 +319,20 @@
   </menupopup>
 
   <menupopup id="sidebar-history-menu">
-    <menuitem data-l10n-id="sidebar-history-sort-by-date"
+    <html:h1 data-l10n-id="sidebar-history-sort-by-heading"
+             id="sidebar-history-sort-by-heading"/>
+    <menuitem data-l10n-id="sidebar-history-sort-option-date"
               id="sidebar-history-sort-by-date"
               type="checkbox"/>
-    <menuitem data-l10n-id="sidebar-history-sort-by-site"
+    <menuitem data-l10n-id="sidebar-history-sort-option-site"
               id="sidebar-history-sort-by-site"
               type="checkbox"/>
+    <menuitem data-l10n-id="sidebar-history-sort-option-date-and-site"
+              id="sidebar-history-sort-by-date-and-site"
+              type="checkbox"/>
+    <menuitem data-l10n-id="sidebar-history-sort-option-last-visited"
+              id="sidebar-history-sort-by-last-visited"
+              type="checkbox"/>
     <menuseparator/>
     <menuitem data-l10n-id="sidebar-history-clear"
               id="sidebar-history-clear"/>

browser/components/firefoxview/HistoryController.sys.mjs

@@ -37,13 +37,22 @@ const HISTORY_MAP_L10N_IDS = {
   },
 };
 
+/**
+ * When sorting by date or site, each card "item" is a single visit.
+ *
+ * When sorting by date *and* site, each card "item" is a mapping of site
+ * domains to their respective list of visits.
+ *
+ * @typedef {HistoryVisit | [string, HistoryVisit[]]} CardItem
+ */
+
 /**
  * A list of visits displayed on a card.
  *
  * @typedef {object} CardEntry
  *
  * @property {string} domain
- * @property {HistoryVisit[]} items
+ * @property {CardItem[]} items
  * @property {string} l10nId
  */
 
@@ -127,7 +136,7 @@ export class HistoryController {
   /**
    * Update cached history.
    *
-   * @param {Map<CacheKey, HistoryVisit[]>} [historyMap]
+   * @param {CachedHistory} [historyMap]
    *   If provided, performs an update using the given data (instead of fetching
    *   it from the db).
    */
@@ -146,7 +155,19 @@ export class HistoryController {
     }
     for (const { items } of entries) {
       for (const item of items) {
-        this.#normalizeVisit(item);
+        switch (sortOption) {
+          case "datesite": {
+            // item is a [ domain, visit[] ] entry.
+            const [, visits] = item;
+            for (const visit of visits) {
+              this.#normalizeVisit(visit);
+            }
+            break;
+          }
+          default:
+            // item is a single visit.
+            this.#normalizeVisit(item);
+        }
       }
     }
     this.historyCache = { entries, searchQuery, sortOption };
@@ -203,6 +224,11 @@ export class HistoryController {
         return this.#getVisitsForDate(historyMap);
       case "site":
         return this.#getVisitsForSite(historyMap);
+      case "datesite":
+        this.#setTodaysDate();
+        return this.#getVisitsForDateSite(historyMap);
+      case "lastvisited":
+        return this.#getVisitsForLastVisited(historyMap);
       default:
         return [];
     }
@@ -285,9 +311,9 @@ export class HistoryController {
    * Get a list of visits per day for each day on this month, excluding today
    * and yesterday.
    *
-   * @param {Map<number, HistoryVisit[]>} cachedHistory
+   * @param {CachedHistory} cachedHistory
    *   The history cache to process.
-   * @returns {HistoryVisit[][]}
+   * @returns {CardItem[]}
    *   A list of visits for each day.
    */
   #getVisitsByDay(cachedHistory) {
@@ -313,9 +339,9 @@ export class HistoryController {
    * excluding yesterday's visits if yesterday happens to fall on the previous
    * month.
    *
-   * @param {Map<number, HistoryVisit[]>} cachedHistory
+   * @param {CachedHistory} cachedHistory
    *   The history cache to process.
-   * @returns {HistoryVisit[][]}
+   * @returns {CardItem[]}
    *   A list of visits for each month.
    */
   #getVisitsByMonth(cachedHistory) {
@@ -332,6 +358,12 @@ export class HistoryController {
       const month = this.placesQuery.getStartOfMonthTimestamp(date);
       if (month !== previousMonth) {
         visitsPerMonth.push(visits);
+      } else if (this.sortOption === "datesite") {
+        // CardItem type is currently Map<string, HistoryVisit[]>.
+        visitsPerMonth[visitsPerMonth.length - 1] = this.#mergeMaps(
+          visitsPerMonth.at(-1),
+          visits
+        );
       } else {
         visitsPerMonth[visitsPerMonth.length - 1] = visitsPerMonth
           .at(-1)
@@ -372,6 +404,22 @@ export class HistoryController {
     );
   }
 
+  /**
+   * Merge two maps of (domain: string) => HistoryVisit[] into a single map.
+   *
+   * @param {Map<string, HistoryVisit[]>} oldMap
+   * @param {Map<string, HistoryVisit[]>} newMap
+   * @returns {Map<string, HistoryVisit[]>}
+   */
+  #mergeMaps(oldMap, newMap) {
+    const map = new Map(oldMap);
+    for (const [domain, newVisits] of newMap) {
+      const oldVisits = map.get(domain);
+      map.set(domain, oldVisits?.concat(newVisits) ?? newVisits);
+    }
+    return map;
+  }
+
   /**
    * Get a list of visits, sorted by site, in alphabetical order.
    *
@@ -386,6 +434,75 @@ export class HistoryController {
     })).sort((a, b) => a.domain.localeCompare(b.domain));
   }
 
+  /**
+   * Get a list of visits, sorted by date and site, in reverse chronological
+   * order.
+   *
+   * @param {Map<number, Map<string, HistoryVisit[]>>} historyMap
+   * @returns {CardEntry[]}
+   */
+  #getVisitsForDateSite(historyMap) {
+    const entries = [];
+    const visitsFromToday = this.#getVisitsFromToday(historyMap);
+    const visitsFromYesterday = this.#getVisitsFromYesterday(historyMap);
+    const visitsByDay = this.#getVisitsByDay(historyMap);
+    const visitsByMonth = this.#getVisitsByMonth(historyMap);
+
+    /**
+     * Sorts items alphabetically by domain name.
+     *
+     * @param {[string, HistoryVisit[]][]} items
+     * @returns {[string, HistoryVisit[]][]} The items in sorted order.
+     */
+    function sortItems(items) {
+      return items.sort(([aDomain], [bDomain]) =>
+        aDomain.localeCompare(bDomain)
+      );
+    }
+
+    // Add visits from today and yesterday.
+    if (visitsFromToday.length) {
+      entries.push({
+        l10nId: HISTORY_MAP_L10N_IDS[this.component]["history-date-today"],
+        items: sortItems(visitsFromToday),
+      });
+    }
+    if (visitsFromYesterday.length) {
+      entries.push({
+        l10nId: HISTORY_MAP_L10N_IDS[this.component]["history-date-yesterday"],
+        items: sortItems(visitsFromYesterday),
+      });
+    }
+
+    // Add visits from this month, grouped by day.
+    visitsByDay.forEach(visits => {
+      entries.push({
+        l10nId: HISTORY_MAP_L10N_IDS[this.component]["history-date-this-month"],
+        items: sortItems([...visits]),
+      });
+    });
+
+    // Add visits from previous months, grouped by month.
+    visitsByMonth.forEach(visits => {
+      entries.push({
+        l10nId: HISTORY_MAP_L10N_IDS[this.component]["history-date-prev-month"],
+        items: sortItems([...visits]),
+      });
+    });
+
+    return entries;
+  }
+
+  /**
+   * Get a list of visits sorted by recency.
+   *
+   * @param {HistoryVisit[]} items
+   * @returns {CardEntry[]}
+   */
+  #getVisitsForLastVisited(items) {
+    return [{ items }];
+  }
+
   async #fetchHistory() {
     return this.placesQuery.getHistory({
       daysOld: 60,

browser/components/sidebar/sidebar-history.css

@@ -15,3 +15,8 @@ fxview-search-textbox {
 .menu-button::part(button) {
   margin-inline-start: var(--space-small);
 }
+
+.nested-card {
+  --card-accordion-closed-icon: url("chrome://global/skin/icons/arrow-right.svg");
+  --card-accordion-open-icon: url("chrome://global/skin/icons/arrow-down.svg");
+}

browser/components/sidebar/sidebar-history.mjs

@@ -45,6 +45,12 @@ export class SidebarHistory extends SidebarPage {
     this._menu = doc.getElementById("sidebar-history-menu");
     this._menuSortByDate = doc.getElementById("sidebar-history-sort-by-date");
     this._menuSortBySite = doc.getElementById("sidebar-history-sort-by-site");
+    this._menuSortByDateSite = doc.getElementById(
+      "sidebar-history-sort-by-date-and-site"
+    );
+    this._menuSortByLastVisited = doc.getElementById(
+      "sidebar-history-sort-by-last-visited"
+    );
     this._menu.addEventListener("command", this);
     this._menu.addEventListener("popuphidden", this.handlePopupEvent);
     this.addContextMenuListeners();
@@ -75,6 +81,12 @@ export class SidebarHistory extends SidebarPage {
       case "sidebar-history-sort-by-site":
         this.controller.onChangeSortOption(e, "site");
         break;
+      case "sidebar-history-sort-by-date-and-site":
+        this.controller.onChangeSortOption(e, "datesite");
+        break;
+      case "sidebar-history-sort-by-last-visited":
+        this.controller.onChangeSortOption(e, "lastvisited");
+        break;
       case "sidebar-history-clear":
         lazy.Sanitizer.showUI(this.topWindow);
         break;
@@ -161,39 +173,63 @@ export class SidebarHistory extends SidebarPage {
     const { historyVisits } = this.controller;
     switch (this.controller.sortOption) {
       case "date":
-        return historyVisits.map(({ l10nId, items }, i) => {
+        return historyVisits.map(({ l10nId, items }, i) =>
-          let tabIndex = i > 0 ? "-1" : undefined;
+          this.#dateCardTemplate(l10nId, i, items)
-          return html` <moz-card
+        );
-            type="accordion"
-            ?expanded=${i < DAYS_EXPANDED_INITIALLY}
-            data-l10n-id=${l10nId}
-            data-l10n-args=${JSON.stringify({
-              date: items[0].time,
-            })}
-            @keydown=${this.handleCardKeydown}
-            tabindex=${ifDefined(tabIndex)}
-          >
-            ${this.#tabListTemplate(this.getTabItems(items))}
-          </moz-card>`;
-        });
       case "site":
-        return historyVisits.map(({ domain, items }, i) => {
+        return historyVisits.map(({ domain, items }, i) =>
-          let tabIndex = i > 0 ? "-1" : undefined;
+          this.#siteCardTemplate(domain, i, items)
-          return html` <moz-card
+        );
-            type="accordion"
+      case "datesite":
-            expanded
+        return historyVisits.map(({ l10nId, items }, i) =>
-            heading=${domain}
+          this.#dateCardTemplate(l10nId, i, items, true)
-            @keydown=${this.handleCardKeydown}
+        );
-            tabindex=${ifDefined(tabIndex)}
+      case "lastvisited":
-          >
+        return historyVisits.map(
-            ${this.#tabListTemplate(this.getTabItems(items))}
+          ({ items }) =>
-          </moz-card>`;
+            html`<moz-card>
-        });
+              ${this.#tabListTemplate(this.getTabItems(items))}
+            </moz-card>`
+        );
       default:
         return [];
     }
   }
 
+  #dateCardTemplate(l10nId, index, items, isDateSite = false) {
+    const tabIndex = index > 0 ? "-1" : undefined;
+    return html` <moz-card
+      type="accordion"
+      ?expanded=${index < DAYS_EXPANDED_INITIALLY}
+      data-l10n-id=${l10nId}
+      data-l10n-args=${JSON.stringify({
+        date: isDateSite ? items[0][1][0].time : items[0].time,
+      })}
+      @keydown=${this.handleCardKeydown}
+      tabindex=${ifDefined(tabIndex)}
+    >
+      ${isDateSite
+        ? items.map(([domain, visits], i) =>
+            this.#siteCardTemplate(domain, i, visits, true)
+          )
+        : this.#tabListTemplate(this.getTabItems(items))}
+    </moz-card>`;
+  }
+
+  #siteCardTemplate(domain, index, items, isDateSite = false) {
+    let tabIndex = index > 0 ? "-1" : undefined;
+    return html` <moz-card
+      class=${isDateSite ? "nested-card" : ""}
+      type="accordion"
+      ?expanded=${!isDateSite}
+      heading=${domain}
+      @keydown=${this.handleCardKeydown}
+      tabindex=${ifDefined(tabIndex)}
+    >
+      ${this.#tabListTemplate(this.getTabItems(items))}
+    </moz-card>`;
+  }
+
   #emptyMessageTemplate() {
     let descriptionHeader;
     let descriptionLabels;
@@ -302,6 +338,14 @@ export class SidebarHistory extends SidebarPage {
       "checked",
       this.controller.sortOption == "site"
     );
+    this._menuSortByDateSite.setAttribute(
+      "checked",
+      this.controller.sortOption == "datesite"
+    );
+    this._menuSortByLastVisited.setAttribute(
+      "checked",
+      this.controller.sortOption == "lastvisited"
+    );
   }
 
   render() {

browser/components/sidebar/tests/browser/browser_history_sidebar.js

@@ -188,6 +188,8 @@ add_task(async function test_history_sort() {
   const menu = component._menu;
   const sortByDateButton = component._menuSortByDate;
   const sortBySiteButton = component._menuSortBySite;
+  const sortByDateSiteButton = component._menuSortByDateSite;
+  const sortByLastVisitedButton = component._menuSortByLastVisited;
 
   info("Sort history by site.");
   let promiseMenuShown = BrowserTestUtils.waitForEvent(menu, "popupshown");
@@ -233,6 +235,58 @@ add_task(async function test_history_sort() {
       "The cards for Today and Yesterday are expanded."
     );
   }
+
+  info("Sort history by date and site.");
+  promiseMenuShown = BrowserTestUtils.waitForEvent(menu, "popupshown");
+  EventUtils.synthesizeMouseAtCenter(menuButton, {}, contentWindow);
+  await promiseMenuShown;
+  menu.activateItem(sortByDateSiteButton);
+  await BrowserTestUtils.waitForMutationCondition(
+    component.shadowRoot,
+    { childList: true, subtree: true },
+    () => component.lists.length === dates.length * URLs.length
+  );
+  Assert.ok(
+    true,
+    "There is a card for each date, and a nested card for each site."
+  );
+  Assert.equal(
+    sortByDateSiteButton.getAttribute("checked"),
+    "true",
+    "Sort by date and site is checked."
+  );
+  const outerCards = [...component.cards].filter(
+    el => !el.classList.contains("nested-card")
+  );
+  for (const [i, card] of outerCards.entries()) {
+    Assert.equal(
+      card.expanded,
+      i === 0 || i === 1,
+      "The cards for Today and Yesterday are expanded."
+    );
+  }
+
+  info("Sort history by last visited.");
+  promiseMenuShown = BrowserTestUtils.waitForEvent(menu, "popupshown");
+  EventUtils.synthesizeMouseAtCenter(menuButton, {}, contentWindow);
+  await promiseMenuShown;
+  menu.activateItem(sortByLastVisitedButton);
+  await BrowserTestUtils.waitForMutationCondition(
+    component.shadowRoot,
+    { childList: true, subtree: true },
+    () => component.lists.length === 1
+  );
+  Assert.equal(
+    component.lists[0].tabItems.length,
+    URLs.length,
+    "There is a single card with a row for each site."
+  );
+  Assert.equal(
+    sortByLastVisitedButton.getAttribute("checked"),
+    "true",
+    "Sort by last visited is checked."
+  );
+
   win.SidebarController.hide();
 });
 

browser/locales/en-US/browser/sidebar.ftl

@@ -30,13 +30,19 @@ sidebar-history-date-prev-month =
 sidebar-history-delete =
   .title = Delete from History
 
-sidebar-history-sort-by-date =
-  .label = Sort by date
-sidebar-history-sort-by-site =
-  .label = Sort by site
 sidebar-history-clear =
   .label = Clear history
 
+sidebar-history-sort-by-heading = Sort by:
+sidebar-history-sort-option-date =
+  .label = Date
+sidebar-history-sort-option-site =
+  .label = Site
+sidebar-history-sort-option-date-and-site =
+  .label = Date and site
+sidebar-history-sort-option-last-visited =
+  .label = Last visited
+
 ## Labels for sidebar search
 
 # "Search" is a noun (as in "Results of the search for")

browser/themes/shared/sidebar.css

@@ -355,3 +355,15 @@ sidebar-main,
       }
     }
   }
+
+/* History Menu */
+#sidebar-history-sort-by-heading {
+  margin-block: var(--space-xsmall);
+}
+
+#sidebar-history-clear {
+  padding: var(--menuitem-padding);
+  &::before {
+    content: unset;
+  }
+}

toolkit/content/widgets/moz-card/moz-card.css

@@ -66,10 +66,10 @@ summary {
   border-radius: var(--card-border-radius);
 
   .chevron-icon {
-    background-image: url("chrome://global/skin/icons/arrow-down.svg");
+    background-image: var(--card-accordion-closed-icon, url("chrome://global/skin/icons/arrow-down.svg"));
 
     details[open] & {
-      background-image: url("chrome://global/skin/icons/arrow-up.svg");
+      background-image: var(--card-accordion-open-icon, url("chrome://global/skin/icons/arrow-up.svg"));
     }
   }