961078 - Make Preferences dialog a sheet on OS X if !instantApply

Status: RESOLVED WONTFIX

(Firefox :: Settings UI, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

This bug is spun off from bug 957209.

With the patch for bug 957209, the Preferences dialog becomes a modal window if browser.preferences.instantApply is false.  But our implementation of modal windows on OS X is broken, and will be very difficult to fix (see bug 957209 comment #5, bug 957209 comment #9 and bug 957209 comment #12).  So we should do what we can to avoid using them.

There is a lot of code in the tree that assumes the Preferences dialog is window-modal if instantApply is false (as it is by default on Windows).  The Preferences dialog is window-modal on Windows.  Native sheets are window-modal on OS X.  So in fact sheets are more appropriate for a modal Preferences dialog on OS X, aside from the problem of our implementation of modal windows being broken on OS X.

Shortly I'll post a patch that does what's needed.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

> Shortly I'll post a patch that does what's needed.

I've discovered a problem with my patch, so it'll take a bit longer.  I still hope to be able to post something by the end of today, though.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

I've given this a moderate amount of testing, without noticing any problems.  I've also started a round of tryserver tests, the results of which should eventually be available here:

https://tbpl.mozilla.org/?tree=Try&rev=372fb3d79602

I don't really know who to ask for a review.  Ehsan, could you give it a shot?  Or pass it along to someone else if you'd prefer?

Comment 3

[(no longer active)]

Comment on attachment 8361884 [details] [diff] [review]
Fix

Review of attachment 8361884 [details] [diff] [review]:
-----------------------------------------------------------------

Minusing for the OpenBrowserWindow() issue, but I'd like to understand where the sheet opening happens in this patch as well.

::: browser/base/content/utilityOverlay.js
@@ +532,5 @@
> +      // properly).  See bug 961078.
> +      if (!instantApply) {
> +        win = Services.wm.getMostRecentWindow("navigator:browser");
> +        if (!win) {
> +          win = OpenBrowserWindow();

I think you should wait on the new window's load event to be fired before you call openDialog on it further below.

@@ +534,5 @@
> +        win = Services.wm.getMostRecentWindow("navigator:browser");
> +        if (!win) {
> +          win = OpenBrowserWindow();
> +        } else if (win.windowState == win.STATE_MINIMIZED) {
> +          win.restore();

So this should only happen when you have one browser window open which is minimized, right?  (Otherwise another browser window should be the most recent active one I think.)

@@ +539,5 @@
> +        }
> +      }
> +#endif
> +      win.openDialog("chrome://browser/content/preferences/preferences.xul",
> +                     "Preferences", features, paneID, extraArgs);

I don't understand what causes the preferences window to open up as a sheet here.  Can you please clarify that?

Thanks!

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8361884 [details] [diff] [review]
Fix

> I think you should wait on the new window's load event to be fired
> before you call openDialog on it further below.

How would you suggest I do this?  Feel free to point me to examples
elsewhere in the tree.  (It's not often I work in JavaScript code.)

> So this should only happen when you have one browser window open
> which is minimized, right?  (Otherwise another browser window should
> be the most recent active one I think.)

The "most recent" list does include minimized windows, but they're at
the bottom of the list.  So if at least one browser window is
"non-mimized", it will get chosen before any of the minimized windows.

(By the way, I also tested my patch with maxmized and even fullscreen
windows.  It worked fine in those cases.)

> I don't understand what causes the preferences window to open up as
> a sheet here.  Can you please clarify that?

I don't fully understand either.

The Preference dialog's "parent" is always the hidden window, which is
why the patch for bug 957209 makes it always a modal window when
instantApply is false.

What's not entirely clear is *why* the Preference dialog has the
hidden window as a parent, though it may have something to do with the
fact that the Preferences menu item is in the Application menu (as
opposed to any of the other submenus of the main menu).

Many of the Application menu items are actually located elsewhere:

http://hg.mozilla.org/mozilla-central/annotate/298f262f21ff/browser/base/content/baseMenuOverlay.xul#l19

But code in nsMenuBarX::AquifyMenuBar() hides these menu items while
copying their "content":

http://hg.mozilla.org/mozilla-central/annotate/298f262f21ff/widget/cocoa/nsMenuBarX.mm#l472

Then code in nsMenuBarX::CreateApplicationMenu() creates native menu
items in the Application menu corresponding to this "content":

http://hg.mozilla.org/mozilla-central/annotate/298f262f21ff/widget/cocoa/nsMenuBarX.mm#l588

Without the hiding, second copies of the Preferences and other menu
items show up in the Tools menu.  But still the Preferences dialog has
the hidden window as parent if it's invoked from the Application menu
(even though this doesn't happen when it's invoked from the Tools
menu).

Comment 5

[(no longer active)]

(In reply to comment #4)
> Comment on attachment 8361884 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=8361884
> Fix
> 
> > I think you should wait on the new window's load event to be fired
> > before you call openDialog on it further below.
> 
> How would you suggest I do this?  Feel free to point me to examples
> elsewhere in the tree.  (It's not often I work in JavaScript code.)

Something like this perhaps: <http://dxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_geoprompt.js#48>

> > So this should only happen when you have one browser window open
> > which is minimized, right?  (Otherwise another browser window should
> > be the most recent active one I think.)
> 
> The "most recent" list does include minimized windows, but they're at
> the bottom of the list.  So if at least one browser window is
> "non-mimized", it will get chosen before any of the minimized windows.
> 
> (By the way, I also tested my patch with maxmized and even fullscreen
> windows.  It worked fine in those cases.)

Sounds great!

> > I don't understand what causes the preferences window to open up as
> > a sheet here.  Can you please clarify that?
> 
> I don't fully understand either.
> 
> The Preference dialog's "parent" is always the hidden window, which is
> why the patch for bug 957209 makes it always a modal window when
> instantApply is false.
> 
> What's not entirely clear is *why* the Preference dialog has the
> hidden window as a parent, though it may have something to do with the
> fact that the Preferences menu item is in the Application menu (as
> opposed to any of the other submenus of the main menu).
> 
> Many of the Application menu items are actually located elsewhere:
> 
> http://hg.mozilla.org/mozilla-central/annotate/298f262f21ff/browser/base/content/baseMenuOverlay.xul#l19
> 
> But code in nsMenuBarX::AquifyMenuBar() hides these menu items while
> copying their "content":
> 
> http://hg.mozilla.org/mozilla-central/annotate/298f262f21ff/widget/cocoa/nsMenuBarX.mm#l472
> 
> Then code in nsMenuBarX::CreateApplicationMenu() creates native menu
> items in the Application menu corresponding to this "content":
> 
> http://hg.mozilla.org/mozilla-central/annotate/298f262f21ff/widget/cocoa/nsMenuBarX.mm#l588
> 
> Without the hiding, second copies of the Preferences and other menu
> items show up in the Tools menu.  But still the Preferences dialog has
> the hidden window as parent if it's invoked from the Application menu
> (even though this doesn't happen when it's invoked from the Tools
> menu).

Oh ok now I understand what's going on here.  This code usually runs in the context of the hidden window, which means that the |window| object is the hidden window, not a regular browser window.  Your patch fixes this by making sure that |win| never points to the hidden window.  This all sounds good!

Comment 6

[Steven Michaud [:smichaud] (Retired)]

http://dxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_geoprompt.js#48

I can't get this to work properly, probably because (as it's currently written) it's asynchronous.  As best I can tell, I need to add an event listener and then poll until it's called.  Do you know of examples in the tree of how this can be done correctly?

Comment 7

[(no longer active)]

Comment on attachment 8361884 [details] [diff] [review]
Fix

Hmm, you're right.  I looked at the implementation of OpenDialog and it indeed doesn't try to touch the DOM in any way, so I guess it's fine to call it on a window that has not loaded yet.  Based on that, r=me if this indeed works as expected in your local testing.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #7)

Thanks!  I'll drop the idea of adding an event listener and polling until it's called -- probably not the best approach in any case.

I saw no problems in my own tests.  And the tryserver tests, which are about half finished, have up til now all passed.  I'll wait for the rest to finish.  Then if things look good I'll land my patch.

Comment 9

[Dão Gottwald [:dao]]

Why is instantApply being disabled something we need to support on OS X?

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8361884 [details] [diff] [review]
Fix

This extra only-on-OS-X-with-a-hidden-pref-changed code path you're adding seems like it either won't be well-maintained and just rot slowly or cause people to waste time on it when they do try to maintain it.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8361884 [details] [diff] [review]
Fix

(In reply to comment #10)

Please tell us why you think this patch will be hard to maintain.  It's very simple and self-contained, and fully explained by its comment.  So if it ever does cause trouble at some point, that should be easy to find.

(In reply to comment #9)

> Why is instantApply being disabled something we need to support on OS X?

No, we don't really need to support it.  But if the functionality is there, people will find it and use it.  And this patch is a very simple and (I think) effective way to get it working properly.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Steven Michaud from comment #11)
> Please tell us why you think this patch will be hard to maintain.

Because it's an OS-X-only code block and behind a hidden pref, so developers need to jump through multiple hoops to test it, it has no automated tests and gets virtually no manual testing in nightlies.

> No, we don't really need to support it.

Then we shouldn't do it.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

I think this is the wrong decision, but I don't care enough about it to pursue matters further ... at least for now.