Closed Bug 1320462 Opened 4 years ago Closed 2 years ago

Add ability to set access key to context menu item

Categories

(WebExtensions :: General, defect, P2)

52 Branch
defect

Tracking

(firefox63 verified)

VERIFIED FIXED
mozilla63
Iteration:
63.3 - Aug 6
Tracking Status
firefox63 --- verified

People

(Reporter: kazz, Assigned: robwu, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: triaged, menus)

Attachments

(4 files)

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.
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Keywords: good-first-bug
Priority: -- → P2
Whiteboard: triaged
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.
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?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(amckay)
This is something we should do.
Flags: needinfo?(amckay)
If this is a matter of providing "accesskey" to the XUL element, mattw can take mentoring this bug.
Mentor: mwein
Flags: needinfo?(kmaglione+bmo)
Status: UNCONFIRMED → NEW
Component: WebExtensions: Untriaged → WebExtensions: General
Ever confirmed: true
(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
Flags: needinfo?(suhanprabhu)
Flags: needinfo?(suhanprabhu)
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.
(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.
Assignee: nobody → gus
Hey Angus! Just wanted to check in and see how this is going.
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. :)
Hey Angus! Absolutely no worries. Mattw, would you be able to offer any pointers on this bug?
Flags: needinfo?(mwein)
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)`.
Flags: needinfo?(mwein)
Blocks: 1358116
Hey Angus, are you still working on this?
Flags: needinfo?(gus)
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.
Flags: needinfo?(gus)
Assignee: gus → nobody
Anyone: nobody@mozilla.orgrob@robwu.nl

? (Since he's working on some similar ones)
Mentor: matthewjwein → mixedpuppy
Summary: WebExtensions: Add ability to set access key to context menu item → Add ability to set access key to context menu item
Whiteboard: triaged → triaged, menus
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.
Assignee: nobody → mstriemer
Hey David, could you help us re-triage this to make sure this is still a P2? Thanks!
Flags: needinfo?(ddurst)
Duplicate of this bug: 1462928
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?
Product: Toolkit → WebExtensions
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.
Assignee: mstriemer → nobody
Assignee: nobody → rob
Status: NEW → ASSIGNED
Iteration: --- → 63.2 - July 23
Iteration: 63.2 - July 23 → 63.3 - Aug 6
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 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 on attachment 8997924 [details]
Bug 1320462 - Support access keys in extension menus.

https://reviewboard.mozilla.org/r/261590/#review268714
Attachment #8997924 - Flags: review?(mixedpuppy) → review+
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.
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/1088432fc83e
Support access keys in extension menus. r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/1088432fc83e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(ddurst)
Attached image 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.
Status: RESOLVED → VERIFIED
The access key is displayed odd on localized context menu.
webExtension with localized contextmenu for test
(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.
Depends on: 1484914
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.
You need to log in before you can comment on or make changes to this bug.