Closed Bug 1381344 Opened 2 years ago Closed 2 years ago

No context menu for OOP popups opened as standalone panel

Categories

(WebExtensions :: Untriaged, defect, P1)

defect

Tracking

(firefox56 verified, firefox57 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox56 --- verified
firefox57 --- verified

People

(Reporter: freaktechnik, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: triaged)

Attachments

(2 files)

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).
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1379508
This is not resolved for popups opened from buttons in the main toolbar but is fixed for popups opened as subview in the overflow.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: Context menu position is wrong for OOP popups → No context menu for OOP popups opened as standalone panel
Shane to test this one.
Flags: needinfo?(mixedpuppy)
Verified comment 2 on artifact build 56.0a1 (2017-07-24) (64-bit)
Status: REOPENED → NEW
Flags: needinfo?(mixedpuppy)
Priority: -- → P2
Whiteboard: triaged
Assignee: nobody → 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?
Flags: needinfo?(kmaglione+bmo)
Duplicate of this bug: 1390456
Priority: P2 → P1
Duplicate of this bug: 1395981
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...
Assignee: mixedpuppy → kmaglione+bmo
Flags: needinfo?(kmaglione+bmo)
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.
Attachment #8909616 - Flags: review?(dkeeler)
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 ?
Attachment #8909616 - Flags: review?(jvarga)
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.
Attachment #8909616 - Flags: review?(bugs)
(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
Attachment #8909616 - Flags: review?(bugs) → review+
(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
Attachment #8909616 - Flags: approval-mozilla-release?
https://hg.mozilla.org/mozilla-central/rev/85af63c044f7
Status: NEW → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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.
Attachment #8909616 - Flags: approval-mozilla-release? → approval-mozilla-release+
Attached image 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.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.