From e5d26fcea874ed23c61c5489b6a85bd24710421f Mon Sep 17 00:00:00 2001 From: Jonathan Sudiaman Date: Thu, 10 Apr 2025 15:20:30 +0000 Subject: [PATCH] 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 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 | 12 +- .../firefoxview/HistoryController.sys.mjs | 131 +++++++++++++++++- .../components/sidebar/sidebar-history.css | 5 + .../components/sidebar/sidebar-history.mjs | 98 +++++++++---- .../tests/browser/browser_history_sidebar.js | 54 ++++++++ browser/locales/en-US/browser/sidebar.ftl | 14 +- browser/themes/shared/sidebar.css | 12 ++ toolkit/content/widgets/moz-card/moz-card.css | 4 +- 8 files changed, 288 insertions(+), 42 deletions(-) diff --git a/browser/base/content/main-popupset.inc.xhtml b/browser/base/content/main-popupset.inc.xhtml index 198c188e074b..801118b184c0 100644 --- a/browser/base/content/main-popupset.inc.xhtml +++ b/browser/base/content/main-popupset.inc.xhtml @@ -319,12 +319,20 @@ - + - + + diff --git a/browser/components/firefoxview/HistoryController.sys.mjs b/browser/components/firefoxview/HistoryController.sys.mjs index cb7fde7ea704..cb5966fce0b4 100644 --- a/browser/components/firefoxview/HistoryController.sys.mjs +++ b/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} [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} 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} 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. + 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} oldMap + * @param {Map} newMap + * @returns {Map} + */ + #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>} 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, diff --git a/browser/components/sidebar/sidebar-history.css b/browser/components/sidebar/sidebar-history.css index ff8e09e6c2d4..0f92b7006d19 100644 --- a/browser/components/sidebar/sidebar-history.css +++ b/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"); +} diff --git a/browser/components/sidebar/sidebar-history.mjs b/browser/components/sidebar/sidebar-history.mjs index 33be75b81148..676456b89b9f 100644 --- a/browser/components/sidebar/sidebar-history.mjs +++ b/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) => { - let tabIndex = i > 0 ? "-1" : undefined; - return html` - ${this.#tabListTemplate(this.getTabItems(items))} - `; - }); + return historyVisits.map(({ l10nId, items }, i) => + this.#dateCardTemplate(l10nId, i, items) + ); case "site": - return historyVisits.map(({ domain, items }, i) => { - let tabIndex = i > 0 ? "-1" : undefined; - return html` - ${this.#tabListTemplate(this.getTabItems(items))} - `; - }); + return historyVisits.map(({ domain, items }, i) => + this.#siteCardTemplate(domain, i, items) + ); + case "datesite": + return historyVisits.map(({ l10nId, items }, i) => + this.#dateCardTemplate(l10nId, i, items, true) + ); + case "lastvisited": + return historyVisits.map( + ({ items }) => + html` + ${this.#tabListTemplate(this.getTabItems(items))} + ` + ); default: return []; } } + #dateCardTemplate(l10nId, index, items, isDateSite = false) { + const tabIndex = index > 0 ? "-1" : undefined; + return html` + ${isDateSite + ? items.map(([domain, visits], i) => + this.#siteCardTemplate(domain, i, visits, true) + ) + : this.#tabListTemplate(this.getTabItems(items))} + `; + } + + #siteCardTemplate(domain, index, items, isDateSite = false) { + let tabIndex = index > 0 ? "-1" : undefined; + return html` + ${this.#tabListTemplate(this.getTabItems(items))} + `; + } + #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() { diff --git a/browser/components/sidebar/tests/browser/browser_history_sidebar.js b/browser/components/sidebar/tests/browser/browser_history_sidebar.js index 304cc2e39e2d..bbee8cc896cd 100644 --- a/browser/components/sidebar/tests/browser/browser_history_sidebar.js +++ b/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(); }); diff --git a/browser/locales/en-US/browser/sidebar.ftl b/browser/locales/en-US/browser/sidebar.ftl index ecbf882e916d..7b7dd52c6918 100644 --- a/browser/locales/en-US/browser/sidebar.ftl +++ b/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") diff --git a/browser/themes/shared/sidebar.css b/browser/themes/shared/sidebar.css index d64fc29bfa1b..a28a0d76f251 100644 --- a/browser/themes/shared/sidebar.css +++ b/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; + } +} diff --git a/toolkit/content/widgets/moz-card/moz-card.css b/toolkit/content/widgets/moz-card/moz-card.css index 91623a0900ce..4f488875699d 100644 --- a/toolkit/content/widgets/moz-card/moz-card.css +++ b/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")); } }