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)
WebExtensions
General
Tracking
(firefox64 verified, firefox65 verified)
VERIFIED
FIXED
mozilla64
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.
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
status-firefox57:
--- → affected
status-firefox58:
--- → affected
status-firefox59:
--- → affected
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Version: 57 Branch → Trunk
Updated•6 years ago
|
Priority: -- → P5
Updated•6 years ago
|
Keywords: good-first-bug
Updated•6 years ago
|
Mentor: mstriemer
Comment 3•6 years ago
|
||
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
Reporter | ||
Comment 5•6 years ago
|
||
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)
Reporter | ||
Comment 7•6 years ago
|
||
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!
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Comment 8•6 years ago
|
||
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.
status-firefox57:
affected → ---
status-firefox58:
affected → ---
Comment 9•6 years ago
|
||
Hi, I would like to take this up for my first bug. Please give me pointers for the same... Thanks, Sachin
Comment 10•6 years ago
|
||
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)
Comment 11•6 years ago
|
||
Yes, I've a build setup with the mozilla central repo.
Comment 12•6 years ago
|
||
Great! Mark, can you help Sachin get started with this?
Flags: needinfo?(sachinhosmani2) → needinfo?(mstriemer)
Comment 13•6 years ago
|
||
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)
Comment 14•6 years ago
|
||
Hey Sachin, how's it going with this bug?
Assignee: nobody → sachinllvm
Flags: needinfo?(sachinllvm)
Comment 15•6 years ago
|
||
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
Updated•6 years ago
|
Flags: needinfo?(sachinllvm)
Assignee | ||
Comment 16•6 years ago
|
||
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.
Comment 17•6 years ago
|
||
Hey Tim, welcome! I've just assigned you to this bug.
Assignee: nobody → accountsemail
Flags: needinfo?(mstriemer)
Comment 18•6 years ago
|
||
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)
Assignee | ||
Comment 19•6 years ago
|
||
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)
Comment 20•6 years ago
|
||
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)
Assignee | ||
Comment 21•6 years ago
|
||
I committed the changes here: https://phabricator.services.mozilla.com/D8535. Thanks for the help with Phabricator.
Assignee | ||
Comment 22•6 years ago
|
||
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.
Comment 23•6 years ago
|
||
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
Comment 24•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/900eabf8fcee
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment 25•6 years ago
|
||
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!
Assignee | ||
Comment 26•6 years ago
|
||
Yes, I would like to an account. This was a great experience and I'd also like to work on another bug. Any suggestions?
Updated•6 years ago
|
status-firefox59:
affected → ---
Comment 27•6 years ago
|
||
(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. :)
Reporter | ||
Comment 28•6 years ago
|
||
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!
Assignee | ||
Comment 29•6 years ago
|
||
@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.
Comment 30•6 years ago
|
||
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)
Assignee | ||
Comment 31•6 years ago
|
||
@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)
Comment 32•6 years ago
|
||
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.
Reporter | ||
Comment 33•6 years ago
|
||
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.
Description
•