Closed Bug 736321 Opened 12 years ago Closed 11 years ago

Support HTML context menus


(Firefox for Android Graveyard :: General, defect)

Not set


(blocking-fennec1.0 -)

Firefox 20
Tracking Status
blocking-fennec1.0 --- -


(Reporter: wesj, Assigned: wesj)



(Keywords: dev-doc-complete)


(4 files, 4 obsolete files)

Desktop did this in bug 617528. We should implement the front end of it in Fennec as well.
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.
likely not blocking 1.0, but let triage make the call.
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → -
Attached patch WIP (obsolete) — Splinter Review
I started playing around with this, and thought I'd upload a patch. To test, I've been using
Attached patch Patch (obsolete) — Splinter Review
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:
Assignee: nobody → wjohnston
Attachment #618797 - Attachment is obsolete: true
Attachment #664268 - Flags: review?(mbrubeck)
Attached image Screenshot from Gingerbread phone (obsolete) —
Attached image JB Nexus 7 (obsolete) —
The nested affect is not my favorite thing.
I have a WIP to move to using real nested menus. I can upload it here and keep working on it.
Comment on attachment 664268 [details] [diff] [review]

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.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)) {
> +    , popupNode, aEvent.clientX, aEvent.clientY);
> +              foundNode = true;

Looks like "foundNode" is unused and can be removed.
Attachment #664268 - Flags: review?(mbrubeck) → review+
Attached patch Patch 1/2Splinter Review
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.
Attachment #664268 - Attachment is obsolete: true
Attachment #693669 - Flags: review+
Attached patch Patch 2/2Splinter Review
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.
Attached image Droid X (GB) screenshot
Attachment #664270 - Attachment is obsolete: true
Attached image S3 screenshot
Attachment #664271 - Attachment is obsolete: true
Blocks: 820511
Attachment #693670 - Flags: review?(mark.finkle)
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.matches(element, aX, aY)) {
>+            this.menuitems.push(item);
>           }

nit: no {} needed
Attachment #693669 - Flags: review+
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.
Attachment #693670 - Flags: review?(mark.finkle) → review+
Folded and pushed since we don't want this in-between state to exist:
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Depends on: 839771
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.