From Bugzilla Helper: User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0) BuildID: .9.4 branch When you right-click with the plugin window, we handle the event and return true. This should signal the browser that we have handled the event and that it should not. Instead, the browser handles it anyway. Reproducible: Always Steps to Reproduce: 1.Install Viewpoint plugin (http://cole.viewpoint.com/~aberger/readme.txt) 2.View above content. 3.Right-click within plugin window. Actual Results: Context menu pops up. It shouldn't. I have found half the cause to this problem, but not the whole thing. 1) Macro problem: In ns4xPluginInstance.cpp, HandleEvent: NS_TRY_SAFE_CALL_RETURN(res, CallNPP_HandleEventProc(fCallbacks->event, &fNPP, (void*)&npEvent), fLibrary); This is sticking the results of the function call into the variable named "res". The definition of the macro is: #define NS_TRY_SAFE_CALL_RETURN(ret, fun, library) \ PR_BEGIN_MACRO \ nsresult res; \ PRBool dontdoit = PR_FALSE; \ nsCOMPtr<nsIPref> prefs(do_GetService(kPrefServiceCID, &res));\ if(NS_SUCCEEDED(res) && (prefs != nsnull)) \ { \ res = prefs->GetBoolPref("plugin.dont_try_safe_calls", &dontdoit);\ if(NS_FAILED(res)) \ dontdoit = FALSE; \ } \ if(dontdoit) \ ret = fun; \ else \ { \ try \ { \ ret = fun; \ } \ catch(...) \ { \ nsCOMPtr<nsIPluginHost> host(do_GetService(kCPluginManagerCID, &res));\ if(NS_SUCCEEDED(res) && (host != nsnull)) \ host->HandleBadPlugin(library); \ ret = (NPError)NS_ERROR_FAILURE; \ } \ } \ PR_END_MACRO Note how all instances of "ret" in the macro will be replaced with "res". Unfortunately, there is also a variable in the macro's {} scope named res. The macro will not funcion properly and you will lose the return code. This means that this function NEVER for ANY EVENT gets back the return code. Changing the variable name in the function is an easy fix (I tried and it now gets the return code), but is probably not the best fix. I'd change the name of the variable in the macro to something less likely to clash. 2) Even when I fix problem 1, the bug still shows- it definitely sends back that the event has been handled and returns that up the stack, but I still see the context menu. I think this could be related to the mouse flickering problem that I spoke to Peter about, but I'm not sure. I've made some fixes to our mouse code and I'll look at that now.

dup of bug 75456 ? or that should be duped with this ?

You are right- sorry, didn't see that one. This is pretty important for us, as the right mouse button is used extensively.

*** Bug 75456 has been marked as a duplicate of this bug. ***

This may not be a compete dup of bug 75456 because that is with all Mac plugins and this is only with windowless. However, it may also depend on bug 103696.

Looking at this in the debugger last night, this totally depends on bug 103696. We do get the CONTEXTMENU event in the object frame but consuming them doesn't do what it should. That bug needs to be fixed before this one. To reproduce: For a testcase, use the windowless sdk sample: http://lxr.mozilla.org/mozilla/source/modules/plugin/tools/sdk/samples/winless/ First run nmake in "sdk". Load up test.html and set a breakpoint in nsObjectFrame::HandleEvent when a CONTEXTMENU comes through. Consume it, but notice the browser's context menu still shows up.

Do you disagree that the macro still needs to be fixed?

I'm not sure about the macro, but it does look like it needs to be fixed. I talked with Kevin and it looks like we do indeed need to implement the nsIDOMContextMenuListener interface in order to consume the event while in the DOM phase. Frames get lower priority than then DOM when processing events which is probably why we can't consume it. The only problem with that is that I lost mouse move and click events when all I did was inherit and implement the QI. I'll take another look. However, looking through the docshell, it looks like we special case some html tags here: http://lxr.mozilla.org/mozilla/source/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp#1469 I'll see if I can hack something together.

Okay, I got it to work by giving nsIDOMContextMenuListener it's own class. Then it doesn't seem to interfear with other mouse evnets but it consumes the event. I'm formulating a patch right now. It should also fix the Mac and dup bug 103696.

*** Bug 103696 has been marked as a duplicate of this bug. ***

Here's a patch that implements a seperate class just for consuming DOM context menu events. Please test and review.

Comment on attachment 64601 [details] [diff] [review] implementing nsPluginDOMContextMenuListener r=av on the menu listener. Do we also want to fix name conflict in the macro Ari mentioned in the original report?

I will try out the patch today. Once again, without fixing the macro, I believe that even when we return 1 from NPP_HandleEvent, you get the return value as 0. Note that we ALWAYS return 1 from HandleEvent. Put a breakpoint in your code after that call. Do you get 1 or 0 as the return value?

This fix looks good.

This should patch should also fix the return code but I'm not sure this will even give the plugin the ability to consume events. Many of the events (like context menu), we just consume by default. Andrei/Patrick, can I get reviews?

Comment on attachment 64817 [details] [diff] [review] updated previous patch to include fix for NPP_HandelEvent This is OK, r=av. In the future we should probably use some less usual name for the |res| variable in the macro itself to ensure it will not conflict with the calling code. How about |rezultat|?

we'd like to get this into 0.9.4 by midnight tomorrow; if it's ready.

Comment on attachment 64817 [details] [diff] [review] updated previous patch to include fix for NPP_HandelEvent Any reason that mCXMenuListener couldn't be an nsCOMPtr? sr=beard

Using mCXMenuListener as a nsCOMPtr, the compiler didn't like my Init/Destroy methods and I wasn't sure what should be done with |new|. Since in doubt, I used the old-fasion way.

can this hit 0.9.4 by midnight?

patch in trunk, 0.9.4 landing shortly

fixed0.9.4

verified fixed on 0.9.4 (0115 br build). Also fixed on mac/win trunk 0116. Browser context menu no longer appears inside plugin area.

Reopening because this is NOT fixed in MFCEmbed. :( I think this time it IS: http://lxr.mozilla.org/mozilla/source/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp#1469 I'll take a closer look.

Here's a patch, effecting embedding only, that will always consume context menu events for applet, embed, and object tags. Please review. One problem is that embedders won't be able to override the context menu for the object tag in the case that it is used for images or documents. That's probably not that big a deal considering that in order to do this correctly, we'd have to fetch the coresponding layout frame for this content to query its type PLUS embedders' code would need to look for a "data" attribute instead of just "src".

Comment on attachment 65944 [details] [diff] [review] patch for embedding r=av

Please see: http://bugscape.mcom.com/show_bug.cgi?id=11811

Comment on attachment 65944 [details] [diff] [review] patch for embedding sr=alecf

Do we really want to always consume the events over plugins, or only when the plugin has handled the event itself?

Plugins don't know anything about context menu events so their isn't any return to check. They usually use left/right down/up events to fire their menus but the browser doesn't know that. Jud, is the 0.9.4 branch open? Can you '+'?

qa->bmartin since this is MFCembed only.

please checkin to 0.9.4 and add "fixed0.9.4" to the keyword field once it's in there; thanks!

checked in on the trunk and fixed0.9.4