325336 - implement better window/sheet support in Cocoa widgets

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Josh Aas]

We have pretty bad window/sheet support in Cocoa widgets (actually, no sheet support at all). This is a general bug for patches to improve the situation, and should probably be closed when we have basic support for sheets and a working window situation.

Comment 1

[Josh Aas]

Attachment: Carbon sheet logic fix, v1.0

This removes an unnecessary call from the Carbon sheet logic, which makes it a bit easier to understand.

-        WindowPtr top = GetWindowTop(parentWindowRef);
+        WindowPtr top = parentWindowRef;

|top| is supposed to be a window that is not a sheet. If GetWindowTop returns anything other than parentWindowRef, then it has returned a sheet, which we don't want. So the call is pointless/wrong.

Comment 2

[Mark Mentovai]

Comment on attachment 210248 [details] [diff] [review]
Carbon sheet logic fix, v1.0

We certainly don't want to attach a sheet to another sheet, and if the parent nsWindow is a sheet, we already set top to the WindowRef corresponding to the window it's attached to.    GetWindowTop is senseless here, as far as I can see.

Comment 3

[Josh Aas]

landed on trunk, pretty much just for reference purposes

Comment 4

[Josh Aas]

Attachment: always save parent, use PRPackedBool

nsCocoaWindow does not inherit from something like nsWindow like nsMacWindow (Carbon widgets) does, so it should always save its parent. We'll need it when we want to do sheets. Also, use PRPackedBool.

Comment 5

[Mark Mentovai]

Comment on attachment 210617 [details] [diff] [review]
always save parent, use PRPackedBool

This review expires in 0.005 seconds.

Comment 6

[Josh Aas]

Comment on attachment 210617 [details] [diff] [review]
always save parent, use PRPackedBool

mento: josh: here is where the mOffsetParent thing might make a difference
mento: right now, we DON'T call WidgetToScreen if you are not a popup
josh: look above that
josh: if ( mWindowType == eWindowType_popup ) {
mento: dammit, haven't i asked you for more context in your patches before? 
josh: I sure fooled you
mento: missed it by *one* line
• josh laughs for 0.005 seconds

Comment 7

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 210617 [details] [diff] [review]
always save parent, use PRPackedBool

sr=pink. still gives me the willies.

Comment 8

[Josh Aas]

"always save parent, use PRPackedBool" landed on trunk

Comment 9

[Josh Aas]

Attachment: clean up window construction, v1.0

Comment 10

[Josh Aas]

"clean up window construction, v1.0" landed on trunk

Comment 11

[Josh Aas]

Attachment: add basic window type support, v1.0

This adds basic window type support. I tried to copy our behavior in Carbon, and at the very least this shouldn't break anything that works now. There is some more to be done, and we'll probably have to iron out this behavior as we hit test cases that it doesn't handle correctly. This does not break Camino. More patches coming up...

Comment 12

[Josh Aas]

Attachment: add basic window type support, v1.1

Same as v1.0, but also fixes bug 326554.

Comment 13

[Mark Mentovai]

Comment on attachment 211282 [details] [diff] [review]
add basic window type support, v1.1

Turn the innermost switch into ifs for the bits, avoiding stuff like:

+                case eBorderStyle_resizeh:
+                case (eBorderStyle_title | eBorderStyle_resizeh):

ick.

+    mWindowMadeHere = PR_FALSE;

Not strictly necessary because mWindowMadeHere is initialized to false.

+    mVisible = PR_TRUE;

mVisible is not initialized in the constructor, could you fix that?

OK to fix these on checkin.  Please also file a followup about windows not being destroyed, as discussed.

Comment 14

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 211282 [details] [diff] [review]
add basic window type support, v1.1

rs=pink

Comment 15

[Josh Aas]

Attachment: add basic window type support, v1.2

Patch I actually checked in.

Comment 16

[Josh Aas]

Attachment: track enable, v1.0

This patch makes Cocoa windows match Carbon windows in terms of their support for the nsIWidget enabling methods. I was doing this to test something, and while this doesn't seem to break anything it doesn't seem to fix anything either. I'm just putting this patch here for when I know more about what is going on with enabling.

Comment 17

[Josh Aas]

Attachment: track enable, v1.1

Includes more cleanup.

IsActive and SetIsActive are totally unnecessary. They aren't even used in Carbon widgets even though they are "implemented" there.

Comment 18

[Josh Aas]

Attachment: build native menu items earlier, v1.0

Right now, we delay menu construction (including the creation of native menu items) until a menu is selected. This is not good because in order for keyboard commands to work, native menu items need to be created. This patch causes all menus and their submenus to get constructed when the menu bar is. It works because when the top-level menus are created, they call MenuConstruct. MenuConstruct creates submenus, and during their creation they call MenuConstruct. This goes on until all menus and their submenus have been constructed. Voila.

Comment 19

[Josh Aas]

Comment on attachment 212094 [details] [diff] [review]
build native menu items earlier, v1.0

I moved the patch from comment #18 into its own bug, 327594.

Comment 20

[Josh Aas]

Attachment: make Window menu work, cleanup

This patch makes the Window menu work. Previously, windows were never removed from its list and the checked window never changed. The fix is to send GUI events when we do things to our window. This also removes code that adds a custom content view in windows we create. That isn't necessary, even with Quickdraw.

Comment 21

[Mark Mentovai]

Comment on attachment 212810 [details] [diff] [review]
make Window menu work, cleanup

Needs more events (you say you're already on it), but this is good to get the window menu working.

Comment 22

[Josh Aas]

Attachment: popup window rollup work, v1.0

This adds some basic support for rolling up popup windows when certain top-level window events happen. There is probably some more refining of this behavior to do, but this stops the worst of floating popups. We can probably drop nsChildView::CaptureRollupEvents since I don't think anything but a top-level window will ever get asked to capture rollup events, but I'm going to wait a bit to make sure.

Comment 23

[Josh Aas]

Attachment: fix window origin on creation, v1.0

This fixes the origin given to Cocoa windows upon creation. There is more work to do with origin/size, but this is a clean start.

Note: This patch conflicts with "popup window rollup work, v1.0" so you can only have one or the other applied at any given time. I just wanted them separated out into different patches.

Comment 24

[Josh Aas]

The function I created called "geckoRectToCocoaRect" does not do the same thing as the similarly named fuction in nsChildView.mm. The latter does not convert coordinate systems, as the NSView objects it is used with have flipped coordinate systems. That isn't an option for me, so one of the two functions should be renamed. Maybe the nsChildView one should be "convertGeckoRectToFlippedCocoaRect", or the function should take an argument specifying whether or not it wants a coordinate flip.

Comment 25

[Mark Mentovai]

Comment on attachment 213433 [details] [diff] [review]
fix window origin on creation, v1.0

>+const short kWindowTitleBarHeight = 22;

Can we avoid hardcoding this?  Maybe play with contentRectForFrameRect:styleMask: or FrameRectForContentRect:styleMask:.

>+static nsresult geckoRectToCocoaRect(nsRect geckoRect, NSRect *cocoaRect)

As you say, I think this one should be named to indicate that it flips.

>+  // All we need to do is convert the origin's Y coordinate by getting
>+  // the screen's height and subtracting the gecko rect's Y coord from
>+  // the screen height.

Seems bad for vertically stacked screens - needs testing as discussed.

(This is where I'm stopping, 'til that's tested.)

Comment 26

[Josh Aas]

"popup window rollup work, v1.0" landed on trunk

Comment 27

[Josh Aas]

Attachment: fix window origin on creation, v2.0

This is an overhaul of the last patch to do things better. There are a lot of comments because coordinates (at least in Mac OS X code) are really messy business. Some of the choices made about how to handle coordinates in window creation have a long and complicated history. I have reproduced the Carbon behavior as accurately as possible. If we want to change that behavior, we'll need to examine all callers and probably change a lot of things around. We should probably do this some other time if we want to.

Comment 28

[Josh Aas]

+    float currScreenHighestPoint = currScreenFrame.origin.x + currScreenFrame.size.height;

That is a typo. It should be ".y" not ".x".

Comment 29

[Josh Aas]

I tested this patch on multiple monitors with the typo fix from comment #28 and it works fine. There are other problems with coordinates, but not really related to this.

Comment 30

[Mark Mentovai]

Comment on attachment 214521 [details] [diff] [review]
fix window origin on creation, v2.0

r+ for bootstrapping with that x turned into a y.  We know there are other issues, and we'll revisit them when the time comes.

I suspect we'll need something like GetDesktopRect in Carbon widgets to really handle the menu bar with accuracy, and to address moving/sizing fully.

Comment 31

[Mark Mentovai]

We need to set up a wiki page with all of these "need to be addressed in future cleanup" cocoa widget issues.  Do you want to reuse http://wiki.mozilla.org/Mac:Cocoa_Widgets , or should we start a fresh one?

Comment 32

[Josh Aas]

"fix window origin on creation, v2.0" landed on trunk with ".y" fix

Comment 33

[Josh Aas]

I'm closing out this bug, all future patches will go to individualized bugs. It will be easier to keep track of them that way.

Comment 34

[Boris Zbarsky [:bzbarsky]]

I think this caused bug 350018.