1320462 - Add ability to set access key to context menu item

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P2)

Description

[Kazumasa Hasegawa (Kazz)]

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




Expected results:

Jetpack SDK can add accesskey to context menu item, but WebExtensions can't.
It should be implemented for accessibility.

Comment 1

[Suhan Prabhu]

Hi I would like to work on this. This is my first bug, so it would help if I got some pointers and also instructions on how to get the code base.

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

I'm not sure about the state of this bug. Did we agree on doing this? If so, Kris, can you confirm it and be the mentor?

Comment 3

[Andy McKay]

This is something we should do.

Comment 4

[Andy McKay]

If this is a matter of providing "accesskey" to the XUL element, mattw can take mentoring this bug.

Comment 5

[Tomislav Jovanovic :zombie]

(In reply to Suhan Prabhu from comment #1)
> Hi I would like to work on this. This is my first bug, so it would help if I
> got some pointers and also instructions on how to get the code base.

Hey Suhan, check this link for starting instructions, and feel free to ask if you get stuck.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide

Comment 6

[Angus Gratton]

I found this behaviour difference while porting a Chrome Extension which used context menus. I wanted to share some notes about the existing Chrome contextMenu API behaviour (which is the API that browser.contextMenus is based on.)

A Chrome extension which does:

  chrome.contextMenus.create({
      "title": "Add &Item",
      ..
  }

... will have 'I' as the access key in the menu. If the same title is passed to browser.contextMenus.create(), Firefox renders the title verbatim.

I can't find this behaviour mentioned in the Chrome API docs, but it is the Chrome behaviour (and extension authors seem to be relying on it)!

It would be great (at least from my perspective porting a Chrome extension) if the Firefox behaviour was the same, to minimise porting/maintenance overhead.

If Suhan is no longer available to work on this, I could take a shot at it.

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to Angus Gratton from comment #6)

> If Suhan is no longer available to work on this, I could take a shot at it.

Suhan hasn't commented in 2 months, if you want to take it I'd say go for it.  Compatibility would be great.

Comment 8

[Caitlin Neiman [:caitmuenster]]

Hey Angus! Just wanted to check in and see how this is going.

Comment 9

[Angus Gratton]

Hi Caitlin, thanks for checking in. I didn't mean to go silent on this for so long.

I think I figured out what needs modifying in the source, but I got a bit confused while working out the matching automated tests. The various test types & suites spun me out a bit. I am hoping to look at this again soon. In the meantime, if anyone has any pointers on testing this (specifically which part(s) of the automated tests overlap with this functionality) then it'd be greatly appreciated.

Also - if anyone else wants to jump on this then please do not let me stop you. :)

Comment 10

[Caitlin Neiman [:caitmuenster]]

Hey Angus! Absolutely no worries. Mattw, would you be able to offer any pointers on this bug?

Comment 11

[Matthew Wein [:mattw]]

Thanks for looking into this, Angus!

It would be helpful if you could upload the diff/patch of what you currently have so I can provide feedback on it.

If you follow this link - http://searchfox.org/mozilla-central/search?q=&path=browser_ext_contextMenus - you can see a suite of context menu tests that we already have. I think it would be a good idea to add a new test to this for testing access keys. 

I think some of our internal methods will be very helpful for testing this - the two that I think would be most useful are the global `openContextMenu(cssSelector)` and `EventUtils.synthesizeKey(key, modifiers || {}, window)`.

Comment 12

[Andreas Wagner [:TheOne] [use NI]]

Hey Angus, are you still working on this?

Comment 13

[Angus Gratton]

Hi Andreas,

Sorry, I still want to do this but it keeps getting pushed down my todo list. As I said earlier, if anyone wants to jump in and take this off me then please do.

Comment 14

[Worcester12345]

Anyone: [e-mail]→ [e-mail]

? (Since he's working on some similar ones)

Comment 15

[Andy McKay]

I'd like to see this done to help improve the story for a11y, and its a P2 so moving over to mstriemer. It looks like the previous contributors haven't had chance to work on it. If that's incorrect and anyone would like to keep going on this, please let me know.

Comment 16

[Andreas Wagner [:TheOne] [use NI]]

Hey David, could you help us re-triage this to make sure this is still a P2? Thanks!

Comment 18

[YUKI "Piro" Hiroshi]

There is a library to provide fake menu-like UI in built-in pages of WebExtensions addons, and it contains some codes to imitate accesskey behavior based on a character prefixed with "&".
https://github.com/piroor/webextensions-lib-menu-ui/blob/master/MenuUI.js#L65
Does it help to write any patch for this bug?

Comment 19

[Caitlin Neiman [:caitmuenster]]

Unassigning Mark and opening this up for contributors to take on. If this is your first contribution, please refer to https://wiki.mozilla.org/Add-ons/Contribute/Code on how to get started.

Comment 20

[Rob Wu [:robwu]]

Attachment: Bug 1320462 - Support access keys in extension menus.

Review commit: https://reviewboard.mozilla.org/r/261590/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261590/

Comment 21

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8997924 [details]
Bug 1320462 - Support access keys in extension menus.

https://reviewboard.mozilla.org/r/261590/#review268620

::: browser/components/extensions/test/browser/browser_ext_menus_accesskey.js:17
(Diff revision 1)
> +      title: "&accesskey",
> +      label: "ccesskey",
> +      key: "a",

This seems strange to me.  If I set the title to "&accesskey", I expect my label to be "accesskey", not "ccesskey".  Why are we dropping the access key from the title?

::: browser/components/extensions/test/browser/browser_ext_menus_accesskey.js:24
(Diff revision 1)
> +      key: "a",
> +    }, {
> +      description: "amp in between",
> +      title: "A& b",
> +      label: "Ab",
> +      key: " ",

not sure space should be allowed, how does this appear in the UI?

::: browser/components/extensions/test/browser/browser_ext_menus_accesskey.js:27
(Diff revision 1)
> +      title: "A& b",
> +      label: "Ab",
> +      key: " ",
> +    }, {
> +      description: "lonely amp",
> +      title: "&",

Why would we drop the & here?  And elsewhere in the other tests.  If &{something} is undesirable as an accesskey, I think we should leave the menu unchanged.  Are we pulling this logic from someplace?

Comment 22

[Rob Wu [:robwu]]

Comment on attachment 8997924 [details]
Bug 1320462 - Support access keys in extension menus.

https://reviewboard.mozilla.org/r/261590/#review268620

> This seems strange to me.  If I set the title to "&accesskey", I expect my label to be "accesskey", not "ccesskey".  Why are we dropping the access key from the title?

I made a mistake - only the & should be dropped. I have updated all tests, and then updated the implementation.

> not sure space should be allowed, how does this appear in the UI?

Underscore under space. Pressing space activates the menu item.

> Why would we drop the & here?  And elsewhere in the other tests.  If &{something} is undesirable as an accesskey, I think we should leave the menu unchanged.  Are we pulling this logic from someplace?

This is logic to match Chrome's implementation. A & should always be followed by an access key. To see a literal &, use &&.

I deviated from Chrome in one respect, when & is followed by %s, I don't use that as an access key.
It might not be a good idea to allow the access key to adapt to the selected text (or is it? For compatibility with Chrome?).

Comment 23

[Rob Wu [:robwu]]

Comment on attachment 8997924 [details]
Bug 1320462 - Support access keys in extension menus.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261590/diff/1-2/

Comment 24

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8997924 [details]
Bug 1320462 - Support access keys in extension menus.

https://reviewboard.mozilla.org/r/261590/#review268714

Comment 25

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8997924 [details]
Bug 1320462 - Support access keys in extension menus.

https://reviewboard.mozilla.org/r/261590/#review268620

> This is logic to match Chrome's implementation. A & should always be followed by an access key. To see a literal &, use &&.
> 
> I deviated from Chrome in one respect, when & is followed by %s, I don't use that as an access key.
> It might not be a good idea to allow the access key to adapt to the selected text (or is it? For compatibility with Chrome?).

"(or is it? For compatibility with Chrome?)"

probably not, but if it becomes an issue it can be a followup later.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1088432fc83e
Support access keys in extension menus. r=mixedpuppy

Comment 27

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/1088432fc83e

Comment 28

[CosminB]

Attachment: Bug1320462.gif

I was able to reproduce this issue on Firefox 61.0.2 (20180807170231) under Win 7 64-bit and Mac OS X 10.13.3.

This issue is verified as fixed on Firefox 63.0a1(20180809101613) under Win 7 64-bit and Mac OS X 10.13.3.

Please see the attached video.

Comment 29

[Kazumasa Hasegawa (Kazz)]

Attachment: screenshot of the localized context menu with accesskey

The access key is displayed odd on localized context menu.

Comment 30

[Kazumasa Hasegawa (Kazz)]

Attachment: webExtension with localized contextmenu

webExtension with localized contextmenu for test

Comment 31

[Francesco Lodolo [:flod]]

(In reply to Kazumasa Hasegawa (Kazz) from comment #29)
> Created attachment 9002654 [details]
> screenshot of the localized context menu with accesskey
> 
> The access key is displayed odd on localized context menu.

Thanks for catching it, but please file always file a follow up bug (I'll do it): this one is already marked as verified and the issue will likely go unnoticed.

Comment 32

[Will Bamberg [:wbamberg]]

I've updated the page for https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/create and the compat data. Marking as dev-doc-complete, but please let me know if we need anything else.