Closed Bug 1565854 Opened 2 years ago Closed 1 month ago

Ctrl+Shift+S should not be allowed to be set as an extension shortcut

Categories

(Toolkit :: Add-ons Manager, defect, P5)

68 Branch
Desktop
All
defect

Tracking

()

RESOLVED FIXED
90 Branch
Tracking Status
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox90 --- fixed

People

(Reporter: mymindstorm, Assigned: rpl)

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

  1. about:addons
  2. Manage extension shortcuts
  3. Set a shortcut to Ctrl+Shift+S

Actual results:

Allowed to set shortcut.

Expected results:

Should have not been allowed to be set as Firefox Screenshots controls that shortcut.

https://searchfox.org/mozilla-central/source/browser/extensions/screenshots/manifest.json#25

I have tested this issue on Windows10, MacOS and also Ubuntu 18.
For Windows and Ubuntu the issue is actually fixed already on the firefox BETA 69.0b5 release and Nightly 70.0a1 release.
On Mac, it still lets you set "Shift+command+S" on two different extension shortcuts at the same time

I installed this addon when i tested it: https://addons.mozilla.org/en-US/firefox/addon/enhancer-for-youtube/

Firefox release 68 win10 and Ubuntu 18 > Affected, if you type it, because screenshot addon is not showing up
Firefox Nightly 70.0a1 win10 and Ubuntu 18 > Not affected, Shows Can´t override a firefox shortcut
Firefox beta 69.0b5 win10 and Ubuntu 18 > Not affected, Shows Can´t override a firefox shortcut

MacOs:

Firefox release 68 MacOs 10.14 > Affected, if you type it, because screenshot addon is not showing up
Firefox Nightly 70.0a1 MacOs 10.14 > Affected, Shows shift+command+S for screenshots, but it does let you put also shift+command+S for another extension shortcut at the same time.
Firefox beta 69.0b5 MacOs 10.14 > Affected, Shows shift+command+S for screenshots, but it does let you put also shift+command+S for another extension shortcut at the same time.

So for MacOs, it does not show the " Can´t override a firefox shortcut", warning, you can set shift+command+S on two different extensions shortcuts.

Attached image macoss.jpg
Status: UNCONFIRMED → NEW
Component: Untriaged → Extension Compatibility
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → Desktop

Mymindstorm;:

Please download Nightly release as it looks fixed already : https://nightly.mozilla.org/ and retest the problem.

Issue still affects Mac0S, shift+command+S actually takes the screenshot after all.
But should not let you put shift+command+S on 2 extension shortcuts.

(In reply to Pablo from comment #3)

Mymindstorm;:

Please download Nightly release as it looks fixed already : https://nightly.mozilla.org/ and retest the problem.

Issue still affects Mac0S, shift+command+S actually takes the screenshot after all.
But should not let you put shift+command+S on 2 extension shortcuts.

Confirmed fixed with 70.0a1 (2019-07-17) (64-bit) on Linux

Component: Extension Compatibility → Untriaged
Product: Firefox → WebExtensions

The Bugbug bot thinks this bug should belong to the 'Toolkit::Add-ons Manager' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Component: Untriaged → Add-ons Manager
Product: WebExtensions → Toolkit
Severity: normal → --
Severity: -- → S4
Priority: -- → P5

I'm going to look into it this issue.

Flags: needinfo?(lgreco)

This patch does prevent about:addons "Manage Extension Shortcuts" view to miss to catch
shortcut conflicts due to the special meaning of "Ctrl" on macOS systems, and its
implicit conversion into "Command":

On Macs, "Ctrl" is interpreted as "Command", so if you actually need "Ctrl", specify "MacCtrl".

  • and so ExtensionShortcuts.buildKeyFromShortcut calls ShortcutUtils.getModifiersAttribute (1)
    which convert both Ctrl and Command are into the same accel modifier, which is what ends into the key element
    appended into the chrome window document.

  • but we still have the original "Ctrl" as part of the shortcut string that
    ExtensionShortcuts keeps into its map of the defined shortcuts loaded from
    the extension manifests and from the one stored on disk (through ExtensionSettingsStore)

  • when the about:addons "Manage Extension Shortcuts" view receive a new shortcut
    from the user, it does convert the accelKey property from the dom event received
    into "Command", and then it does check if the shortcut string exists in the
    map of the existing shortcuts.

  • when the extension using the same shortcut does use "Ctrl+..." in its commands
    suggested shortcuts, shortcuts.js (the "Manage Extension Shortcuts" view implementation
    script) wouldn't find the "Command+..." shortcut in the map of the existing shortcuts
    and it would assume that it is not used and let the user to set a duplicate shortcut
    without any of the warnings or errors expected.

The approach in this patch does change the existing normalizeShortcut helper function
to also do this additional "platform specific" normalization, changing Ctrl into Command,
because it should allow us to make sure we do the same convertion for the shortcuts
defined in the manifest and the ones loaded from the ExtensionSettingsStore.

The patch that I've just attached is currently marked as work in progress because I'd like to think a bit more into other potential side-effect of the proposed fix (and because the test case included in the patch does cover the issue with a test that cover the scenario described in this issue, but it does not provide all the coverage that I would like it to).

In the meantime, the patch description does provide a bit more detail about what is going on and how it does trigger this issue, which is good.

And I've also pushed it to try to see if it does break any other test in its current form:

Flags: needinfo?(lgreco)
Assignee: nobody → lgreco
Status: NEW → ASSIGNED

As I was expecting there are a couple of test failing because in the attached patch the shortcuts using Ctrl are translated automatically into shortcuts using Command and so they don't match if the extension expects that the shortcuts string got from command.getAll will have to be strictly equal to the one is the manifest or the one passed in a commands.update call.

I do have another approach in mind, which would require a bit more changes to both shortcuts.js and ExtensionShortcuts.jsm, basically "abstracting out the part in shortcuts.js that build the shortcutKeyMap into a subclass of DefaultMap", I feel that even if it requires a bit more work that may be a better way to deal with this issue (and also make shortcuts.js a bit simpler and more readable in the process) and so I'm looking into that and changing the patch accordingly.

Attachment #9219032 - Attachment description: WIP: Bug 1565854 - Normalize Ctrl to Command from ExtensionShortcuts to ensure about:addons Manage Shortcut does not miss shortcut conflicts on macOS. r?mixedpuppy! → WIP: Bug 1565854 - Abstract out about:addons Manage Shortcut shortcutKeyMap into a specialized DefaultMap subclass. r?mixedpuppy!

I just updated the attach patch to use the alternative approach briefly described in comment 9 (I also tweaked the patch description accordingly but phabricator doesn't post that in the bugzilla comment and so it is only visible on the phabricator side at the moment).

The push to try does also look better:

(and it does pass the new test case, which with the new approach should actually be covering the change in a good enough way, but I can add an additional unit test just for the ExtensionShotcutKeyMap class if we want to).

Attachment #9219032 - Attachment description: WIP: Bug 1565854 - Abstract out about:addons Manage Shortcut shortcutKeyMap into a specialized DefaultMap subclass. r?mixedpuppy! → Bug 1565854 - Abstract out about:addons Manage Shortcut shortcutKeyMap into a specialized DefaultMap subclass. r?mixedpuppy!
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/9d0d26dacd7f
Abstract out about:addons Manage Shortcut shortcutKeyMap into a specialized DefaultMap subclass. r=mixedpuppy
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
You need to log in before you can comment on or make changes to this bug.