Closed Bug 387164 Opened 16 years ago Closed 15 years ago

Consolidated patch for issues with popup windows in Cocoa Widgets

Categories

(Core :: Widget: Cocoa, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: smichaud, Assigned: smichaud)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch Popup patch (obsolete) — Splinter Review
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).
Attachment #271270 - Flags: review?(joshmoz)
Comment on attachment 271270 [details] [diff] [review]
Popup patch

+    if (gRollupListener != nsnull && gRollupWidget != nsnull)

Should just be

+    if (gRollupListener && gRollupWidget)

more comments coming soon
Blocks: 346015
Blocks: 344367
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!
Attachment #271270 - Flags: review?(joshmoz) → review-
Here it is.
Attachment #271270 - Attachment is obsolete: true
Attachment #271738 - Flags: review?(joshmoz)
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
Attachment #271270 - Attachment is obsolete: false
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".
Attachment #271270 - Attachment is obsolete: true
Attachment #271738 - Attachment is obsolete: true
Attachment #271738 - Flags: review?(joshmoz)
Attachment #271848 - Flags: review?(joshmoz)
Attachment #271848 - Flags: superreview?(mikepinkerton)
Attachment #271848 - Flags: review?(joshmoz)
Attachment #271848 - Flags: review+
+  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.
Attachment #271848 - Attachment is obsolete: true
Attachment #271875 - Flags: superreview?(mikepinkerton)
Attachment #271848 - Flags: superreview?(mikepinkerton)
Comment on attachment 271875 [details] [diff] [review]
Popup patch rev3 (add Mike's change)

sr=pink
Attachment #271875 - Flags: superreview?(mikepinkerton) → superreview+
(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().
Attachment #272061 - Flags: review?(joshmoz)
Attachment #272061 - Flags: review?(cbarrett)
Attachment #272061 - Flags: review?
Attachment #271875 - Attachment is obsolete: true
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.
Attachment #272061 - Attachment is obsolete: true
Attachment #272258 - Flags: review?(joshmoz)
Attachment #272258 - Flags: review?(cbarrett)
Attachment #272061 - Flags: review?(joshmoz)
Attachment #272061 - Flags: review?(cbarrett)
No longer blocks: 344367
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.
Attachment #272258 - Flags: review?(cbarrett) → review+
Comment on attachment 272258 [details] [diff] [review]
Popup patch rev5 (merge similar functions)

colin's review for the updated patch is enough
Attachment #272258 - Flags: review?(joshmoz)
Landed on trunk.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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?
You need to log in before you can comment on or make changes to this bug.