Closed Bug 395465 Opened 12 years ago Closed 12 years ago

showModalDialog is wonky on OS X

Categories

(Core :: Widget: Cocoa, defect, P1)

All
Mac OS X
defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: Dolske, Assigned: jaas)

References

Details

Attachments

(1 file, 9 obsolete files)

I was taking a driveby look at bug 393900 (showModalDialog doesn't show a location bar, making spoofing trivial), and noticed that Jesse's testcase (attachment 278445 [details]) acts very oddly on OS X.

The sheet being used for the dialog covers the existing window, hiding the toolbars and such. You can't dismiss it, as there are no buttons. Trying to close the window seems to actually minimize it, and restoring it brings the window back but leaves the icon in the dock. The restored window often looks wierd, with content shifted to the right and the scrollbar hanging out past the title bar.

You can also use the History menu to load different sites in this weird page/dialog, but after talking with jst that probably isn't a problem.
Oddly enough, I just ran across some of the same behavior in the Firefox Preferences window...

1) Open Preferences
2) Go to Content tab
3) Click Advanced in the Fonts&Colors section
4) Click the yellow "minimize" button  in the prefwindow title bar

The sheet disappears, but the button stays depresses, and from this point on the window is acting "wonky" again.
Flags: blocking1.9?
The pref code triggered at step 3 is in browser/components/preferences/content.js...

configureFonts: function ()
{
  document.documentElement.openSubDialog(
    "chrome://browser/content/preferences/fonts.xul",
    "",
    null);
},
Assignee: joshmoz → smichaud
I did some quick tests that suggest my new appshell patch (bug 395397)
doesn't make any difference here.

The worst problem (to my mind) is that the modal sheet doesn't give
you any GUI to close it (though File : Close works just fine).  But
I'm not sure if it's possible for a sheet to have a close button.
(The buttons that you see actually belong to the "normal" window
underneath the sheet.)
Maybe we need to have some default chrome on mac that allows you to close these then? On windows (and I assume linux) you can use the normal close button since it's a separate window.
Alternatively, can we not open this as a sheet, but as a normal window? Does mac do modality without using sheets?
> Alternatively, can we not open this as a sheet, but as a normal
> window? Does mac do modality without using sheets?

Safari opens the modal window from Jesse's testcase as an ordinary
(separate) modal window.

But OS X Firefox has consistently used sheets for modal dialogs for
quite some time.  For example it also does so on the 1.8 branch (one
example is the modal dialog you get when you try to Quit and at least
one of your windows has two or more tabs).  Surely now isn't the time
to change this.

In ordinary usage, modal dialogs have at least an OK button, and can
be dismissed by either clicking on that button or pressing the ESC
key.  If that's all it takes to make the wierdness reported in this
bug go away, I suspect this bug should be marked "invalid", or put
aside for some future version of the browser.

But I haven't yet had a chance to test this (to see if the wierdness
goes away when you add at least one button).
(In reply to comment #7)
> > Alternatively, can we not open this as a sheet, but as a normal
> > window? Does mac do modality without using sheets?
> 
> Safari opens the modal window from Jesse's testcase as an ordinary
> (separate) modal window.
> 
> But OS X Firefox has consistently used sheets for modal dialogs for
> quite some time.  For example it also does so on the 1.8 branch (one
> example is the modal dialog you get when you try to Quit and at least
> one of your windows has two or more tabs).  Surely now isn't the time
> to change this.
> 
> In ordinary usage, modal dialogs have at least an OK button, and can
> be dismissed by either clicking on that button or pressing the ESC
> key.  If that's all it takes to make the wierdness reported in this
> bug go away, I suspect this bug should be marked "invalid", or put
> aside for some future version of the browser.

The whole point here is that the modal dialogs opened thourh showModalDialog() come from webpages and are completely out of our control. We shouldn't change how we do normal modal dialogs, but we *must* change how we do modal dialogs that come from showModalDialog() for that reason. If safari opens a normal window as the modal dialog, then we better do that too. And now *is* the time to change that, this is a new feature in gecko 1.9.
> The whole point here is that the modal dialogs opened thourh
> showModalDialog() come from webpages and are completely out of our
> control. We shouldn't change how we do normal modal dialogs, but we
> *must* change how we do modal dialogs that come from
> showModalDialog() for that reason. If safari opens a normal window
> as the modal dialog, then we better do that too.

This isn't something I know a whole lot about.

Josh, what do you think? :-)

> And now *is* the time to change that, this is a new feature in gecko
> 1.9.

Well if we _do_ have to change how some or all modal dialogs are
handled, the sooner the better.
Flags: blocking1.9? → blocking1.9+
Sounds like we better make it not a sheet.
OK.

The next question is, how do we distinguish modal dialogs that come
from showModalDialog() from the other modal dialogs?
(In reply to comment #11)
> OK.
> 
> The next question is, how do we distinguish modal dialogs that come
> from showModalDialog() from the other modal dialogs?
> 

See http://lxr.mozilla.org/mozilla/source/docshell/base/nsDocShell.cpp#8614
Sounds like you'd want to have the code that actually creates the window for showModalDialog() use eWindowType_dialog instead of eWindowType_sheet, to me.
Target Milestone: --- → mozilla1.9 M10
Duplicate of this bug: 402095
Priority: -- → P3
Target Milestone: mozilla1.9 M10 → ---
Attached patch This seems like the obvious thing to do (obsolete) (deleted) — Splinter Review
Assignee: smichaud → bzbarsky
Status: NEW → ASSIGNED
Attachment #291296 - Flags: superreview?(jst)
Attachment #291296 - Flags: review?(cbarrett)
Summary: showModalDialog is wonky on OS X → [FIX]showModalDialog is wonky on OS X
Comment on attachment 291296 [details] [diff] [review]
This seems like the obvious thing to do

That won't work well with this code:

http://lxr.mozilla.org/mozilla/source/docshell/base/nsDocShell.cpp#8763

:(
Attachment #291296 - Flags: superreview?(jst) → superreview-
Comment on attachment 291296 [details] [diff] [review]
This seems like the obvious thing to do

Oh, duh, this is just the sheet mask, not something that leaks out as chrome flags anywhere. Then there's no problem with this that I can see. sr=jst
Attachment #291296 - Flags: superreview- → superreview+
Attachment #291296 - Flags: review?(smichaud)
Attachment #291296 - Flags: review?(cbarrett) → review+
The problem with this patch is that it doesn't fix the issue of modality. With Jesse's test case, you get a window but not a modal one. You can switch back and forth between the two windows.
Indeed.  I'd assumed comment 13 was right.

I really don't know where to start looking for the mishandling (i.e. not making modal) of eWindowType_dialog in the cocoa widget code.  :(  Back to Steven, I guess.
Assignee: bzbarsky → smichaud
Status: ASSIGNED → NEW
Summary: [FIX]showModalDialog is wonky on OS X → showModalDialog is wonky on OS X
Maybe nsCocoaWindow::SetModal needs to actually do something?  It doesn't so much seem to...
Yeah, Steven and I talked about this last week and he is working on it. Thanks for taking a shot though!
Attachment #291296 - Attachment is obsolete: true
Attachment #291296 - Flags: review?(smichaud)
Priority: P3 → P2
Attached patch Fix (obsolete) (deleted) — Splinter Review
Here's a fix that incorporates Boris's patch (attachment 291296 [details] [diff] [review]) and
also hacks out support for modality.

OS X has native support for both modal sheets and modal
windows/dialogs.  But the way it does modal windows/dialogs is
incompatible with the Gecko modal event loop in
nsXULWindow::ShowModal().  And even the native support for modal
sheets (which _is_ compatible) is patchy, and needs some help.

So just as we've had to simulate our own context menus (because we
couldn't use the native implementation), we're going to have to
simulate modality.

I _think_ I've gotten it more-or-less right (though I'm aware of some
minor flaws).  But I'd appreciate any suggestions.

By the way, my patch also fixes bug 403924.

I should be able to post a link to a tryserver patch in an hour or
two.
Attachment #299310 - Flags: review?(joshmoz)
> By the way, my patch also fixes bug 403924.

It's bug 403942 that it fixes.
Attached patch Fix rev1 (further refinements) (obsolete) (deleted) — Splinter Review
Here's a revision that neatens things up a bit, and also fixes a
problem that only happened on OS X 10.5.X (with the patch's previous
version):

Though most of the main menu gets disabled when an app-modal dialog is
displayed, it was still possible to access certain menus via
key-combinations (e.g. command-n for new window and command-t for new
tab).

I'll post the URL to a new tryserver build in an hour or so.
Attachment #299310 - Attachment is obsolete: true
Attachment #299433 - Flags: review?(joshmoz)
Attachment #299310 - Flags: review?(joshmoz)
Blocks: 194404
Over the weekend I realized that, when (in nsCocoaWindow::SetModal())
I disable most of the main menu (except for its "Firefox" submenu) for
an app-modal window, I should also disable the "Firefox" menu's
"About" and "Preferences" submenus (because they open new windows).

I should have a rev2 that does this in an hour or two.
A tryserver build should follow in about an hour.
Attachment #299433 - Attachment is obsolete: true
Attachment #299783 - Flags: review?(joshmoz)
Attachment #299433 - Flags: review?(joshmoz)
Attached patch Fix rev3 (Oops, get rid of debug logging) (obsolete) (deleted) — Splinter Review
Attachment #299783 - Attachment is obsolete: true
Attachment #299785 - Flags: review?(joshmoz)
Attachment #299783 - Flags: review?(joshmoz)
Attachment #299785 - Flags: review?(joshmoz)
Priority: P2 → P1
Note from today's Mac Gecko Meeting:  This bug is at risk for making beta 4 due to the fact Steven is working on mochitests for focus issues and is time constrained.
(In reply to comment #33)
> Note from today's Mac Gecko Meeting:  This bug is at risk for making beta 4 due
> to the fact Steven is working on mochitests for focus issues and is time
> constrained.
> 

So what's the plan here then?  Remove showModalDialog?
> So what's the plan here then?  Remove showModalDialog?

Yes, that's my understanding -- if this patch can't be finished in
time, remove showModalDIalog.

As you can see, I've already done a lot of work on this.  But it was
rejected (by jst and josh, if I understand correctly) for the
following reasons:

1) It disables parts of the main menu.

2) It makes the showModalDialog window/dialog app-modal, as opposed to
   window-modal.

Not disabling parts of the menu will create traps for unwary users (so
some time should be spent trying to minimize the damage).  And, while
I don't know whether or not it will be possible to make non-sheet
modal dialogs window-modal, I suspect it will be possible with a lot
of hacking.

I think all this will take at least a week.  If we don't have that
week, we should probably push showModalDialog to a point release.
Gonna block on this P1, though if we can't get it in time for final beta, it'll mean backing out showModalDialog (which would require beta exposure)
Flags: tracking1.9+ → blocking1.9+
Attached patch fix rev4 (obsolete) (deleted) — Splinter Review
This is an update of Steven's patch, a work in progress. This applies to current trunk and gets rid of menu item disabling code. At least on Linux, and probably Windows, key commands for any of those items work fine and you can open new regular windows from modal windows. The result is that the modal window behaves as if it is window modal and not application modal, something I'm not sure we should be concerned about. It isn't all that pretty, but it might do for now.
Attachment #299785 - Attachment is obsolete: true
Attached patch fix rev5 (obsolete) (deleted) — Splinter Review
Further improvements. One major change is that the undocumented borderView and associated titlebar button methods are unnecessary. We can use NSWindow's "standardWindowButton:" method to get what we want.
Assignee: smichaud → joshmoz
Attachment #309249 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch fix rev6 (obsolete) (deleted) — Splinter Review
Sorry, the fix rev4 patch was generated incorrectly (left out a file). This is a full patch.
Attachment #309320 - Attachment is obsolete: true
Attached patch fix rev7 (obsolete) (deleted) — Splinter Review
little more cleanup, saving my work
Attachment #309321 - Attachment is obsolete: true
Comment on attachment 309431 [details] [diff] [review]
fix rev7

This patch fulfills the contract for the showModalDialog API. It is a modification of Steven's earlier patch which presents the modal window in an application-modal fashion.

The downside to this approach is that it is possible to open new windows while the modal window is up. Any new windows opened become modal children of the modal that created them. This can create a confusing window situation for users.

What we'd really like is to stop users from creating new windows while the modal window is up, but there isn't an easy way to do that. Disabling menus only makes it a little more difficult to open a new window and adds to the confusion of the situation because commands that should be allowed in the modal window, like copy and paste, are disabled. I spoke with jst today and we might be able to add  some code that stops window.open from working when we don't want it to on top of this patch.

Basically, we have two options. One option is to disable showModalDialog(), at the very least on the Mac. This has the downside of losing a major feature and potentially creates a platform parity problem. The other option is to take this patch along with its potentially confusing window situation and then perhaps attempt to mitigate the problem later by restricting the creation of new windows. My feeling is that we should try the latter, starting with this patch.
Attachment #309431 - Flags: review?(jst)
(In reply to comment #41)

> The downside to this approach is that it is possible to open new windows while
> the modal window is up. Any new windows opened become modal children of the
> modal that created them. This can create a confusing window situation for
> users.
> 
> What we'd really like is to stop users from creating new windows while the
> modal window is up, but there isn't an easy way to do that. Disabling menus
> only makes it a little more difficult to open a new window and adds to the
> confusion of the situation because commands that should be allowed in the modal
> window, like copy and paste, are disabled. I spoke with jst today and we might
> be able to add  some code that stops window.open from working when we don't
> want it to on top of this patch.
> 

How do other mac apps solve this problem?
You can get Safari to open new windows while showModalDialog has a dialog up by doing things like putting a link targeting a new window in the dialog page. Safari opens up the new windows as non-modal windows behind the modal window. That is a nice solution, but for us to do that we'd need to do some extra work in Gecko. When you open up a window from a modal window Gecko assumes that it is also modal, a modal child of whatever opened it.

We could take this patch for b5, which gives us coverage for much of the risk and allows us to keep the showModalDialog feature enabled. Then we could follow-up after b5 with better handling of the situation where users open new windows. Johnny had some good ideas about how to do that.
Comment on attachment 309431 [details] [diff] [review]
fix rev7

r=jst for the non-mac specific changes which I don't know enough about to review.
Attachment #309431 - Flags: review?(jst) → review+
Attachment #309431 - Flags: superreview?(roc)
Target Milestone: --- → mozilla1.9beta5
Should somebody (Steven) review the Mac changes in this patch before sr?  (If roc's not available, I could sr, but since I don't know much about the code this patch is touching, I'd like to know that somebody reviewed it.)
Steven is going to do a review ASAP.
I've gone through Josh's latest patch and didn't notice any problems.
It's basically my original patch, minus the code to disable some menus
and their keyboard shortcuts, and with a new method (documented
instead of undocumented) to grey out a window's Close, Minimize and
Zoom buttons when a modal window is displayed above it.

I also compiled the patch (on top of the current trunk code) and did
some tests, in which I discovered one fairly significant bug:  Context
menus don't work (none of their menu items are usable) for as long as
a showModalDialog window is displayed.  No doubt this is caused by a
bug in my original patch.  I'm not sure if you guys will consider it
important enough to stop this patch from landing.

I haven't seen this bug before (I didn't get that far in my original
testing, back in January).  I could probably fix this bug in 2-3 days
... but we probably don't have that much time to get this into beta5.

And here's an example of the kind of trap for unwary users that you
create when you don't disable any menus:

1) Run Minefield and open a showModalDialog-style modal dialog.

2) Choose Minefield : About Minefield -- it will open as a modal
   dialog on top of the showModalDialog window, usually at its center.

3) Click on the showModalDialog window.  The About Minefield window
   will disappear behind it.  And you won't be able to close the
   showModalDialog window, or do anything in it, until you move it
   aside and close the About Minefield window.

This isn't a bug -- it's a design flaw.  There's no reasonable way (on
OS X) to make the About Minefield window always stay on top of the
showModalDialog window (you don't have an infinite number of window
"levels").  And any window underneath a modal window should be
disabled, even if it is itself a modal window.
I didn't r+ Josh's patch.  But I'm willing to if you think the
problems I found shouldn't stop it from landing as is.
> This isn't a bug -- it's a design flaw.

I suppose one way to work around this is what Josh talked about in
comment #43 -- make windows non-modal that appear "above" modal
windows.
I think th(In reply to comment #48)
> I didn't r+ Josh's patch.  But I'm willing to if you think the
> problems I found shouldn't stop it from landing as is.

*I* think it's landable, but I'd like at least the context menu issue filed as a followup P2 blocker.
Attached patch fix rev8 (deleted) — Splinter Review
Should fix the context menu issue, needs more testing. Steven, can you help me test when you review?
Attachment #309431 - Attachment is obsolete: true
Attachment #310983 - Flags: review?(smichaud)
Attachment #309431 - Flags: superreview?(roc)
Josh, I'm pretty sure your new patch will regress my patch's fix for bug 403942.  But since Stan already's landed a workaround for that bug, perhaps that doesn't matter (at least for the time being).

I'm about to start testing.
Comment on attachment 310983 [details] [diff] [review]
fix rev8

> Josh, I'm pretty sure your new patch will regress my patch's fix for
> bug 403942.

It does.  But (like I said) this probably doesn't matter, at least for
the time being.

My tests didn't turn up any other problems (aside from the design flaw
I already mentioned in comment #47).

Yes, this is pretty ugly.  But I hope that over time it can be
improved (which may involve design changes to showModalDialog).
Attachment #310983 - Flags: review?(smichaud) → review+
In case I didn't make it sufficiently clear:

Josh's new patch (rev8) _does_ fix the context menu problem.
To summarize, my updated patch fixes the context menu issue but it doesn't fix bug 403942 like Steven's patch did. My updated patch does not regress anything that is in the tree. I think we should file a separate bug on a more correct fix for bug 403942 and take care of it separately.
Attachment #310983 - Flags: superreview?(dbaron)
> I think we should file a separate bug on a more correct fix for bug
> 403942 and take care of it separately.

I've opened bug 424424.
Comment on attachment 310983 [details] [diff] [review]
fix rev8

This looks fine ("In principal" -> "In principle"), but it seems that in SetModal, when the window is being set as no longer modal, the windowList's window should be checked to make sure that it's the right window, right?
Attachment #310983 - Flags: superreview?(dbaron) → superreview+
> but it seems that in SetModal, when the window is being set as no
> longer modal, the windowList's window should be checked to make sure
> that it's the right window, right?

There shouldn't ever be a mismatch.  But I suppose we could check for
one and do an assertion if we find it.
Whiteboard: [reviewed patch in hand]
Attachment #310983 - Flags: approval1.9b5?
Comment on attachment 310983 [details] [diff] [review]
fix rev8

a=beltzner, please do add the assertion, and also file a follow up for adding tests?
Attachment #310983 - Flags: approval1.9b5? → approval1.9b5+
landed on trunk, please file other bugs on any followup issues
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Josh, doing the STR from comment 1 don't let me minimize the preferences window when the modal dialog is open. Is this by design or should I file a new bug?
Hardware: PC → All
Whiteboard: [reviewed patch in hand]
You need to log in before you can comment on or make changes to this bug.