Skip to content

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

Merged
merged 2 commits into from
Apr 30, 2025

Conversation

apostrophest
Copy link
Contributor

  • I agree to license my code under the MPL 2.0 license.
  • I rebased my work on top of the main branch.
  • I ran npm test and all tests passed.
  • I added test coverages if relevant.

Description

Remove suggested keyboard shortcut for the sort_tabs operation. For users updating from v8.2.0 of this addon, clear the sort_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:

  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.

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.

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.
Copy link
Member

@Rob--W Rob--W left a 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.

});
}
}
}).catch(err => console.error(err));
Copy link
Member

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.

Suggested change
}).catch(err => console.error(err));
});

Comment on lines 65 to 68
browser.commands.update({
name: "sort_tabs",
shortcut: "",
});
Copy link
Member

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).

Suggested change
browser.commands.update({
name: "sort_tabs",
shortcut: "",
});
browser.commands.reset("sort_tabs");

Copy link
Contributor Author

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.

@Rob--W Rob--W mentioned this pull request Apr 29, 2025
- Use `commands.reset` insead of `commands.update`
- Do not swallow errors
@dannycolin dannycolin merged commit 9434147 into mozilla:main Apr 30, 2025
2 checks passed
apostrophest added a commit to apostrophest/multi-account-containers that referenced this pull request May 6, 2025
## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants