This is a (fairly large and complex) patch for issues with popup windows in Cocoa Widgets, and particularly with context menu popups. The complexity is needed because Cocoa Widgets uses custom popup windows -- so the (often undocumented) things that the OS does automatically for "native" popups (particularly for context menus) need to be done explicitly by Cocoa Widgets code. As far as I know this patch resolves all bugs that have been opened on issues with popup windows in Cocoa Widgets, and all problems that I found on my own while writing the patch. But I wouldn't be surprised if I've missed something. If I have, please let me know! Important Note: This patch can't be landed until a patch is landed for bug 385221. It uses APIs (CGEventTapCreate() and friends) that are only available on OS X 10.4 and above. Builds that use older SDKs will die with errors about missing headers and/or missing symbols, because the CGEventTap methods aren't found in the older SDKs (10.3.9 and earlier).

Comment on attachment 271270 [details] [diff] [review] Popup patch + if (gRollupListener != nsnull && gRollupWidget != nsnull) Should just be + if (gRollupListener && gRollupWidget) more comments coming soon

Comment on attachment 271270 [details] [diff] [review] Popup patch >+- (BOOL)maybeRerouteMouseEventToRollupWidget:(NSEvent *)anEvent Lets call this "shouldRerouteMouseEventToRollupWidget". That language is more clear in the context it is called. >+ NSWindow *ctxMenuWindow = (NSWindow *) Please no space between the type and the '*' in the cast, this happens in multiple places. You can probably find them all by searching for " *)". >+@interface NSApplication (Undocumented) >+ >+- (void)_removeWindowFromCache:(NSWindow *)aWindow; >+ >+@end >+ >+ >+@interface NSWindow (Undocumented) >+ >+- (void)_setWindowNumber:(int)aNumber; >+ >+@end Maybe make a note about what this stuff is for where you declare it here. >+ if ([mWindow isKindOfClass:[PopupWindow class]] && >+ [(PopupWindow *) mWindow getIsContextMenu]) >+ { Curly bracket should not have its own line. This happens in multiple places. >-static OSStatus AllAppMouseEventHandler(EventHandlerCallRef aCaller, EventRef aEvent, void* aRefcon) >+static OSStatus EventMonitorHandler(EventHandlerCallRef aCaller, EventRef aEvent, void* aRefcon) > { >- if (![NSApp isActive]) { >- if (gRollupListener != nsnull && gRollupWidget != nsnull) >- gRollupListener->Rollup(); >- } >+ return eventNotHandledErr; >+} Can you make a note here that points people to the comment about why we do nothing in this handler? Or clarify in some other way. Please post a new patch with this stuff fixed plus any previous comments. Thanks!

Here it is.

Comment on attachment 271270 [details] [diff] [review] Popup patch >+- (BOOL)maybeRerouteMouseEventToRollupWidget:(NSEvent *)anEvent >+{ For what it's worth, I think you could just drop the first word and call it rerouteMouseEventToRollupWidget. Josh's suggestion of shouldReroute... is a bit misleading -- it doesn't make clear that this method is doing the rerouting. >+ if (gRollupWidget) { >+ NSWindow *ctxMenuWindow = (NSWindow *) >+ gRollupWidget->GetNativeData(NS_NATIVE_WINDOW); >+ if (ctxMenuWindow && [ctxMenuWindow isKindOfClass:[PopupWindow class]]) { >+ [[NSDistributedNotificationCenter defaultCenter] >+ postNotificationName:@"com.apple.HIToolbox.beginMenuTrackingNotification" >+ object:@"PopupWindow"]; >+ [(PopupWindow *) ctxMenuWindow setIsContextMenu:YES]; >+ } >+ } This code appears to be duplicated a number of times, you should write a helper method if possible. >+- (BOOL)getIsContextMenu; I think this should be named isContextMenu. If this were C++ GetIsContextMenu() would be fine, but this ObjC. > // Create the window. If we're a top level window, we want to be able to > // have the toolbar pill button, so use the special ToolbarWindow class. > Class windowClass = (mWindowType == eWindowType_toplevel) >- ? [ToolbarWindow class] : [NSWindow class]; >+ ? [ToolbarWindow class] : >+ (mWindowType == eWindowType_popup >+ ? [PopupWindow class] : [NSWindow class]); Could you not use nested ternary operators? Something like: Class windowClass = [NSWindow class]; // Comment if (mWindowType == eWindowType_toplevel) windowClass = [ToolbarWindow class]; // Comment else if (mWindowType == eWindowType_popup) windowClass = [PopupWindow class]; would be much more readable. >+ // Unless it's explicitly removed from NSApp's "window cache", a popup >+ // window will keep receiving mouse-moved events even after it's been >+ // "ordered out" (instead of the browser window that was underneath it, >+ // until you click on that window). This is bmo bug 378645, but it's >+ // surely an Apple bug. The "window cache" is an undocumented subsystem, Make sure to file a radar and preferably include the radar number in the comment. There are a number of other places where you should be filing radars, if you haven't already. >+ NSRect nsScreenFrame = [[ctxMenuWindow screen] frame]; >+ nsLocation.x = cgLocation.x; >+ nsLocation.y = nsScreenFrame.size.height - cgLocation.y; 1) Using the ns prefix makes these look like class names, which is a bit confusing. 2) Don't we have methods that do this translation *and* make sure we do things right for multiple monitors? >+ // Using an event tap for mouseDown events (instead of installing a >+ // handler for them on the EventMonitor target) works around an Apple >+ // bug that causes OS menus (like the Clock menu) not to work properly >+ // on OS X 10.4.X and below (bmo bug 381448). But event taps are only >+ // supported on OS X 10.4 and above. Could you also list the radar number here? I'm also not sure the comments about event taps only being on 10.4.x are totally necessary since we're going to need be building with 10.4u before this patch lands (or

Yeah, I changed my mind about shouldRerouteMouseEventToRollupWidget. I think we should go back to "maybe". Sorry!

I've mostly followed your suggestions, Colin. But there were a couple that I didn't follow: > 2) Don't we have methods that do this translation *and* make sure we > do things right for multiple monitors? Are you talking about nsChildView::ConvertToDeviceCoordinates()? That translates from one kind of Cocoa coordinates (NSView-based) to another (NSWindow-based). But what we need here is (as I say in my comment) a translation from CG/QD "global coordinates" to Cocoa "screen coordinates". So ConvertToDeviceCoordinates() won't work. If [NSWindow screen] works properly, we shouldn't need to worry about having the wrong NSScreen. > Make sure to file a radar and preferably include the radar number in > the comment. There are a number of other places where you should be > filing radars, if you haven't already. There are three Apple bugs that I mention in my comments, which I ran across while writing this patch: 1) The "event tap menu hang". 2) The "custom context menu stops receiving mouseMoved events" bug. 3) The "popup window keeps receiving mouseMoved events after being ordered out" bug. The first one is quite severe (it prevents event taps from being used to filter mouseDown events). So it's likely that Apple will want to fix it. I'll put in the radar number once I've written a test case and opened a bug with Apple. The other two are likely to only effect someone who's trying to implement custom context menus ... which means that Apple's quite unlikely to care about them. It'd also be quite a bit more difficult to write test cases for them. I don't think it's worth the trouble to do so. Side Note: Apple'd make me a lot more willing to file bugs with them if they'd open their bug database. As things now stand only the person who filed the bug can see Apple's response. So radar numbers in comments are actually pretty useless -- all they tell you is "a bug has been filed".

+ if (!sender || ![sender isEqualToString:@"PopupWindow"]) { we should probably scope this so there's no chance of overlap, eg, |org.mozilla.gecko.PopupWindow|. + // If a popup window is shown after being hidden, it needs to be "reset" + // for it to receive any mouse events aside from mouse-moved events + // (because it was removed from the "window cache" when it was hidden + // -- see below). has this been tested on leopard and panther? it could be a tiger bug. I also need to ask if these fixes have been tested to make sure they don't mess up camino. Just making sure...

i forget panther is off the table. just leopard then...

> + if (!sender || ![sender isEqualToString:@"PopupWindow"]) { > > we should probably scope this so there's no chance of overlap, eg, > |org.mozilla.gecko.PopupWindow|. Good idea. I'll do it. > has this been tested on leopard and panther? it could be a tiger bug. I haven't tested for this particular bug on Leopard. And just now problems (I think with pre-Dwarf debugging info) prevented me from doing so. But the solution has been quite throughly tested on Leopard (both in my Popup patch and in the Java Embedding Plugin, which has to play a similar trick). > I also need to ask if these fixes have been tested to make sure they > don't mess up camino. Just making sure... Yes, on both Tiger and Leopard.

Comment on attachment 271875 [details] [diff] [review] Popup patch rev3 (add Mike's change) sr=pink

(In reply to comment #6) > I've mostly followed your suggestions, Colin. But there were a couple > that I didn't follow: > > > 2) Don't we have methods that do this translation *and* make sure we > > do things right for multiple monitors? > > Are you talking about nsChildView::ConvertToDeviceCoordinates()? > > That translates from one kind of Cocoa coordinates (NSView-based) to > another (NSWindow-based). But what we need here is (as I say in my > comment) a translation from CG/QD "global coordinates" to Cocoa > "screen coordinates". So ConvertToDeviceCoordinates() won't work. > > If [NSWindow screen] works properly, we shouldn't need to worry about > having the wrong NSScreen. I meant aren't there methods on NSWindow and NSView to convert from one coordinate system to the other. > > The other two are likely to only effect someone who's trying to > implement custom context menus ... which means that Apple's quite > unlikely to care about them. It'd also be quite a bit more difficult > to write test cases for them. I don't think it's worth the trouble to > do so. > > Side Note: Apple'd make me a lot more willing to file bugs with them > if they'd open their bug database. As things now stand only the > person who filed the bug can see Apple's response. So radar numbers > in comments are actually pretty useless -- all they tell you is "a bug > has been filed". IME, Apple people live in a bit of a bubble WRT things happening outside. Radar is really the only way we have into that bubble. Even if Apple is less likely to care about them, they are still legitimate bugs, and there's 0 chance of them getting fixed if we don't file them. Sometimes Apple also uses bug activity to gauge how often things are being used -- for example one of the reasons they didn't bother to update the Cocoa-Java bridge was that nobody had been filing radars about it. We all assumed that they'd never fix our bugs, so why should we file radars? It's just a good idea to file an issue with Apple, even if you can't/don't write up a reduced testcase. At least, that's my personal stance on things. If you feel differently, no worries.

> I meant aren't there methods on NSWindow and NSView to convert from > one coordinate system to the other. Those methods all translate from one Cocoa coordinate system to another, so none of them will do the job. > If [NSWindow screen] works properly, we shouldn't need to worry > about having the wrong NSScreen. This is correct. But there's no way to determine which "screen" the incoming mouse event ('event') belongs to, so there's actually a (small) chance that my code in EventTapCallback() will cause trouble on systems will multiple monitors. The incoming event's location (as returned by CGEventGetLocation()) is in "global display coordinates". Apple's documentation for CGDisplayAddressForPosition() (among other places) implies that this is a single coordinate system for all displays/screens, whose "origin is the upper left corner of the main display". If the rectangle returned by [NSScreen frame] is also in a coordinate system that includes all displays/screens (but which has a bottom-left origin), then we won't have any problems. But Apple's documentation [NSScreen frame] is quite vague (it's said to return "the full screen rectangle at the current resolution"). So this question can't be resolved except by trial and error, and I don't have access to more than one monitor. We will need to keep an eye out for problems on multiple-monitor systems.

> If the rectangle returned by [NSScreen frame] is also in a > coordinate system that includes all displays/screens (but which has > a bottom-left origin), then we won't have any problems. On second thought this isn't true -- my current code assumes that the height of the entire coordinate system is the same as the height of rollup widget's NSWindow's NSScreen. I'll need to post a revision.

Comment on attachment 272061 [details] [diff] [review] Popup patch rev4 (deal better with multiple displays) This revision deals with the problem I mentioned in comment #15. Josh and Colin, please take a look at it. The only part that's changed is in nsToolkit.mm, where I've added CocoaScreenCoordsHeight() and ConvertCGGlobalToCocoaScreen() and changed EventTapCallback().

Colin pointed out a couple of fuctions I hadn't noticed -- FlipCocoaScreenCoordinates() (in nsChildView.mm) and HighestPointOnAnyScreen() (in nsCocoaUtils.mm). But my CocoaScreenCoordsHeight() (formerly in nsToolkit.mm) does a better job than HighestPointOnAnyScreen() -- my function accounts for the possibility that part of the global screen coordinate space might have negative values. So I (more or less) replaced HighestPointOnAnyScreen() with CocoaScreenCoordsHeight(). I decided not to call FlipCocoaScreenCoordinates() from nsToolkit.mm, though -- both it and my ConvertCGGlobalToCocoaScreen() are very simple (though they do exactly the same thing), so it didn't seem worth the trouble.

Comment on attachment 272258 [details] [diff] [review] Popup patch rev5 (merge similar functions) Tested this patch on a multiple monitor setup, seemed to work fine. Go for it.

Comment on attachment 272258 [details] [diff] [review] Popup patch rev5 (merge similar functions) colin's review for the updated patch is enough

Landed on trunk.

Steven: Any suggestions on some tests that we could run to check for possible regressions?

I can't think of anything (aside from the obvious -- going through each bug in this bug's "blocks" list to make sure they have been fixed). Do you have specific regressions in mind?