Closed
Bug 83643
Opened 23 years ago
Closed 23 years ago
Dialogs are app-modal on Mac OS X
Categories
(Core :: XUL, defect)
Tracking
()
RESOLVED
FIXED
mozilla0.9.9
People
(Reporter: hsivonen, Assigned: danm.moz)
References
Details
Attachments
(4 files, 5 obsolete files)
54.29 KB,
image/png
|
Details | |
39.24 KB,
image/png
|
Details | |
39.37 KB,
image/png
|
Details | |
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.)
Comment 1•23 years ago
|
||
This is not specific to Composer, and is happening *only* on the Mac.
Comment 2•23 years ago
|
||
should go to xpfe then
Assignee: beppe → blakeross
Component: Editor → XP Apps: GUI Features
QA Contact: sujay → sairuh
Comment 3•23 years ago
|
||
This bug should probably go to danm or sfraser
Comment 4•23 years ago
|
||
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
Updated•23 years ago
|
QA Contact: sairuh → jrgm
Updated•23 years ago
|
Whiteboard: OSX
Comment 6•23 years ago
|
||
danm, how hard do you think it would be to make dialogs window modal for MacOS X?
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.
Comment 8•23 years ago
|
||
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. :)
Comment 9•23 years ago
|
||
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.
Comment 10•23 years ago
|
||
Screenshot please!
Comment 11•23 years ago
|
||
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>)
Comment 12•23 years ago
|
||
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>)
Comment 13•23 years ago
|
||
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>)
Assignee | ||
Comment 14•23 years ago
|
||
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?
Comment 15•23 years ago
|
||
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. :)
Comment 16•23 years ago
|
||
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.
Comment 17•23 years ago
|
||
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
Comment 18•23 years ago
|
||
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.
Comment 19•23 years ago
|
||
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.)
Comment 20•23 years ago
|
||
> 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. :)
Assignee | ||
Comment 21•23 years ago
|
||
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.
Comment 22•23 years ago
|
||
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(...)
Comment 23•23 years ago
|
||
Latest sheet changes with workarounds for parenting/clipping issues in widget-land.
Attachment #59488 -
Attachment is obsolete: true
Comment 24•23 years ago
|
||
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.
Assignee | ||
Comment 25•23 years ago
|
||
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.
Comment 26•23 years ago
|
||
> 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.
Comment 27•23 years ago
|
||
I'd like to see an r= from pinkerton before I give the sr.
Comment 28•23 years ago
|
||
With the way this is used: +nsMacWindow::UpdateWindowMenubar Will it be possible to paste into an edit field in a sheet?
Comment 29•23 years ago
|
||
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
Assignee | ||
Comment 30•23 years ago
|
||
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.
Comment 31•23 years ago
|
||
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.
Comment 32•23 years ago
|
||
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
Comment 33•23 years ago
|
||
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.
Comment 34•23 years ago
|
||
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.
Comment 35•23 years ago
|
||
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...
Comment 36•23 years ago
|
||
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.
Assignee | ||
Comment 37•23 years ago
|
||
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.
Comment 38•23 years ago
|
||
> 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.
Comment 39•23 years ago
|
||
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.
Comment 40•23 years ago
|
||
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.)
Assignee | ||
Comment 41•23 years ago
|
||
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.
Comment 42•23 years ago
|
||
I just posted bug 119571 to remove unnecessary modality.
Updated•23 years ago
|
Whiteboard: OSX
Comment 43•23 years ago
|
||
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
Comment 44•23 years ago
|
||
Pink: care to review this?
Comment 45•23 years ago
|
||
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+
Comment 46•23 years ago
|
||
Pink, thanks. (Indeed, I saw your changes go in... I'll merge 'em.) Simon, you are the man now! :) Need an sr.
Comment 47•23 years ago
|
||
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
Comment 48•23 years ago
|
||
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.
Comment 49•23 years ago
|
||
Pink: I don't understand your last round of comments, that's not code I've added or even touched.
Comment 50•23 years ago
|
||
then why is it in your patch?
Comment 51•23 years ago
|
||
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.
Comment 52•23 years ago
|
||
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
Comment 53•23 years ago
|
||
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+
Comment 54•23 years ago
|
||
Simon: care to review? :)
Comment 55•23 years ago
|
||
Comment on attachment 65145 [details] [diff] [review] Diffs of latest hand-merging to tip sr=ben@netscape.com
Attachment #65145 -
Flags: superreview+
Comment 56•23 years ago
|
||
Marking FIXED.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 57•23 years ago
|
||
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.
Description
•