-
Notifications
You must be signed in to change notification settings - Fork 375
Remove suggested key for sort_tabs
#2758
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove suggested key for sort_tabs
#2758
Conversation
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. mozilla#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 mozilla#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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to merge, preferably with the requested changes, but they are not critical.
src/js/background/backgroundLogic.js
Outdated
}); | ||
} | ||
} | ||
}).catch(err => console.error(err)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't swallow unexpected errors. The code above is not expected to fail. And even if it did, it would be preferable to have logspam once than to ignore the error completely.
}).catch(err => console.error(err)); | |
}); |
src/js/background/backgroundLogic.js
Outdated
browser.commands.update({ | ||
name: "sort_tabs", | ||
shortcut: "", | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's reset instead. The effective behavior is the same, except update
stores the specified shortcut in storage, whereas reset
resets to whatever the manifest defaults are (in this case, empty).
browser.commands.update({ | |
name: "sort_tabs", | |
shortcut: "", | |
}); | |
browser.commands.reset("sort_tabs"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's helpful. I wasn't sure whether commands.reset
would have different logic if the shortcut was previously set by extension manifest default vs. by the user. I should have checked the Firefox source -- looks like it unconditionally removes the existing command and then registers the manifest default command, if specified.
- Use `commands.reset` insead of `commands.update` - Do not swallow errors
## IMPORTANT NOTE Version 8.2.0 of this add-on configured Ctrl + Comma as the default keyboard shortcut for sorting the tab strip by container. This keyboard shortcut was removed from 8.3.0 in patch mozilla#2758. If you use Ctrl + Comma in order to quickly sort tabs by container, then you will need to explicitly reconfigure the keyboard shortcut. https://support.mozilla.org/en-US/kb/manage-extension-shortcuts-firefox ## Features - mozilla#2753 Avoid sorting tabs in Firefox tab groups - mozilla#2758 Removed suggested keyboard shortcut for tab sorting - mozilla#2722 Removed Mozilla VPN logo banner ## Bugs - mozilla#2572 Fixed add/remove site assignments logic bug - mozilla#2754 Fixed "open/reopen in container" bug that would reopen outside of a Firefox tab group - mozilla#2760 Removed console log spam related to context menu cleanup ## Developer - mozilla#2671 Updated contributor documentation with tips and corrected links - mozilla#2723 Updated GitHub Actions build image
npm test
and all tests passed.Description
Remove suggested keyboard shortcut for the
sort_tabs
operation. For users updating from v8.2.0 of this addon, clear thesort_tabs
keyboard shortcut if the configured keyboard shortcut is the default suggested key (Ctrl + Comma) from 8.2.0.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. See some of the following: #1309, #2731
#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:
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.
Type of change
I'm not sure how to categorize this. Since this will cause some users relying on Ctrl + Comma to be unable to sort tabs by keyboard shortcut until they take an extra action to configure one in Manage Extension Shortcuts, it's kind of like a breaking change. But in principle, I think of this more like a bug fix that improves the default behavior of a feature that is otherwise not changing.