Pass modifier (shift,cmd,...) to onclick callback for chrome.contextMenus.create()

RESOLVED FIXED in Firefox 54

Status

enhancement
P3
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: Fallen, Assigned: zombie)

Tracking

({dev-doc-complete})

unspecified
mozilla54
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [design-decision-approved][triaged][contextMenus])

Attachments

(1 attachment)

(This is the Firefox side of a Chrome bug I just filed, which I think will be very useful: https://bugs.chromium.org/p/chromium/issues/detail?id=646862 )


This is a feature request to add a property to the info object of the onclick callback for chrome.contextMenus.create() and friends.

A common use case for context menu items is opening tabs based on the clicked item. Also, it is common that Cmd+Click on a link opens a link in a new tab.

It would be great if the modifier(s) used were exposed on to the callback that gets called when the context menu item is clicked. This way I can have a normal click open the link in the current tab, and a Cmd+Click open it in a new tab.

Suggested addition to onclick callback object info:

{
   ...
   "modifiers": ["Shift", "Ctrl", "MacCtrl"]
   ...
}

The modifiers should use the same format as in chrome.commands.


The only way to work around currently is to use separate context menu items for opening and opening in a new tab. This quickly gets cluttered when there are 4+ items in the context menu and having to add an "open in new tab" option for each of these, even with menu separators.
Component: WebExtensions: Untriaged → WebExtensions: General
Whiteboard: [design-decision-needed][triaged]
Flags: needinfo?(amckay)
General approval to this idea, let's do it.
Flags: needinfo?(amckay)
Priority: -- → P3
Whiteboard: [design-decision-needed][triaged] → [design-decision-approved][triaged][contextMenus]
Thanks for proposing this. I will need it to port my Context Search add-on to WebExtensions.
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Comment on attachment 8831268 [details]
Bug 1302742 - Add keyboard modifiers to contextMenus ClickInfo

https://reviewboard.mozilla.org/r/107824/#review109036

::: browser/components/extensions/ext-contextMenus.js:240
(Diff revision 1)
>        let info = item.getClickInfo(contextData, wasChecked);
> +
> +      const map = {shiftKey: "Shift", altKey: "Alt", metaKey: "Command", ctrlKey: "Ctrl"};
> +      info.modifiers = Object.keys(map).filter(key => event[key]).map(key => map[key]);
> +      if (event.ctrlKey && AppConstants.platform === "macosx") {
> +        info.modifiers.push("MacCtrl");

My idea with including both `Ctrl` and `MacCtrl` to maybe make it easier to write cross-platform code by accident, but I'm not sure that's correct.
Attachment #8831268 - Flags: review?(aswan)
Comment on attachment 8831268 [details]
Bug 1302742 - Add keyboard modifiers to contextMenus ClickInfo

https://reviewboard.mozilla.org/r/107824/#review109328

Don't forget to set dev-doc-needed, if for no other reason than I can eventually read what MacCtrl is supposed to mean...

::: browser/components/extensions/test/browser/browser_ext_contextMenus_onclick.js:242
(Diff revision 2)
> +    const meta = yield click({metaKey: true});
> +    is(meta.modifiers.join(), "Command", "Correct modifier: Command");
> +  }
> +
> +  const altShift = yield click({altKey: true, shiftKey: true});
> +  is(altShift.modifiers.join(), "Shift,Alt", "Correct modifiers: Shift+Alt");

This relies on the ordering in modifiers.  It seems unlikely to ever change but if it does, breaking this test as a side effect would suck.  How about instead:
```
is(modifiers.length, 2, ...);
ok(modifiers.includes("Shift"), ...);
ok(modifiers.includes("Alt"), ...);
```
Attachment #8831268 - Flags: review?(aswan) → review+
Comment on attachment 8831268 [details]
Bug 1302742 - Add keyboard modifiers to contextMenus ClickInfo

https://reviewboard.mozilla.org/r/107824/#review109328

> This relies on the ordering in modifiers.  It seems unlikely to ever change but if it does, breaking this test as a side effect would suck.  How about instead:
> ```
> is(modifiers.length, 2, ...);
> ok(modifiers.includes("Shift"), ...);
> ok(modifiers.includes("Alt"), ...);
> ```

I `sort`ed it out.
Keywords: checkin-needed
Keywords: dev-doc-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f3295e5dbd2a
Add keyboard modifiers to contextMenus ClickInfo r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f3295e5dbd2a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Blocks: 1335743
Depends on: 1335847
I've updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus/OnClickData, and the compat data.

Let me know if this covers it!
Flags: needinfo?(tomica)
Looks good, thanks.
Flags: needinfo?(tomica)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.