729720 - Consider using sheets for all native modal dialogs on OS X

Status: RESOLVED INCOMPLETE

(Core :: Widget: Cocoa, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

Currently we use native modal windows to implement the following menu
items on OS X:

File : Open File
File : Save Page As
File : Page Setup
File : Print

All of them can be implemented as sheets.  We should consider doing
this, for a number of reasons:

1) Native sheets are window-modal while native modal windows are
   app-modal.

   All of these menu items are page-specific (except perhaps Page
   Setup).  So ideally their modal dialogs would be page-modal
   (i.e. tab-modal).  OS X has no native support for tab-modal
   dialogs, so this isn't an option.  But at least window-modal sheets
   block only a single window, while app-modal windows block the whole
   app.

2) Stopping the use of native modal windows may allow us to work
   around bug 476541.

   More about bug 476541 in the next comment.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

At least partly for political reasons, bug 476541 is going to be very
difficult to fix.  The main reason is that we've tried to impose a
Windows-and-Unix-like design for modal windows on OS X, despite the
fact that this design doesn't work properly there.  I go into much
greater detail about this at bug 478073.

In the past I've tried to hack around these difficulties, starting at
bug 395465.  But this is very tricky work, and I was never allowed to
finish it (see bug 468393).  I'm now convinced that we need to back
out my hacks, and accept that modal dialogs work differently on OS X.

More specifically, we need to change how showModalDialog is
implemented on OS X.  We could make it display a sheet or a tab-modal
dialog.  Or if we leave it a window we'd need to make it app-modal.
But it will be difficult to get consensus on these questions from the
powers that be.

However, it may also be possible to work around bug 476541 by
converting all our system modal dialogs (the file picker and the print
dialogs) to sheets instead of windows.	I will be experimenting	with
that here.

Comment 2

[Justin Dolske [:Dolske]]

(In reply to Steven Michaud from comment #1)

> More specifically, we need to change how showModalDialog is
> implemented on OS X. 

Are you talking about the content-accessible window.showModalDialog()? [And if so, can you expand on why it matters?] I talked with some folks about that while working on the tab-modal prompt stuff. General feel seemed it's a terrible feature and would be nice to kill, but it was unclear if it was being used in rare-but-important cases (intranet sites or whatnot).

I think Frank Yan suggested we just put it behind an annoying opt-in notification bar (like popup blocking).

> We could make it display a sheet or a tab-modal
> dialog.  Or if we leave it a window we'd need to make it app-modal.
> But it will be difficult to get consensus on these questions from the
> powers that be.

Not sure who these "powers that be" are. But I would suggest turning that around, and starting with figuring out what our options are for doing all this stuff in a manner that doesn't cause hangs or crashes, and then we can work towards what should be done. Seems like the current architecture is hang-prone, and that's not really a viable long-term thing.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #2)

> Are you talking about the content-accessible
> window.showModalDialog()?

Yes.

> [And if so, can you expand on why it matters?]

The hacks that I wrote to support our current implementation of
showModalDialog on OS X, which I was never allowed to complete,
(probably) cause the hangs reported at bug 476541.  To fix the hangs
we need to back out the hacks.  To back out the hacks we need to
change how showModalDialog is implemented on OS X.

That said, we might also be able to work around these hangs by doing
what I suggest here (at bug 729720).

> General feel seemed it's [showModalDialog is] a terrible feature and
> would be nice to kill, but it was unclear if it was being used in
> rare-but-important cases (intranet sites or whatnot).

Times have changed since you wrote bug 395465 comment #0 :-)

If I remember correctly, adding support for showModalDialog was once a
big deal, in order to achieve parity with IE (yes IE, this was a few
years ago).  This probably means we can't drop support for it
altogether.  But I hope there will now be less resistance to
implementing it differently on OS X than on other platforms.

Comment 4

[Mounir Lamouri (:mounir)]

FWIW, I have a patch that tries to use sheets instead of modal dialog for the filepicker but the main issue I had was that the filepicker API is sync but the sheet API assumes that we go back to Cocoa's event loop. I gave up with that patch but we will change the filepicker API to make it async and in that case, it would be very easy to use sheets for the filepicker I believe.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> the main issue I had was that the filepicker API is sync but the
> sheet API assumes that we go back to Cocoa's event loop

You're absolutely right.  Thanks for reminding me.

This does complicate things.  But it should be possible to get around
the problem by nesting another instance of the Cocoa event loop
(something we're already doing in nsAppShell.mm).

I'll still try to write a proof-of-concept patch, just to see if I'm
right about this being one way to work around bug 476541.  But it may
turn out to be simpler just to fix bug 476541 (i.e. to implement
showModalDialog in an OS-X-friendly way, and to back out the hacks
that support our current non-friendly implementation).

> we will change the filepicker API to make it async

Do you have any idea when this might happen, or where it's being
worked on (which bug)?

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to Steven Michaud from comment #5)
> > we will change the filepicker API to make it async
> 
> Do you have any idea when this might happen, or where it's being
> worked on (which bug)?

I could work on that. It would be easy to change the webfacing filepicker to make it async but all internal uses will have to be audited and some might required some fixes. I don't know how hard that part will be. I also don't know how other platforms would behave with an async file picker. I don't think there is a bug open. Though, if you think that could make your work easier, we could open a bug and I could try to dedicate some cycles on it.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #6)

Thanks for the information.

By "async filepicker API" do you mean something like the following?

A callback gets added to the current webfacing filepicker API, and
it's changed to return immediately.  Then when results are available
from the filepicker, the callback gets called with those results.

Yes, this sounds pretty	straightforward, and should work on any
platform.  But it'd (presumably) require changes to web	pages, which
would (presumably) take a long time to happen.	(And the old
synchronous API would need to remain available during the transition.)

> I don't think there is a bug open. Though, if you think that could
> make your work easier, we could open a bug and I could try to
> dedicate some cycles on it.

I think an async filepicker API is a good idea in the medium to long
term.  But I don't know if it'll come soon enough to be much help with
this bug.

Comment 8

[Mounir Lamouri (:mounir)]

No, web contents will not have to change. The idea is when the filepicker is shown, we will not block script executions and when a file will be selected, a change event will be fired (as it already does). FWIW, Chrome does that so this must be safe regarding web compat.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #8)

Oh, OK.  Thanks.  This is good news.

This would definitely make my work on this bug easier, so I'd appreciate it if others could start working on the async filepicker stuff.  I should be able to help in that work ... at least in testing on the Mac.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Attachment: Proof of concept patch

Here's a proof-of-concept patch.

It has some UI problems, which may not be repairable.  But I think it
should fix the modal dialog hangs on OS X (i.e. bug 476541).

Problem is I can't really test for that, since I haven't been able to
reproduce the most common cause of the hangs --	an "unresponsive
script" dialog appearing while a filepicker or print dialog is still
open.

So I'll need to rely on others to test my patch.  I'm currently doing
a tryserver build, which should be available in a few hours.

When it's done I'll announce my patch in bug 729328 and bug 718542
(and bug 476541), and ask people to test it.

The most significant UI problem is that the native filepickers and
print dialogs are still (sort-of) app-modal, though sheets are
normally window-modal.  This is due to the way nsIFilePicker::Show is
currently implemented, in combination with the fact that OS X has
(basically) just one event loop (unlike Windows and XWindows on Linux,
which have one event loop per window).

One way to (partially) work around this would be to have two different
implementations of nsIFilePicker::Show -- the current synchronous one
for web-facing use, and another asynchronous one with a callback for
"internal" use.

This, by the way, appears not to be what Mounir is suggesting at bug
731307.  At some point I'll open a new bug on it.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my patch from comment #10:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b1bb6a8c1a89/try-macosx64/firefox-13.0a1.en-US.mac.dmg

Whoever can reproduce modal dialog hangs in current nightlies, please try it out and let us know your results.

In fact I'd like you to live with this build for a day or two, and try every conceivable way you can think of to trigger one of the modal-dialog hangs.  If you do see a hang, though, please remember how it happened, and report the steps-to-reproduce here.

Comment 12

[Markus Stange [:mstange]]

Here's an easy way to trigger a delayed hang on demand: Open the Web Console (Cmd+Option+K) and enter this code:
function hangfor(numSeconds, afterDelay) { setTimeout(function () { var startTime = Date.now(); while (Date.now() - startTime < numSeconds * 1000) {}; }, afterDelay * 1000); } hangfor(3, 5)

Comment 13

[Nomis101]

(In reply to Steven Michaud from comment #10)
> But I think it
> should fix the modal dialog hangs on OS X (i.e. bug 476541).

Will it also fix Bug 482811?

Comment 14

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #12)

The web console and error console don't accept that syntax.

They both do accept the following syntax, which does trigger a hang.  But for some reason the "unresponsive script" dialog still doesn't appear.

function hang(numSeconds) { var startTime = Date.now(); while (Date.now() - startTime < numSeconds * 1000) {}; }; function hangfor(numSeconds, afterDelay) { setTimeout(hang(numSeconds), afterDelay); }; hangfor(15, 5);

Comment 15

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #14)

Actually even this syntax isn't quite right.  I get the following error:

Error: useless setTimeout call (missing quotes around argument?)

And the hang starts immediately -- there isn't any delay.

I *did* start seeing the "unresponsive script" dialog, though, after I set 'numSeconds' to '25'.

Comment 16

[Hal Wine [:hwine] use NI!]

(In reply to Steven Michaud from comment #11)
> Whoever can reproduce modal dialog hangs in current nightlies, please try it
> out and let us know your results.

Woot! Initial runs shows I can not hang either the print "save as" dialog or the "open file" dialog. I even waited 20 min for "open file".

One note - the print-save-pdf-as path is very clean. The save-as dialog still displays as the normal modal dialog users are familiar with.

The open-file path has a different visual appearance (because it's a sheet), and pegs one CPU at 100%. <= not at all complaining this is 10,000 times better than a hang!!!! :-D :-D
> 
> In fact I'd like you to live with this build for a day or two, and try every
> conceivable way you can think of to trigger one of the modal-dialog hangs. 
> If you do see a hang, though, please remember how it happened, and report
> the steps-to-reproduce here.

My pleasure! Thanks, Steven!

Comment 17

[Steven Michaud [:smichaud] (Retired)]

> Woot! Initial runs shows I can not hang either the print "save as"
> dialog or the "open file" dialog. I even waited 20 min for "open
> file".

Very glad to hear it!

> The open-file path has a different visual appearance (because it's a
> sheet), and pegs one CPU at 100%.

I think I know the cause.  I'll post a revised patch shortly (probably
tomorrow).

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Attachment: Proof of concept patch rev1 (fix CPU pegging)

Here's a patch that should fix the CPU pegging.

And here's a tryserver build made from it:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-267946635e05/try-macosx64/firefox-13.0a1.en-US.mac.dmg

Please try it out!

Comment 19

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #10)

> The most significant UI problem is that the native filepickers and
> print dialogs are still (sort-of) app-modal, though sheets are
> normally window-modal.  This is due to the way nsIFilePicker::Show
> is currently implemented, in combination with the fact that OS X has
> (basically) just one event loop (unlike Windows and XWindows on
> Linux, which have one event loop per window).
>
> One way to (partially) work around this would be to have two
> different implementations of nsIFilePicker::Show -- the current
> synchronous one for web-facing use, and another asynchronous one
> with a callback for "internal" use.

I've found another significant UI problem:  If you have any of the
native modal sheets open (the filepickers or the print dialogs),
attempting to close a second window with more than one tab will fail
with a beep.  (By "second window" I mean a different window than the
one the native modal sheet is open above.)

This is because Gecko modal dialogs are currently all synchronous, and
I can't allow another synchronous modal dialog to open "above" one of
the native modal sheets (otherwise hangs would follow).  The modal
dialog that's failing to open in this case is the one that asks if you
want to close a window with multiple tabs.

This problem could also be fixed by creating an asynchronous
nsXULWindow::ShowModal() variant for "internal" use.  But that'd
probably be a non-trivial amount of work.

Comment 20

[Hal Wine [:hwine] use NI!]

(In reply to Steven Michaud from comment #18)
> Here's a patch that should fix the CPU pegging.
> 
> And here's a tryserver build made from it:
> Please try it out!

Running, and no CPU pegging :)  I only left it "alone" with open file open for about 5 minutes - I'll more give that an overnight run tonight.

Comment 21

[Hal Wine [:hwine] use NI!]

One minor issue with the print dialog now (in both try-built versions). It prints a job even if I save-as-pdf. 

Since my default printer is a shared one at home, I observe that the printer status app up starts up about 5-10 seconds after I click save-as-pdf (which dismisses both the save-as dialog, and the original print sheet). This does not occur on 10.x, or on 13.x w/o your patch.

I guess if I actually was connected to a functioning printer, I'd always get a printout - which would make this more than a minor annoyance. :/

Comment 22

[Steven Michaud [:smichaud] (Retired)]

> One minor issue with the print dialog now (in both try-built versions). It prints a > job even if I save-as-pdf.

Odd.  Are you sure this doesn't also happen with current trunk nightlies?

I can't test for myself, since I don't have a printer.

Comment 23

[Steven Michaud [:smichaud] (Retired)]

But now I just found that Save As PDF doesn't work at all for me (on a computer with no printer installed).

This is probably related to the problem you report.  I'll look into it.

Comment 24

[Hal Wine [:hwine] use NI!]

(In reply to Steven Michaud from comment #22)
> > One minor issue with the print dialog now (in both try-built versions). It prints a > job even if I save-as-pdf.
> 
> Odd.  Are you sure this doesn't also happen with current trunk nightlies?

Yes - at least as of 2012-03-04 (http://hg.mozilla.org/mozilla-central/rev/baec1efc87a4)

Comment 25

[Joe Sewell]

Too bad that this decision comes too late for those of us who are getting the End Of Support boot with Firefox 3 & Thunderbird 3 (i.e., PowerMac owners who cannot, for whatever reason, upgrade to Intel and/or newer OS versions). It will factor into whether I continue with the Mozilla suite if I ever can afford to upgrade hardware and still maintain backward compatibility requirements.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

Just to let people know:

I've done more work on this since rev1 of my proof of concept patch.  I've now got the Print dialog running in a truly window-modal fashion (using native sheets, and an asynchronous variant of nsIFilePicker::Show(), with a callback).  I haven't posted it because I'd prefer to wait until I've done the same for the the File : Open and File : Save As dialogs.

For the last month or so I've been swamped by topcrashers and other critical bugs.  But I hope to get back to this bug before too long.

(By the way, my code (as it now stands) only uses the asynchronous variant of nsIFilePicker::Show when it's called "internally" (e.g. from a menu).  We'd still use an app-modal native window if nsIFilePicker::Show is called from some web page.)

Comment 27

[Steven Michaud [:smichaud] (Retired)]

Attachment: Rev3 patch (work in progress) -- add asynchronous option to nsIPrintDialogService::Show()

I still haven't had a chance to do any more work on this bug.  But now it looks like my work here (some of it) might also be useful on Windows Metro.  So I'm posting the work in progress that I talked about in comment #26.

(By the way, it's nsIPrintDialogService::Show() to which I added an asynchronous option -- not (yet) nsIFilePicker::Show().)

Since this is work in progress, I left in all by debug logging.

Here's a tryserver build made with this patch:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-81c58afd008e/try-macosx64/firefox-17.0a1.en-US.mac.dmg

Please try it out.  So far the only thing I've changed is the Print dialog.

Comment 28

[Steven Michaud [:smichaud] (Retired)]

Attachment: Testcase of printing from web content

My rev3 patch only uses window-modal sheets for "internal" invocations of the Print dialog (for example using the File : Print menu).  It still uses the old (app-modal) when someone tries to print from web content.

The reason why is that window.print() has always been synchronous, so that's what authors of JavaScript code will continue to expect.  If we create a variant of window.print() for "external" consumption (and I think we should), we'll need to give it a different name.

Comment 29

[Mounir Lamouri (:mounir)]

Regarding ease of implementation, wouldn't that be better to have the internal method being async and window.print() being sync by blocking the thread until the print dialog is closed?
It seems that allowing the print dialog to be sync or async depending on caller wishes adds a lot of code overhead and complexity. If we can move that to the only place we actually need sync calls, that might be a win.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

> Regarding ease of implementation, wouldn't that be better to have
> the internal method being async and window.print() being sync by
> blocking the thread until the print dialog is closed?

No.  This wouldn't work at all.

Remember that it's the main thread that would be blocked, so you'd
need to run another (in effect modal) event loop on top of it, to
prevent events being lost/ignored.  This would work fine if you
allowed at most one print dialog to run at any one time, and prevented
most other browser actions from taking place -- in effect if the print
dialog was app-modal, as it is now.  But you'd soon have a horrible
mess if you (for example) ran one print dialog on top of another, then
closed the first print dialog before you closed the second one.

Comment 31

[André Reinald]

Native Mac OS X print and file dialogs (and maybe others I can't think of right now) are designed to be at least window modal (but often are app modal). What we'd like in most cases is to have them tab modal. But AFAIK this is not an option.

Having a fake window with a print or file sheet would probably look terrible, but it would allow the calling window to stay modeless.

The other solution I can think of is delegating such dialogs to a helper app (with IPC) so we don't block anything in Firefox itself? It would allow asynchronous handling of those operations.

Of course this either requires blocking any changed to the calling tab, or extra care in handling the result from such an async dialog, as the state of the tab may have changed since the dialog was called.

Comment 32

[Steven Michaud [:smichaud] (Retired)]

> Native Mac OS X print and file dialogs (and maybe others I
> can't think of right now) are designed to be at least window
> modal (but often are app modal).

If they're sheets they're window-modal.  But if they're windows
they need to be app-modal.

> What we'd like in most cases is to have them tab modal. But
> AFAIK this is not an option.

I don't expect native modal dialogs on OS X ever to be tab-modal.
But I do hope Firefox's own (non-native) modal dialogs become
exclusively tab-modal at some point in the future.

The other alternatives you suggest would be a *lot* of work, and
might be very fragile.

I think it makes sense to stick with with what can (we hope) be
accomplished without too much work or risk -- like using sheets
for all native modal dialogs.

That said, I'm not sure when I'll have time to come back to this
bug.  I think there's some chance of that within the next six
months.  But similar hopes have been shot down many times
before :-(

Comment 33

[Steven Michaud [:smichaud] (Retired)]

This is left over from when I was a Mozilla employee. I'll never get back to it.