776446 - Cannot use contextmenu <menu> elements within the desktop webapp runtime

Status: RESOLVED FIXED

(Firefox Graveyard :: Webapp Runtime, defect, P1)

Description

[haydntrowell]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713134347

Steps to reproduce:

In the desktop webapp runtime, right-click on an element linked to a <menu> element via the "contextmenu" attribute.


Actual results:

Nothing. No context menu opened.


Expected results:

The linked context menu should have opened, as it would have in the browser.

Comment 1

[Jason Smith [:jsmith]]

Confirmed by doing the following:

1. Open up http://people.mozilla.com/~prouget/bugs/context-menu-test/test.html in a web app in the desktop web runtime

Expected:

If you right click on the image or the text, you should see a context menu appear with specialized context menu items for this site such as rotate, resize, and share. Go to http://people.mozilla.com/~prouget/bugs/context-menu-test/test.html on Firefox to see this in action.

Actual:

Right-clicking does nothing.

Comment 2

[Marco Castelluccio [:marco]]

Attachment: Patch

This patch adds support for html5 custom context menus.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

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

This doesn't work in my testing on Mac, although it isn't clear why.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

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

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

I also have these few nits.  I know you copied this code from browser/, and these nits are present there, so it's understandable.  But it still seems worth fixing them in the copied code, at least the multiline XUL attributes, which are quite cumbersome to read and hack.

::: webapprt/content/webapp.js
@@ +225,5 @@
> +  Cu.import("resource://gre/modules/PageMenu.jsm", tmp);
> +  return new tmp.PageMenu();
> +});
> +
> +function nsContextMenu(aXulMenu) {

Nit: I would prefer to consistently capitalize all letters of acronyms like XUL (and ID, URL, UI, etc.) in symbol names.

@@ +239,5 @@
> +    this.showItem("page-menu-separator", this.hasPageMenu);
> +  },
> +
> +  showItem: function(aItemOrId, aShow) {
> +    var item = aItemOrId.constructor == String ?

Nit: var -> let

::: webapprt/content/webapp.xul
@@ +167,5 @@
> +                               return gContextMenu.shouldDisplay;"
> +               onpopuphiding="if (event.target != this)
> +                                return;
> +                              gContextMenu = null;
> +                              updateEditUIVisibility();">

Nit: this code would be easier to read as functions in webapps.js that these handler attributes invoked.

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Patch v2

I've also tested on Mac, looks like it works.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 786092 [details] [diff] [review]
Patch v2

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

(In reply to Marco Castelluccio [:marco] from comment #5)
> I've also tested on Mac, looks like it works.

My earlier testing may have been faulty.  This time around, I noticed that the stub would sometimes find /Applications/FirefoxNightly.app instead of my custom build when looking for the Firefox binary with the "org.mozilla.nightly" key (which makes me wonder if custom builds should have a distinct CFBundleIdentifier).

Once I make sure the right binary is found, the patch works as expected!  And now it looks great, too! r=myk :-)

::: webapprt/content/webapp.js
@@ +211,5 @@
> +}
> +
> +function hideContextMenu(aEvent, aXULMenu) {
> +  if (aEvent.target != aXULMenu)
> +    return;

Nit: surround conditional statement with block.

@@ +235,5 @@
> +  showItem: function(aItemOrID, aShow) {
> +    let item = aItemOrID.constructor == String ?
> +      document.getElementById(aItemOrID) : aItemOrID;
> +    if (item)
> +      item.hidden = !aShow;

Nit: surround conditional statement with block.

::: webapprt/content/webapp.xul
@@ +159,5 @@
> +  <popupset>
> +    <menuseparator id="page-menu-separator"/>
> +    <menupopup id="contentAreaContextMenu" pagemenu="start"
> +               onpopupshowing="return showContextMenu(event, this);"
> +               onpopuphiding="hideContextMenu(event, this);">

Nit: omit semi-colons from the ends of one-line statements in on* attributes, as in these examples from earlier in the file:

    <command id="cmd_quitApplication" oncommand="goQuitApplication()"/>

    <menupopup id="menu_EditPopup"
               onpopupshowing="updateEditUIVisibility()"
               onpopuphidden="updateEditUIVisibility()">

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Carrying r+.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9e1379af6902

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e1379af6902