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

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Kris Maglione [:kmag]]

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

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

Comment 1

[Andreas Wagner [:TheOne] [use NI]]

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?

Comment 2

[Kris Maglione [:kmag]]

Gabor is the mentor for this bug.

Comment 3

[Gabor Krizsanits (INACTIVE)]

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.

Comment 4

[Kris Maglione [:kmag]]

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

Comment 5

[Matthew Wein [:mattw]]

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.

Comment 6

[Matthew Wein [:mattw]]

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

Comment 7

[Matthew Wein [:mattw]]

Attachment: Add support for checkboxes, radio groups, and icons for context menu items

Comment 8

[Matthew Wein [:mattw]]

Attachment: Add support for checkboxes, radio groups, and icons for context menu items.

Comment 9

[Matthew Wein [:mattw]]

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 10

[Kris Maglione [:kmag]]

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.

Comment 11

[Matthew Wein [:mattw]]

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?

Comment 12

[Kris Maglione [:kmag]]

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

Comment 13

[Matthew Wein [:mattw]]

Attachment: Add support for checkboxes, radio groups, and icons

Comment 14

[Kris Maglione [:kmag]]

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.

Comment 15

[Matthew Wein [:mattw]]

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.

Comment 16

[Matthew Wein [:mattw]]

Attachment: Add support for checkboxes, radio groups, and icons

Comment 17

[Kris Maglione [:kmag]]

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.

Comment 18

[Matthew Wein [:mattw]]

Attachment: Add support for checkboxes, radio groups, and icons

Comment 19

[Matthew Wein [:mattw]]

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.

Comment 20

[Matthew Wein [:mattw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41a2e7a295a8

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/428778584d51

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/428778584d51

Comment 23

[erosman]

Is there any hack/CSS to add icon to WebExtension contextMenus in FF45?

Comment 24

[Kris Maglione [:kmag]]

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

Nope.