2016307 - incorrect event mask in Cocoa widget mouse event monitoring

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Josh Aas]

A mistake was made in the fix for bug 1683723 (Phabricator):

diff --git a/widget/cocoa/nsToolkit.mm b/widget/cocoa/nsToolkit.mm
--- a/widget/cocoa/nsToolkit.mm
+++ b/widget/cocoa/nsToolkit.mm
@@ -142,11 +142,11 @@
 
   if (getenv("MOZ_NO_GLOBAL_MOUSE_MONITOR")) return;
 
   if (mAllProcessMouseMonitor == nil) {
     mAllProcessMouseMonitor = [NSEvent
-        addGlobalMonitorForEventsMatchingMask:NSLeftMouseDownMask | NSOtherMouseDownMask
+        addGlobalMonitorForEventsMatchingMask:NSEventMaskLeftMouseDown | NSEventMaskLeftMouseDown
                                       handler:^(NSEvent* evt) {
                                         if ([NSApp isActive]) {
                                           return;
                                         }

NSOtherMouseDownMask was incorrectly replaced with NSEventMaskLeftMouseDown, resulting in masking the same value twice.

The simplest correction here is to do what the patch should have done - use NSEventMaskOtherMouseDown for the second masked value.

However, the comment above this function muddies the water. It says:

// Cocoa Firefox's use of custom context menus requires that we explicitly
// handle mouse events from other processes that the OS handles
// "automatically" for native context menus -- mouseMoved events so that
// right-click context menus work properly when our browser doesn't have the
// focus (bmo bug 368077), and mouseDown events so that our browser can
// dismiss a context menu when a mouseDown happens in another process (bmo
// bug 339945).

This implies that mouse moved, left down, right down, and other down should all be masked.

Comment 1

[Josh Aas]

I'll also note that if someone fixes this they could also fix the double "#include "nsIWidget.h" in this file at the same time.

Comment 2

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1683723

:mstange, since you are the author of the regressor, bug 1683723, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 3

[Markus Stange [:mstange]]

Ah, nice find, thanks.

These days we don't hit this code unless somebody manually turned off native context menus. By default, we take the early return at the beginning of the function.

Comment 4

[Josh Aas]

Attachment: Bug 2016307 - Fix incorrect event mask in Cocoa widget mouse event monitoring. r?mstange

Comment 5

[Josh Aas]

I don't think it's within the scope of this issue to verify that everything about non-native context menus is working.

I think we should fix the mistake, restoring the code to the intended meaning, and remove the misleading information from the comment. Per git history, that comment has been incorrect for a long time and I don't think the bugs it references are actually helpful in understanding what's going on any more. If someone's needs to work on this code for some reason, better to take a fresh perspective on the issue.

This is what the patch I just uploaded does.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/1f37fe117e24
https://hg.mozilla.org/integration/autoland/rev/fa63cf9ba047
Fix incorrect event mask in Cocoa widget mouse event monitoring. r=mstange

Comment 7

[Iulian Moraru]

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