Closed Bug 1424708 Opened 6 years ago Closed 6 years ago

Numeric keys doesn't work as keyboard shortcuts commands

Categories

(WebExtensions :: General, defect, P5)

defect

Tracking

(firefox64 verified, firefox65 verified)

VERIFIED FIXED
mozilla64
Tracking Status
firefox64 --- verified
firefox65 --- verified

People

(Reporter: juraj.masiar, Assigned: tb120, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171207170405

Steps to reproduce:

Using `commands` API:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/commands

1) in manifest define short-key for "Ctrl + Shift + 1"
2) press "Ctrl + Shift + 1" BUT use the "1" from the numeric part of your keyboard (on the right, not those above letters)


Actual results:

Nothing happened.


Expected results:

Action associated with "Ctrl + Shift + 1" should work also with numeric keys.

Alternatively support for "Ctrl + Shift + Num1" or "Ctrl + Shift + Numpad1" should be added.
Attached file commands.zip
I can reproduce this issue on Firefox 57.0.2 (20171206182557), 58.0b11 (20171211020921) and Firefox 59.0a1(20171211220110) under Win 7 64-bit and Ubuntu 16.04 32-bit.

The command  "Ctrl + Shift + 1" defined in manifest.json is not working with the numeric keypad.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Version: 57 Branch → Trunk
Priority: -- → P5
Keywords: good-first-bug
Mentor: mstriemer
I'd like to work on this bug. Please guide me further.
I think what you'll want to do is create two keys for numeric shortcuts in registerKeysToDocument [1]. If the command is Alt+Shift+8 you'll want to call `this.buildKey()` with "Alt+Shift+8" and "Alt+Shift+NUMPAD8". You can create this similar to how to key is pulled out here [2].

Please make sure that `SidebarUI.updateShortcut()` [3] is only called with the "Alt+Shift+8" version and an id is only set on one of the keys [4].

You'll need to add a test, it might be easiest to add one to the update tests [5] and you can verify that both keys are cleaned up if it changes.

Let me know if you have any other questions.

[1] https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/browser/components/extensions/ext-commands.js#164
[2] https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/browser/components/extensions/ext-commands.js#236-239
[3] https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/browser/components/extensions/ext-commands.js#173
[4] https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/browser/components/extensions/ext-commands.js#245
[5] https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_commands_update.js
Thank you! I'm setting up mercurial... I'll let you know when I make some changes or need some help... (In reply to Mark Striemer [:mstriemer] from comment #3)
> I think what you'll want to do is create two keys for numeric shortcuts in
> registerKeysToDocument [1]. If the command is Alt+Shift+8 you'll want to
> call `this.buildKey()` with "Alt+Shift+8" and "Alt+Shift+NUMPAD8". You can
> create this similar to how to key is pulled out here [2].
> 
> Please make sure that `SidebarUI.updateShortcut()` [3] is only called with
> the "Alt+Shift+8" version and an id is only set on one of the keys [4].
> 
> You'll need to add a test, it might be easiest to add one to the update
> tests [5] and you can verify that both keys are cleaned up if it changes.
> 
> Let me know if you have any other questions.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> d2b4b40901c15614fad2fa34718eea428774306e/browser/components/extensions/ext-
> commands.js#164
> [2]
> https://searchfox.org/mozilla-central/rev/
> d2b4b40901c15614fad2fa34718eea428774306e/browser/components/extensions/ext-
> commands.js#236-239
> [3]
> https://searchfox.org/mozilla-central/rev/
> d2b4b40901c15614fad2fa34718eea428774306e/browser/components/extensions/ext-
> commands.js#173
> [4]
> https://searchfox.org/mozilla-central/rev/
> d2b4b40901c15614fad2fa34718eea428774306e/browser/components/extensions/ext-
> commands.js#245
> [5]
> https://searchfox.org/mozilla-central/source/browser/components/extensions/
> test/browser/browser_ext_commands_update.js
Hello @insiyeah,
Are you still working on this bug? Is there some progress?
Flags: needinfo?(insiyahhajoori)
(In reply to juraj.masiar from comment #5)
> Hello @insiyeah,
> Are you still working on this bug? Is there some progress?

Hey, I am stuck right now with some others stuffs. It might take this May for me to look into this... You can go for it if you want:)
Flags: needinfo?(insiyahhajoori)
Sadly this is not my area of expertise and I'm also very busy with more than 100 items on my todo list for my add-ons.

I would be glad if you could fix it, even if it takes 2 months. Without you the bug with such a low priority will probably stay open for years. 

I will put the info about this bug as a first sentence in my add-on description so maybe in the meantime somebody skilled comes and fixes it :)

Thank you for your help!
Product: Toolkit → WebExtensions
Hey insiyeah, since we haven't heard from you in awhile I am going to open this bug up to other contributors. 

If this is your first contribution, please refer to https://wiki.mozilla.org/Add-ons/Contribute/Code on how to get started. After you have set up your dev environment, please needinfo :mstriemer if you need help with next steps.
Hi,

I would like to take this up for my first bug. Please give me pointers for the same...

Thanks,
Sachin
Hey Sachin, welcome! Have you read the getting started documentation and set up your dev environment? (You can find this info on 
https://wiki.mozilla.org/Add-ons/Contribute/Code). 

Let us know when you have and we'll assign you to the bug.
Flags: needinfo?(sachinhosmani2)
Yes, I've a build setup with the mozilla central repo.
Great! Mark, can you help Sachin get started with this?
Flags: needinfo?(sachinhosmani2) → needinfo?(mstriemer)
Hi Sachin, comment 3 should give you some pointers are where to start for this one. Let me know if you have any other questions!
Flags: needinfo?(mstriemer)
Hey Sachin, how's it going with this bug?
Assignee: nobody → sachinllvm
Flags: needinfo?(sachinllvm)
Hi Sachin, since we haven't heard from you in awhile I'm going to unassign you from this bug. If you'd still like to work on it, let me know and we'll reassign.
Assignee: sachinllvm → nobody
Flags: needinfo?(sachinllvm)
Hi, I saw this bug on @startmozilla and I'd like to work on it. I have the dev environment setup and was able to build. 

I looked at the suggestions above (comment 3) and am going to go based on those. The directory structure looks to have changed a bit since then with `ext-commands.js` in a folder called `parent`. Does that have any effect on the suggestions given? Also, is the test referenced in [5] an xpcshell test? This is new to me so I just want to make sure I understand the structure a bit more. Thanks.
Hey Tim, welcome! I've just assigned you to this bug.
Assignee: nobody → accountsemail
Flags: needinfo?(mstriemer)
The file is indeed in the `parent` folder now. I don't think that should have any affect on the suggestions.

The referenced test is a mochitest, so running it will start up a browser instance and it will run the tests in there.

Let me know if you have any other questions!
Flags: needinfo?(mstriemer)
I have an initial implementation done and I was hoping to get some feedback. Can I have some help though because I'm not sure how to make it available. The contributing docs recommend phabricator, which is new to me. Is that what I should use? Thanks.
Flags: needinfo?(mstriemer)
Phabricator is the way to go. You'll want to follow the docs [1] to get arcanist setup. An alternative that I use is phabsend [2], you install it as a mercurial extension and provide some config like this in your ~/.hgrc:

  [phabricator]
  url = https://phabricator.services.mozilla.com/
  callsign = MOZILLACENTRAL

  [auth]
  mozilla.schemes = https
  mozilla.prefix = phabricator.services.mozilla.com
  mozilla.phabtoken = cli-...

You can find your API token at https://phabricator.services.mozilla.com/conduit/login/.

[1] https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#setting-up-arcanist
[2] https://bitbucket.org/kmaglione/hgext/src/default/phabricator.py
Flags: needinfo?(mstriemer)
I committed the changes here: https://phabricator.services.mozilla.com/D8535. Thanks for the help with Phabricator.
Prior to this patch, if a user attempted to use a keyboard shortcut that used a numeric key, the action would not be triggered if they used the numeric key from the number pad. This patch triggers the shortcut regardless of the source.
Pushed by mstriemer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/900eabf8fcee
trigger a shortcut even when the number pad is used for the key r=mstriemer,mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/900eabf8fcee
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Woohoo! Thanks for the patch, Tim. Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!
Yes, I would like to an account. 

This was a great experience and I'd also like to work on another bug. Any suggestions?
(In reply to Tim B [:tb120] from comment #26)
> Yes, I would like to an account. 

Awesome! Go ahead and create one on the site, then ping me when you're done and I will vouch for you. 

> This was a great experience and I'd also like to work on another bug. Any
> suggestions?

Taking on another good-first-bug would be a good way to get started. You can check out the list of current WebExtensions good-first-bugs at https://mzl.la/2yq1XA8 or look for other Firefox good-first-bugs at https://codetribute.mozilla.org. Once you have a feel for the workflow, you'll be ready to take on other more challenging bugs that you're interested in. :)
Hello Tim. Thank you very much for fixing this! I've just tested it in 64.0a1 (2018-10-16) and it works great!

One question - it seems that short-keys like "Ctrl + Shift + number" doesn't work - I'm not sure if it's fixable though since the "Shift" key is probably overriding "NumLock" and the numeric keys stops being numbers. Right?


And there is one related bug here if you would like to do something similar:
https://bugzilla.mozilla.org/show_bug.cgi?id=1432718

Thank you again for fixing this! Have a great day!
@juraj.masiar You're welcome. Regarding Shift+X, I don't have enough confidence to be sure of my answer but I think that you're correct. I tried it locally and although Ctrl+Shift+0 (from top) works properly, Ctrl+Shift+0 (numpad) doesn't. But when I turn the number lock off, 0 from the number pad does work as expected. Sorry if that's not much help; I'm just not sure if there's a way around that. I have seen shortcuts in IntelliJ, for instance, use shift as the first modifier, which may be done to prevent this issue but that's a guess.
I tested on Firefox 65.0a1(20181024100114) and Firefox 64.0b3 (20181022150107) under Win 7 64-bit, Mac OS X 10.12.6, Ubuntu 14.04 32-bit and Win 10 64-bit.

"Ctrl + Shift + 0 to 9” - separate numeric keypad off.
- Does not work on Win 7 64-bit.
- Does not work Win 10 64-bit.
- Works on Ubuntu 14.04 32-bit.
- Works on Mac OS X 10.12.6.

"Ctrl + Shift + 0” - numeric key above the letters.
- Does not work on Win 7 64-bit.
- Does not work Win 10 64-bit.
- Works on Ubuntu 14.04 32-bit.
- Works on Mac OS X 10.12.6.

“Ctrl + Shift + 1 to 9” - numeric keys above the letters.
- Works on Win 7 64-bit.
- Works on Win 10 64-bit.
- Works on Ubuntu 14.04 32-bit.
- Works on Mac OS X 10.12.6.

Is this the expected outcome?
Flags: needinfo?(mozbug)
@CosminB I'm not sure what the problem is in Windows. I don't have a Windows instance (only Mac and Linux) to test on but it appears that there's some issue with '0'. The issue is tracked in Bug 1432718 (I know that you're already involved in that bug but I add it for tracking purposes.). The fix that I added creates another keycode for numpadX, so if "CTRL + SHIFT + 0" is in the manifest, there will also be a mapping for "CTRL + SHIFT + NUMPAD0". I see that it doesn't work even with the 0 above the keys though. 
Looking around, I found a few posts on MS forums ([1], [2], [3]) that mention this shortcut. Collectively they seem to suggest that the shortcut "CTRL + SHIFT + 0" is a Windows-specific shortcut. The fact that it works on other OSs (OSes?) suggests it to be mapped by Windows as well. Interestingly, I don't see that shortcut listed on their keyboard shortcut page [4]. I know [1] is referring to Vista, but if you try what they have suggested (or something emulating it), does that help?

[1] https://support.microsoft.com/en-us/help/967893/input-method-editor-keyboard-shortcut-ctrl-shift-0-switches-the-input 
[2] https://answers.microsoft.com/en-us/windows/forum/games_windows_10/ctrlshift0-in-windows-10/337b6542-4159-4143-83a8-a4496f8f995c
[3] https://answers.microsoft.com/en-us/windows/forum/windows_10-desktop/excel-and-ctrlshift0-in-windows-10/b3af3d3f-7f3d-4957-8aa9-746c07a9416e
[4] https://support.microsoft.com/en-us/help/12445/windows-keyboard-shortcuts
Flags: needinfo?(mozbug)
Thanks for the infos Tim!

I will mark this bug verified as fixed according to comment 30 and comment 31 and I will update the Bug1432718 for the Windows issue.
Status: RESOLVED → VERIFIED
I would like to note one unexpected consequence - add-ons that uses "Alt + number" will now work numeric keys as well - so users that are used to use "Alt + Number" to insert ASCII characters (for example "Alt + 64" for writing "@") will now trigger also "Alt + 6" and "Alt + 4" commands.
You need to log in before you can comment on or make changes to this bug.