Closed Bug 83643 Opened 23 years ago Closed 23 years ago

Dialogs are app-modal on Mac OS X

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Platform:
PowerPC
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.9

People

(Reporter: hsivonen, Assigned: danm.moz)

References

Details

Attachments

(4 files, 5 obsolete files)

Editor: Format->Text-Color sheet
54.29 KB, image/png
Details
Editor: Save Changes? sheet
39.24 KB, image/png
Details
Browser: Open Location sheet
39.37 KB, image/png
Details
Diffs of latest hand-merging to tip
23.76 KB, patch
mikepinkerton
: review+
bugs
: superreview+
Details | Diff | Splinter Review 
Build ID: 2001-05-30 FizzillaCFM

Steps to reproduce:
1) Open a document in Editor.
2) Make a selection (within a block)
3) Click the link button in the toolbar

Actual results:
You get an app-modal dialog. (You can go to a browser window to copy an URL, for example.)

Expected results:
Expected a window-modal sheet to slide from under the title bar. (Or at least expected the dialog to be 
window-modal.)
This is not specific to Composer, and is happening *only* on the Mac.
should go to xpfe then
Assignee: beppe → blakeross
Component: Editor → XP Apps: GUI Features
QA Contact: sujay → sairuh
This bug should probably go to danm or sfraser

Um, all dialogs are app-modal on Mac. Now, the fact that they are app-modal, and 
not window-modal on Mac OS X is a toolkit bug.
Assignee: blakeross → danm
Component: XP Apps: GUI Features → XP Toolkit/Widgets
Summary: Editor dialogs are app-modal (should be window-modal) → Dialogs are app-modal on Mac OS X
QA Contact: sairuh → jrgm
Whiteboard: OSX
Bug 88709 is related to this one if not a duplicate.
Target Milestone: --- → mozilla0.9.4
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Target Milestone: mozilla0.9.5 → mozilla0.9.6
danm, how hard do you think it would be to make dialogs window modal 
for MacOS X?
Blocks: 102998
Currently we implement our own modality on Mac OS because we don't want to get 
stuck inside ::ModalDialog. We wrote it to be app modal because that's what Mac 
OS <X expects. Mozilla itself has no such requirement. I need to figure out what 
OS services we have to make a window behave the way we want without getting 
stuck, and replace the current Mac modality code with something new (#ifdef 
TARGET_CARBON, of course). Probably cribbing heavily from our gtk+ version.

Not that tough, really. This bug mostly serves as a reminder for me to get jiggy 
with OS X.
Target Milestone: mozilla0.9.6 → mozilla0.9.7
I've been playing around with "sheet" support in Mac OS X.  Here's a small set
of diffs which convert dialogs into sheets. After applying the diffs and
rebuilding widget, try the following:

(all examples are menus chosen with a browser window open)
o  Search -> Find-in-this-page
o  File -> Open-web-location
o  Bookmarks -> File-bookmarks

The changes aren't perfect, of course... as danm mentioned, modality handling
in event management needs tweaking too, for example.  However, just having some
sheets in Mozilla makes it feel more like a real Mac OS app.  :)
BTW: I should mention some of the things that aren't 100% with the current set of 
sheet-diffs.  :)  We really need a way, at the actually window creation level in 
widget-land, to distinguish between dialogs that should be made into window-modal 
sheets and dialogs that shouldn't.

For example, with these changes applied, if you have more than one user profile, 
when you run Mozilla the red splash screen with come up, and then the profile 
manager "dialog" will "sheet out" from it!  Actually, I think it looks kind of 
cool, but the behavior is wrong from a UI perspective.  (I'm sure that there are 
other dialogs with this incorrect behavior.)

At the time of actually creating a window, if we basically could get to the 
"options" parameter of a window|dialog.open() call (or the equivalent), the code 
could make smarter decisions about what to make a sheet and what not to.

Screenshot please!

Screen shot (PNG) of Editor's Format->Text-Color dialog, morphed into a "sheet"
on Mac OS X.  (Opening/closing animation not shown, of course!	<grin>)
Another screen shot (PNG) of Editor's Save Changes dialog, morphed into a
"sheet"
on Mac OS X.  (Opening/closing animation not shown, of course!	<grin>)
One last screen shot (PNG) of Navigator's Open Location dialog, morphed into a
"sheet" on Mac OS X.  (Opening/closing animation not shown, of course!	<grin>)
  Nifty. Thanks for taking this up, Robert! As a, um ... not-yet participating
member of the OSX community, I wonder what menubar behaviour is expected from
sheet dialogs. Namely, will this magically fix bug 21296?
  I'm not sure what to do about the peculiar API requirements, though. We
already have a window.open option that describes the dialog's relationship with
its parent ("dependent"). You're suggesting we add another one, ignored by all
but OSX. "sheet" or maybe more generically, "attached". It's possible but I'd
like to try everything else first.
  Since modal implies dependent perhaps we do a cleanup pass on our XUL and use
that. Or maybe hit it from the other angle: only certain window types (browsers,
composers ... nearly everything but not, for example, the splash screen!) can
take a dependent dialog. Much as I like the idea of a user profile dialog sheet,
it seems we could stop at least that one. What others?
Danm:  yeah, for yucks, I played around with adding a "sheet" parameter to the
window.open option. However, I don't believe that's a requirement though.
Basically, the problem is that at the widget (i.e. window) creation level, it
doesn't have access to the list of window.open options, it only has access to a
different structure (chrome flags or something like that, forget at the moment
exactly what its called) from which it has to basically "guess".  If we could
shoehorn a bit more info in there, we'd be golden.

I'll come and bug you after macdev today.  <grin>  We can talk about modality
event handling.  :)

Does this patch make all dialogs into sheets? I think that some dialogs like
preferences, cookie manager, open location, and all other dialogs that apply to
more than one window should not be sheets.
Here's a new set of diffs for sheet support on Mac OS X.  Works pretty well!!!
... including strong modality handling (ONLY modal dialogs are morphed into
sheets), multi-level sheets function properly, better focus handling, and even
includes some work-arounds for other modality bugs that have been in Mozilla
for a long time.

Anyone bold enough to run with these latest changes a bit?  I'd like a
functionality review from someone who has actually ran with the changes for a
while. :)
Attachment #58669 - Attachment is obsolete: true
I think it's great that someone is trying to implement this, but I think this
patch (attachment 59488 [details] [diff] [review]) takes the wrong approach. As far as I can tell, it
makes the parent window of a sheet be the frontmost window that is of the class
'kDocumentWindowClass' whatever that window is. Although this approach causes
sheets to appear, it defeats the whole purpose of sheets. Sheets are meant to be
used when there is a need for a dialog box that only applies to one document
window in the application. The parent of a sheet should be the window to which
the content of the sheet applies, not just any arbitrary document window. For
example, if the sheet implements a save confirmation dialog, its parent should
be the window that contains the document that would be affected, not some other
window that contains some other document.

*** You cannot assume that the frontmost window is the window to which the
dialog applies. For example, a page loading in a window that is behind other
browser windows may bring up a cookie confirmation dialog. That dialog should be
associated with the window loading the page that is trying to set the cookie,
not some other window that happens to be in front of it.

Also, as I said before, sheets should only be used to implement dialogs that
apply to only one window. Dialogs, even modal dialogs, that apply to multiple
windows like preferences should not be implemented using a sheet.

Finally, there are many Mozilla windows which are not modal and are considered
top-level (like Page Info) that should be sheets. Your patch does not account
for these.
nsAppShellService::JustCreateTopWindow() has the parent window in "aParent"... it 
just needs to be passed up or registered through the call chain. That's not a 
problem, it just needs to be done... and its in the plan.  :)

Its the caller's responsibility (not the widgetry code) to decide on modality. 
[Note: Currently on Mac OS X, a modal dialog in Mozilla is modal to the entire 
app.  These diffs just scope the dialog modality to a window.]

Running Mozilla with these diffs applied, one would notice that the "Preferences" 
dialog is NOT modal because that dialog isn't opened with a modal flag.  The same 
is true for "Page Info".  (As an aside, why should "Page Info" be modal?  As an 
example, "File Info" in the Finder is not modal either.)

> nsAppShellService::JustCreateTopWindow() has the parent window in "aParent"...
> it just needs to be passed up or registered through the call chain.


To be more clear:  the parent reference is already sort of passed up through the 
creation hierarchy, but seems to be getting munged somewhere on Mac. The issue 
just needs to be cleared up.  :)

  On the Mac platform, the parent window is faithfully carried up to the point
of nsWindow::StandardCreate. It's dropped there because that method shares the
parent's native window pointer as if it were not an entirely different top-level
window in its own right. It's part of the Mac and Windows windows concept confusion.
  It's right at the nsMacWindow level, where Robert is playing, that the parent
story starts getting mixed up. I agree with ajfeldman; we need to get that
straightened out and useable rather than rely on the frontmost window.
  Quick eyeballing of the patch though, seems generally nice.
yes, nsWindow::Create is broken in this respect. conrad and i were just talking
about it last week, and i think he was planning on fixing it once he was freed
up from landing pro7. 

the way it should be implemented is in mozilla/widget/src/cocoa/nsChildView.mm's
version of Create(...)
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Latest sheet changes with workarounds for parenting/clipping issues in
widget-land.
Attachment #59488 - Attachment is obsolete: true
We probably should have a centralized place for the OnMacOSX() function so that
we don't create bloat by having multiple copies of it in different places. I
have no idea where that place should be though.
Sorry to be a pain, but I am curious about a couple of things. Just to prove
I've been paying attention.

Why'd you have to add kEventWindowUpdate/DrawContent to nsMacWindow's event
handler? Why now if not before?

The thing with the global OnMacOSX() function. You're defining it in
nsAppShellService and nsWindow. But you're also using it in nsMacWindow,
seemingly without having given a prototype. Obviously I'm missing something
there. And one of them is static while the other seems shared. Be nice to clean
that up a bit.

I'm worried about passing the parent in to nsWindow::StandardCreate from
nsMacwindow::StandardCreate without adjusting for it in the base class. See
comment 22 for previous thoughts about that. I do notice you've adjusted
nsWindow::GetParent. Is that really all that's necessary? I know you're going to
say yes. I'm surprised a bit.

In your adjustment to nsMacWindow::Show, you added a call to ::BringToFront.
Could you try it with ComeToFront; see if that's sufficient? The former is more
severe. The latter is the preferred method: it takes layering into account. I
forget why Show() already had to resort to BringToFront (popups, yeah) but it's
to be avoided if possible.

I do have to complain about indentation style agnosticism. Please do make those
new member variables in nsMacWindow.h match up with their siblings.
Otherwise, it seems good to me from a general widget/interface and code goodness
kind of level, though see . Of course I'm going to defer to a real Mac reviewer.

The patch seems good to me from a general widget/interface and code goodness
kind of level. My only real worry is the parent thing. It should be obvious if
that's not working. Give my other worries a nice pat on the back and I'll r= it.
Of course I'm going to defer to a real Mac reviewer.
>  Why'd you have to add kEventWindowUpdate/DrawContent to nsMacWindow's
> event handler? Why now if not before?

These events are now needed as they are called by the OS to draw the contents of 
the window/dialog into an offscreen bitmap so that the sheet can be made to 
visually "roll out".


> OnMacOSX()

If someone has a suggestion about where this could nicely live, I'm all ears. 


> I'm worried about passing the parent in to nsWindow::StandardCreate from
> nsMacwindow::StandardCreate without adjusting for it in the base class.
> I do notice you've adjusted nsWindow::GetParent. Is that really all that's
> necessary? I know you're going to say yes.

Well...  as far as I can tell...  yes.  :)

Of course, you'll note that "mIsTopWidgetWindow" is used in a few other places to 
break out of loops for clipping, etc.



> ::BringToFront  vs   ComeToFront

I'm merely continuing the usage that Show() already employed;  as you mentioned, 
that it was doing it for a reason (such as rolling up popups).



> I do have to complain about indentation style agnosticism.

The editor is screwing with me.  I'll certainly clean up an indentation problems 
that you point out.



> Give my other worries a nice pat on the back and I'll r= it. Of course I'm
> going to defer to a real Mac reviewer.

If you r= it, the plan is to get an sr= from sfraser.

I'd like to see an r= from pinkerton before I give the sr.

With the way this is used:
+nsMacWindow::UpdateWindowMenubar
Will it be possible to paste into an edit field in a sheet?

Conrad:  yes, using  cmd-V  but no, not from Edit->Paste.

While that is obviously bad, realize that previous to these changes all the
menus were unusable anyway when a modal dialog was being shown. The situation is
the same, except that with these changes the menus are at least dimmed to
provide some visual indication that they won't work.  danm has a bug on this:  21296
  Woo hoo! Bug 21296! Yeah, I'm sort of hoping this bug could be taken as at
least a partial fix for that one. We've been (off and on) trying to figure out
what could be done about that one for years, and I'm not the only one in "we."
  Robert: about the ComeToFront vs BringToFront thing, my reading of this
function (correct me if I'm wrong) is that your new method GetWindowTop can
return a normal, non-sheet window, and that Show will now be calling
BringToFront directly on it, rather than going through ComeToFront. I forget
exactly why the BringToFront already in Show had to be added, but I'd like to
restrict its proliferation if possible. Please try this with ComeToFront. If it
doesn't work, then leave it as you have it and I'll probably have to go figure
it out when I make Mozilla layers work, sometime post 1.0.
comments:

- like danm said, fix inconsistent spacing/tabbing

- perhaps OnMacOSX() could live in nsToolkit rather than nsMacWindow. At least
there it would be in a central location for widget. I think we're still boned on
appshell. Plus, it's already in nsMacWindow.cpp, so it should be also in
nsWindow.cpp

+    // so, Mac OS X window groups might be the easiest way to do this,
+    // but barring that...  we need a way to find the topmost sheet

why not investigate using groups? Now that we do have headers that support it,
we should be using it if it is really the easiest way.

+    nsWindow *parentWindow = static_cast<nsWindow *>(static_cast<nsIWidget
*>(windowWidget));
+    if (!parentWindow)  return;
+    nsMacWindow *parentMacWindow = static_cast<nsMacWindow*>(parentWindow);
+    if (!parentMacWindow) return;
+    nsIMenuBar *menubar = parentMacWindow->GetMenuBar();

this is icky. add GetMenuBar() to nsPIWidgetMac.

+
PRBool          mIsTopWidgetWindow;

your diff is confused, i think. is this in nsWindow.h or nsMacWindow.h? It
should be in the former, but the diff says the latter.

Pink, review?	Note that...

o OnMacOSX() now lives in nsToolkit.
o can't use window groups, as the APIs don't exist in Mac OS 9 Carbon
o added GetMenuBar() to nsPIWidgetMac
o mIsTopWidgetWindow is in nsWindow.h
o I switched over to using ComeToFront() as danm wanted, as I noticed no
problems; pink, speak up if you know of why that shouldn't be done
Attachment #63609 - Attachment is obsolete: true
Several comments in no particular order:
1. We either need BringToFront() or we need to fix ComeToFront(). The reason for
this is that although popups appear in front of everything else, more recent
popups appear behind less recent popups if we don't use BringToFront().
Hierarchical menus like the bookmarks menu are a good example of this problem
because you can see that submenus will appear behind instead of in front of
their parent menus.

2. I tried this patch and it makes many dialogs which should not be sheets into
sheets such as cookie/image manager, text zoom, open location, and probably a
lot of others.
 
3. I think bug 21296 blocks this bug because sheets are much less useful if the
menu bar is inaccessible while they are visible. The whole point of sheets that
only the affected window is blocked until the user enters input while the rest
of the app is not.

4. Including the call to Gestalt in OnMacOSX in nsAppShellService creates a link
error, at least in the mach-o build, because I think appshell is not intended to
link to any native libraries.
o  I will defer to pink/danm on whether to use BringToFront() or ComeToFront()

o  I agree that cookie windows/dialogs should not be modal, and that's a simple
bug to be fixed; open up a bug and assign it to "morse@netscape.com". However, I
disagree regarding others (such as "Open Location" which I believe is most
appropriate as a sheet since by default it applies to the current window. 
Remember, what used to be a modal-to-the-app dialog is now a sheet which is
modal to just its parent window, so its a far better world that previously. 
I'll say it YET AGAIN: this bug fix only enables sheets, it doesn't decide on
what should be or should not be a sheet; that is left up to the caller.

o Bug # 21296 does NOT block this bug. (In fact, when a sheet is up, you can
activate another window.)

o If there is an issue with macho, I'm sure that pink will be more than willing
to help with it.
haven't yet looked at the patch but..

o i agree with rjc that 'open location' should be a sheet
o if it is really easy to knock down a bunch of dialogs that shouldn't be
sheets, i think it's appropriate to do so before landing to avoid the hassle and
administrative mumbo-jumbo. just make it better.
o when i review the patch, i'll look at btf vs ctf, but it sounds from ajfeldman
that we need to use comeToFront for the hierarchical popups...
The issue of dialogs that shouldn't be sheets being sheets is very important.
Either we need to change how we decide which dialog gets to be a sheet or we
need to fix all the callers. There are many dialogs that shouldn't be sheets
that become sheets. Those that are mentioned are only few of probably many
examples. Another example is the 'add card to address book' dialog. We really
need to fix this before checking this in because otherwise this patch makes
things worse, not better.

(At the risk of starting a ui holy war, I really think open location shouldn't
be a sheet because it can open new navigator and composer windows. Remember, you
should be able to use the open location dialog even if no navigator windows are
open. By the way, using open location with no open windows breaks the current
patch because it creates a sheet without a parent that can't be moved.)

The disabled menu issue is major because although you can activate other windows
by clicking on them, there is a lot you still can't do. You can't open new
windows, you can't use the 'tools' menu to select a differnt window, you can't
use the 'edit' menu to cut and paste text, ans so on. One could argue that these
are minor issues, but unless we want to check in a half-assed patch, we need to
fix these.

The link issue may be more than a simple makfile tweak because, correct me if
I'm wrong, I think appshell isn't supposed to link to native libs like carbon.
Really, this patch is independent of the disabled menu problem. As a side effect
it affects that problem in a good way, but it's not fair to hold its failure to
completely fix that problem against it.

As it stands, a "this dialog should be a sheet" flag is synthesized from other
flags. To differentiate between dialogs that should and shouldn't at a finer
grain, I think we'd have to add a new window.open feature flag; make it explicit
in the open() call. I suggest we check this patch in, attempt to collect useful
data from the resulting religious onslaught and then consider adding a new
open() flag as a later refinement.

If ComeToFront is causing problems, go with BringToFront. Somebody will have to
go fix that someday, but not soon.
> unless we want to check in a half-assed patch


ajfeldman, no one wants a "half-assed patch". (Thus, code reviews as well as the
various patch revisions already attached to this bug.) However, perhaps it would
be best to either edit your comments to remove insults or take your issues to
the Mac OS X newsgroup for more public discussion instead of adding such
comments to this bug which I'll then feel inclined to ignore.

That said, in my mind I compare the current state of the world to what we'd have
after this patch is added:

Currently:
  o a modal dialog blocks the entire app. Menu items are unusable but without
visual indication.

After:
  o a sheet is modal to only its parent window, allowing other windows/dialogs
to be switched to. While a sheet is topmost, its menu items are disabled and
visually grayed out. By using sheets, the app begins to feel more like a Mac OS
X app.


In my mind, the issue of what of what dialogs should be modal/sheeted or not
isn't best represented in this bug. As much as I would like to rid the world of
famine and plight, these sheet changes won't do either. I obviously agree with
danm's suggestions of getting people using these changes earlier rather than
later, for the reasons I've stated above, and getting various informed people
together to examine the state of the app as a whole WHILE using it to point out
problem areas.  Hopefully by doing so, we can keep religion out of it, or at
least to a minimum.

I'm not sure how this would affect other platforms, but instead of adding a flag
to window.open, could we just remove the parent reference and the
CHROME_DEPENDENT flag from from dialogs that don't need them? If that would
work, we could open a new bug to that effect.
In JavaScript, window/dialog.open() take an options parameter which includes 
"modal" and "dependant".  So, to make the cookie dialogs non-modal, it should 
just be a matter of removing said keywords.  (I'd be willing to guess that the 
various cookie dialogs shouldn't be modal on ANY platform.)

This patch makes dependent (or modal) dialogs into sheets. Unsheeting them by
making them independent or nonmodal makes them independent or nonmodal on all
platforms. And, with this patch, makes them top-level, unsheeted windows in
their own right. I think most people will agree that modal dialogs are overused
in Mozilla. I'm prepared to believe that the best answer is to make the
offending dialogs nonmodal. I'd rather do this than add a new window.open flag;
especially one that has no cross-platform meaning.

Again, pending Mike's review, I think the patch is ready to go in as it stands.
The issue of whether certain dialogs should be made nonmodal is a larger and
different one.
I just posted bug 119571 to remove unnecessary modality.
Whiteboard: OSX
Same diff as previous, but with a couple extra lines for a bug that ajfeldman
noticed where a modal dialog shouldn't be sheet'ed if it doesn't have a parent
window (or, more specifically, don't allow the hidden window to be considered a
parent)
Attachment #64452 - Attachment is obsolete: true
Pink:  care to review this?
Comment on attachment 64784 [details] [diff] [review]
Same diff as previous, but with a couple extra lines for bug fix

r=pink

be careful updating to the tip, you'll probably get conflicts with some of the
stuff i landed today (notably the SetWindowGroup popup stuff and adding an
extra case of OnOSX in nsMacWindow).
Attachment #64784 - Flags: review+
Pink, thanks.  (Indeed, I saw your changes go in... I'll merge 'em.)

Simon, you are the man now!  :)  Need an sr.
I need to see a patch to the tip. There is lots of cruft in that diff, like

+    if ( mWindowType == eWindowType_popup ) {
+      // OSX enforces window layering so we have to make sure that popups can
+      // appear over modal dialogs (at the top of the layering chain). Create

Danger Will Robinson!

+      else if ( mWindowType == eWindowType_toplevel || mWindowType ==
eWindowType_invisible ) {
+        // enable toolbar collapse/expand box 
+        ::ChangeWindowAttributes(mWindowPtr, kWindowToolbarButtonAttribute, 0L );
+
+        // Setup the live window resizing
+        WindowAttributes removeAttributes = kWindowNoAttributes;
+        if ( mWindowType == eWindowType_invisible )
+          removeAttributes |= kWindowInWindowMenuAttribute;     
+        ::ChangeWindowAttributes ( mWindowPtr, kWindowLiveResizeAttribute,
removeAttributes );
+    }
+

these changes are VERY VERY old. be careful and don't just blindly put them back
into nsMacWindow. That code has been totally rewritten.
Pink: I don't understand your last round of comments, that's not code I've added
or even touched.
then why is it in your patch?
Something that you changed recently which has gotten out of sync?
Guess I'll remerge that file by hand.

New set of diffs coming up shortly.

Ah, the joys of diffing.  Here's another diff, hand-merged with fresh files
from the tip.

BTW: What Pink was pointing out appeared to be a combination of small
indentation changes with a touch of bitrot.

Here 'ya go, Simon.
Attachment #64784 - Attachment is obsolete: true
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Comment on attachment 65145 [details] [diff] [review]
Diffs of latest hand-merging to tip

r=pink on attachment 65145 [details] [diff] [review]
Attachment #65145 - Flags: review+
Simon:  care to review?  :)
Comment on attachment 65145 [details] [diff] [review]
Diffs of latest hand-merging to tip

sr=ben@netscape.com
Attachment #65145 - Flags: superreview+
Marking FIXED.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Cool. Thank you! (Thanks for the title bar toolbar show/hide button, too.)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: