Justin Dolske [:Dolske] Reporter
	Description • 17 years ago

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.

	Justin Dolske [:Dolske] Reporter
	Comment 1 • 17 years ago

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.

	Justin Dolske [:Dolske] Reporter
	Comment 2 • 17 years ago

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);
},

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 3 • 17 years ago

Might bug 395397 help here?

	Steven Michaud [:smichaud] (Retired)
	Comment 4 • 17 years ago

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.)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 5 • 17 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 6 • 17 years ago

Alternatively, can we not open this as a sheet, but as a normal window? Does mac do modality without using sheets?

	Steven Michaud [:smichaud] (Retired)
	Comment 7 • 17 years ago

> 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).

	Johnny Stenback (:jst)
	Comment 8 • 17 years ago

(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.

	Steven Michaud [:smichaud] (Retired)
	Comment 9 • 17 years ago

> 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.

	Josh Aas Assignee
	Comment 10 • 17 years ago

Sounds like we better make it not a sheet.

	Steven Michaud [:smichaud] (Retired)
	Comment 11 • 17 years ago

OK.

The next question is, how do we distinguish modal dialogs that come
from showModalDialog() from the other modal dialogs?

	Johnny Stenback (:jst)
	Comment 12 • 17 years ago

(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

	Colin Barrett [:cbarrett]
	Comment 13 • 17 years ago

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.

	Johnny Stenback (:jst)
	Comment 16 • 17 years ago

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

:(

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 17 years ago

jst, why not?

	Johnny Stenback (:jst)
	Comment 18 • 17 years ago

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

	Josh Aas Assignee
	Comment 19 • 17 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 17 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 17 years ago

Maybe nsCocoaWindow::SetModal needs to actually do something?  It doesn't so much seem to...

	Josh Aas Assignee
	Comment 22 • 17 years ago

Yeah, Steven and I talked about this last week and he is working on it. Thanks for taking a shot though!

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 23 • 17 years ago

Shouldn't we differentiate between the different types of dialogs as Apple describes in its HIG?

http://developer.apple.com/documentation/UserExperience/Conceptual/OSXHIGuidelines/XHIGWindows/chapter_17_section_6.html#//apple_ref/doc/uid/20000961-CJECBHJE

	Steven Michaud [:smichaud] (Retired)
	Comment 24 • 17 years ago

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.

	Steven Michaud [:smichaud] (Retired)
	Comment 25 • 17 years ago

> By the way, my patch also fixes bug 403924.

It's bug 403942 that it fixes.

	Steven Michaud [:smichaud] (Retired)
	Comment 26 • 17 years ago

Here's a link to a tryserver build made using this patch:

https://build.mozilla.org/tryserver-builds/2008-01-25_15:07-smichaud@pobox.com-bugzilla395465/smichaud@pobox.com-bugzilla395465-firefox-try-mac.dmg

	Steven Michaud [:smichaud] (Retired)
	Comment 27 • 17 years ago

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.

	Steven Michaud [:smichaud] (Retired)
	Comment 28 • 17 years ago

Here's tryserver build made with my rev1 patch:

https://build.mozilla.org/tryserver-builds/2008-01-26_10:53-smichaud@pobox.com-bugzilla395465-rev1/smichaud@pobox.com-bugzilla395465-rev1-firefox-try-mac.dmg

	Steven Michaud [:smichaud] (Retired)
	Comment 29 • 17 years ago

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.

	Steven Michaud [:smichaud] (Retired)
	Comment 30 • 17 years ago

A tryserver build should follow in about an hour.

	Steven Michaud [:smichaud] (Retired)
	Comment 32 • 17 years ago

Tryserver build made using rev2 (i.e. rev3 plus a little debug
logging):

https://build.mozilla.org/tryserver-builds/2008-01-28_10:04-smichaud@pobox.com-bugzilla395465-rev2/smichaud@pobox.com-bugzilla395465-rev2-firefox-try-mac.dmg

	Damon Sicore (:damons)
	Comment 33 • 16 years ago

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.

	Mike Schroepfer
	Comment 34 • 16 years ago

(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?

	Steven Michaud [:smichaud] (Retired)
	Comment 35 • 16 years ago

> 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.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 36 • 16 years ago

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)

	Josh Aas Assignee
	Comment 37 • 16 years ago

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.

	Josh Aas Assignee
	Comment 38 • 16 years ago

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.

	Josh Aas Assignee
	Comment 39 • 16 years ago

Sorry, the fix rev4 patch was generated incorrectly (left out a file). This is a full patch.

	Josh Aas Assignee
	Comment 40 • 16 years ago

little more cleanup, saving my work

	Josh Aas Assignee
	Comment 41 • 16 years ago

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.

	Mike Schroepfer
	Comment 42 • 16 years ago

(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?

	Josh Aas Assignee
	Comment 43 • 16 years ago

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.

	Johnny Stenback (:jst)
	Comment 44 • 16 years ago

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.

	David Baron :dbaron:
	Comment 45 • 16 years ago

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.)

	Damon Sicore (:damons)
	Comment 46 • 16 years ago

Steven is going to do a review ASAP.

	Steven Michaud [:smichaud] (Retired)
	Comment 47 • 16 years ago

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.

	Steven Michaud [:smichaud] (Retired)
	Comment 48 • 16 years ago

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.

	Steven Michaud [:smichaud] (Retired)
	Comment 49 • 16 years ago

> 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.

	Johnny Stenback (:jst)
	Comment 50 • 16 years ago

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.

	Josh Aas Assignee
	Comment 51 • 16 years ago

Should fix the context menu issue, needs more testing. Steven, can you help me test when you review?

	Steven Michaud [:smichaud] (Retired)
	Comment 52 • 16 years ago

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.

	Steven Michaud [:smichaud] (Retired)
	Comment 53 • 16 years ago

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).

	Steven Michaud [:smichaud] (Retired)
	Comment 54 • 16 years ago

In case I didn't make it sufficiently clear:

Josh's new patch (rev8) _does_ fix the context menu problem.

	Josh Aas Assignee
	Comment 55 • 16 years ago

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.

	Steven Michaud [:smichaud] (Retired)
	Comment 56 • 16 years ago

> 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.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 57 • 16 years ago

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?

	Steven Michaud [:smichaud] (Retired)
	Comment 58 • 16 years ago

> 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.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 59 • 16 years ago

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?

	Josh Aas Assignee
	Comment 60 • 16 years ago

landed on trunk, please file other bugs on any followup issues

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 61 • 16 years ago

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?