1337545 - Webextension commands API does not support shortcuts with space or numbers

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[sblask]

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

Comment 1

[Patrick Dark]

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.

Comment 2

[Patrick Dark]

Attachment: bug.1337545.test.extension.xpi

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"
    }
}

Comment 3

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1337545 - Use keydown event for keycodes in commands

When you create a <key> element, which is what the commands API
does under the hood to allow keyboard shortcuts, the event type
that the handler listens to defaults to keypress. For printable
characters, the keyCode property of a keypress event is 0,
causing keycode bindings for these printable characters to not
work. We could use key for these properties, but the shift key
interacts with the key property in ways that make it simpler to
simply fix the keycode usage for our purposes.

Review commit: https://reviewboard.mozilla.org/r/135264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135264/

Comment 4

[Caitlin Neiman [:caitmuenster]]

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#

Comment 5

[Alex Thayer [:alexical] (she/her)]

(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!

Comment 6

[Kris Maglione [:kmag]]

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.

Comment 7

[Caitlin Neiman [:caitmuenster]]

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!

Comment 8

[Patrick Dark]

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?

Comment 9

[Patrick Dark]

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 10

[Kris Maglione [:kmag]]

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

Comment 11

[Alex Thayer [:alexical] (she/her)]

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

Comment 12

[Kris Maglione [:kmag]]

Unless something has changed recently, the ordering of events is keydown, keyup, keypress.

Comment 13

[Kris Maglione [:kmag]]

It looks like we use event="keydown" elsewhere, though, so I don't really have a strong opinion on this.

Comment 14

[Alex Thayer [:alexical] (she/her)]

(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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Comment 16

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8863485 [details]
Bug 1337545 - Use keydown event for keycodes in commands

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ffc010c94edb
Use keydown event for keycodes in commands r=kmag

Comment 18

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/ffc010c94edb

Comment 19

[sblask]

Nice, Ctrl-Space and Shift-Ctrl-Space work in latest Nightly. Thank you!

Comment 22

[Samuel Jun]

A note should be added to this page for this bug: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/commands