Closed
Bug 1302742
Opened 8 years ago
Closed 7 years ago
Pass modifier (shift,cmd,...) to onclick callback for chrome.contextMenus.create()
Categories
(WebExtensions :: General, enhancement, P3)
WebExtensions
General
Tracking
(firefox54 fixed)
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: Fallen, Assigned: zombie)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved][triaged][contextMenus])
Attachments
(1 file)
(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.
Updated•8 years ago
|
Component: WebExtensions: Untriaged → WebExtensions: General
Whiteboard: [design-decision-needed][triaged]
Comment 1•8 years ago
|
||
To be discussed at Dec. 13, 2016 WE triage meeting. Agenda: https://docs.google.com/document/d/1S1QrBK1hrulE7dlLiQzjMupHUUSwDYRYAOXiqtMHe-k/edit#
Updated•8 years ago
|
Flags: needinfo?(amckay)
Comment 2•8 years ago
|
||
General approval to this idea, let's do it.
Flags: needinfo?(amckay)
Priority: -- → P3
Whiteboard: [design-decision-needed][triaged] → [design-decision-approved][triaged][contextMenus]
Comment 3•7 years ago
|
||
Thanks for proposing this. I will need it to port my Context Search add-on to WebExtensions.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review |
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8831268 -
Flags: review?(aswan)
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 10•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/f3295e5dbd2a Add keyboard modifiers to contextMenus ClickInfo r=aswan
Keywords: checkin-needed
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f3295e5dbd2a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 12•7 years ago
|
||
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)
Updated•7 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•