Support HTML context menus

RESOLVED FIXED in Firefox 20



7 years ago
4 years ago


(Reporter: wesj, Assigned: wesj)



Firefox 20
Dependency tree / graph

Firefox Tracking Flags

(blocking-fennec1.0 -)



(4 attachments, 4 obsolete attachments)



7 years ago
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: ? → -
Posted patch WIP (obsolete) — Splinter Review
I started playing around with this, and thought I'd upload a patch. To test, I've been using

Comment 4

7 years ago
Posted 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)

Comment 5

7 years ago
Posted image Screenshot from Gingerbread phone (obsolete) —

Comment 6

7 years ago
Posted image JB Nexus 7 (obsolete) —
The nested affect is not my favorite thing.

Comment 8

7 years ago
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+

Comment 10

7 years ago
Posted 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+

Comment 11

7 years ago
Posted 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.

Comment 12

7 years ago
Attachment #664270 - Attachment is obsolete: true

Comment 13

7 years ago
Posted image S3 screenshot
Attachment #664271 - Attachment is obsolete: true


7 years ago
Blocks: 820511


7 years ago
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+

Comment 16

7 years ago
Folded and pushed since we don't want this in-between state to exist:
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Depends on: 839771
You need to log in before you can comment on or make changes to this bug.