Wesley Johnston (:wesj) Assignee
	Description • 12 years ago

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

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 1 • 12 years ago

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.

	Tony Chung [:tchung]
	Comment 2 • 12 years ago

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

	:Margaret Leibovic
	Comment 3 • 12 years ago

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

	Wesley Johnston (:wesj) Assignee
	Comment 4 • 12 years ago

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

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 7 • 12 years ago

The nested affect is not my favorite thing.

	Wesley Johnston (:wesj) Assignee
	Comment 8 • 12 years ago

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

	Matt Brubeck (:mbrubeck)
	Comment 9 • 12 years ago

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.

	Wesley Johnston (:wesj) Assignee
	Comment 10 • 12 years ago

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.

	Wesley Johnston (:wesj) Assignee
	Comment 11 • 12 years ago

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.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 14 • 12 years ago

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

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 15 • 12 years ago

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.

	Wesley Johnston (:wesj) Assignee
	Comment 16 • 12 years ago

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

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

	Phil Ringnalda (:philor)
	Comment 17 • 12 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 18 • 9 years ago

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