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

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[Philipp Kewisch [:Fallen][📱📆]]

(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.

Comment 1

[Caitlin Neiman [:caitmuenster]]

To be discussed at Dec. 13, 2016 WE triage meeting. 

Agenda: https://docs.google.com/document/d/1S1QrBK1hrulE7dlLiQzjMupHUUSwDYRYAOXiqtMHe-k/edit#

Comment 2

[Andy McKay]

General approval to this idea, let's do it.

Comment 3

[Ben Basson]

Thanks for proposing this. I will need it to port my Context Search add-on to WebExtensions.

Comment 4

[Tomislav Jovanovic :zombie]

Attachment: Bug 1302742 - Add keyboard modifiers to contextMenus ClickInfo

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

Comment 5

[Tomislav Jovanovic :zombie]

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.

Comment 6

[Tomislav Jovanovic :zombie]

Comment on attachment 8831268 [details]
Bug 1302742 - Add keyboard modifiers to contextMenus ClickInfo

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

Comment 7

[Andrew Swan [: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"), ...);
```

Comment 8

[Tomislav Jovanovic :zombie]

Comment on attachment 8831268 [details]
Bug 1302742 - Add keyboard modifiers to contextMenus ClickInfo

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

Comment 9

[Tomislav Jovanovic :zombie]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3295e5dbd2a
Add keyboard modifiers to contextMenus ClickInfo r=aswan

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f3295e5dbd2a

Comment 12

[Will Bamberg [:wbamberg]]

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!

Comment 13

[Tomislav Jovanovic :zombie]

Looks good, thanks.