Closed Bug 907016 Opened 11 years ago Closed 7 years ago

Add ability to associate hotkeys with menuitems

Categories

(Add-on SDK Graveyard :: General, defect, P2)

Product:
Add-on SDK Graveyard
Component:
General
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE

People

(Reporter: irakli, Unassigned)

References

Details

(Keywords: steps-wanted)

Attachments

(1 file)

Screen Shot 2013-08-21 at 8.36.05 PM.png
215.83 KB, image/png
Details 
Current menuitem JEP does not supports a way to associate hotkey with it:
https://github.com/mozilla/addon-sdk/wiki/JEP-Menuitems

And even if user creates hotkey and performs same actions as a specific menuitem there still will be no label showing associated keys as in native menu items:

http://cl.ly/Qtco

We should make it possible to create menu items with associated hotkeys.
It looks like adding support for hotkeys is not as difficult and can be achieved without rewriting all of it. There is `acceltext` attribute on `menuitem` which is a string displayed in the menuitem as a hotkey.

I also tried it in scratchpad and it seems to work fine, with that I'd say adding support for hotkeys should be trivial:

menuitem.setAttribute("acceltext", hotkey.combo)
hotkey.on("press", _ => emit(this, "click", this))
Flags: needinfo?(evold)
Oh it looks like documentation is also pretty clear about it :)
https://developer.mozilla.org/en-US/docs/XUL/menuitem#a-acceltext
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #3)
> Oh it looks like documentation is also pretty clear about it :)
> https://developer.mozilla.org/en-US/docs/XUL/menuitem#a-acceltext

The last sentence says "This attribute does not apply to menus directly on the menubar." so I doubt it'll work.

Also the rest of the description is pretty vague, I'm not sure it is meant to be a replacement for the key attribute.


I'll try it out though.
Flags: needinfo?(evold)
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #4)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #3)
> > Oh it looks like documentation is also pretty clear about it :)
> > https://developer.mozilla.org/en-US/docs/XUL/menuitem#a-acceltext
> 
> The last sentence says "This attribute does not apply to menus directly on
> the menubar." so I doubt it'll work.

Ah it must mean <menu> and not <menuitem>
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #1)
> It looks like adding support for hotkeys is not as difficult and can be
> achieved without rewriting all of it. There is `acceltext` attribute on
> `menuitem` which is a string displayed in the menuitem as a hotkey.
> 
> I also tried it in scratchpad and it seems to work fine, with that I'd say
> adding support for hotkeys should be trivial:
> 
> menuitem.setAttribute("acceltext", hotkey.combo)
> hotkey.on("press", _ => emit(this, "click", this))

What did you try in Scratchpad?

I've tried the following, with many different types of `acceltext` and nothing seems to work.

let fileMenu = document.getElementById('menu_FilePopup');
let menuitem = document.createElement('menuitem');
menuitem.setAttribute('label', 'Test');
menuitem.setAttribute('acceltext', 'accel-alt-shift-o');
fileMenu.appendChild(menuitem);
Flags: needinfo?(rFobic)
Mossop do you know if there is another way to show hotkey text on a menuitem without using a <key> ? or if not do you know someone that would have a definitive answer for us?

We're trying to determine if using XUL <key>s are necessary or if they can be avoided.
Flags: needinfo?(dtownsend+bugmail)
Note there are advantages to using a <key> element, for one it'd be easier to maintain, since we won't need code to convert the combo in to keyCodes, and no special cases to handle.
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #6)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #1)
> > It looks like adding support for hotkeys is not as difficult and can be
> > achieved without rewriting all of it. There is `acceltext` attribute on
> > `menuitem` which is a string displayed in the menuitem as a hotkey.
> > 
> > I also tried it in scratchpad and it seems to work fine, with that I'd say
> > adding support for hotkeys should be trivial:
> > 
> > menuitem.setAttribute("acceltext", hotkey.combo)
> > hotkey.on("press", _ => emit(this, "click", this))
> 
> What did you try in Scratchpad?
> 
> I've tried the following, with many different types of `acceltext` and
> nothing seems to work.
> 
> let fileMenu = document.getElementById('menu_FilePopup');
> let menuitem = document.createElement('menuitem');
> menuitem.setAttribute('label', 'Test');
> menuitem.setAttribute('acceltext', 'accel-alt-shift-o');
> fileMenu.appendChild(menuitem);

One thing that may give a clue, is that I tried following:

let document = window.document;
let menu = document.querySelector("#menu_FilePopup")
let menuitem = document.createElement('menuitem');

menuitem.setAttribute('label', 'Test');
menuitem.setAttribute('acceltext', 'k');
menu.appendChild(menuitem);

In the console for a tab with a following document loaded in it: chrome://browser/content/browser.xul

And it worked as expected. When I tried to do a same thing but from scratchpad and on actual window, I saw new item added but it did not had a acceltext present.
Flags: needinfo?(rFobic)
Another interesting observation:

let WW = Cc["@mozilla.org/appshell/window-mediator;1"].
    getService(Ci.nsIWindowMediator);
    
let window = WW.getMostRecentWindow("browser")
let document = window.document;
let menu = document.querySelector("#menu_EditPopup")
let menuitem = document.createElement('menuitem');
let clone = document.querySelector("#menu_copy").cloneNode(true)
menu.appendChild(clone)

This code will add a menuitem but acceltext won't show up, I suspect some bug in a menuitem implementation itself.
As Irakli said, acceltext should do it, why do you want to avoid using <key> though?
Flags: needinfo?(dtownsend+bugmail)
Flags: needinfo?(gkrizsanits)
Flags: needinfo?(ejpbruel)
(In reply to Dave Townsend (:Mossop) from comment #11)
> As Irakli said, acceltext should do it, why do you want to avoid using <key>
> though?

I'd like to use <key> but our current hotkey module doesn't use them.
Screen shot of acceltext in dubious action
So with my screen shot in comment @13 I think it's pretty clear that even if acceltext worked in the <menubar> it would still not be very useful, because we would have to convert combo's to proper charCodes.  Moreover we'd have to check the OS and convert modifiers to their appropriate shorthand notation (example: control -> ctrl only on windows or maybe linux too? idk), and there is no way to automate this, we can only create a mapping and hope it is correct, and patch errors where they are found...

I suspect that XUL is not using a mapping, but instead relying on the OS to do this correctly, thus we should rely on XUL.
Ok I have three main points why I think xul:keys (or platform implementations when xul is dead) are better than our current implementation which hopes to be platform independent:

1. Less code has to be written and maintained, because we do not need to do the work that the platform would do for us (ie converting hotkey combos to keyCodes and charCodes), and we don't have to write tests which the platform already tests (ie converting hotkey combos to keyCodes and charCodes, and other special cases).
2. It's easy to associate platform implementations of hotkeys with platform implementations of ui elements.
3. It's easy for other extensions to manipulate platform implemented hotkeys, and pretty much impossible (or at least very hard) for them to manipulate hotkeys with our current implementation.

I implied only #1 back in Bug 664361 and I was dealing with #2 when I raised the bug, and I pointed out the usefulness of #2 in https://bugzilla.mozilla.org/show_bug.cgi?id=664361#c15 again implicitly.

Irakli since you are the tech lead, I think you should decide whether or not we use a platform implementation here or our own.  The former would mean using my xulkey module, the latter would mean getting a platform fix and making substantial upgrades to our current implementation.

I honestly think the latter is far more work and involves more actors and coordination.
Flags: needinfo?(rFobic)
By the way I was digging through the bugs and I cannot find one for the original hotkey implementation, nor do I find any discussion about it anywhere.  So I do not know why it was considered the better route back then.
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #16)
> By the way I was digging through the bugs and I cannot find one for the
> original hotkey implementation, nor do I find any discussion about it
> anywhere.  So I do not know why it was considered the better route back then.

I have pointed out it here:
https://bugzilla.mozilla.org/show_bug.cgi?id=664361#c12

Discussion I mentioned was on IRC so I don't think there are any comments in the bug or mailing list.
Flags: needinfo?(rFobic)
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #15)
> Ok I have three main points why I think xul:keys (or platform
> implementations when xul is dead) are better than our current implementation
> which hopes to be platform independent:
> 
> 1. Less code has to be written and maintained, because we do not need to do
> the work that the platform would do for us (ie converting hotkey combos to
> keyCodes and charCodes), and we don't have to write tests which the platform
> already tests (ie converting hotkey combos to keyCodes and charCodes, and
> other special cases).

I do agree, but would point out that it's matter of creating a map and writing tests for it should be very simple. Also I already had to copy that map from
platform code to do combo to charcode and translations back.

I think it won't be fare to not point out cons of this too:

1. We would have to add / remove xul:key nodes per each window and have tests
   guaranteeing that everything was smooth. Currently that's not an issue because
   removal of hotkey just removes compbo from local map.
2. I'm not sure how xul:keys deal with conflicts, for example if two different
   add-on's create conflicting hotkeys, currently all add-ons handlers will be
   invoked.

> 2. It's easy to associate platform implementations of hotkeys with platform
> implementations of ui elements.

I don't understand this, can you elaborate / give an example ?

> 3. It's easy for other extensions to manipulate platform implemented
> hotkeys, and pretty much impossible (or at least very hard) for them to
> manipulate hotkeys with our current implementation.
>

I'm not sure this is +1 or -1 so I'll treat it as 0. To elaborate whenever we get enough users depending on features we have not committed to it get's into the way of making changes.

> 
> I implied only #1 back in Bug 664361 and I was dealing with #2 when I raised
> the bug, and I pointed out the usefulness of #2 in
> https://bugzilla.mozilla.org/show_bug.cgi?id=664361#c15 again implicitly.
> 
> Irakli since you are the tech lead, I think you should decide whether or not
> we use a platform implementation here or our own.  The former would mean
> using my xulkey module, the latter would mean getting a platform fix and
> making substantial upgrades to our current implementation.
> 
> I honestly think the latter is far more work and involves more actors and
> coordination.


Erik, I don't think I agree we will need substation or any changes to our implementation, all we need is toAccelText(comboString) function & that can
very much be part of menuitem code.

That being said, I do not mind switching to `xul:keys` as long as API remains
same and we won't introduce more random failures to our test suite (luckily
we had no issues with hotkeys yet).

Only issue is I wanna play safe and don't want to couple this two things. Cause in
that case if we find our self regressing hotkeys we won't be able to revert that change without regressing menu items.
I've lost interest in this module, I have made my points as clear as possible, and they are not understood afaict.  Anyhow it appears Irakli wants a to model extension API development after google chrome's authoritarian approach, which I dislike.

Whatever you decide Irakli go for it, I'm going to implement my approach in pathfinder.
Erik,

So knowing what we know today I would definitely choose to implement hotkeys with xul:key. But the thing is it is already implemented, works & has very low maintenance cost. Rewrite at this point includes only risks and some potential benefit in a future. I would be more comfortable leaving hotkeys alone until that
future use case is relevant enough to justify the risks.

As of this specific feature I don't think that adding dependency on the hotkeys is
even a good idea. Not to mention `toAccelText` would be useful for users if they ever want to display hotkeys in any other user interface, for example I can see devtools using hotkeys API some day and they do have pieces of UI there to display
hotkeys associated with a specific commands and they would not be able to take advantage of xul:key to do that.

Implementing that function would also be matter of copying source from the underlying implementation of menuitems that translates key to acceltext. It maybe even worth factoring that code out so that code is shared.


As of platform fixes, I was not able to make it work with xul:keys either so it actually may be a same platform fix. Either way I'd like if Eddy and Gabor could
comment on how hard or easy it would be to fix this bug
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #14)
> So with my screen shot in comment @13 I think it's pretty clear that even if
> acceltext worked in the <menubar> it would still not be very useful, because
> we would have to convert combo's to proper charCodes.  Moreover we'd have to
> check the OS and convert modifiers to their appropriate shorthand notation
> (example: control -> ctrl only on windows or maybe linux too? idk), and
> there is no way to automate this, we can only create a mapping and hope it
> is correct, and patch errors where they are found...
> 
> I suspect that XUL is not using a mapping, but instead relying on the OS to
> do this correctly, thus we should rely on XUL.

Erik could you please share the code you used to make it work ?
Flags: needinfo?(evold)
What I am reading from this is this:

You want to avoid having to rewrite the hotkeys module (which is what you'd have to do if you used XUL the way it was intended), so instead you propose to write a lot of code to simulate behavior that XUL already provides.

I'm not convinced that either approach will be less work than the other. Even if it would be, I think being clever with XUL to avoid rewriting some JavaScript code is shortsighted, and will lead to maintenance nightmare in the long run: many people on the team understand JS. Almost none of us understand XUL.

For what it's worth, I'm with Erik on this one.
Flags: needinfo?(ejpbruel)
(In reply to Eddy Bruel [:ejpbruel] from comment #22)
> What I am reading from this is this:
> 
> You want to avoid having to rewrite the hotkeys module (which is what you'd
> have to do if you used XUL the way it was intended), so instead you propose
> to write a lot of code to simulate behavior that XUL already provides.

A lot? We only need a small piece of code to generate the acceltext from the hotkey combo. That sounds a lot more straightforward and safe then re-implementing the hotkey module.

> I'm not convinced that either approach will be less work than the other.
> Even if it would be, I think being clever with XUL to avoid rewriting some
> JavaScript code is shortsighted, and will lead to maintenance nightmare in
> the long run: many people on the team understand JS. Almost none of us
> understand XUL.

I'd actually take that as an argument to avoid switching to using more XUL if truly almost no-one on the team understands it, look at where we're at trying to maintain the python code we have. I don't think it matters much though, I just don't see the keyboard and menu code in Firefox changing much in the future in any way that will cause us to have to make changes. Long-term maintenance doesn't look like a big cost to us here.
(In reply to Dave Townsend (:Mossop) from comment #23)
> (In reply to Eddy Bruel [:ejpbruel] from comment #22)
> > What I am reading from this is this:
> > 
> > You want to avoid having to rewrite the hotkeys module (which is what you'd
> > have to do if you used XUL the way it was intended), so instead you propose
> > to write a lot of code to simulate behavior that XUL already provides.
> 
> A lot? We only need a small piece of code to generate the acceltext from the
> hotkey combo. That sounds a lot more straightforward and safe then
> re-implementing the hotkey module.

My point is that I'm not convinced that rewriting the existing code to use <key> would be a disproportionally larger amount of work. But if we're really talking about several weeks vs a single day here, I agree it might be worth the risk.

> 
> > I'm not convinced that either approach will be less work than the other.
> > Even if it would be, I think being clever with XUL to avoid rewriting some
> > JavaScript code is shortsighted, and will lead to maintenance nightmare in
> > the long run: many people on the team understand JS. Almost none of us
> > understand XUL.
> 
> I'd actually take that as an argument to avoid switching to using more XUL
> if truly almost no-one on the team understands it, look at where we're at
> trying to maintain the python code we have. I don't think it matters much
> though, I just don't see the keyboard and menu code in Firefox changing much
> in the future in any way that will cause us to have to make changes.
> Long-term maintenance doesn't look like a big cost to us here.

I'd counter that <key> is hardly one of the most complicated features of XUL. It's fairly well documented, and it's *supposed* to be used for implementing hotkeys. In my opinion, if we care about 'playing it safe', when working with a piece of technology we're not comfortable with, its best to stick with what you know, and not try to be too clever.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #21)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #14)
> > So with my screen shot in comment @13 I think it's pretty clear that even if
> > acceltext worked in the <menubar> it would still not be very useful, because
> > we would have to convert combo's to proper charCodes.  Moreover we'd have to
> > check the OS and convert modifiers to their appropriate shorthand notation
> > (example: control -> ctrl only on windows or maybe linux too? idk), and
> > there is no way to automate this, we can only create a mapping and hope it
> > is correct, and patch errors where they are found...
> > 
> > I suspect that XUL is not using a mapping, but instead relying on the OS to
> > do this correctly, thus we should rely on XUL.
> 
> Erik could you please share the code you used to make it work ?

I used your code in https://bugzilla.mozilla.org/show_bug.cgi?id=907016#c9
Flags: needinfo?(evold)
(In reply to Eddy Bruel [:ejpbruel] from comment #24)
> (In reply to Dave Townsend (:Mossop) from comment #23)
> > (In reply to Eddy Bruel [:ejpbruel] from comment #22)
> > > I'm not convinced that either approach will be less work than the other.
> > > Even if it would be, I think being clever with XUL to avoid rewriting some
> > > JavaScript code is shortsighted, and will lead to maintenance nightmare in
> > > the long run: many people on the team understand JS. Almost none of us
> > > understand XUL.
> > 
> > I'd actually take that as an argument to avoid switching to using more XUL
> > if truly almost no-one on the team understands it, look at where we're at
> > trying to maintain the python code we have. I don't think it matters much
> > though, I just don't see the keyboard and menu code in Firefox changing much
> > in the future in any way that will cause us to have to make changes.
> > Long-term maintenance doesn't look like a big cost to us here.
> 
> I'd counter that <key> is hardly one of the most complicated features of
> XUL. It's fairly well documented, and it's *supposed* to be used for
> implementing hotkeys. In my opinion, if we care about 'playing it safe',
> when working with a piece of technology we're not comfortable with, its best
> to stick with what you know, and not try to be too clever.

That's true but it cuts both ways. We already have a hotkeys module and know how it works and that it is used by add-ons now. As Irakli says if we were starting from scratch the choice might become different but right now it's between reimplementing something that works and adding some string translation function.
I think we should switch from discussion weather we should rewrite hotkey API or not, to how do we implement this feature. At the moment xul:key does not solves any problems and has same issues as acceltext. It does appears in real window while it does in the tab.

So can we please concentrate on how to fix make this specific thing work ? And if best way to make it work is xul:key than that's what we going to do. Right now rewrite only gives us risk to regress without some theoretical benefit.

Eddy Cc-id you and Gabor to see if any of you can take a look why we don't see shortcut text as expected. I have also submitted bug 908318 for that.
Flags: needinfo?(ejpbruel)
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #9)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #6)
> > let fileMenu = document.getElementById('menu_FilePopup');
> > let menuitem = document.createElement('menuitem');
> > menuitem.setAttribute('label', 'Test');
> > menuitem.setAttribute('acceltext', 'accel-alt-shift-o');
> > fileMenu.appendChild(menuitem);

This gives nothing to me in scratchpad.

> let document = window.document;
> let menu = document.querySelector("#menu_FilePopup")
> let menuitem = document.createElement('menuitem');
> 
> menuitem.setAttribute('label', 'Test');
> menuitem.setAttribute('acceltext', 'k');
> menu.appendChild(menuitem);

> When I tried to do a same thing but from
> scratchpad and on actual window, I saw new item added but it did not had a
> acceltext present.

Could you be a bit more specific? I tried this but running this code in scratchpad but nothing happened, no new menu item.

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #10)
> Another interesting observation:
> 
> let WW = Cc["@mozilla.org/appshell/window-mediator;1"].
>     getService(Ci.nsIWindowMediator);
>     
> let window = WW.getMostRecentWindow("browser")
> let document = window.document;
> let menu = document.querySelector("#menu_EditPopup")
> let menuitem = document.createElement('menuitem');
> let clone = document.querySelector("#menu_copy").cloneNode(true)
> menu.appendChild(clone)
> 
> This code will add a menuitem but acceltext won't show up, I suspect some
> bug in a menuitem implementation itself.

This code gives me window is null.


Clearly I'm missing something (maybe it's just late..) but could someone give me some help to reproduce the problem so me or Eddy could debug it? Also a working example in contrast would be very helpful. (it was mentioned that somehow from the console it did work, how exactly?) Thanks.
Flags: needinfo?(gkrizsanits)
Keywords: steps-wanted
Clearing the needinfo request on this bug because it seems to be no longer relevant. Feel free to reset it if that's not the case.
Flags: needinfo?(ejpbruel)
https://bugzilla.mozilla.org/show_bug.cgi?id=1399562
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: