Implement checkboxes, radio groups, and icons for context menu items

RESOLVED FIXED in Firefox 47

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: kmag, Assigned: mattw, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla47
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [contextMenus][good first bug]triaged)

Attachments

(1 attachment, 4 obsolete attachments)

+++ This bug was initially created as a clone of Bug #1211368 +++

See https://developer.chrome.com/extensions/contextMenus
Blocks: 1211368

Updated

2 years ago
Flags: blocking-webextensions+
Whiteboard: [contextMenus] → [contextMenus][good first bug]
Kris, as you added the "good first bug" tag, could you please add some details about the scope of the bug and some guidance how and where to fix this?
Flags: needinfo?(kmaglione+bmo)
Gabor is the mentor for this bug.
Flags: needinfo?(kmaglione+bmo) → needinfo?(gkrizsanits)
This should be a fairly straightforward bug. The documentation for these features was already mentioned above. XUL menus already have all these features, and a good guide can be found here for them: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/PopupGuide/MenuItems

The icon part is the least trivial one I guess, but I would expect our icon API can be used for this already and then it should not be too hard http://mxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-utils.js#29

Ideally only the menuBuilder should be changed in ext-contextMenus.js to get all the features working.
Flags: needinfo?(gkrizsanits)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> The icon part is the least trivial one I guess, but I would expect our icon
> API can be used for this already and then it should not be too hard
> http://mxr.mozilla.org/mozilla-central/source/browser/components/extensions/
> ext-utils.js#29

The sanitized icon data for this is incidentally available in `extension.manifest.icons` since bug 1225715, so that should make it a bit easier.
(Assignee)

Updated

a year ago
Assignee: nobody → mwein

Updated

a year ago
Whiteboard: [contextMenus][good first bug] → [contextMenus][good first bug]triaged
(Assignee)

Comment 5

a year ago
Is there any API documentation for how icons should be added? 

It doesn't look like there's any support for them in the schema, and when I try using the property "icon" I get an error: "Type error for parameter createProperties (Unexpected property 'icon') for contextMenus.create".

I didn't have any trouble getting checkboxes or radio groups added.
(Assignee)

Comment 6

a year ago
Gabor, fyi - Kris helped me get past the problem I was experiencing in the previous comment since we are on the same time zone.
(Assignee)

Comment 7

a year ago
Created attachment 8717342 [details] [diff] [review]
Add support for checkboxes, radio groups, and icons for context menu items
Attachment #8717342 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Comment 8

a year ago
Created attachment 8717343 [details] [diff] [review]
Add support for checkboxes, radio groups, and icons for context menu items.
Attachment #8717343 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Updated

a year ago
Attachment #8717342 - Attachment is obsolete: true
Attachment #8717342 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Comment 9

a year ago
It's still a work in progress, but I'm interested in feedback on how I implemented the radio items, because I think it's a little messy. I'll work on writing the unit tests in the meantime.
Comment on attachment 8717343 [details] [diff] [review]
Add support for checkboxes, radio groups, and icons for context menu items.

Review of attachment 8717343 [details] [diff] [review]:
-----------------------------------------------------------------

You're on the right track. We just need to be a bit more careful about the radio group processing.

Your current strategy is going to lead to leaks after items are removed, or extensions are shut down. And it has a few corner cases where we'll wind up with the wrong grouping.

::: browser/components/extensions/ext-contextMenus.js
@@ +36,5 @@
> +  if (!radioGroups[item.name]) {
> +    radioGroups[item.name] = [];
> +  }
> +  radioGroups[item.name].push(item);
> +};

We shouldn't be using a global for this, especially if we're not being extremely careful to remove elements when they're destroyed.

@@ +64,5 @@
> +      if (root.extension.manifest.icons) {
> +        let parentWindow = contextData.menu.ownerDocument.defaultView;
> +        let extension = root.extension;
> +
> +        let url = IconDetails.getURL(extension.manifest.icons, parentWindow, extension);

We need a 16px icon here, but getURL provides an 18px icon. We should probably add an extra parameter for icon size that defaults to 18.

@@ +97,5 @@
>      // root menu element for the extension.
>      let menuPopup = element.firstChild;
>      if (menuPopup && menuPopup.childNodes.length == 1) {
>        let onlyChild = menuPopup.firstChild;
> +      onlyChild.setAttribute("class", "menuitem-iconic");

We should only set this if it's set on the root item. I think we need to copy the image attribute in that case, too.

@@ +159,5 @@
> +          buildingRadioGroup = true;
> +          let name = addRadioItem(item);
> +          element.setAttribute("name", name);
> +        } else {
> +          element.setAttribute("name", item.name);

I think there are probably a few problems with this radio group processing strategy. We could fix them, but I think we should try for a simpler approach.

We can keep the nextRadioGroupID global (please rename it gNextRadioGroupID, or something along those lines, though), but I think we should get rid of the other globals, and just loop through all child items in buildElementWithChildren, and assign a group name to all groups of consecutive radio items.

We should also give the name attribute a fairly specific prefix. `webext-radio-group-${groupId}`, or something along those lines.
Attachment #8717343 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Comment 11

a year ago
I like this simpler approach for the radio groups, but I don't see how to handle updates with it. The way I'm currently doing it is by mapping the radio group names to their items. Whenever one is clicked, I use its name to access the rest of the group and uncheck any that are currently selected.  Do you know of a way to do this with the simpler approach?
(In reply to Matthew Wein [:mattw] from comment #11)
> I like this simpler approach for the radio groups, but I don't see how to
> handle updates with it. The way I'm currently doing it is by mapping the
> radio group names to their items. Whenever one is clicked, I use its name to
> access the rest of the group and uncheck any that are currently selected. 
> Do you know of a way to do this with the simpler approach?

Just do the same thing, but do it for any items with the same parent and the same group name.
(Assignee)

Comment 13

a year ago
Created attachment 8718635 [details] [diff] [review]
Add support for checkboxes, radio groups, and icons
Attachment #8718635 - Flags: review?(kmaglione+bmo)
Comment on attachment 8718635 [details] [diff] [review]
Add support for checkboxes, radio groups, and icons

Review of attachment 8718635 [details] [diff] [review]:
-----------------------------------------------------------------

This is nicely done. My comments are mostly just nitpicking.

::: browser/components/extensions/ext-contextMenus.js
@@ +58,5 @@
> +
> +        let url = IconDetails.getURL(extension.manifest.icons, parentWindow, extension, 16 /* size */);
> +        let resolvedURL = root.extension.baseURI.resolve(url);
> +
> +        if (rootElement.hasAttribute("ext-type", "top-level-menu")) {

We should check the tag name instead. `if (rootElement.localName == "menu") { ... } else if (rootElement.localName == "menuitem") { ... }`

@@ +83,5 @@
> +        child.name = `webext-radio-group-${gNextRadioGroupID}`;
> +      } else if (buildingRadioGroup) {
> +        buildingRadioGroup = false;
> +        gNextRadioGroupID++;
> +      }

Not bad, but I think we can simplify it a bit:

   let groupName;
   for (let child of item.children) {
     if (child.type == "radio") {
       if (!groupName) {
         groupName = `webext-radio-group-${gNextRadioGroupID++}`;
       }
       child.name = groupName;
     } else {
       groupName = null;
     }

Also, let's go with `child.groupName` rather than `child.name`, since the latter is likely to be confusing.

@@ +153,5 @@
>      }
>  
> +    if (item.type == "checkbox") {
> +      element.setAttribute("type", "checkbox");
> +      if (item.checked == true) {

Never compare to `true`. `if (item.checked) {`

@@ +154,5 @@
>  
> +    if (item.type == "checkbox") {
> +      element.setAttribute("type", "checkbox");
> +      if (item.checked == true) {
> +        element.setAttribute("checked", true);

`setAttribute` takes a string. `true` will be stringified to "true", so it works, but it's better to be explicit. Same for the one below.

@@ +172,5 @@
>      element.addEventListener("command", event => {  // eslint-disable-line mozilla/balanced-listeners
> +      if (item.type == "checkbox") {
> +        item.checked = !item.checked;
> +      } else if (item.type == "radio") {
> +        // deselect all radio items in the current radio group.

Please always capitalize the first word of a comment.

@@ +173,5 @@
> +      if (item.type == "checkbox") {
> +        item.checked = !item.checked;
> +      } else if (item.type == "radio") {
> +        // deselect all radio items in the current radio group.
> +        if (item.parent) {

This check isn't necessary. Every item but the root item has a parent, and that's never a radio item.

@@ +180,5 @@
> +              child.checked = false;
> +            }
> +          }
> +        }
> +        // select the clicked radio item.

Please capitalize.

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js
@@ +125,5 @@
> +
> +  function openExtensionMenu() {
> +    let items = contentAreaContextMenu.getElementsByAttribute("ext-type", "top-level-menu");
> +    is(items.length, 1, "top level item was found (context=selection)");
> +    let topItem = items.item(0);

Better to just use `items[0]`.

@@ +127,5 @@
> +    let items = contentAreaContextMenu.getElementsByAttribute("ext-type", "top-level-menu");
> +    is(items.length, 1, "top level item was found (context=selection)");
> +    let topItem = items.item(0);
> +    let top = topItem.childNodes[0];
> +    top.openPopup(topItem, "end_before", 0, 0, true, false);

Hmm... I know this works, and I know you didn't write it, but it would really be better to just synthesize a click and have that open the menu...

@@ +133,5 @@
> +  }
> +
> +  function* selectContextMenuItem(item, expectedClickInfo) {
> +    function checkClickInfo(info) {
> +      info = JSON.parse(info);

Messages are allowed to contain complex objects. Let's remove the JSON stringification/destringification.

@@ +134,5 @@
> +
> +  function* selectContextMenuItem(item, expectedClickInfo) {
> +    function checkClickInfo(info) {
> +      info = JSON.parse(info);
> +      for (let i in expectedClickInfo) {

It's generally better to use `for (let i of Object.keys(expectedClickInfo))`

@@ +162,5 @@
> +    is(radioGroup2.length, 2, "the second radio group should only have 2 radio items");
> +
> +    is(radioGroup1.item(0).hasAttribute("checked"), expectedStates[0], `radio item 1 has state (checked=${expectedStates[0]})`);
> +    is(radioGroup2.item(0).hasAttribute("checked"), expectedStates[1], `radio item 2 has state (checked=${expectedStates[1]})`);
> +    is(radioGroup2.item(1).hasAttribute("checked"), expectedStates[2], `radio item 3 has state (checked=${expectedStates[2]})`);

Please use `[0]` and `[1]` rather than `.item(0)` and `.item(1)`. Same for the similar uses below.

@@ +283,3 @@
>  
>    items = contentAreaContextMenu.getElementsByAttribute("ext-type", "top-level-menu");
>    is(items.length, 0, "top level item was not found (after removeAll()");

We should close the context menu after this check. Leaving popups open at the end of a test can cause issues.
Attachment #8718635 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Updated

a year ago
Iteration: --- → 47.2 - Feb 22
(Assignee)

Updated

a year ago
Iteration: 47.2 - Feb 22 → 47.3 - Mar 7
(Assignee)

Comment 15

a year ago
Thanks for the review.

> > +    let top = topItem.childNodes[0];
> > +    top.openPopup(topItem, "end_before", 0, 0, true, false);
> 
> Hmm... I know this works, and I know you didn't write it, but it would
> really be better to just synthesize a click and have that open the menu...

Yeah, I think this change would be good. I tried using EventUtils to synthesize the click but wasn't able to find an event to yield on that would fire when the menu finished opening. Do you have any ideas on how I could do this?

I made all of the other requested changes.
(Assignee)

Comment 16

a year ago
Created attachment 8724613 [details] [diff] [review]
Add support for checkboxes, radio groups, and icons
I think you should just need to use synthesizeMouseAtCenter on the menu item (not on the child node that getTop() currently returns). There's already code to handle waiting for the menu to show.
(Assignee)

Comment 18

a year ago
Created attachment 8725921 [details] [diff] [review]
Add support for checkboxes, radio groups, and icons
(Assignee)

Comment 19

a year ago
It turns out that I was yielding on the wrong event.  Clicking on the menu item for the extension does not yield a "menuItemClick" event.  However, since the extension menu is also a popup, a "popupshown" event does get fired when it finishes opening.
(Assignee)

Comment 20

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41a2e7a295a8
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Attachment #8717343 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8718635 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8724613 - Attachment is obsolete: true

Comment 21

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/428778584d51
Keywords: checkin-needed

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/428778584d51
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Comment 23

a year ago
Is there any hack/CSS to add icon to WebExtension contextMenus in FF45?
(In reply to erosman from comment #23)
> Is there any hack/CSS to add icon to WebExtension contextMenus in FF45?

Nope.

Updated

a year ago
Duplicate of this bug: 1272977
You need to log in before you can comment on or make changes to this bug.