"accel-alt-r" hotkey does not work

RESOLVED FIXED in Future

Status

P1
normal
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: erikvvold, Assigned: irakli)

Tracking

unspecified
Future

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(4 attachments)

(Reporter)

Description

8 years ago
Created attachment 539427 [details]
"accel-alt-r"

I've made a quick add-on which I want to use to make a "accel-alt-r" hotkey, but when I try that hotkey it dnw. When I try "control-alt-r" or "alt-r" that all works.
(Reporter)

Comment 1

8 years ago
Created attachment 539429 [details]
"control-alt-r" example which works
Priority: -- → P1
Target Milestone: --- → Future
Assignee: nobody → rFobic
What platform do you run this on ?
On OSX there is actually no way to type this combination, because alt + r gives ® so users won't be able to type accel-alt-r. I will try to test this on windows.
Ok so on windows it works just fine.
Crash Signature: ®®®
So my understanding of the problem is that when you type 'alt-command-r' on mac at the os level diff keyboard input is generated 'alt-command-®'. At least that's a keyboard event dispatched. Not sure if this is platform limitation or expected behavior on the given OS.

I noticed no combos are used on OSX on FF other than ones that are handled by OS itself like alt-command-t -> special character input.

Also, `alt-control-t` combo does not has this issue as in such case t is not substituted with `®`.

Finally we can not workaround this by building a map of exceptions like: `®` -> `r` since on diff keyboard layout on make alt-command-r may be mapped to diff character.
(Reporter)

Comment 5

8 years ago
(In reply to comment #2)
> What platform do you run this on ?
> On OSX there is actually no way to type this combination, because alt + r
> gives ® so users won't be able to type accel-alt-r. I will try to test this
> on windows.

I'm on osx.

When I setup a "accel-alt-r" hotkey with a regular xul key element, that works, I didn't expect the alt to work differently using a addon sdk Hotkey class than it does when using a xul key element. Why would that be? it should be explained in the documentation that the two do not work the same and why that is imo.
That's interesting I did not knew about that, I'll try to investigate how key element works, maybe I'll find out some way!
After digging into this further I've discovered that keydown / keyup events may be used to get a better behavior.

It seems that key combinations that include alt-* sequences emit keydown / keyup with events with a charCodes for those keys, for example alt-r combination will emit:

keydown:   alt-r
keypress:  alt-®
keyup:     alt-r

also combinations that include shift key act:

keydown:   shift-'
kepress:   shift-"
kedown:    shift-'

So keypress seems to be a worst option to go with. That being said there are still set of combinations that do not work with neither with kedown nor keyup. There are two groups of such combinations:

1. All combination that includes following sequences:

   shift-.
   shift-,
   shift-/
   shift-\
   shift-`

   Such combinations trigger events that have charCode, keyCode and which
   properties equal 0, making it impossible to find out which non-modifier
   key was pressed.

2. Following combinations (adding additional modifier solves issue):

   alt-`
   alt-control-`
   alt-n
   alt-e
   alt-u
   alt-i
 
   With this exact combinations keydown / keyup events are not dispatched,
   or maybe there are some handlers that stop propagation earlier.

Finally there is a difference between kedown / keyup events. keyup is not triggered for any combination that contains `command` key.

With all this in mind it looks like it would be best to switch over to a `keydown` events. Also, we might want to mention somewhere in docs that combinations that don't work (feels bit awkward without explaining a reasons).
Created attachment 541406 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/194

Pointer to Github pull-request
Attachment #541406 - Flags: review?(myk)
(Reporter)

Comment 9

8 years ago
I'm curious why the Hotkey class doesn't create xul key elements?
(In reply to comment #9)
> I'm curious why the Hotkey class doesn't create xul key elements?

Because if such elements are added to the chrome window dom after it's loaded platform ignores them.
(Reporter)

Comment 11

8 years ago
(In reply to comment #10)
> (In reply to comment #9)
> > I'm curious why the Hotkey class doesn't create xul key elements?
> 
> Because if such elements are added to the chrome window dom after it's
> loaded platform ignores them.

I don't think this is entirely true, because I have a addon on AMO called Restartless Restart, and if you install it then it will add a xul key element to your already loaded windows which does work.

https://addons.mozilla.org/en-US/firefox/addon/restartless-restart/

I have noticed that removing the xul element wasn't always working though.. and haven't spent the time nailing that issue down to make a bug # yet..

Do you know if the behavior you state is the intended design? I don't understand why that would not be a bug..
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > I'm curious why the Hotkey class doesn't create xul key elements?
> > 
> > Because if such elements are added to the chrome window dom after it's
> > loaded platform ignores them.
> 
> I don't think this is entirely true, because I have a addon on AMO called
> Restartless Restart, and if you install it then it will add a xul key
> element to your already loaded windows which does work.
> 

I remember talking about this with Dietrich when deciding between this two options and he pointed out back than issues with key elements & I remember I tried anyway and run into what he told me, so I went back to this option.

> https://addons.mozilla.org/en-US/firefox/addon/restartless-restart/
> 
> I have noticed that removing the xul element wasn't always working though..
> and haven't spent the time nailing that issue down to make a bug # yet..
> 

Is there source somewhere, I'd be curious to see how you do it.

> Do you know if the behavior you state is the intended design? I don't
> understand why that would not be a bug..

Don't know, CC-ing Dietrich, probably he knows better.
I'd expect that <key> elements will have the same limitations actually, if it's easy to tweak your addon I'd like to verify actually.
(Reporter)

Comment 14

8 years ago
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > I'm curious why the Hotkey class doesn't create xul key elements?
> > > 
> > > Because if such elements are added to the chrome window dom after it's
> > > loaded platform ignores them.
> > 
> > I don't think this is entirely true, because I have a addon on AMO called
> > Restartless Restart, and if you install it then it will add a xul key
> > element to your already loaded windows which does work.
> > 
> 
> I remember talking about this with Dietrich when deciding between this two
> options and he pointed out back than issues with key elements & I remember I
> tried anyway and run into what he told me, so I went back to this option.
> 
> > https://addons.mozilla.org/en-US/firefox/addon/restartless-restart/
> > 
> > I have noticed that removing the xul element wasn't always working though..
> > and haven't spent the time nailing that issue down to make a bug # yet..
> > 
> 
> Is there source somewhere, I'd be curious to see how you do it.

https://github.com/erikvold/restartless-restart-ffext/blob/master/src/bootstrap.js#L183

I'm working on making a jp module out of this which should be easier to tweak if you want to wait for that.
(Reporter)

Comment 16

8 years ago
oops here is the github link for the module: https://github.com/erikvold/xulkeys-jplib
(Reporter)

Comment 17

8 years ago
ok I think that https://github.com/erikvold/xulkeys-jplib/commit/17114ab29ef5eb9f6868e9b04caff43477199343 is the bug that you ran into Irakli.
(Reporter)

Comment 18

8 years ago
That example jetpack is on AMO now: https://addons.mozilla.org/en-US/firefox/addon/restart-jetpack/
Based on comment 7, it sounds like there are some underlying platform issues here.  However, it isn't clear whether they are bugs in the platform or intentional limitations on the registration of hotkeys.

So the thing to do first here is to determine why the platform treats some hotkeys differently.  In particular, I want to make sure there isn't some good reason why we can't listen for certain combinations.

Provided there is no good reason, and it's just a bug in the platform, the next thing to do is to file a platform bug that comprehensively describes the problem, including steps to reproduce and a simplified testcase.

Finally, if it looks like it'll take a while to fix the platform bug, then let's identify a workaround we can implement in the meantime, whether it's keydown/keyup, XUL <key> elements, or some other solution.

Regardless of the solution, I don't want us to lose the ability to register some hotkeys in order to gain the ability to register others.  If keydown/keyup, XUL <key> elements, or whatever other solution we come up with covers additional hotkeys but fails to cover ones handled by the existing implementation, then let's keep the existing implementation for the current set of hotkeys it covers and add a workaround that specifically handles the hotkeys the existing implementation fails to handle.
Comment on attachment 541406 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/194

Cancelling review, as it seems like this patch'll need revising based on comment 19.
Attachment #541406 - Flags: review?(myk)
(In reply to comment #19)
> Based on comment 7, it sounds like there are some underlying platform issues
> here.  However, it isn't clear whether they are bugs in the platform or
> intentional limitations on the registration of hotkeys.
> 
> So the thing to do first here is to determine why the platform treats some
> hotkeys differently.  In particular, I want to make sure there isn't some
> good reason why we can't listen for certain combinations.
> 
> Provided there is no good reason, and it's just a bug in the platform, the
> next thing to do is to file a platform bug that comprehensively describes
> the problem, including steps to reproduce and a simplified testcase.
> 
> Finally, if it looks like it'll take a while to fix the platform bug, then
> let's identify a workaround we can implement in the meantime, whether it's
> keydown/keyup, XUL <key> elements, or some other solution.
> 
> Regardless of the solution, I don't want us to lose the ability to register
> some hotkeys in order to gain the ability to register others.  If
> keydown/keyup, XUL <key> elements, or whatever other solution we come up
> with covers additional hotkeys but fails to cover ones handled by the
> existing implementation, then let's keep the existing implementation for the
> current set of hotkeys it covers and add a workaround that specifically
> handles the hotkeys the existing implementation fails to handle.

Myk current implementation will fail all combinations that include 'alt', so applying this patch is good anyway as it covers >90% of cases and cases that are not handled maybe intentional limitations, which I'll investigate to see if keyset do support. In other words, it's better to add workaround on top of this patch if workarounds will turn out to be necessary.
Created attachment 543725 [details]
Test with keyset.

As it was expected keyset has same limitation as solution with keydown event listener described in comment: https://bugzilla.mozilla.org/show_bug.cgi?id=664361#c7

This file contains lines of JS verifying that.
Comment on attachment 541406 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/194

re-requesting review on this patch, after discussing it with Myk.
Attachment #541406 - Flags: review?(myk)
Comment on attachment 541406 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/194

It looks like this patch has rotted.  I tried to resolve the conflicts, but I wasn't able to do so easily, as my initial attempt resulted in some test failures and warnings.

Irakli: can you update the patch?
Attachment #541406 - Flags: review?(myk)
Comment on attachment 541406 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/194

Hi Myk,

I have resolved all merge conflicts & updated pull request. With it all tests pass both on nightly and FF5 on my machine.
Attachment #541406 - Flags: review?(myk)
Comment on attachment 541406 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/194

r=myk, see nits in pull request.
Attachment #541406 - Flags: review?(myk) → review+
https://github.com/mozilla/addon-sdk/commit/6655460d9b6458dd66dfcd427d9f98d40fd33183
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.