Closed Bug 406362 Opened 14 years ago Closed 14 years ago

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

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

Details

Attachments

(1 file, 3 obsolete files)

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.
Flags: blocking1.9+
Priority: -- → P2
Attached patch fix v0.9 (obsolete) — Splinter Review
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.
Attached patch fix v1.0 (obsolete) — Splinter Review
Attachment #291017 - Attachment is obsolete: true
Attachment #291113 - Flags: review?(smichaud)
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.
Attachment #291113 - Flags: review?(smichaud) → review+
Attached patch fix v1.1 (obsolete) — Splinter Review
Address Steven's review comments.
Attachment #291113 - Attachment is obsolete: true
Attachment #291566 - Flags: superreview?(roc)
Moving to a P1.  We'll do what we can to get this reviewed an in for beta 2.
Priority: P2 → P1
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.
(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.
Attached patch fix v1.2Splinter Review
Added a comment about the z-order and changed all nsCocoaUtils methods to public static members of nsCocoaUtils.
Attachment #291566 - Attachment is obsolete: true
Attachment #291764 - Flags: superreview?(roc)
Attachment #291566 - Flags: superreview?(roc)
Attachment #291764 - Flags: superreview?(roc) → superreview+
landed on trunk
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.