957209 - Don't set ewindowtype_sheet if parent is hidden window

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Stefan [:stefanh]]

Spin-off from bug 889085. The problem is that it's possible to open a sheet from the hidden window and doing that will result in a weird-looking "dialog".

Comment 1

[Stefan [:stefanh]]

Attachment: 957209.1.diff

I'm going to test this a bit more, to see if I can find any problems that gets worse than the current situation.

Comment 2

[Markus Stange [:mstange]]

Just a small comment: I'd collapse all conditions into one if, like this:

-  if (parent && ((aChromeMask & sheetMask) == sheetMask))
+  if (parent &&
+      (parent != mHiddenWindow && parent != mHiddenPrivateWindow) &&
+      ((aChromeMask & sheetMask) == sheetMask)) {
     widgetInitData.mWindowType = eWindowType_sheet;
+  }

Comment 3

[Stefan [:stefanh]]

Yes (and I should remove the whitespace that crept in).

Comment 4

[Stefan [:stefanh]]

Attachment: New version

New patch according to Markus recommendations.

The reason why I want to do this is because I want to avoid that sheets opened from the hidden window turns semi-transparent (which they will do if the patch in bug 889085 lands). I would also like these ”sheets” to look like regular dialogs which, I think, makes sense since these ”sheets” doesn’t have a visible window they can be attached to.

Then I discovered bug 641288 and became interested if the patch here made the situation in bug 641288 better or worse. So I’ve tested this patch by setting the ”browser.preferences.instantApply” pref to false and then opening the preferences window.

1) With the patch I get a dialog (not a sheet) that can be moved - it’s still modal, though. But it looks OK. It doesn’t open offscreen, but otoh it doesn’t do that without the patch either.
2) With the patch I don’t get any white square after I’ve closed the window.
3) With the patch; If you go to the Security pane in the prefwindow and open the Exceptions dialog/window (click ”Exceptions…” in the Security pane), it will appear under the prefwindow. This doesn’t happen without the patch the first time you open the prefwindow. I can then make the dialog/window appear above the prefwindow by clicking it. But: If you (without the patch) close the prefwindow and open it again, the dialog/window will always open behind the prefwindow.

So, I think the situation in bug 641288 becomes much better with this patch - I’m tempted to say that the patch here fixes all the issues in bug 641288 (except the modality if that’s really an issue here).

I’ve just pushed the patch to try, builds will appear later at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e8529b824d74

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> it’s still modal

We *don't* want any new modal windows (as opposed to sheets) on OS X.  The reasons are complicated, but basically it's because we don't do modal windows properly on OS X.  We pretend that they can be window-modal, but they really should only be app-modal.  For more information see bug 729720, bug 478073 and bug 395465.

If at all possible, we need to add code to the patch to stop the "former sheet" being modal on OS X.  Let me dig around a bit to see if this might be likely to cause trouble.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Please provide a testcase that causes us to try to open a sheet whose parent window is hidden.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

> Please provide a testcase that causes us to try to open a sheet whose parent window is hidden.

Specifically I want the one that you tested with, Stefan, which causes a modal window to be displayed with your current patch.

Comment 8

[Stefan [:stefanh]]

(In reply to Steven Michaud from comment #7)
> > Please provide a testcase that causes us to try to open a sheet whose parent window is hidden.
> 
> Specifically I want the one that you tested with, Stefan, which causes a
> modal window to be displayed with your current patch.

I used the prefwindow with instantapply set to false (comment #4) :-)

1) Open about:config and set the pref "browser.preferences.instantApply" to false
2) From the Application menu, choose "Preferences..."

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Making the Preferences dialog a modal window on OS X is exactly the disaster I expected it to be.  For example try this with your patch (and "browser.preferences.instantApply" set to false):

1) Choose Firefox : Preferences (or press Cmd+,) to display the Preferences dialog.
2) While it's open display another dialog window (for example Bookmarks : Show All Bookmarks).
3) Close the Show All Bookmarks dialog.
4) Notice that none of the buttons now work in the Preferences dialog.

We can't just fix this one bug.  There are hundreds of them.  It's a design flaw (on OS X) that step 2 is allowed to work while a modal window is open.

However, since (as per the documentation at http://kb.mozillazine.org/About:config_entries) setting "browser.preferences.instantApply" to false *requires* that the Preferences dialog be modal, we can't just unset its modality in this case.

Which started me wondering why can't we display the Preferences dialog as a true, native sheet when it has to be modal.  If at least one browser window is already open, we could make it a sheet above the active browser window.  If no window is open, we could first open one.

I also wonder exactly how (and where) the Preferences dialog is made the child of the hidden window.  We'd need to change that in this case.  Do you know where in the tree this happens?  Though I've been looking for it for a couple of hours, I still haven't been able to find it.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Your patch's one saving grace is that what currently happens without it is even worse than what happens with it.  Which I think means we'll have to land it in some form.

But before that happens I want to do as much as I can to limit its collateral damage.

Comment 11

[Stefan [:stefanh]]

(In reply to Steven Michaud from comment #9)

> However, since (as per the documentation at
> http://kb.mozillazine.org/About:config_entries) setting
> "browser.preferences.instantApply" to false *requires* that the Preferences
> dialog be modal, we can't just unset its modality in this case.

Bug 641288, comment #60 is what makes the preference window modal (the "modal" features). So, yes - the caller expect it to be modal.

One could actually work-around the whole issue by not adding "modal" to the features when calling openDialog (it doesn't help us here though, since we want a general solution - and it's of course wrong if it's supposed to be modal - I'm not 100% sure it is "required", though).

> Which started me wondering why can't we display the Preferences dialog as a
> true, native sheet when it has to be modal.  If at least one browser window
> is already open, we could make it a sheet above the active browser window. 
> If no window is open, we could first open one.
> 
> I also wonder exactly how (and where) the Preferences dialog is made the
> child of the hidden window.  We'd need to change that in this case.  Do you
> know where in the tree this happens?  Though I've been looking for it for a
> couple of hours, I still haven't been able to find it.
Hmm, I think this is because the Preferences menuitem is not created by any xul menu. Since it's in the Application menu, it's created/added in nsMenuBarX::CreateApplicationMenu (if the menuitem exists in a xul menu it gets removed and then re-created in the Application menu, from what I understand).

Comment 12

[Steven Michaud [:smichaud] (Retired)]

I'm now resigned to taking your patch as is.  I'll r+ it later on, unless I find a better alternative along the way I'm currently taking (which I think is very unlikely).  Though I'm sorely tempted, I think it's too risky to alter your patch to make all modal windows non-modal.

We're stuck with broken modal windows on OS X until I can manage to build a consensus that they should all be app-modal.  That may take years, and in fact it may never happen.  Even once we have that consensus (if we ever do), it will take a lot of work to achieve -- for example we'll need to stop all but a very short list of menu items from working (like Quit and perhaps a few others) when a modal window is open, whether via the menu itself or via key combinations.

In the meantime, though, a lot of our modal dialogs are now implemented as either native sheets (which are window-modal, on OS X) or non-native tab-modal constructs (whose correct name I've forgotten).  So there aren't too many occasions on which we're forced to use modal windows on OS X.  (And once bug 729720 is finished there will be even fewer ... if it ever *does* get finished.)

I'd already managed to find utilityOverlay.js, and that the Preferences dialog is displayed by a call to the following line:

https://hg.mozilla.org/mozilla-central/annotate/09a8ed7cbb0b/browser/base/content/utilityOverlay.js#l526

Somehow just calling openDialog() in that context is equivalent to calling hiddenWindow.openDialog().

But I hope I can get Services.wm.getMostRecentWindow() (or something similar) to return the currently active browser window, over which we'd open the Preferences dialog as a sheet.  And if there isn't a currently active browser window, we could create one.  I'm going to spend a few more hours on this, today and tomorrow.  This is the "way I'm currently taking" that I mentioned above.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8358985 [details] [diff] [review]
New version

Go ahead and land this, Stefan, together with your patch for bug 889085.

I've opened bug 961078 to deal with making the Preferences dialog a sheet on OS X if instantApply is false.  The fix for that bug will lag yours somewhat.  But fortunately instantApply is true by default on OS X, and the number of people who change this setting is probably very small.

Comment 14

[Stefan [:stefanh]]

(In reply to Steven Michaud from comment #13)
> Go ahead and land this, Stefan, together with your patch for bug 889085.

OK. Thanks!

Comment 15

[Stefan [:stefanh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15b13a7c66a5

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/15b13a7c66a5