406780 - In OSX The customizeToolbars window should be in a <panel> popup.

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[Philip Chee]

https://bugzilla.mozilla.org/show_bug.cgi?id=394288#c40
[quote]
Using the dialog has 2 issues:

1) Mac users would expect the whole, original, menubar to stay unaffected when
the customizetoolbar window opens. This will require that your new window has a
menubar - the widget code takes the menubar from the frontmost window and puts
it at the top of the desktop. This is probably one of the reasons why toolkit
has that iframe-hack on mac.

2) Once you click on the toolbar, the dialog vanishes behind the navigator
window. While 1) is something we could live with (we have the same issue in
lots of other dialogs), this problem is a bit worse since you can only drag
stuff from the dialog to the toolbar. Thinking of it, this might be the main
reason why it's done different on mac.
[/quote]

See Bug 250129 and Bug 350282 For the Firefox equivalent and Bug 394873 for the Thunderbird equivalent.

Comment 1

[Philip Chee]

> See Bug 250129
Err I got this from CVS Blame?

Comment 2

[[e-mail]]

Anyone know why a window is better than a panel on other OSes?

Comment 3

[Robert Kaiser]

Actually, bug 350129 added this - your number has a typo ;-)
You should also look into bug 206649 for this, by the way.
I didn't find any mentioning why we would only want this on Mac but not on other platforms, though. Maybe the FF people working/commenting in the other bugs know that better...

Comment 4

[Phil Ringnalda (:philor)]

Fx actually started out with a cross-platform ripoff of the OS X native customization sheet, but it did a bad job of sizing to content, and then fixing that broken the hidechrome, and that was shortly before 1.0, so giving up and looking like a dialog was easiest, and then since only Mac users had anything to compare with for what toolbar customization should look like, nobody felt the need to change for anyone else.

But I'm sure that if you want to hammer out the possible problems with things like sizing (Linux users, especially, seem to have a tendency to use enormous fonts, see dozens of dupes in Fx and Tb) and movement (I sort of think that bug 395123 and bug 395334 probably mean that noautohide panels move with their parent cross-platform, maybe), Firefox and Thunderbird will watch with interest :)

Comment 5

[Serge Gautherie (:sgautherie)]

Filter "spam" on "guifeatures-nobody-20080610".

Comment 6

[Philip Chee]

(In reply to comment 4)
> But I'm sure that if you want to hammer out the possible problems with things
> like sizing (Linux users, especially, seem to have a tendency to use enormous
> fonts, see dozens of dupes in Fx and Tb) and movement (I sort of think that
> bug 395123 and bug 395334 probably mean that noautohide panels move with their
> parent cross-platform, maybe), Firefox and Thunderbird will watch with interest

Phil,

I have a WIP patch that seems to work in WinXP. Are the issues on Linux fixed these days or are there some outstanding issues?

Comment 7

[Philip Chee]

Attachment: Patch 0.1

OK, this is a working patch, so I'm putting this up to get some early feedback.

+++ b/suite/common/customizeToolbar.js
....
+++ b/suite/common/customizeToolbar.xul
....
+++ b/suite/common/jar.mn
@@ -1,12 +1,13 @@ comm.jar:
....
+% override chrome://global/content/customizeToolbar.xul chrome://communicator/content/customizeToolbar.xul

Toolkit doesn't support OSX sheets. So all implementations (Firefox, Thunderbird) fork the toolkit files. What I've done here is to unify the toolkit/customizeToolbar.* and the browser/customizeToolbar* files so one set of files can work as either a popup window or a panel/sheet. I'll comment more on this subsequently.

+++ b/suite/common/utilityOverlay.xul
....
+<!ENTITY % customizeToolbarDTD SYSTEM "chrome://global/locale/customizeToolbar.dtd">
+%customizeToolbarDTD;
....
+  <panel id="customizeToolbarSheetPopup" noautohide="true">
+    <iframe id="customizeToolbarSheetIFrame"
+            style="&dialog.style;"
+            hidden="true"/>
+  </panel>

I don't like pulling in a whole DTD just for one entity "&dialog.style;" but this is what Firefox does and Thunderbird et el have copied this.

+/* XXXRatty: adapted from pinstripe. This should only be for the Mac
+   or rather for a sheet only, but I like the effect. */
+#CustomizeToolbarWindow > #main-box {
+  border-top: none;
+  border-left: 2px solid;
+  border-right: 2px solid;
+  border-bottom: 3px solid;
+/*
+  -moz-border-right-colors: ThreeDHighlight ThreeDLightShadow;
+  -moz-border-bottom-colors: ThreeDDarkShadow ThreeDDarkShadow ThreeDShadow;
+  -moz-border-left-colors: ThreeDLightShadow ThreeDHighlight;
+   TODO: change the colour names to #colour codes.
+*/
+  -moz-border-right-colors: #FFFFFF #F1EFE2;
+  -moz-border-bottom-colors: #716F64 #716F64 #9D9DA1;
+  -moz-border-left-colors: #F1EFE2 #FFFFFF;
+}

I probably have the colour codes wrong. Can anyone suggest better values?

+++ b/suite/themes/modern/global/popup.css
.... 
+panel,

Adding missing support for panel in modern. What else are we missing I wonder.

Comment 8

[Philip Chee]

Comment on attachment 359759 [details] [diff] [review]
Patch 0.1

stefanh, could you test this on OSX? There is a pref (with no UI at the moment) that should be true for OSX "toolbar.customization.usesheet". Looks fine on WinXP but of course OSX is what the sheet is actually for.

Comment 9

[Philip Chee]

Attachment: Manual diff against the toolkit/customizeToolbar.* files.

This is a manual diff between the toolkit customizeToolbar files and my attempt at unification. My aim is to have one set of xul/js files that can be used as either a popup window or a panel/sheet. In the Suite patch I have a pref that allows me to dynamically switch between a pop and a panel without having to rebuild, or even restart.

If you disregard the differences due to the Suite style licence blocks, you'll see that the actual changes are rather minimal. Ideally I'd like to get this into toolkit 1.9.1 but this close to Firefox 3.1, this might be problematical. My plan is to bake this in Suite and then submit it to the toolkit reviewers in a separate bug as a proven and tested patch.

Comment 10

[Philip Chee]

Attachment: Manual diff against the toolkit/customizeToolbar.* files.

Ooops. Wrong diff.

Comment 11

[Philip Chee]

Attachment: Screenshot of panel mode in SeaMonkey Navigator

Here is how it looks in Navigator with the Modern Theme.

Comment 12

[[e-mail]]

Comment on attachment 359759 [details] [diff] [review]
Patch 0.1

>+#ifdef XP_MACOSX
I think we've already got an #ifdef XP_MACOX block in browser-prefs.js

>diff --git a/suite/common/customizeToolbar.xul b/suite/common/customizeToolbar.xul
Maybe we can just overlay it instead of replacing it?

>+    sheetFrame.setAttribute("toolbox", toolbox.id);
Probably simpler to do sheetFrame.toolbox = toolbox; instead

>+  -moz-border-right-colors: ThreeDHighlight ThreeDLightShadow;
>+  -moz-border-bottom-colors: ThreeDDarkShadow ThreeDDarkShadow ThreeDShadow;
>+  -moz-border-left-colors: ThreeDLightShadow ThreeDHighlight;
>+   TODO: change the colour names to #colour codes.
>+*/
>+  -moz-border-right-colors: #FFFFFF #F1EFE2;
>+  -moz-border-bottom-colors: #716F64 #716F64 #9D9DA1;
>+  -moz-border-left-colors: #F1EFE2 #FFFFFF;
>+}
Hmm, those colours don't look "Modern"...
Try #F8FAFE #5D616E, #5D616E #5D616E #BEC3D3 and #5D616E #F8FAFE
Or #EEF4FC #7F8893, #7F8893 #7F8893 #B9BFC7 and #7F8893 #EEF4FC

Comment 13

[Stefan [:stefanh]]

The "sheet" version seems to work fine on mac. My only complaint so far would be the "flickering" when the pane loads - it looks a bit like the panel loads twice when you open it. But the same effect can be seen in Firefox.

One question, though (I might have missed the point here, though):

gCustomizeSheet = getBoolPref("toolbar.customization.usesheet", false);

Is there any point making this a pref? Seriously, why would anyone want to have a choice here? I mean, mac users doesn't want any choice here - do win/nix users want to choose between a faked sheet (real "sheets" slide out from underneath the toolbar) and a dialog? Instead of checking the pref here can't you just check for Mac?

Comment 14

[Philip Chee]

> Instead of checking the pref here can't you just check for Mac?

1. Because I don't have a Mac, and if I check for a Mac in the code, I would never be able to test this patch?

2. IMHO even a fake sheet on WinXP looks better than the popup window.

3. Because I can?

4. Let's get this out to the nightly testers. Who knows, perhaps the Windows and Linux users might like the fake sheet better.

So do I get a r+ for the UI on OSX?

Comment 15

[Stefan [:stefanh]]

Comment on attachment 359759 [details] [diff] [review]
Patch 0.1

(In reply to comment #14)
> > Instead of checking the pref here can't you just check for Mac?
> 
> 1. Because I don't have a Mac, and if I check for a Mac in the code, I would
> never be able to test this patch?

I didn't know the pref is only there to test the sheet. That said, you could have tested it by just checking for windows, then you could have flipped the check to mac. 

> 2. IMHO even a fake sheet on WinXP looks better than the popup window.

I'm glad you like it ;-)

> 3. Because I can?

What do you mean?

> 4. Let's get this out to the nightly testers. Who knows, perhaps the Windows
> and Linux users might like the fake sheet better.

Looking at comment #7, I get the impression that this is a patch you put up in order to get "some early feedback". That said, what is the plan if Windows/Linux users like the sheet (and how many should like it for you to decide that "they" like it?)? Will there still be a pref?
> 
> So do I get a r+ for the UI on OSX?

Sure.

Comment 16

[Philip Chee]

Attachment: Patch 0.2 Nits Fixed

Carrying forward r+ asking for sr+

(In reply to Comment 12)
>(From update of attachment 359759 [details] [diff] [review])
>>+#ifdef XP_MACOSX
> I think we've already got an #ifdef XP_MACOX block in browser-prefs.js

Well we have two, the first is a group of related prefs. The second appears to be a grab-bag of miscellany so I've put the new pref there.
 
>>diff --git a/suite/common/customizeToolbar.xul b/suite/common/customizeToolbar.xul
> Maybe we can just overlay it instead of replacing it?

We are blocked by the presence of duplicate element IDs in the toolkit customizeToolbar.xul. I'll file a bug on this.
 
>>+    sheetFrame.setAttribute("toolbox", toolbox.id);
> Probably simpler to do sheetFrame.toolbox = toolbox; instead

Done.
 
>>+  -moz-border-right-colors: ThreeDHighlight ThreeDLightShadow;
>>+  -moz-border-bottom-colors: ThreeDDarkShadow ThreeDDarkShadow ThreeDShadow;
>>+  -moz-border-left-colors: ThreeDLightShadow ThreeDHighlight;
>>+   TODO: change the colour names to #colour codes.
>>+*/
>>+  -moz-border-right-colors: #FFFFFF #F1EFE2;
>>+  -moz-border-bottom-colors: #716F64 #716F64 #9D9DA1;
>>+  -moz-border-left-colors: #F1EFE2 #FFFFFF;
>>+}
> Hmm, those colours don't look "Modern"...
> Try #F8FAFE #5D616E, #5D616E #5D616E #BEC3D3 and #5D616E #F8FAFE

I like this one better. Updated.


(In reply to Comment 15)
> Looking at comment #7, I get the impression that this is a patch you put up in
> order to get "some early feedback". That said, what is the plan if
> Windows/Linux users like the sheet (and how many should like it for you to
> decide that "they" like it?)? Will there still be a pref?

Well Neil is the UI Tsar so it's his decision. Having said that I'm rather attached to the pref and the ability to switch back and forth.

Comment 17

[[e-mail]]

(In reply to comment #16)
>(In reply to comment #12)
>>(From update of attachment 359759 [details] [diff] [review])
>>>+#ifdef XP_MACOSX
>> I think we've already got an #ifdef XP_MACOX block in browser-prefs.js
>Well we have two, the first is a group of related prefs.
Hmm, so we do... /me wonders whether OS/2 should be instant apply
>The second appears to be a grab-bag of miscellany so I've put the new pref there.
Right, but instead of an else, add non-mac version somewhere before the ifdef.

Comment 18

[Philip Chee]

>>The second appears to be a grab-bag of miscellany so I've put the new pref there.
>Right, but instead of an else, add non-mac version somewhere before the ifdef.

I don't know much about how the pre-processor works, but won't this put _two_ entries for this pref in the Mac builds?

Comment 19

[Phil Ringnalda (:philor)]

That's not so much "how the preprocessor works," as "how prefs files work":

pref("foo", true);
...
pref("foo", false);

sets foo to false whether that ... is zero lines, fifty lines, or the space between all.js and browser-prefs.js. Before 2004, there were separate files in /modules/libpref/win/winprefs.js, etc., that did the platform overrides, but then bug 224578 split application prefs out of all.js and also put the platform overrides into preprocessed hunks at the end of the appropriate files.

It's just confusing because since then, some people have remembered that platform overrides are at the end of the file, and some haven't, and since Thunderbird had very few platform overrides and Firefox had none (at the time it came in any color you like as long as it's the color Windows likes), people who grew up with win/winprefs.js and suite expect platforms prefs to be separate at the bottom, and people who grew up in a bird cage scatter the ifdefs next to the default value.

Comment 20

[[e-mail]]

Comment on attachment 359927 [details] [diff] [review]
Patch 0.2 Nits Fixed

Yeah, well I guess if you don't like that it's OK to go back to the 0.1 version of browser-prefs.js, but the real reason I'm minusing this is that focus gets lost when you customise with the panel (tabbrowser's updateCurrentBrowser might give you a few ideas).

Comment 21

[Philip Chee]

Attachment: Patch 0.2 Flyspecked and Nitpicked.

>> The second appears to be a grab-bag of miscellany so I've put the new pref there.
> Right, but instead of an else, add non-mac version somewhere before the ifdef.
Fixed.

> but the real reason I'm minusing this is that focus gets
> lost when you customise with the panel (tabbrowser's updateCurrentBrowser might
> give you a few ideas).
Fixed.

Asking for r/sr

Comment 22

[[e-mail]]

(In reply to comment #20)
>focus gets lost when you customise with the panel
Hmm, it's worse than I thought. Your latest patch does fix it for the case where focus was in content (about:config for example). But more nasty is the case where focus was in the location bar, since customising destroys the XBL...

Comment 23

[Philip Chee]

So I hope this is what you meant on irc by: "just focus content (if it exists) otherwise window"

  if (gCustomizeSheet) {
    var sheetFrame = document.getElementById("customizeToolbarSheetIFrame");
    sheetFrame.hidden = true;
    document.getElementById("customizeToolbarSheetPopup").hidePopup();
    if (content)
      content.focus();
    else
      window.focus();
  }

Comment 24

[[e-mail]]

Comment on attachment 360095 [details] [diff] [review]
Patch 0.2 Flyspecked and Nitpicked.

Right. sr=me with that fixed.

Comment 25

[Philip Chee]

Attachment: Patch 1.0 Final [for checkin]

> Right. sr=me with that fixed.

Carrying forward r+/sr+

Changes: on exit customize - if (content) focus on content; else focus on window;

Comment 26

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/cbafeb23a8ba

Comment 27

[Philip Chee]

Attachment: Patch 2.0 Backport fix from toolkit Bug 471508

Sigh. My toolkit fix in Bug 471508 is now in mozilla-central but I forgot to include it in the suite patch. Carrying forward the toolkit r+ from gavin. Asking for suite sr/rs.

Comment 28

[Robert Kaiser]

Checked in attachment 360458 [details] [diff] [review] as http://hg.mozilla.org/comm-central/rev/709d6c67890b