From e96b275e0257c01848858ab2161158ce52492487 Mon Sep 17 00:00:00 2001 From: Stephen Thompson Date: Fri, 25 Apr 2025 13:08:44 -0400 Subject: [PATCH 1/2] Remove suggested key for `sort_tabs` A number of users have accidentally pressed Ctrl + Comma and sorted their tabs by container. For some of those users, they did not know what had happened. #2492 added Ctrl + Comma as a default shortcut key for sorting tabs by container. This change was released with addon v8.2.0 in Sept 2024. It is useful to make the sort-tabs-by-container operation accessible by keyboard shortcut, but I think Ctrl + Comma is too close to the main Ctrl + Period shortcut for this addon. I think it's reasonable to make the default sort_tabs keyboard shortcut more complex, but in this patch, I'm recommending that we: 1. do not set a shortcut by default for any future users. Users can still configure a shortcut via Firefox's Manage Extension Shortcuts UI. 2. for existing users upgrading from 8.2.0 who configured their own, non-default keyboard shortcut for sort_tabs, keep their shortcut in place. These users demonstrated a strong interest in using the sort_tabs feature and I do not want to interfere. 3. for existing users upgrading from 8.2.0 who have the default keyboard shortcut configured, clear the shortcut. Users who did not use the sort_tabs keyboard shortcut will no longer be at risk of accidentally using it. Users who did use the default sort_tabs keyboard shortcut will be harmed, but those users can use the Manage Extension Shortcuts UI to manually configure Ctrl + Comma (or another key combination). I am not aware of any simple ways to ensure that existing users using Ctrl + Comma can continue to do so without interruption. Ideally, I think #2492 should not have set a suggested key -- the other keyboard shortcuts are "non-destructive" and easy to understand. However, since that already happened, I think the best harm reduction approach at this point is to inconvenience existing users of Ctrl + Comma. --- src/js/background/backgroundLogic.js | 35 +++++++++++++++++++++++++++- src/manifest.json | 4 ---- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/js/background/backgroundLogic.js b/src/js/background/backgroundLogic.js index 54bdd0e..c5b53b3 100644 --- a/src/js/background/backgroundLogic.js +++ b/src/js/background/backgroundLogic.js @@ -35,10 +35,43 @@ const backgroundLogic = { browser.permissions.onRemoved.addListener(permissions => this.resetPermissions(permissions)); // Update Translation in Manifest - browser.runtime.onInstalled.addListener(this.updateTranslationInManifest); + browser.runtime.onInstalled.addListener((details) => { + this.updateTranslationInManifest(); + this._undoDefault820SortTabsKeyboardShortcut(details); + }); browser.runtime.onStartup.addListener(this.updateTranslationInManifest); }, + /** + * One-time migration after updating from v8.2.0: + * Unset the default keyboard shortcut (Ctrl+Comma) for the `sort_tabs` + * command if it was set in v8.2.0 of this addon. If the user remapped + * a different shortcut manually, retain their shortcut. Users who used + * the default keyboard shortcut will need to manually set a shortcut. + * See https://support.mozilla.org/en-US/kb/manage-extension-shortcuts-firefox + * + * @param {{reason: runtime.OnInstalledReason, previousVersion?: string}} details + */ + _undoDefault820SortTabsKeyboardShortcut(details) { + if (details.reason === "update" && details.previousVersion === "8.2.0") { + browser.commands.getAll().then((commands) => { + const sortTabsCommand = commands.find(command => command.name === "sort_tabs"); + if (sortTabsCommand) { + const previouslySuggestedKeys = [ + "Ctrl+Comma", // "default" + "MacCtrl+Comma", // "mac" + ]; + if (previouslySuggestedKeys.includes(sortTabsCommand.shortcut)) { + browser.commands.update({ + name: "sort_tabs", + shortcut: "", + }); + } + } + }).catch(err => console.error(err)); + } + }, + updateTranslationInManifest() { for (let index = 0; index < 10; index++) { const ajustedIndex = index + 1; // We want to start from 1 instead of 0 in the UI. diff --git a/src/manifest.json b/src/manifest.json index daebff1..30e3be6 100644 --- a/src/manifest.json +++ b/src/manifest.json @@ -45,10 +45,6 @@ "description": "__MSG_openContainerPanel__" }, "sort_tabs": { - "suggested_key": { - "default": "Ctrl+Comma", - "mac": "MacCtrl+Comma" - }, "description": "__MSG_sortTabsByContainer__" }, "open_container_0": { From f1a24ed6fba92eaa63853966228b731f81c28f3d Mon Sep 17 00:00:00 2001 From: Stephen Thompson Date: Tue, 29 Apr 2025 16:36:19 -0400 Subject: [PATCH 2/2] Address code review comments for #2758 - Use `commands.reset` insead of `commands.update` - Do not swallow errors --- src/js/background/backgroundLogic.js | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/src/js/background/backgroundLogic.js b/src/js/background/backgroundLogic.js index c5b53b3..01402fa 100644 --- a/src/js/background/backgroundLogic.js +++ b/src/js/background/backgroundLogic.js @@ -52,23 +52,19 @@ const backgroundLogic = { * * @param {{reason: runtime.OnInstalledReason, previousVersion?: string}} details */ - _undoDefault820SortTabsKeyboardShortcut(details) { + async _undoDefault820SortTabsKeyboardShortcut(details) { if (details.reason === "update" && details.previousVersion === "8.2.0") { - browser.commands.getAll().then((commands) => { - const sortTabsCommand = commands.find(command => command.name === "sort_tabs"); - if (sortTabsCommand) { - const previouslySuggestedKeys = [ - "Ctrl+Comma", // "default" - "MacCtrl+Comma", // "mac" - ]; - if (previouslySuggestedKeys.includes(sortTabsCommand.shortcut)) { - browser.commands.update({ - name: "sort_tabs", - shortcut: "", - }); - } + const commands = await browser.commands.getAll(); + const sortTabsCommand = commands.find(command => command.name === "sort_tabs"); + if (sortTabsCommand) { + const previouslySuggestedKeys = [ + "Ctrl+Comma", // "default" + "MacCtrl+Comma", // "mac" + ]; + if (previouslySuggestedKeys.includes(sortTabsCommand.shortcut)) { + browser.commands.reset("sort_tabs"); } - }).catch(err => console.error(err)); + } } },