Closed Bug 756371 Opened 12 years ago Closed 12 years ago

Add ability to parent of mozbrowser to handle contextmenu

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16

People

(Reporter: daleharvey, Assigned: daleharvey)

References

Details

Attachments

(1 file, 9 obsolete files)

Add ability to parent of mozbrowser to handle contextmenu
42.40 KB, patch
justin.lebar+bug
: checkin+
Details | Diff | Splinter Review 
The Browser app in Boot2Gecko needs to be able to receive contextmenu events fired from its child browser instances and display a UI to the user
So is this going to need a new api for the browser app to apply the custom context menu callbacks within the scope of the window that initially fired the contextmenu event?

to explain it further

document has a context menu in it with some <menuitem onclick="myFunction()">Run myfunction</menuitem>

user longpresses, contextmenu event is triggered on content frame, BrowserChild catches it, searches for the context menu, collects its data and fires a contextMenu event on the iframe

The Browser app catches that context menu, builds a ui depending on the custom data (and the default items for the target type) the user picks an item, the browser app now has to fire myfunction within the scope of the BrowserChild

I write a basic example of the flow @ http://pastie.org/3928973

That last part kinda has to have an api as far as I can see (I called it iframe.applyCallback), its also a pretty dangerous api as it allows the browser to do pretty much anything it wants, but being as its the browser that has to be expected
Blocks: browser-api
The only reason to handle this in the browser parent is if different browsers will want to handle context menus differently.  That seems tenuous in this case.

The browser API is trusted, but this would be a pretty large expansion of trust...

What do you think?
I think ideally the browser handles how to display the context menu, but I think its totally fine for gecko to do all the magic, my only on that end is where do I stuff the context menu ui?
> my only on that end is where do I stuff the context menu ui?

BrowserElementChild.js?
I meant in the DOM, I assumed interfering with the DOM in content was a big nono
Oh, hum.  We have no window in the child process in which to draw context menus!

I'm not sure how we should handle this except to do what you suggested and draw the contextmenu in the parent.  Boris, do you have any other ideas, perchance?
Jan implemented HTML context menu support also for E10s, IIRC.
https://bugzilla.mozilla.org/show_bug.cgi?id=617528#c163

you don't need to build xul in the content process, you can build anything you want using nsIMenuBuilder and nsIHTMLMenu

let me know if you need additional details

btw, it seems someone else is working on HTML5 context menu for Fennec
http://starkravingfinkle.org/blog/2012/04/firefox-for-android-fx-mobile-work-week/
Part of me wants to implement this whole thing without interaction with the "embedder" (the page containing the iframe mozbrowser).  Otherwise, as you point out in comment 1, the embedder suddenly acquires a lot of control over the page.  Also, context menus should look the same across different apps, so there's not much to be won by letting the embedder style the context menu.

If we're implementing without interaction with the embedder, we might be able to reuse some of the existing machinery, since we could play with xul at that point.

On the other hand, if we were to take that approach, we'd have to stick a chrome window somewhere for us to draw xul into.  And that may be more trouble than it's worth, particularly because we have no other use for it at this point.

If it were me, I'd probably go the non-XUL route to start with and do what you suggested in comment 1.  If that really doesn't work out, we can try something else.
No longer blocks: browser-api
Yeh agreed, will implement it as I first thought, at some point we will probably end up having to implement 'stringByEvaluatingJavascriptFromString' as it is pretty heavily used in most 'embed a web browser' API's and I would rather not spend a bunch of time avoiding it to end up needing it at some later point.
> at some point we will probably end up having to implement 'stringByEvaluatingJavascriptFromString' 
> as it is pretty heavily used in most 'embed a web browser' API's and I would rather not spend a 
> bunch of time avoiding it to end up needing it at some later point.

A few weeks before you started we had a long discussion with cjones about adding that API.  His position is that we will need that API eventually, but we should wait as long as possible before adding it.  From a security standpoint, it is of course undesirable to add it at all.

Anyway I don't think we should spend a lot of time hacking around it either, but in the case of this context menu, I don't see why we couldn't just send back the selected index.
(In reply to Justin Lebar [:jlebar] from comment #11)
> Anyway I don't think we should spend a lot of time hacking around it either,
> but in the case of this context menu, I don't see why we couldn't just send
> back the selected index.

I'm not sure if I understand what you guys are talking about, but "send back the selected index" is exactly what XUL implementation does at the moment.
Actually, it returns the generated id of a menu item.
Ok cool, will do it that way, selected Id is needed since the context menu can be nested (although the first ui will likely just flatten it for now)
This isnt ready for merging, I needed feedback on 2/3 things before I go ahead, the API is currently 

> iframe.addEventListener('mozbrowsercontextmenu', function(e) { 
>   var items = e.detail;
>   .... UI
>   iframe.contextMenuSelected(id);
> });

Where the menu data is in the form 
> {
>  type: 'menu',
>  label: 'top',
>  items: [
>   {type: 'menuitem', label: 'first item', id: 'menuitem_0'},
>   { 
>    type: 'menu', 
>    label: 'nested menu', 
>    items: [
>     {type: 'menuitem', label: 'second item', id: 'menuitem_1', icon: 'ico.png'},
>    ]
>   }   
>  ]
> }

Things I am looking for feedback on are: 

1. The overall API, I would prefer to provide a promise in the callback (somewhat like e.details.selected(id)) but that isnt possible right?

2. How do execute the callback safely

> (new Function( "with(this) { " + callback + "}")).call(content.document.defaultView);

was a best guess, but I will just assume this is wrong / dangerous

Any other feedback appreciated
Assignee: nobody → dale
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Also in case you are testing this, it will be bitrotted against mozilla-central, but should apply against the b2g mozilla central (which is older)
> 1. The overall API, I would prefer to provide a promise in the callback (somewhat like 
> e.details.selected(id)) but that isnt possible right?

I'm doing that in window.alert; works fine.  It's a lot better from a re-entrancy standpoint, if nothing else.  (What I discovered you can't do is define a property on the event itself, so |e.selected()| didn't work when I tried.)

> 2. How do execute the callback safely

I'm not sure.  In theory, we're in strict mode, so |this| should be null, not the chrome global.  So maybe you just do |callback()| and everything just works.  Bobby?
Thanks for the heads up with providing promises via the e.detail, the new API looks like 

> iframe.addEventListener('mozbrowsercontextmenu', function(e) { 
>   var items = e.detail.items;
>   .... UI
>   e.detail.selectMenuItem(items[0].id);
> });

And is generally cleaner

Apart from some code cleanup and renaming, my only issue is the error handling: when giving the wrong id to the selectMenuItem fun or if the onclick handler has a syntax / runtime error, Given the cross process its impossible to trigger an exception that could be caught by 

> try { 
>   e.detail.selectMenuItem(....

but we could use a DomRequest?
Attachment #627794 - Attachment is obsolete: true
Mounir wrote in bug 759512 comment 2 that <select> should be handled by the virtual keyboard.

I wonder if that means that context menus should also be handled by the virtual keyboard, although I don't know what that means in this context.  Mounir or Vivien, can you elaborate?
(In reply to Justin Lebar [:jlebar] from comment #18)
> I wonder if that means that context menus should also be handled by the
> virtual keyboard, although I don't know what that means in this context. 
> Mounir or Vivien, can you elaborate?

I don't think context menus should be part of the VK. However, I don't know Vivien's opinion on that.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
> Apart from some code cleanup and renaming, my only issue is the error handling

I think I'm OK if this silently fails.  That will only happen if you pass in a bogus ID, right?
Comment on attachment 628124 [details] [diff] [review]
WIP: Add ability to parent of mozbrowser to handle contextmenu

f? Vivien re comment 19.  I don't really understand how the VK works, so it's hard to say something intelligent here, but it stm that context menus are basically the same thing as <select>, so I don't see why we should handle them differently.
Attachment #628124 - Flags: feedback?(21)
(In reply to Justin Lebar [:jlebar] from comment #21)
> Comment on attachment 628124 [details] [diff] [review]
> WIP: Add ability to parent of mozbrowser to handle contextmenu
> 
> f? Vivien re comment 19.  I don't really understand how the VK works, so
> it's hard to say something intelligent here, but it stm that context menus
> are basically the same thing as <select>, so I don't see why we should
> handle them differently.

Context menus are not the same thing as <select>. They are technically similar (showing a window) but that is the only common point AFAICT.
Can you explain how, from the point of view of the embedder (the page which owns <iframe mozbrowser>), a context menu is different from <select>?

In fact, couldn't we implement <select> using context menus?  That is, BrowserElementChild registers a listener which hears when the user clicks a <select>.  The BrowserElementChild constructs a contextmenu request and sends it up to the parent.

I don't mean to suggest we'd necessarily want to implement <select> this way, but only to illustrate the similarity I see.
> I think I'm OK if this silently fails.  That will only happen if you pass in a bogus ID, right?

Not only, if the onclick handler code has errors in it (syntax errors, undefined references etc), which I feel is a bit more important to not silently fail
> Not only, if the onclick handler code has errors in it (syntax errors, undefined references etc), 

Hm.  Should the embedder care if content's onclick handler throws an exception?  I kind of think not.  

For example, if I do elem.dispatchEvent(e) and some event handler throws an exception, dispatchEvent won't throw...
The embedder shouldnt care, but the user does, for debugging its a bit of a nightmare if your broken code silently fails

http://pastebin.me/0d74f22fa13335beb3cd68eeb70587d1

not opposed to noting it and fixing in a follow up though
> The embedder shouldnt care, but the user does, for debugging its a bit of a nightmare if your broken 
> code silently fails

Absolutely.  And I'm not sure where exactly the error message is going to go if the function we call from chrome throws an exception.

But that's all orthogonal to the question of the browser API, which it stm should ignore these exceptions from content.
oh sorry you are right, a dom request returned to selectMenuItem makes no sense, I dont think there is anywhere inside the api that does

throwing from BrowserElementChild isnt catchable by window.onerror (in the embedee) which isnt surprising, shouldnt be hard though will just figure out how (any pointers welcome)
(In reply to Justin Lebar [:jlebar] from comment #23)
> Can you explain how, from the point of view of the embedder (the page which
> owns <iframe mozbrowser>), a context menu is different from <select>?
> 
> In fact, couldn't we implement <select> using context menus?  That is,
> BrowserElementChild registers a listener which hears when the user clicks a
> <select>.  The BrowserElementChild constructs a contextmenu request and
> sends it up to the parent.
> 
> I don't mean to suggest we'd necessarily want to implement <select> this
> way, but only to illustrate the similarity I see.

For me the main difference in that <select> is part of user interaction/input with the page/form. Filing a form involves selecting an option in a <select> list and doing so can be done in different ways. We can make this part of the VKB but we could also make this part of the system UI like context menus. This is really a decision to make and it's actually not mine.
Will rebase and clean up some of the code, but otherwise happy with this approach
Attachment #628124 - Attachment is obsolete: true
Attachment #628124 - Flags: feedback?(21)

        Attachment #628920 -
        Attachment is obsolete: true

        Attachment #629014 -
        Flags: review?(justin.lebar+bug)

    
    

    
  
Comment on attachment 629014 [details] [diff] [review]
Add ability to parent of mozbrowser to handle contextmenu

Vivien, can you give us some feedback on this approach?  (Contextmenu is handled just like alert() -- fire an event on the iframe element, and the embedder is responsible for all UI.)

I don't entirely get the keyboard work that's being done, and I'm not (yet) convinced that we should be doing something different here.

        Attachment #629014 -
        Flags: feedback?(21)

    
    

    
  
Comment on attachment 629014 [details] [diff] [review]
Add ability to parent of mozbrowser to handle contextmenu

>diff --git a/dom/base/BrowserElementChild.js b/dom/base/BrowserElementChild.js
>index f38b74b..a6d3b6a 100644
>--- a/dom/base/BrowserElementChild.js
>+++ b/dom/base/BrowserElementChild.js
>@@ -72,32 +72,44 @@ BrowserElementChild.prototype = {
> 
>     if (!!appManifestURL) {
>       windowUtils.setIsApp(true);
>       windowUtils.setApp(appManifestURL);
>     } else {
>       windowUtils.setIsApp(false);
>     }
> 
>+    // A cache of the menuitem dom objects keyed by the id we generate
>+    // and pass to the embedder
>+    this.ctxHandlers = null;

The convention has been _ctxHandlers.

>+
>     addEventListener('DOMTitleChanged',
>                      this._titleChangedHandler.bind(this),
>                      /* useCapture = */ true,
>                      /* wantsUntrusted = */ false);
> 
>     addEventListener('DOMLinkAdded',
>                      this._iconChangedHandler.bind(this),
>                      /* useCapture = */ true,
>                      /* wantsUntrusted = */ false);
> 
>+    addEventListener('contextmenu',
>+                     this._contextmenuHandler.bind(this),
>+                     /* useCapture = */ true,
>+                     /* wantsUntrusted = */ false);
>+
>     addMessageListener("browser-element-api:get-screenshot",
>                        this._recvGetScreenshot.bind(this));
> 
>     addMessageListener("browser-element-api:set-visible",
>                         this._recvSetVisible.bind(this));
> 
>+    addMessageListener("browser-element-api:fire-ctx-callback",
>+                       this._recvFireCtxCallback.bind(this));
>+
>     let els = Cc["@mozilla.org/eventlistenerservice;1"]
>                 .getService(Ci.nsIEventListenerService);
> 
>     // We are using the system group for those events so if something in the
>     // content called .stopPropagation() this will still be called.
>     els.addSystemEventListener(global, 'keydown',
>                                this._keyEventHandler.bind(this),
>                                /* useCapture = */ true);
>@@ -137,16 +149,53 @@ BrowserElementChild.prototype = {
>         sendAsyncMsg('iconchange', e.target.href);
>       }
>       else {
>         debug("Not top level!");
>       }
>     }
>   },
> 
>+  _contextmenuHandler: function(e) {
>+    debug("Got contextmenu");
>+
>+    var target = e.target;
>+    var targetDocument = target.ownerDocument;
>+
>+    if (e.defaultPrevented) {
>+      return;
>+    }
>+
>+    if (targetDocument.defaultView != content) {
>+      debug("Not top level!");
>+      return;
>+    }

Surely this is wrong; we want to handle contextmenus in child frames too, right?

>+    var ctxMenuId = null;
>+    var menuData = {nodeName: e.target.nodeName, menu: null};
>+
>+    while (target.hasAttribute) {
>+      if (target.hasAttribute('contextmenu')) {
>+        ctxMenuId = target.getAttribute('contextmenu');
>+        break;
>+      }
>+      target = target.parentNode;
>+    }

You probably mean |while (target && target.hasAttribute)|.

Also, I don't think you're testing this behavior.

>+    if (ctxMenuId) {
>+      var menu = targetDocument.getElementById(target.getAttribute('contextmenu'));
>+      if (menu) {
>+        menuData.menu = this._buildMenuObj(menu, this._idGenerator());
>+        sendAsyncMsg('contextmenu', menuData);
>+        return;

Why not just fall through?  You're going to do the same thing anyway.

>+      }
>+    }
>+    sendAsyncMsg('contextmenu', menuData);
>+  },
>+
>   _recvGetScreenshot: function(data) {
>     debug("Received getScreenshot message: (" + data.json.id + ")");
>     var canvas = content.document
>       .createElementNS("http://www.w3.org/1999/xhtml", "canvas");
>     var ctx = canvas.getContext("2d");
>     canvas.mozOpaque = true;
>     canvas.height = content.innerHeight;
>     canvas.width = content.innerWidth;
>@@ -170,16 +219,65 @@ BrowserElementChild.prototype = {
>       sendAsyncMsg('keyevent', {
>         type: e.type,
>         code: e.keyCode,
>         charCode: e.charCode,
>       });
>     }
>   },
> 
>+  _recvFireCtxCallback: function(data) {
>+    debug("Received fireCtxCallback message: (" + data.json.menuitem + ")");
>+    // We silently ignore if the embedder uses an incorrect id in the callback
>+    if (this.ctxHandlers && this.ctxHandlers[data.json.menuitem]) {
>+      var ev = content.document.createEvent('HTMLEvents');
>+      ev.initEvent('click', true, false);
>+      this.ctxHandlers[data.json.menuitem].dispatchEvent(ev);
>+      this.ctxHandlers = null;
>+    }
>+  },
>+
>+  _idGenerator: function() {
>+    // We wipe the handlers on each contextmenu event
>+    this.ctxHandlers = {};
>+    var index = 0;
>+    return (function(menuitem) {
>+      var id = 'menuitem_' + index++;
>+      if (menuitem.hasAttribute('onclick')) {
>+        this.ctxHandlers[id] = menuitem;
>+      }
>+      return id;

What about if the menuitem has an onclick handler registered via addEventListener?

>+    }).bind(this);
>+  },

If the intent is for the new one to cancel the old one, we're not accomplishing that here, because this dictionary is keyed on |menuitem_N|, right?  So if a new contextmenu event comes in and then the embedder says "okay, I got a click on the original contextmenu, index 0", we'd simulate a click on index 0 of the new context menu, right?

In this case, we should either simulate a click on the correct menu, or simulate no click at all.  Perhaps the spec has some guidance.

Also, I'd rather if idGenerator were nested inside _buildMenuObj; that's logically what you're doing afaict.

>+  _buildMenuObj: function(menu, createId) {
>+    var menuObj = {type: 'menu', items: []};
>+    this._copyAttribute(menu, menuObj, 'label');
>+
>+    for (var i = 0, child; child = menu.children[i++];) {
>+      if (child.nodeName === 'MENU') {
>+        menuObj.items.push(this._buildMenuObj(child, createId));
>+        break;
>+      }
>+      if (child.nodeName === 'MENUITEM') {
>+        var menuitem = {id: createId(child), type: 'menuitem'};
>+        this._copyAttribute(child, menuitem, 'label');
>+        this._copyAttribute(child, menuitem, 'icon');
>+        menuObj.items.push(menuitem);
>+      }
>+    }
>+    return menuObj;
>+  },

>+  _copyAttribute: function(src, target, attribute) {
>+    if (src.getAttribute(attribute)) {
>+      target[attribute] = src.getAttribute(attribute);
>+    }
>+  },

Again, I'd prefer if this were nested into its caller. 

>   // The docShell keeps a weak reference to the progress listener, so we need
>   // to keep a strong ref to it ourselves.
>   _progressListener: {
>     QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
>                                            Ci.nsISupportsWeakReference,
>                                            Ci.nsISupports]),
>     _seenLoadStart: false,
> 
>diff --git a/dom/base/BrowserElementParent.js b/dom/base/BrowserElementParent.js
>index 027a425..3119990 100644
>--- a/dom/base/BrowserElementParent.js
>+++ b/dom/base/BrowserElementParent.js
>@@ -87,53 +87,79 @@ BrowserElementParent.prototype = {
>     if (!frameElement) {
>       debug("No frame element?");
>       return;
>     }
> 
>     let mm = frameLoader.messageManager;
> 
>     // Messages we receive are handled by functions with parameters
>-    // (frameElement, data), where |data| is the message manager's data object.
>+    // (frameElement, mm, data), where |data| is the message manager's data object.
> 
>     let self = this;
>     function addMessageListener(msg, handler) {
>-      mm.addMessageListener('browser-element-api:' + msg, handler.bind(self, frameElement));
>+      mm.addMessageListener('browser-element-api:' + msg,
>+                            handler.bind(self, frameElement, mm));

We should probably be consistent with the functions you define below and do |this, frameElement, mm| or |this, mm, frameElement| everywhere.

Alternatively, you could send just |this, frameElement| and write a function to grab the mm off the frame element.  (I'm kind of in favor of that; fewer arguments is beter.)

In any case, please check the param ordering on all the relevant functions; I don't think it's right at the moment.

>     addMessageListener("hello", this._recvHello);
>+    addMessageListener("contextmenu", this._fireCtxMenuEvent);
>     addMessageListener("locationchange", this._fireEventFromMsg);
>     addMessageListener("loadstart", this._fireEventFromMsg);
>     addMessageListener("loadend", this._fireEventFromMsg);
>     addMessageListener("titlechange", this._fireEventFromMsg);
>     addMessageListener("iconchange", this._fireEventFromMsg);
>     addMessageListener("get-mozapp-manifest-url", this._sendMozAppManifestURL);
>     addMessageListener("keyevent", this._fireKeyEvent);
>     mm.addMessageListener('browser-element-api:got-screenshot',
>                           this._recvGotScreenshot.bind(this));
> 
>     XPCNativeWrapper.unwrap(frameElement).getScreenshot =
>       this._getScreenshot.bind(this, mm, frameElement);
> 
>     XPCNativeWrapper.unwrap(frameElement).setVisible =
>       this._setVisible.bind(this, mm, frameElement);
> 
>+    XPCNativeWrapper.unwrap(frameElement).contextMenuSelected =
>+      this._contextMenuSelected.bind(this, mm, frameElement);

Perhaps contextMenuItemSelected?

>+  _fireCtxMenuEvent: function(frameElement, mm, data) {
>+    let name = data.name;
>+    let detail = data.json;
>+
>+    debug('fireCtxMenuEventFromMsg: ' + name + ' ' + detail);
>+    let evtName = name.substring('browser-element-api:'.length);
>+    let win = frameElement.ownerDocument.defaultView;
>+    let evt = new win.CustomEvent('mozbrowser' + evtName, {detail: detail});
>+
>+    // The embedder may have default actions on context menu events so
>+    // we still fire events if there isnt a custom context menu defined

Nit: Period at end of sentence, please.  Also, "isn't".

Also, are you testing this behavior?

>+    if (detail.menu) {
>+      XPCNativeWrapper.unwrap(evt.detail).selectMenuItem = function(id) {
>+        debug('here');

Remove debugging statement (or make it more useful).

>+        mm.sendAsyncMessage('browser-element-api:fire-ctx-callback', {menuitem: id});
>+      };
>+    }


>+  _contextMenuSelected: function(mm, frameElement, itemId) {
>+    mm.sendAsyncMessage('browser-element-api:select-ctxmenuitem', {id: itemId});
>+  },

Looks like you're not still using this anymore.

>diff --git a/dom/tests/mochitest/browser-frame/Makefile.in b/dom/tests/mochitest/browser-frame/Makefile.in
>index 6b4a71d..eda1888 100644
>--- a/dom/tests/mochitest/browser-frame/Makefile.in
>+++ b/dom/tests/mochitest/browser-frame/Makefile.in
>@@ -21,13 +21,14 @@ _TEST_FILES = \
> 		test_browserFrame3.html \
> 		test_browserFrame4.html \
> 		test_browserFrame5.html \
> 		test_browserFrame6.html \
> 		test_browserFrame7.html \
> 		test_browserFrame8.html \
> 		test_browserFrame9.html \
> 		test_browserFrame10.html \
>+		test_browserFrame_contextmenuEvents.html \
> 		test_browserFrame_keyEvents.html \
> 		$(NULL)
> 
> libs:: $(_TEST_FILES)
> 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)
>diff --git a/dom/tests/mochitest/browser-frame/test_browserFrame_contextmenuEvents.html b/dom/tests/mochitest/browser-frame/test_browserFrame_contextmenuEvents.html
>new file mode 100644
>index 0000000..5a1ee7d
>--- /dev/null
>+++ b/dom/tests/mochitest/browser-frame/test_browserFrame_contextmenuEvents.html
>@@ -0,0 +1,153 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=756371
>+-->
>+<head>
>+  <title>Test for Bug 756371</title>
>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <script type="application/javascript" src="browserFrameHelpers.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+</head>
>+<body>
>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=756371">Mozilla Bug 753595</a>
>+
>+<!--
>+   Test Aability to parent of mozbrowser to handle contextmenu events
>+   from child

Aability.

I didn't read the test, but I'll come back to it during a later review.

        Attachment #629014 -
        Flags: review?(justin.lebar+bug) → review-

    
    

    
  
> If the intent is for the new one to cancel the old one, we're not accomplishing 
> that here, because this dictionary is keyed on |menuitem_N|, right?  So if a new 
> contextmenu event comes in and then the embedder says "okay, I got a click on the 
> original contextmenu, index 0", we'd simulate a click on index 0 of the new context 
> menu, right?

> In this case, we should either simulate a click on the correct menu, or simulate 
> no click at all.  Perhaps the spec has some guidance.

This bit of code was just to cache the dom references, but you are right, it is possible to have 2 events fired and reply to the first in error. The spec doesnt have any guidance but given the standard behaviour of context menus I think it will be sensible to disable. Including a simple incrementing counter of contextmenu clicks in the menuitem id's should be sufficient?

* When I read through the spec I found that I implemented it wrong, a contextmenu event is supposed to trigger a 'show' event on the menu that is being shown, the show event is what triggers it visually, unless I hear a reason not to ill fix this up to use the proper events

> Also, are you testing this behavior?

Yup its the first test, a contextmenu on BODY

Cheers for the review, sorry I should have really caught a few of those myself

    
    

    
  
> You probably mean |while (target && target.hasAttribute)|.
> Also, I don't think you're testing this behavior.

Forgot to follow this up, I couldnt think of a reason the target would not be a dom element, so following up the parentNodes I will eventually get to the Document, which doesnt have hasAttribute, it was on purpose but if there is a better way?

And theres 2 tests for it, one fires an event on a child and walks up to find a menu in its parent, one fires an event on BODY and walks up until it hits Document

    
    

    
  
> Forgot to follow this up, I couldnt think of a reason the target would not be a dom element, so 
> following up the parentNodes I will eventually get to the Document

Ah, I see what you're doing.

What if the target is not in the DOM?  It's parentNode would be null?

    
    

    
  
Yup, and that is possible, will add your check in

    
    

    
  
New issue while implementing the actual contextmenu in the browser

When the user triggers a contextmenu from a link, the embedder / browser needs to know the url of the href of the link in order to create a new tab with that url

Similiarly if the user triggers context menu from an image, a common action is 'Save Image', which would require the source of the image.

Especially given the roundabout event cycle we are / should be implementing, I cant think of a particularly clean way to implement this inside the api, will try to think about it more

    
    

    
  
> When the user triggers a contextmenu from a link, the embedder / browser needs to know the url of 
> the href of the link in order to create a new tab with that url

Oh, I understand.  You're talking about "vanilla" contextmenus, not "HTML" contextmenus.  It's just that we're implementing the first using the second, in some sense.

Normally this wouldn't be a problem because the embedder would get the relevant info off the event target, but that doesn't work with OOP. 

I'm happy if you want to punt this to a follow-up.

    
    

    
  
Comment on attachment 629014 [details] [diff] [review]
Add ability to parent of mozbrowser to handle contextmenu

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

Sounds good. I still think there is a need for a standard mechanism to return a result from an event but I guess having more use cases will help to figure out what is needed exactly.

::: dom/base/BrowserElementChild.js
@@ +161,5 @@
> +    var targetDocument = target.ownerDocument;
> +
> +    if (e.defaultPrevented) {
> +      return;
> +    }

No need to declare target and targetDocument before the check for e.defaultPrevented.

@@ +166,5 @@
> +
> +    if (targetDocument.defaultView != content) {
> +      debug("Not top level!");
> +      return;
> +    }

Why does the code should not work with embed iframe of an application/web site?

@@ +169,5 @@
> +      return;
> +    }
> +
> +    var ctxMenuId = null;
> +    var menuData = {nodeName: e.target.nodeName, menu: null};

e.target -> target

Also declare menuData closer to where it is used (after the 'while (target.hasAttribute)' block)

@@ +171,5 @@
> +
> +    var ctxMenuId = null;
> +    var menuData = {nodeName: e.target.nodeName, menu: null};
> +
> +    while (target.hasAttribute) {

Why do you check target.hasAttribute here? You're looking for the html root?

@@ +182,5 @@
> +
> +    if (ctxMenuId) {
> +      var menu = targetDocument.getElementById(target.getAttribute('contextmenu'));
> +      if (menu) {
> +        menuData.menu = this._buildMenuObj(menu, this._idGenerator());

Any reason for passing the id as a parameter here?

@@ +226,5 @@
>  
> +  _recvFireCtxCallback: function(data) {
> +    debug("Received fireCtxCallback message: (" + data.json.menuitem + ")");
> +    // We silently ignore if the embedder uses an incorrect id in the callback
> +    if (this.ctxHandlers && this.ctxHandlers[data.json.menuitem]) {

This will fire a warning if data.json.menuitem is not in this.ctxHandlers if you are in strict mode. You likely want to check for 'data.json.menuitem in this.ctxHandlers'.

@@ +228,5 @@
> +    debug("Received fireCtxCallback message: (" + data.json.menuitem + ")");
> +    // We silently ignore if the embedder uses an incorrect id in the callback
> +    if (this.ctxHandlers && this.ctxHandlers[data.json.menuitem]) {
> +      var ev = content.document.createEvent('HTMLEvents');
> +      ev.initEvent('click', true, false);

Can't we call element.click() instead of generating an event?

@@ +236,5 @@
> +  },
> +
> +  _idGenerator: function() {
> +    // We wipe the handlers on each contextmenu event
> +    this.ctxHandlers = {};

Sounds like idGenerator lies a bit. It resets this.ctxHandlers while from the name I would have guess that it returns an id without affecting anything else.

@@ +259,5 @@
> +      }
> +      if (child.nodeName === 'MENUITEM') {
> +        var menuitem = {id: createId(child), type: 'menuitem'};
> +        this._copyAttribute(child, menuitem, 'label');
> +        this._copyAttribute(child, menuitem, 'icon');

copyAttribute -> maybeCopyAttribute

::: dom/base/BrowserElementParent.js
@@ +135,5 @@
> +
> +    debug('fireCtxMenuEventFromMsg: ' + name + ' ' + detail);
> +    let evtName = name.substring('browser-element-api:'.length);
> +    let win = frameElement.ownerDocument.defaultView;
> +    let evt = new win.CustomEvent('mozbrowser' + evtName, {detail: detail});

nit: declare evtName/win/evt next to where it is used (next to frameElement.dispatchEvent)

@@ +140,5 @@
> +
> +    // The embedder may have default actions on context menu events so
> +    // we still fire events if there isnt a custom context menu defined
> +    if (detail.menu) {
> +      XPCNativeWrapper.unwrap(evt.detail).selectMenuItem = function(id) {

nit: I wonder if it should be evt.detail.menu.select(id)? (no strong opinion on that)

@@ +141,5 @@
> +    // The embedder may have default actions on context menu events so
> +    // we still fire events if there isnt a custom context menu defined
> +    if (detail.menu) {
> +      XPCNativeWrapper.unwrap(evt.detail).selectMenuItem = function(id) {
> +        debug('here');

nit: looks like a great debugging statement ;)

::: dom/tests/mochitest/browser-frame/test_browserFrame_contextmenuEvents.html
@@ +12,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=756371">Mozilla Bug 753595</a>
> +
> +<!--
> +   Test Aability to parent of mozbrowser to handle contextmenu events

nit: Aability -> abitlity

@@ +18,5 @@
> +-->
> +
> +<script type="application/javascript;version=1.7">
> +
> +"use strict";

nit: there is a mix of ' and "

@@ +94,5 @@
> +      ok(e.detail.menu.items.length === 2, 'Inner clicks trigger correct menu');
> +    } else if (ctxMenuEvents === 4) {
> +      var innerMenu = e.detail.menu.items.filter(function(x) {
> +        return x.type === 'menu';
> +      });

var detail = e.detail;

        Attachment #629014 -
        Flags: feedback?(21) → feedback+

    
    

    
  


  
I dont think we can punt on the target data otherwise we can only implement html5 contextmenus which all of 2 websites in the world implement.

I have just embedded the src + href data in here where applicable, not sure if this is ok for a workaround

        Attachment #629014 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #630188 -
        Attachment is obsolete: true

    
  

        Attachment #630553 -
        Flags: feedback?(21)

    
    

    
  
Comment on attachment 630553 [details] [diff] [review]
Add ability to parent of mozbrowser to handle contextmenu

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

::: dom/base/BrowserElementChild.js
@@ +92,5 @@
>                       /* wantsUntrusted = */ false);
>  
> +    addEventListener('contextmenu',
> +                     this._contextmenuHandler.bind(this),
> +                     /* useCapture = */ true,

Are you sure you want to catch the event in the capture phase? I have the feeling that it should be on the bubbling phase?

@@ +183,5 @@
> +      menuData.href = target.getAttribute('href');
> +    }
> +    if (menuData.nodeName === 'IMG') {
> +      menuData.src = target.getAttribute('src');
> +    }

I suggest that you look here (http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1312) to see how it has been achieve for Fennec.

@@ +184,5 @@
> +    }
> +    if (menuData.nodeName === 'IMG') {
> +      menuData.src = target.getAttribute('src');
> +    }
> +

Declare ctxMenuId here.

@@ +194,5 @@
> +      target = target.parentNode;
> +    }
> +
> +    if (ctxMenuId) {
> +      var menu = targetDocument.getElementById(target.getAttribute('contextmenu'));

target.getAttribute('contextmenu') -> ctxMenuId

@@ +258,5 @@
> +
> +    for (var i = 0, child; child = menu.children[i++];) {
> +      if (child.nodeName === 'MENU') {
> +        menuObj.items.push(this._buildMenuObj(child, createId));
> +        break;

Do you mean continue? Otherwise you will ignore al <menuitem>s after a <menu>

::: dom/base/BrowserElementParent.js
@@ +132,5 @@
> +
> +    debug('fireCtxMenuEventFromMsg: ' + name + ' ' + detail);
> +    let evtName = name.substring('browser-element-api:'.length);
> +    let win = frameElement.ownerDocument.defaultView;
> +    let evt = new win.CustomEvent('mozbrowser' + evtName, {detail: detail});

Is there a reason why you can't use _fireEventFromMsg here?

        Attachment #630553 -
        Flags: feedback?(21)

    
    

    
  


  

        Attachment #630553 -
        Attachment is obsolete: true

        Attachment #634431 -
        Flags: review?(justin.lebar+bug)

    
    

    
  
>From: Dale Harvey <dale@arandomurl.com>
>BUG 756371 - Add ability to parent of mozbrowser to handle contextmenu. r=jlebar
>
>
>diff --git a/dom/browser-element/BrowserElementChild.js b/dom/browser-element/BrowserElementChild.js
>index 3e8d83d..e26571d 100644
>--- a/dom/browser-element/BrowserElementChild.js
>+++ b/dom/browser-element/BrowserElementChild.js
>@@ -83,34 +83,44 @@ BrowserElementChild.prototype = {
> 
>     if (!!appManifestURL) {
>       windowUtils.setIsApp(true);
>       windowUtils.setApp(appManifestURL);
>     } else {
>       windowUtils.setIsApp(false);
>     }
> 
>+    // A cache of the menuitem dom objects keyed by the id we generate
>+    // and pass to the embedder
>+    this._ctxHandlers = {};
>+
>     addEventListener('DOMTitleChanged',
>                      this._titleChangedHandler.bind(this),
>                      /* useCapture = */ true,
>                      /* wantsUntrusted = */ false);
> 
>     addEventListener('DOMLinkAdded',
>                      this._iconChangedHandler.bind(this),
>                      /* useCapture = */ true,
>                      /* wantsUntrusted = */ false);
> 
>+    addEventListener('contextmenu',
>+                     this._contextmenuHandler.bind(this),
>+                     /* useCapture = */ false,
>+                     /* wantsUntrusted = */ false);

Since you want to check e.defaultPrevented but also call e.preventDefault(), I
think you need to run this handler in the system event group.  The system event
group's bubbling phase happens after all content has had a chance to handle the
event.

>@@ -284,16 +294,66 @@ BrowserElementChild.prototype = {
>         sendAsyncMsg('iconchange', e.target.href);
>       }
>       else {
>         debug("Not top level!");
>       }
>     }
>   },
> 
>+  _contextmenuHandler: function(e) {
>+    debug("Got contextmenu");
>+
>+    if (e.defaultPrevented) {
>+      return;
>+    }
>+
>+    e.preventDefault();
>+    this._ctxHandlers = {};

XXX how do we handle multiple contextmenu requests?

>+
>+    var target = e.target;
>+    var targetDocument = target.ownerDocument;
>+    var index = 0;
>+
>+    var idGenerator = (function(menuitem) {
>+      var id = 'menuitem_' + index++;
>+      this._ctxHandlers[id] = menuitem;
>+      return id;
>+    }).bind(this);

Please move this (and index) closer to where it's used.  But I don't get why
you need this at all; can't buildMenuObj handle this logic?

>+    var menuData = {nodeName: target.nodeName, menu: null};
>+
>+    if ((target instanceof Ci.nsIDOMHTMLAnchorElement && target.href) ||
>+        (target instanceof Ci.nsIDOMHTMLAreaElement && target.href)) {
>+      menuData.href = target.href;
>+    } else if (target instanceof Ci.nsIImageLoadingContent && target.currentURI) {
>+      menuData.title = target.currentURI.spec;
>+    } else if (target instanceof Ci.nsIDOMHTMLMediaElement) {
>+      menuData.src = (target.currentSrc || target.src);
>+    }

The test doesn't appear to cover this.

Also, the android code lifts the event target's title attribute, presumably so that can be used as a title on the contextmenu.  Maybe we should do the same?

One other thing: android uses e.originalTarget, instead of e.target.  I have zero idea what the difference is, but we should check with smaug.

>+  _recvFireCtxCallback: function(data) {
>+    debug("Received fireCtxCallback message: (" + data.json.menuitem + ")");
>+    // We silently ignore if the embedder uses an incorrect id in the callback
>+    if (data.json.menuitem in this._ctxHandlers) {
>+      this._ctxHandlers[data.json.menuitem].click();
>+      this._ctxHandlers = {};
>+    }

I think you want some kind of check here that the callback we got is actually for the most rececent contextmenu call; this doesn't protect against that.

>+  },
>+
>+  _buildMenuObj: function(menu, createId) {
>+
>+    var maybeCopyAttribute = function(src, target, attribute) {
>+      if (src.getAttribute(attribute)) {
>+        target[attribute] = src.getAttribute(attribute);
>+      }
>+    }

If you think it's clearer, you could say instead |function maybeCopyAttribute(...) { }|.

>+
>+    var menuObj = {type: 'menu', items: []};
>+    maybeCopyAttribute(menu, menuObj, 'label');
>+
>+    for (var i = 0, child; child = menu.children[i++];) {
>+      if (child.nodeName === 'MENU') {
>+        menuObj.items.push(this._buildMenuObj(child, createId));
>+        continue;
>+      }
>+      if (child.nodeName === 'MENUITEM') {

I think |else if| would be clearer, but whatever you prefer.

>+        var menuitem = {id: createId(child), type: 'menuitem'};
>+        maybeCopyAttribute(child, menuitem, 'label');
>+        maybeCopyAttribute(child, menuitem, 'icon');
>+        menuObj.items.push(menuitem);
>+      }
>+    }
>+    return menuObj;
>+  },

I'd prefer if we didn't have to pass in this createId function; it's complicated.  Surely we can work around this, perhaps by creating nested menu IDs, e.g. "2_4_1".  If you do want to use a generator-type thing, I'd prefer if you created it in the top-level buildMenuObj call.

>diff --git a/dom/browser-element/mochitest/browserElement_ContextmenuEvents.js b/dom/browser-element/mochitest/browserElement_ContextmenuEvents.js
>new file mode 100644
>index 0000000..698388a
>--- /dev/null
>+++ b/dom/browser-element/mochitest/browserElement_ContextmenuEvents.js

God these tests are complicated.

Did you mean to remove this?

>+    ctxMenuEvents++;
>+    if (ctxMenuEvents === 1) {
>+      info('wtf: ' + detail.nodeName);

And this?

>\ No newline at end of file

Nit: newline at end of file is probably good.

>+++ b/dom/browser-element/mochitest/test_browserElement_inproc_ContextmenuEvents.html
>@@ -0,0 +1,18 @@
>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=756371">Mozilla Bug 753595</a>

Wrong bug number inside the link.

I'm OK if you just want to leave out this link (and take the bug number out of the <title>).  I wish we didn't need these shell files to begin with. :-/

>+
>+++ b/dom/browser-element/mochitest/test_browserElement_oop_ContextmenuEvents.html
>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=756371">Mozilla Bug 753595</a>

Also here.

    
    

    
  
> XXX how do we handle multiple contextmenu requests?

That was a note to myself; I addressed it elsewhere in the review comments.

    
    

    
  
Comment on attachment 634431 [details] [diff] [review]
Add ability to parent of mozbrowser to handle contextmenu

I think this is basically right, but I'd like to take another look.

        Attachment #634431 -
        Flags: review?(justin.lebar+bug) → review-

    
    

    
  


  

        Attachment #634431 -
        Attachment is obsolete: true

        Attachment #635588 -
        Flags: review?(justin.lebar+bug)

    
    

    
  
>+  _contextmenuHandler: function(e) {
>+    debug("Got contextmenu");
>+
>+    if (e.defaultPrevented) {
>+      return;
>+    }
>+
>+    e.preventDefault();
>+
>+    this._ctxCounter++;
>+    this._ctxHandlers = {};
>+
>+    var target = e.target;
>+    var targetDocument = target.ownerDocument;
>+    var menuData = {systemTargets: [], contextmenu: null};
>+    var ctxMenuId = null;
>+
>+    while (target && target.hasAttribute) {
>+      var ctxData = this._getSystemCtxMenuData(target);
>+      if (ctxData) {
>+        menuData.systemTargets.push({

I'm not wild about this name, but I don't have a better idea, so I guess we'll
just go with it.  :)

>+          nodeName: target.nodeName,
>+          data: ctxData
>+        });
>+      }
>+
>+      if (target.hasAttribute('contextmenu')) {
>+        ctxMenuId = target.getAttribute('contextmenu');
>+      }

Hm, this means that we'll take the ID off the outermost thing with a
contextmenu attribute, right?  I'd guess that we should use the ID from the
innermost thing, but does the spec offer any guidance?

>+      target = target.parentNode;
>+    }

Nit: Could you iterate over a new variable, |elem|, here?  That would match the
variable name in getSystemCtxMenuData, and anyway, the target is e.target --
doesn't change.  Then you can delete |var target| altogether.

>+  _buildMenuObj: function(menu, idPrefix) {
>+    function maybeCopyAttribute(src, target, attribute) {
>+      if (src.getAttribute(attribute)) {
>+        target[attribute] = src.getAttribute(attribute);
>+      }
>+    }
>+
>+    var menuObj = {type: 'menu', items: []};
>+    maybeCopyAttribute(menu, menuObj, 'label');
>+
>+    for (var i = 0, child; child = menu.children[i++];) {
>+      if (child.nodeName === 'MENU') {
>+        menuObj.items.push(this._buildMenuObj(child, idPrefix + i + '_'));
>+      } else if (child.nodeName === 'MENUITEM') {
>+        var id = this._ctxCounter + '_' + idPrefix + i;

Would the test fail if you took out this._ctxCounter here?  It doesn't look
like it to me, but I could be misreading it.  In any case, the test should fail
if we make that change; that's important.

>+  _fireCtxMenuEvent: function(data) {
>+    let evtName = data.name.substring('browser-element-api:'.length);
>+    let detail = data.json;
>+
>+    debug('fireCtxMenuEventFromMsg: ' + evtName + ' ' + detail);
>+    let evt = this._createEvent(evtName, detail);
>+
>+    // The embedder may have default actions on context menu events so
>+    // we still fire events if there isn't a custom context menu defined.

This comment has confused me each of the three times I read it, so I now think
we should change it.  I think it's the passive voice that's tripping me up.
How about

  The embedder may have default actions on context menu events, so
  we fire a context menu event even if the child didn't define a custom context
  menu.

and move the comment right above the dispatchEvent call?

>+    if (detail.contextmenu) {
>+      var self = this;
>+      XPCNativeWrapper.unwrap(evt.detail).contextMenuItemSelected = function(id) {
>+        self._sendAsyncMsg('fire-ctx-callback', {menuitem: id});
>+      };
>+    }
>+
>+    this._frameElement.dispatchEvent(evt);
>+  },

        Attachment #635588 -
        Flags: review?(justin.lebar+bug) → review+

    
    

    
  
> I'm not wild about this name, but I don't have a better idea, so I guess we'll
> just go with it.  :)

3 problems of computer science

> Hm, this means that we'll take the ID off the outermost thing with a
> contextmenu attribute, right?  I'd guess that we should use the ID from the
> innermost thing, but does the spec offer any guidance?

I didnt count for that, the spec does specify the innermost 

> Would the test fail if you took out this._ctxCounter here?  It doesn't look
> like it to me, but I could be misreading it.  In any case, the test should fail
> if we make that change; that's important.

Yup this behaviour isnt test properly

Will fixup

    
    

    
  


  
Apologies for the churn on this patch :)

        Attachment #635588 -
        Attachment is obsolete: true

        Attachment #635813 -
        Flags: review?(justin.lebar+bug)

    
    

    
  
(In reply to dale@arandomurl.com from comment #51)
> Apologies for the churn on this patch :)

No worries.

Are there changes here you need me to review?  If it's just what we discussed above, I don't need to look at it again; r+ means I trust you to make the right changes.  But of course if there are things you think I should look at, let me know.

    
    

    
  
ah cool, thanks

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=96116c0fff77

    
    

    
  
Comment on attachment 635813 [details] [diff] [review]
Add ability to parent of mozbrowser to handle contextmenu

Clearing the review flag here because we established there's nothing I need to do at the moment.

        Attachment #635813 -
        Flags: review?(justin.lebar+bug)

    
    

    
  
Sorry, this has bitrotted.  :(

    
    

    
  


  

        Attachment #635813 -
        Attachment is obsolete: true

        Attachment #636296 -
        Flags: checkin?(justin.lebar+bug)

    
    

    
  
Nit: Next time, can you use "Bug" instead of "BUG"?

$ git log --pretty=oneline | grep 'Bug' | wc -l
  109334
$ git log --pretty=oneline | grep 'BUG' | wc -l
     740

I fixed it in this patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d87705c71464
Target Milestone: --- → mozilla16

        Attachment #636296 -
        Flags: checkin?(justin.lebar+bug) → checkin+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/d87705c71464
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: