Add ability to set access key to context menu item

VERIFIED FIXED in Firefox 63

Status

P2
normal
VERIFIED FIXED
2 years ago
5 months ago

People

(Reporter: kazz, Assigned: robwu, Mentored)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

52 Branch
mozilla63
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(firefox63 verified)

Details

(Whiteboard: triaged, menus)

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
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.

Updated

2 years ago
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit

Updated

2 years ago
Keywords: good-first-bug
Priority: -- → P2
Whiteboard: triaged

Comment 1

2 years ago
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)

Comment 3

2 years ago
This is something we should do.
Flags: needinfo?(amckay)

Comment 4

2 years ago
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)

Comment 6

2 years ago
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.

Comment 9

2 years ago
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)

Comment 13

2 years ago
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

Comment 14

2 years ago
Anyone: nobody@mozilla.orgrob@robwu.nl

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

Updated

a year ago
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

Comment 15

a year ago
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

Comment 18

10 months ago
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?
Blocks: 1466876

Updated

9 months ago
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)

Updated

8 months ago
Assignee: nobody → rob
Status: NEW → ASSIGNED
Iteration: --- → 63.2 - July 23
(Assignee)

Updated

8 months ago
Iteration: 63.2 - July 23 → 63.3 - Aug 6
Comment hidden (mozreview-request)

Comment 21

8 months ago
mozreview-review
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?
(Assignee)

Comment 22

8 months ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 24

8 months ago
mozreview-review
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 25

8 months ago
mozreview-review-reply
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

8 months ago
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/1088432fc83e
Support access keys in extension menus. r=mixedpuppy

Comment 27

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1088432fc83e
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Assignee)

Updated

8 months ago
Keywords: good-first-bug → dev-doc-needed
(Assignee)

Updated

8 months ago
Flags: needinfo?(ddurst)

Comment 28

8 months ago
Posted 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.

Updated

8 months ago
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
(Reporter)

Comment 29

7 months ago
The access key is displayed odd on localized context menu.
(Reporter)

Comment 30

7 months ago
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.