330587 - no support for displaying sheets in Cocoa widgets

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Josh Aas]

Cocoa widgets do not support displaying sheets. We need to flesh out the Cocoa widget implementation of nsCocoaWindow::Show.

Comment 1

[Josh Aas]

Attachment: fix v0.5

This is a partial patch, I'm not requesting review. It is here as a backup and comments about interface stuff.

Comment 2

[Josh Aas]

Attachment: fix v0.6

Comment 3

[Josh Aas]

Attachment: fix v0.6.5

sync to latest trunk, just a backup

Comment 4

[Josh Aas]

Attachment: window focus/activate fix v1.0

Part of implementing nsCocoaWindow::Show in a better way, which is a part of sheet support, is calling makeKeyAndOrderFront: instead of orderFront: on top-level windows. This means that we will be creating windows and programmatically making them key instead of forcing the user to make the window key by clicking on it. When a window is created, its first responder is the window itself and therefore if we programmatically make it key then the first responder won't change and the nsWindowMap system won't send our focus/activation events for us (nsWindowMap requires the first responder to be a ChildView or it won't send focus/activate events). In order to fix this, we need to detect whether or not the first responder is something he nsWindowMap system can deal with, and if it isn't then we need to send focus/activate events ourselves. This way we work in embedding and non-embedding situations.

Comment 5

[Josh Aas]

Attachment: window focus/activate fix v2.0

This is a completely different way of doing this and ultimately cleaner I think.

Comment 6

[Josh Aas]

id delegate = [window delegate];
if ([delegate isKindOfClass:[WindowDelegate class]])
  [delegate sendGotFocusAndActivate];

Before sending sendGotFocusAndActivate we should make sure the delegate is one that will handle that message. Embedding apps won't have it. Same for lostfocus/deactivate.

Comment 7

[Mark Mentovai]

Comment on attachment 219779 [details] [diff] [review]
window focus/activate fix v2.0

r+ with changes requested on irc: fix deactivate/lostfocus event variable names, use respondsToSelector.

Comment 8

[Josh Aas]

Attachment: window focus/activate fix v2.1

Comment 9

[Josh Aas]

window focus/activate fix v2.1 checked in on trunk

Comment 10

[Josh Aas]

Attachment: fix v0.6.6

updated to trunk, cleaner code, still not there, backup

Comment 11

[Josh Aas]

Attachment: fix v0.7

Fixes a bug where once you showed a sheet you could never show one again. Now all that is left really is the menu bar and the fact that the sheet doesn't have focus...

Comment 12

[Josh Aas]

Attachment: fix v0.8

Sheets actually work now. Three issues remain:
- show a reasonable menu bar when sheets are open
- handle nested sheets (only half implemented in v0.8)
- sheet windows are a little too small for their content

Comment 13

[Josh Aas]

Attachment: nsPIWidgetCocoa interface (checked in)

This is the patch I checked in to add the nsPIWidgetCocoa interface to top-level Cocoa window widgets.

Comment 14

[Josh Aas]

Attachment: fix v0.9

This sheets impl works pretty well, but it has one major bug - content in the sheets is shifted down by about 20 pixels. Posting this here for people to play around with and as a backup until I fix the shifted content problem. This patch contains support for child and sibling sheets, though I haven't tested it as thoroughly as I'd like yet.

Comment 15

[Josh Aas]

Attachment: fix v1.0

Finally! Everything should work now.

Comment 16

[Mark Mentovai]

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

I see problems with sheet contention.

Sibling sheet contention  (testcase https://bugzilla.mozilla.org/attachment.cgi?id=199697) seems to be handled improperly.  This is more reliably reproduced on the green testcase.  When you mouse over green, you're supposed to get a pair of alerts ("green1 n" and "green2 n").  This occurs properly in cocoafox, but any subsequent attempt to show a sheet will hang much of the app: no sheet is displayed (a beep is heard instead), and the app waits for the user to dismiss the nonexistent sheet.  Note that when the "green2" sheet is dismissed, the computer beeps.

Parent-child sheet contention isn't working properly, either.  Testcase: visit https://verisign.com/.  A sheet will appear, in which you should click "View Certificate."  The first sheet should disappear.  Expected behavior: a new sheet appears.  Observed behavior: instead of a new sheet, a dialog is displayed.  The sheet should not be converted into a dialog.  The parent sheet should hide and be replaced by the new child sheet.  Compare the behavior in carbonfox.

Comment 17

[Josh Aas]

Attachment: fix v1.0.1

fixes sibling sheets and has some other cleanup, does not fix parent/child sheets

Comment 18

[Josh Aas]

Attachment: fix v1.1

This fixes sibling sheets, parent/child sheets, and has some other cleanup.

Comment 19

[Mark Mentovai]

Comment on attachment 224712 [details] [diff] [review]
fix v1.1

r+ on all substantive functional grounds.

>Index: nsCocoaWindow.h
>+  NSWindow*            mSheetWindowParent; // if this is a sheet, this is the window it is attached to

How many things does "this" refer to?  How about "if this is a sheet, the NSWindow it's attached to"?

>Index: nsPIWidgetCocoa.idl
> [scriptable, uuid(97e466b1-967f-40fd-81d1-577eaa2ac4d3)]

Bump uuid.

>+  // If the object is a sheet, this will return the window it is attached to
>+  NSWindow GetSheetWindowParent();

This should be a readonly attribute:
  readonly attribute NSWindow sheetWindowParent;

GetSheetWindowParent can remain implemented as it is now, the getter for an attribute has the same signature as what you've already done.

>+  // Set the window that this should attach to if it is a sheet
>+  void SetSheetWindowParent(in NSWindow aWindow);

Get rid of this and the associated implementation in nsCocoaWindow.  It's never used and there's never a good reason to call it.  (That's why the attribute should be readonly.)

Comment 20

[Josh Aas]

Attachment: fix v1.1.1

Comment 21

[Mark Mentovai]

Comment on attachment 224793 [details] [diff] [review]
fix v1.1.1

>Index: nsCocoaWindow.h
>+  NSWindow*            mSheetWindowParent; // if this is a sheet, this is the window it is attached to

You didn't fix this text (see comment 19).  OK to make that change on checkin, everything else looks good and works great!

Comment 22

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 224793 [details] [diff] [review]
fix v1.1.1

sr=pink

Comment 23

[Asaf Romano (gone)]

Why is nsPIWidgetCocoa marked scriptable? (also wondering about the use of "interface NSWindow;" instead of nativeptr).

Comment 24

[Asaf Romano (gone)]

(Also, the license header is bogus).

Comment 25

[Josh Aas]

landed on trunk, mento and mano's comments addressed