1381344 - No context menu for OOP popups opened as standalone panel

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Martin Giger [:freaktechnik]]

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).

Comment 2

[Martin Giger [:freaktechnik]]

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

Comment 3

[Andy McKay]

Shane to test this one.

Comment 4

[Shane Caraveo (:mixedpuppy)]

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

Comment 5

[Shane Caraveo (:mixedpuppy)]

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?

Comment 8

[Kris Maglione [:kmag]]

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 9

[Kris Maglione [:kmag]]

Attachment: Bug 1381344: Use original target to determine if context node is remote.

Review commit: https://reviewboard.mozilla.org/r/181094/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181094/

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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 11

[Jan Varga [:janv]]

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 12

[Kris Maglione [:kmag]]

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.

Comment 13

[Jan Varga [:janv]]

(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 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8909616 [details]
Bug 1381344: Use original target to determine if context node is remote.

https://reviewboard.mozilla.org/r/181094/#review186706

Comment 15

[Kris Maglione [:kmag]]

(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.

Comment 16

[Kris Maglione [:kmag]]

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

Comment 17

[Kris Maglione [:kmag]]

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 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/85af63c044f7

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/1549babb34d4

Comment 21

[Vasilica Tamas, QA [:vtamas]]

Attachment: Animation.gif

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.