Support HTML context menus

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

({dev-doc-complete})

Trunk
Firefox 20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-fennec1.0 -)

Details

Attachments

(4 attachments, 4 obsolete attachments)

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 http://people.mozilla.com/~mleibovic/test/contextmenu.html
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:
http://dl.dropbox.com/u/72157/testContextMenu.html
Assignee: nobody → wjohnston
Attachment #618797 - Attachment is obsolete: true
Attachment #664268 - Flags: review?(mbrubeck)
Posted image Screenshot from Gingerbread phone (obsolete) —
Posted 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]
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.
Attachment #664268 - Flags: review?(mbrubeck) → review+
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+
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.
Attachment #664270 - Attachment is obsolete: true
Posted 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.id) && 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:

https://hg.mozilla.org/integration/mozilla-inbound/rev/561ba958f34b
https://hg.mozilla.org/mozilla-central/rev/561ba958f34b
Status: NEW → RESOLVED
Closed: 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.