406362 - Mouse move events can be double processed, causes menu flicker and other issues

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Josh Aas]

It is possible for mouse move events to be processed by more than one widget. Furthermore, rerouted events often use the wrong coordinates because we don't correct for the fact that event coords are relative to the window the event originally went to. This causes at least the following two problems:

1. Under certain conditions submenus may flicker when moving the mouse around within their menu item in the parent menu. This shows up most often in the default Places bookmark menu bar folder.
2. When using the mouse within a popup window, chrome and content items in the parent window can be activated at an offset from the popup menu such that you can see mouse over activations in other places on the page.

Comment 1

[Josh Aas]

Attachment: fix v0.9

This patch fixes all of the issues described in the summary for this bug. The only thing I haven't done yet is investigate the potential impact on embedding clients.

Comment 2

[Josh Aas]

Attachment: fix v1.0

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 291113 [details] [diff] [review]
fix v1.0

I discovered a couple of bugs in this patch, both of which make it
fail to build on Tiger (as opposed to Leopard).  But these problems
are minor (and easily corrected).  And once I fixed them, all my other
tests went just fine.

1) NSInteger is undefined in any of the ordinary locations on Tiger
   (/usr/include, /System/Library/Frameworks, /Developer/SDKs).  The
   simplest fix for this is to replace every occurance of 'NSInteger'
   (in nsCocoaUtils.mm) with 'int'.  (On Leopard, NSInteger is defined
   in NSObjCRuntime.h, and is meant to help the transition from
   32-bits to 64-bits.)

2) The 'inline' keyword on screenLocationForEvent (also in
   nsCocoaUtils.mm) causes a linker error when building Minefield.
   The simplest fix is to get rid of it.

For reasons that I haven't been able to figure out, neither of these
problems happen when building Camino (on Tiger).

My other tests were (on Leopard) to check that this bug's symptoms are
fixed (they are), and (on both Tiger and Leopard) to run through
various operations with the Places menu and with context menus.  I
tested in both Minefield and Camino, on both Tiger and Leopard.

Comment 4

[Josh Aas]

Attachment: fix v1.1

Address Steven's review comments.

Comment 5

[Damon Sicore (:damons)]

Moving to a P1.  We'll do what we can to get this reviewed an in for beta 2.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So clicks not over a rollup widget don't get handled by us? Shouldn't we be handling them and rolling up the widget? If not, how does the rollup happen, does Appkit tell us directly?

Your C++ CocoaUtils functions should start Uppercase. Also, we generally try not to pollute the global namespace like this. The normal approach is to make them static methods of a class nsCocoaUtils or something.

NSWindowList is guaranteed to return items in decreasing z-order? Please comment that if so. If not, there's a bug.

Comment 7

[Josh Aas]

(In reply to comment #6)
> So clicks not over a rollup widget don't get handled by us? Shouldn't we be
> handling them and rolling up the widget? If not, how does the rollup happen,
> does Appkit tell us directly?

If a click is not over any window then it doesn't get handled by us. In that case we'll roll up in the resulting notification that app-level focus changed. If it is not over a rollup widget but it is over some other window, that window will get the click and do rollup.

> Your C++ CocoaUtils functions should start Uppercase. Also, we generally try
> not to pollute the global namespace like this. The normal approach is to make
> them static methods of a class nsCocoaUtils or something.

I'll post a new patch fixing this.

> NSWindowList is guaranteed to return items in decreasing z-order? Please
> comment that if so. If not, there's a bug.

The list is in a particular z-order. The first item is the foremost.

Comment 8

[Josh Aas]

Attachment: fix v1.2

Added a comment about the z-order and changed all nsCocoaUtils methods to public static members of nsCocoaUtils.

Comment 9

[Josh Aas]

landed on trunk