736321 - Support HTML context menus

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Desktop did this in bug 617528. We should implement the front end of it in Fennec as well.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

One issue I saw with implementing this on mobile would be sub-menus. We don't automatically have sub-menu support. We could fake it somehow.

I would be pleased to see even single level menu support added.

Comment 2

[Tony Chung [:tchung]]

likely not blocking 1.0, but let triage make the call.

Comment 3

[:Margaret Leibovic]

Attachment: WIP

I started playing around with this, and thought I'd upload a patch. To test, I've been using http://people.mozilla.com/~mleibovic/test/contextmenu.html

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch

This gives us basic support. Items are always inserted at the top of the menu. We show nested menus in the same way we handle groups in select elements. I played with more advanced support, but I'm not sure we need it.

I'll attach some screenshots from two devices. Also put together a test page at:
http://dl.dropbox.com/u/72157/testContextMenu.html

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Screenshot from Gingerbread phone

Comment 6

[Wesley Johnston (:wesj)]

Attachment: JB Nexus 7

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

The nested affect is not my favorite thing.

Comment 8

[Wesley Johnston (:wesj)]

I have a WIP to move to using real nested menus. I can upload it here and keep working on it.

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 664268 [details] [diff] [review]
Patch

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

::: mobile/android/chrome/content/browser.js
@@ +1514,5 @@
> +    _getMenuItemForId: function(aId) {
> +      if (!this.menuitems)
> +        return null;
> +
> +      for (let i = 0; i < this.menuitems.length; i++) {

I prefer "for (let item of this.menuitems)" for readability.

@@ +1551,3 @@
>          for each (let item in this.items) {
> +          if (!this._getMenuItemForId(item.id) && item.matches(element, aX, aY)) {
> +            this.menuitems.push(item);

I'm slightly worried this could get expensive for long menus. _getMenuItemForId loops through the whole menuitems list, and we're calling it once per registered item, so this is worst-case O(n^2)... and we're potentially calling it for each parent as we walk up the DOM ancestry.

Hmm... probably not a problem for realistic use cases though, with this.menuitems.length < 20.  Optimizing it would require using an ordered map, or rolling our own by using both an array *and* a map.  I think this is fine unless we find that it's slow in practice.

@@ +1596,5 @@
>  
>        // convert this.menuitems object to an array for sending to native code
>        let itemArray = [];
> +      for (let i = 0; i < this.menuitems.length; i++) {
> +        itemArray.push(this.menuitems[i].getValue(popupNode));

for (let item of this.menuitems)

(As a bonus, this would make it easier to switch to an ordered map if that's added to future version of JavaScript.)

@@ +1616,5 @@
> +          // for menuitems added using the native UI, pass the dom element that matched that item to the callback
> +          while (popupNode) {
> +            if (selectedItem.matches(popupNode, aEvent.clientX, aEvent.clientY)) {
> +              selectedItem.callback.call(selectedItem, popupNode, aEvent.clientX, aEvent.clientY);
> +              foundNode = true;

Looks like "foundNode" is unused and can be removed.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2

I held on to this after talking to mfinkle. We'd rather a real "nested" menu approach. I have that now, so I'm putting this back up. No changes here, just unbitrotted.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

This adds support for showing these as a submenu. I do that by basically catching the click in Javascript and showing a new menu. It feels good.

I'm using the stock "more" icon since we're using stock menu styling. Screenshots coming.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Droid X (GB) screenshot

Comment 13

[Wesley Johnston (:wesj)]

Attachment: S3 screenshot

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 693669 [details] [diff] [review]
Patch 1/2

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>   contextmenus: {
>     items: {}, //  a list of context menu items that we may show
>+    _nativeItemsSeparator: 0, // the index to insert native context menu items at

nativeItemsSeparator -> _nativeStart

>+    _addHTMLContextMenuItems: function cm_addContextMenuItems(aMenu, aParent) {

Would you mind changing this to _addFromContent

>+        this.menuitems.splice(this._nativeItemsSeparator, 0, menuitem);
>+        this._nativeItemsSeparator++;
>+        // if this is a menu, add the submenu to it

nit: Add a blank line before the comment

>+        if (item instanceof Ci.nsIDOMHTMLMenuElement) {
>+          menuitem.isGroup = true;
>+          this._addHTMLContextMenuItems(item, item);
>+        }
>+
>+      }

nit: Remove the blank line

>     _sendToContent: function(aX, aY) {

>+        // then check for any context menu items registered in the ui
>+         for each (let item in this.items) {

Bad indent on the "for" line

>+          if (!this._getMenuItemForId(item.id) && item.matches(element, aX, aY)) {
>+            this.menuitems.push(item);
>           }

nit: no {} needed

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 693670 [details] [diff] [review]
Patch 2/2

Getting tests for this feature would be nice. If you land this in Fx20, we need to make sure we are ready to uplift regression fixes.

Comment 16

[Wesley Johnston (:wesj)]

Folded and pushed since we don't want this in-between state to exist:

https://hg.mozilla.org/integration/mozilla-inbound/rev/561ba958f34b

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/561ba958f34b

Comment 18

[Jean-Yves Perrier [:teoli]]

Updated:
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/contextmenu#Browser_compatibility
and
https://developer.mozilla.org/en-US/Firefox/Releases/20$edit#HTML