Webextension commands API does not support shortcuts with space or numbers

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
5 months ago
a month ago

People

(Reporter: sblask, Assigned: dthayer)

Tracking

51 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

5 months ago
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

Updated

5 months ago
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit

Comment 1

4 months ago
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

4 months ago
Created attachment 8837682 [details]
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"
    }
}

Updated

4 months ago
Whiteboard: [design-decision-needed]
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
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#
(Assignee)

Comment 5

2 months ago
(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

2 months ago
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!

Comment 8

2 months ago
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

2 months ago
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

2 months ago
mozreview-review
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+
(Assignee)

Comment 11

2 months ago
(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.
(Assignee)

Comment 14

2 months ago
(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
(Assignee)

Updated

2 months ago
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
(Assignee)

Comment 16

2 months ago
mozreview-review
Comment on attachment 8863485 [details]
Bug 1337545 - Use keydown event for keycodes in commands

https://reviewboard.mozilla.org/r/135264/#review138978
(Assignee)

Updated

2 months ago
Keywords: checkin-needed

Comment 17

2 months ago
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

Comment 18

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ffc010c94edb
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Comment 19

2 months ago
Nice, Ctrl-Space and Shift-Ctrl-Space work in latest Nightly. Thank you!
Duplicate of this bug: 1352823

Updated

a month ago
Duplicate of this bug: 1366684

Comment 22

a month ago
A note should be added to this page for this bug: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/commands
You need to log in before you can comment on or make changes to this bug.