Closed Bug 776446 Opened 8 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

(firefox16 wontfix)

RESOLVED FIXED
Firefox 26
Tracking Status
firefox16 --- wontfix

People

(Reporter: haydntrowell, Assigned: marco)

References

Details

(Keywords: productwanted)

Attachments

(1 file, 2 obsolete files)

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.
Component: Untriaged → Webapp Runtime
OS: Windows 7 → All
Hardware: x86 → All
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Attached patch Patch (obsolete) — Splinter Review
This patch adds support for html5 custom context menus.
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Attachment #782098 - Flags: review?(myk)
Comment on attachment 782098 [details] [diff] [review]
Patch

This doesn't work in my testing on Mac, although it isn't clear why.
Attachment #782098 - Flags: review?(myk) → review-
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.
Attached patch Patch v2 (obsolete) — Splinter Review
I've also tested on Mac, looks like it works.
Attachment #782098 - Attachment is obsolete: true
Attachment #786092 - Flags: review?(myk)
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()">
Attachment #786092 - Flags: review?(myk) → review+
Attached patch Patch v2Splinter Review
Carrying r+.
Attachment #786092 - Attachment is obsolete: true
Attachment #787858 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9e1379af6902
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Depends on: 907324
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.