1591140 - The context menu is shown in the inspector when right clicking a DOM node

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Peter]

Attachment: firefox-double-context-menu.jpg

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

1. Go to about:config and prevent JS from generally disabling the context menu via "dom.event.contextmenu.enabled" (set to non-standard "false").
2. Now open up the inspector (developer tools, CMD+I) and right click any DOM node.

Actual results:

Notice how the document's (for the lack of a better word) context menu opens up in addition to the one the inspector provides - even lying above the latter.
This is super anyoing, to say the least.

Expected results:

Up until a couple of days, the inspector's menu was the only menu shown when right clicking a DOM node, regardless of the value of the "dom.event.contextmenu.enabled" setting, which I haven't tinkered with in months.

The, imho, correct behaviour should be to ignore the setting within the context of the developer tools and never show the document's context menu ... just like it used to be.

Comment 1

[Brad Werth [:bradwerth]]

Thank you for filing. I can reproduce with the dom.event.contextmenu.enabled pref set to false. I'll attempt a regression.

Comment 2

[Brad Werth [:bradwerth]]

First attempt at mozregression narrowed the window to something that landed between October 9th and October 30th, but was otherwise inconclusive. I'm trying again.

Comment 3

[Brad Werth [:bradwerth]]

mozregression identified Bug 1581925 as the source. Nika, can you take a look?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

I am guessing that the reason for this behaviour is that devtools uses context menu events in order to control the context menu, like normal web content. Before the changes in bug 1581925, we were not running the context menu for the toolbox, so it was always allowed to control menus. However, not that we treat it more like a usual content browser, the content code is running, meaning it is subject to the same requirements as web content.

We might be able to fix this by not overriding the defaultPrevented behaviour when the event is fired in a chrome-privileged document here: https://searchfox.org/mozilla-central/rev/ce02064d8afc8673cef83c92896ee873bd35e7ae/browser/actors/ContextMenuChild.jsm#542. That being said, I don't know if this about:config setup is one we intend to support.

Comment 5

[Julian Descottes [:jdescottes]]

I can confirm we use contextmenu events in many places in DevTools. I actually had to preventDefault on such events before we switched the toolbox to a <browser> element in https://hg.mozilla.org/mozilla-central/rev/7d309560313a, but of course with this setting it no longer works.

This preference is quite old but still seems supported in enterprise policies, so I assume we want to keep it. Let's try to skip it for DevTools only as you suggested.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1591140 - Allow chrome privileged documents to preventDefault contextmenu events regardless of dom.event.contextmenu.enabled

@gl can you take a look at the DevTools test added here?
@nika @mconley I used this.browsingContext.docShell.isContent here to rule out chrome-privileged documents. Do you know if there is better way to check if the current document is chrome-privileged or not?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5ca446ceff7
Allow chrome privileged documents to preventDefault contextmenu events regardless of dom.event.contextmenu.enabled r=gl,nika,mconley

Comment 8

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/e5ca446ceff7

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 10

[Julian Descottes [:jdescottes]]

Let's check with @mconley first, the fix is small but it's on a very central piece of the context menu code (and not my usual area).
Mike, do you think it's ok to uplift as is, or should it bake a bit more on Nightly?
(also considering it only impacts DevTools users with dom.event.contextmenu.enabled set to false)

Comment 11

[Mike Conley (:mconley) (:⚙️)]

I think this can be safely uplifted.

Comment 12

[Julian Descottes [:jdescottes]]

Thanks for the feedback, let's request an uplift then.

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 9105641 [details]
Bug 1591140 - Allow chrome privileged documents to preventDefault contextmenu events regardless of dom.event.contextmenu.enabled

Beta/Release Uplift Approval Request

• User impact if declined: Users with the pref dom.event.contextmenu.enabled set to false cannot use context menus in DevTools (the regular context menu is overlayed on top of the DevTools one).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small javascript fix, tested in automation.
• String changes made/needed:

Comment 14

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9105641 [details]
Bug 1591140 - Allow chrome privileged documents to preventDefault contextmenu events regardless of dom.event.contextmenu.enabled

Low risk, has tests, uplift approved for 71 beta 9, thanks.

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f0bc4fd31372