Closed Bug 1337545 Opened 4 years ago Closed 4 years ago

Webextension commands API does not support shortcuts with space or numbers

Categories

(WebExtensions :: Untriaged, defect)

51 Branch
defect
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: blask, Assigned: dthayer)

References

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170201180315

Steps to reproduce:

I tried porting an add-on that uses Ctrl-Space and Shift-Ctrl-Space as keyboard shortcuts.


Actual results:

I can defined the shortcuts and do not get any error, but nothing happens when I press the keys. Simply replacing Space with another key makes it work. The same problem applies to number keys.


Expected results:

Shortcuts using Space and numbers should work as stated in the documentation
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
I'm having the same problem as mentioned at https://mail.mozilla.org/pipermail/dev-addons/2017-February/002566.html. However, I can't get the Alt+Comma or Alt+Period key combinations to work.

I tested the key Ctrl+Space and Ctrl+Shift+Space combinations that sblask mentioned and wasn't able to get those to work either.

I was able to get Alt+1 to work (i.e., a number key), but not Ctrl+1, which is apparently overridden by a browser shortcut that focuses the first tab (i.e., tab number one). If shortcuts are going to be overridden, it would seem that a console warning is called for.

According to the code at https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-commands.js, shortcuts are remapped into "VK_" codes for use with XUL |key| elements and some of these mappings appear to be wrong.

I don't see anything wrong with the space key mappings, but VK_COMMA seems to be mapped to 0xBC/188, which is the VULGAR FRACTION ONE QUARTER character, and VK_PERIOD seems to be mapped to 0xBE/190, which is the VULGAR FRACTION THREE QUARTERS character, in several places. See https://dxr.mozilla.org/mozilla-central/search?q=VK_COMMA and https://dxr.mozilla.org/mozilla-central/search?q=VK_PERIOD.

When I tried early last year to use XUL |key| element with VK_SLASH for the XUL/XPCOM version of my extension, I couldn't get the shortcut to work and ended up using |document.defaultView.addEventListener("keyup")| instead. VK_SLASH likewise seems mapped to the wrong character, which might explain the problems I was having back then. See https://dxr.mozilla.org/mozilla-central/search?q=VK_SLASH. (The slash character isn't allowed in WebExtension commands at the moment, but I've proposed including it at https://mail.mozilla.org/pipermail/dev-addons/2017-February/002564.html.
Barebones test extension that writes to the browser console (Ctrl+Shift+J). The relevant code defining shortcuts is:

"commands": {
    "Test Ctrl+Shift+Y (Shortcut That Works)": {
        "suggested_key": {
            "default": "Ctrl+Shift+Y"
        },
        "description": "Test Shortcut"
    },
    "Test Ctrl+Space": {
        "suggested_key": {
            "default": "Ctrl+Space"
        },
        "description": "Test Shortcut"
    },
    "Test Ctrl+Shift+Space": {
        "suggested_key": {
            "default": "Ctrl+Shift+Space"
        },
        "description": "Test Shortcut"
    },
    "Test Alt+1 (Shortcut That Works)": {
        "suggested_key": {
            "default": "Alt+1"
        },
        "description": "Test Shortcut"
    },
    "Test Ctrl+1": {
        "suggested_key": {
            "default": "Ctrl+1"
        },
        "description": "Test Shortcut"
    },
    "Test Ctrl+Comma": {
        "suggested_key": {
            "default": "Ctrl+Comma"
        },
        "description": "Test Shortcut"
    },
    "Test Ctrl+Period": {
        "suggested_key": {
            "default": "Ctrl+Period"
        },
        "description": "Test Shortcut"
    }
}
Whiteboard: [design-decision-needed]
Assignee: nobody → dothayer
Hi all, this has been added to the agenda for the May 2 WebExtensions Triage. Sblask and Doug, will you be able to join us?

Wiki: https://wiki.mozilla.org/Add-ons/Contribute/Triage#Next_Meeting

Agenda: https://docs.google.com/document/d/1vf8AaW8tKKbMn4KhsqEYhrYqVTUGaERHQmzanEp2Cls/edit#
(In reply to Caitlin Neiman (http://pronoun.is/she) from comment #4)
> Hi all, this has been added to the agenda for the May 2 WebExtensions
> Triage. Sblask and Doug, will you be able to join us?
> 
> Wiki: https://wiki.mozilla.org/Add-ons/Contribute/Triage#Next_Meeting
> 
> Agenda:
> https://docs.google.com/document/d/
> 1vf8AaW8tKKbMn4KhsqEYhrYqVTUGaERHQmzanEp2Cls/edit#

Sure thing!
Sorry, this doesn't need to be discussed. This is already meant to be supported. The fact that it doesn't work is a bug.
Whiteboard: [design-decision-needed]
Thanks, Kris! Do you know if this has an assigned priority? If not, we can briefly touch on it  tomorrow. Doug and sbask, pyou are released from the meeting!
design-decision-needed if there's any consideration for adding missing potential for commands like Alt+/, which my extension used while XUL/XPCOM-based. That's what brought me here from dev.addons.mozilla.org. Or maybe I should file a separate enhancement bug?
Also, the description is wrong since more than space and numbers are broken. (Comma and period do not work either, at least, though I never tested most of the other keys.)
Comment on attachment 8863485 [details]
Bug 1337545 - Use keydown event for keycodes in commands

https://reviewboard.mozilla.org/r/135264/#review138734

Thanks!

::: browser/components/extensions/ext-commands.js:175
(Diff revision 1)
> -    if (/^[A-Z0-9]$/.test(chromeKey)) {
> +    if (/^[A-Z]$/.test(chromeKey)) {
>        // We use the key attribute for all single digits and characters.
>        keyElement.setAttribute("key", chromeKey);
>      } else {
>        keyElement.setAttribute("keycode", this.getKeycodeAttribute(chromeKey));
> +      keyElement.setAttribute("event", "keydown");

keyup, please
Attachment #8863485 - Flags: review?(kmaglione+bmo) → review+
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #10)
> Comment on attachment 8863485 [details]
> Bug 1337545 - Use keydown event for keycodes in commands
> 
> https://reviewboard.mozilla.org/r/135264/#review138734
> 
> Thanks!
> 
> ::: browser/components/extensions/ext-commands.js:175
> (Diff revision 1)
> > -    if (/^[A-Z0-9]$/.test(chromeKey)) {
> > +    if (/^[A-Z]$/.test(chromeKey)) {
> >        // We use the key attribute for all single digits and characters.
> >        keyElement.setAttribute("key", chromeKey);
> >      } else {
> >        keyElement.setAttribute("keycode", this.getKeycodeAttribute(chromeKey));
> > +      keyElement.setAttribute("event", "keydown");
> 
> keyup, please

Thanks Kris! But could you clarify why keyup? keypress occurs when you press the key down, and typically our shortcuts activate when the key goes down, not when it comes up.
Flags: needinfo?(kmaglione+bmo)
Unless something has changed recently, the ordering of events is keydown, keyup, keypress.
Flags: needinfo?(kmaglione+bmo)
It looks like we use event="keydown" elsewhere, though, so I don't really have a strong opinion on this.
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #12)
> Unless something has changed recently, the ordering of events is keydown,
> keyup, keypress.

Hmm, it may have. I did a quick test with this:

["keyup", "keypress", "keydown"].map(t => document.addEventListener(t, (e) => console.log(t)))

And the result was

keydown
keypress
keyup
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
Comment on attachment 8863485 [details]
Bug 1337545 - Use keydown event for keycodes in commands

https://reviewboard.mozilla.org/r/135264/#review138978
Keywords: checkin-needed
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/ffc010c94edb
Use keydown event for keycodes in commands r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ffc010c94edb
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Nice, Ctrl-Space and Shift-Ctrl-Space work in latest Nightly. Thank you!
Duplicate of this bug: 1352823
Duplicate of this bug: 1366684
A note should be added to this page for this bug: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/commands
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.