Context menus on popups seem to be positioned wrongly. For popups opened to browser actions pinned to the overflow menu they open further left but visible on the screen, for browser actions in the toolbar I cannot see the context menu on my screen. I've only tested this on browser actions on Windows 10 with today's Nightly (2017-07-16).

This is not resolved for popups opened from buttons in the main toolbar but is fixed for popups opened as subview in the overflow.

Shane to test this one.

Verified comment 2 on artifact build 56.0a1 (2017-07-24) (64-bit)

Failure to see a context menu only happens with browserAction in the toolbar if remote=true. The context menu works fine if you move the browserAction to the overflow menu. I'm not seeing any position issues (I believe those were fixed in another bug by Kris). pageAction works fine. content.js sets a system event listener that never gets triggered in the failure case. I'm thinking that something about the view panel being built on the fly may have something to do with it, but am not seeing how. Kris, do you have any thoughts on this?

The problem is that the code that handles deciding whether a remote context menu event should be ignored looks at the event `target`, rather than its `originalTarget`. In the case of a popup in a view node, that's a <photonpanelmultiview> node rather than the <browser>, so it treats it as non-remote. This is easy enough to fix, but I don't have time to write tests this week...

Comment on attachment 8909616 [details] Bug 1381344: Use original target to determine if context node is remote. https://reviewboard.mozilla.org/r/181094/#review186604 I suspect I'm not familiar enough with dom/xul to review this. Redirecting to Jan.

Comment on attachment 8909616 [details] Bug 1381344: Use original target to determine if context node is remote. https://reviewboard.mozilla.org/r/181094/#review186662 I think bz or smaug should take a look at this too. ::: dom/xul/nsXULPopupListener.cpp:118 (Diff revision 1) > //non-ui event passed in. bad things. > return NS_OK; > } > > // Get the node that was clicked on. > EventTarget* target = mouseEvent->AsEvent()->InternalDOMEvent()->GetTarget(); Hm, can we change this to GetOriginTarget() instead ?

Comment on attachment 8909616 [details] Bug 1381344: Use original target to determine if context node is remote. https://reviewboard.mozilla.org/r/181094/#review186662 > Hm, can we change this to GetOriginTarget() instead ? That's what I initially did, but I'm worried that it would have unexpected effects in the other places it's used. There are a couple of places that check if the target is a "menu" or "menuitem" node, and if we use the original target instead, we're likely to see an <image> or <label> element anonymous child, rather than the <menu>/<menuitem> we're checking for.

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #12) > Comment on attachment 8909616 [details] > Bug 1381344: Use original target to determine if context node is remote. > > https://reviewboard.mozilla.org/r/181094/#review186662 > > > Hm, can we change this to GetOriginTarget() instead ? > > That's what I initially did, but I'm worried that it would have unexpected > effects in the other places it's used. There are a couple of places that > check if the target is a "menu" or "menuitem" node, and if we use the > original target instead, we're likely to see an <image> or <label> element > anonymous child, rather than the <menu>/<menuitem> we're checking for. So does it mean that this C++ code depends on how stuff in XUL/XBL is defined ? Well, if there's no better way ...

Comment on attachment 8909616 [details] Bug 1381344: Use original target to determine if context node is remote. https://reviewboard.mozilla.org/r/181094/#review186706

(In reply to Jan Varga [:janv] from comment #13) > So does it mean that this C++ code depends on how stuff in XUL/XBL is > defined ? > Well, if there's no better way ... Yes, at least parts of it: http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/dom/xul/nsXULPopupListener.cpp#184-193 And if not for the fact that LaunchPopup ignores its aTargetContent arg, I'd be worried that it might have similar issues.

https://hg.mozilla.org/integration/mozilla-inbound/rev/85af63c044f75f0a7b9909c70205829dbb7abe00 Bug 1381344: Use original target to determine if context node is remote. r=smaug

Comment on attachment 8909616 [details] Bug 1381344: Use original target to determine if context node is remote. Approval Request Comment [Feature/Bug causing the regression]: Bug 1357486 [User impact if declined]: This prevents the context menu from opening for certain extension popups. [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: No [Needs manual test from QE? If yes, steps to reproduce]: Detailed in comment 5 [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No [Why is the change risky/not risky?]: It's a fairly trivial change that simply makes sure we're inspecting the correct node to determine if an event belongs to a remote <browser> element. [String changes made/needed]: None

Comment on attachment 8909616 [details] Bug 1381344: Use original target to determine if context node is remote. Fix for some webextension context menus. OK to uplift for the 56 RC2.

I was able to reproduce the initial issue on Firefox 56.0a1 (2017-07-16) under Windows 10 64-bit. Verified as fixed on Firefox DevEdition 57.0b2 (20170921191414) and Firefox 56.0 RC build 3 (20170921234614) under Windows 10 64-bit and Mac OS X 10.12.3. The context menu is successfully opened for webextensions pop-ups opened from a toolbar icon. See attached screencast.