465089 - Default toolbar appearance should follow the appearance settings in the Preferences window

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Karsten Düsterloh]

Toolbarbutons are shown as icon plus text, regardless of the setting in Preferences -> Appearance.
You might probably (please!?) fix bug 253007 as a side effect... ;-)

Comment 1

[Karsten Düsterloh]

> You might probably (please!?) fix bug 253007 as a side effect... ;-)

Oh, heh, I didn't see it was!

Comment 2

[Philip Chee]

For the nav-bar in browser and msgToolbar in messenger, the button size and mode are now controlled by the toolbar context menu.

Comment 3

[Karsten Düsterloh]

The context menu's submenu says it's following the default, which it clearly isn't...

Comment 4

[Karsten Düsterloh]

Or let me put it this way: the default it is following should be that from the prefs, not the one set in the XUL markup.

Comment 5

[Stefan [:stefanh]]

Assume I have "icons" in the prefs, then I change all toolbars to "icons and text", by right-clicking on each and pick "icons and text"... There's no way you can have a good ui here. I think you have to choose between:

1) having the possibility to change toolbar mode for each toolbar
2) changing toolbar modes globally

Comment 6

[Karsten Düsterloh]

IMO, you should be able to to:
- set a global default mode (in prefs)
- override that for individual toolbars (in their context menu)
Having a button "reset all toolbars to default" in the preferences would be a plus.
Making the global default setting change when you change a random toolbar mode does not sound appealing.

Comment 7

[Stefan [:stefanh]]

IMO, the most sensible would be to only have a button to reset all toolbars to default. Initially - when no individual toolbar mode differs from the default - it should be disabled. Changing a mode in a toolbar should then enable the button.

Comment 8

[Stefan [:stefanh]]

or well, I guess it could work with a default mode in prefs as well. But I really think that you'll need that button then. Otherwise it doesn't make sense, suppose I change all my toolbars back and forth. If I want to restore everything to default, I shouldn't have to right-click every toolbar.

Comment 9

[Philip Chee]

Once we turn on customization we should get the toolkit "Customize Toolbars" dialog/sheet which has a global (to that toolbox/window) reset to defaults button.

Comment 10

[Stefan [:stefanh]]

hmm, the prefs deals with all toolbars

Comment 11

[u107962]

Previously it was possible to have icon without text for back, forward, reload, stop, and print while having both for search. I, for one, was unaware that the search button fell into the same category as the others until I noticed the change in today's nightly. (And I'm not complaining, but I like the way it was before better.)

Comment 12

[Serge Gautherie (:sgautherie)]

In case this is not fixed before 2.0a2, it will need a relnote.

Comment 13

[Philip Chee]

From my point of view this is a feature not a bug. :P

Comment 14

[Robert Kaiser]

(In reply to comment #13)
> From my point of view this is a feature not a bug. :P

In that case, the bug is that we still have this pref in the prefwindow, which doesn't actually influence the appearance. Either the pref needs to work or it needs to be removed.

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> [...] or it needs to be removed.

And, in this case, a bonus point would be to "migrate" the old pref, so that people who don't care about the new feature [whatever it is] don't suffer from an "appearance/setting regression".

Comment 16

[Philip Chee]

Hmm. I could write a pref listener that would sync the default attributes on the <toolbox> with the prefs.

Comment 17

[Ian Neal]

Not blocking an alpha on this, adding relnote keyword

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to comment #18)
> *** Bug 466853 has been marked as a duplicate of this bug. ***

That bug wanted
{
Also the global setting should have the following settings:
Large icons and text, Large icons only, Small icons only, Text only
}
too.

I don't know about that, but copying it, just in case.

Comment 20

[asmpgmr]

Up until recently the main toolbar appearance was controlled by Preferences ->
Appearance -> Show toolbars as (Pictures and text, Pictures only, Text only)

This was changed to a context menu which really isn't good. Generally you want
the toolbar to appear the same way across components (browser, mailnews,
mailnews composer, address book, editor) so the global setting makes much more
sense than having to do this for every component's toolbar. Also how many toolbars do users have that there is a need to configure them separately anyway ? I only have the one toolbar. This seems like unnecessary (and for some users like myself unwanted) copying of Firefox's UI. Please restore the original behavior or allow the global setting to override.

Also the global setting should have the following settings:
Large icons and text, Large icons only, Small icons only, Text only

Comment 21

[Stefan [:stefanh]]

> ? I only have the one toolbar. This seems like unnecessary (and for some users
> like myself unwanted) copying of Firefox's UI.

It's really not Firefox ui. afaik they don't have any "toolbar settings" options in their context menu...

Comment 22

[Philip Chee]

Well it's true that they have a Customize... menu item, but I added the "per toolbar settings" because KaiRo really wanted it so how could I say no?

Comment 23

[Robert Kaiser]

The per-toolbar controls are needed if you ever want to convert the personal toolbar to contain a larger icon or things like that.
The current situation with the pref is unfortunate IMHO, and we should either remove/disable the UI pref or make it working for a2, I guess.

Comment 24

[Tony Mechelynck [:tonymec]]

IMO it all depends on the theme. The Modern theme has nice icons both large and small for the Browser toolbar, but for the MailNews toolbar there is only one (large) set; however on that same MailNews toolbar the RemoveDupes extension has a nice "large" icon and no "small" icon at all (or grey-background-only) in its current version. So when I'm using the Modern theme I want "small" icons in the Browser but "large" icons in Mail&News.

OTOH the EarlyBlue theme has nice "small" icons throughout (and "large" icons too): there I want only "small" icons to conserve screen real estate.

I don't use the "SeaMonkey default" theme but it's quite possible that if I did my preferences would be something else again.

I suggest a preference setting as follows:
[ ] Set icon/text appearance separately for each toolbar using its context menu
( ) Text only   ( ) Icons and text  (*) Icons only   [x] with small icons

(The above example does not necessarily show the default settings. Irrelevant parts, i.e. the whole bottom line when the first checkbox is ticked, and the icon size when "text only" is selected, would of course be grayed-out.)

Comment 25

[asmpgmr]

Tony's suggestion is good but the global method should be the default for consistency with older versions.

A separate but related issue is that currently there are no small Classic theme mailnews icons. Hopefully these will be done before the next alpha.

Comment 26

[Robert Kaiser]

I disagree with Ian on this, we shouldn't ship an option on the main appearance pref panel that doesn't work in browser or mail, IMHO, so I actually think this should block alpha 2. Even disabling/hiding the pref from the panel would make this agreeable for Alpha for me though, but a real solution would be preferred.

Comment 27

[Philip Chee]

I'm working on a patch.

Comment 28

[Philip Chee]

Attachment: Patch 1.0: Listen to the global preference

So here is the design goal:

1. As long as the user hasn't changed the (new) local toolbar mode, the toolbar will respond to the global preference controlled by the Preference Window (Appearance pane).

2. Once the user has changed the toolbar mode, the toolbar stops responding to the global preference.

3. To sync the toolbar to the preference again, choose the "Use default settings" from the toolbar context menu.

I have an alternative patch for:
3a. Where IN ADDITION to (#3), if the user subsequently chooses a local mode that matches the global preference then the toolbar starts sync'ing to the preference again, but I'm not sure if this is a level of complexity that is needed or wanted or might cause unnecessary confusion. For example the user might want to "make this toolbar permanently mode X no matter what the global pref is set to for all the other primary toolbars".

Notes:

1. The new customize context menu is only available in the navigator and messenger/message windows. All the other primary toolbars were never affected by Bug 428216.

2. Only the primary toolbars are affected because even in the old system only the primary toolbars listened to the preference. Thus the Personal toolbar will respond only to the local context menu controls.

Comment 29

[Robert Kaiser]

Comment on attachment 350563 [details] [diff] [review]
Patch 1.0: Listen to the global preference

r=me on this functionality-wise. I agree with you that 3a might be confusing again to some people, esp. when trying different setting via instant apply on Mac/Linux. What I found though is that when the custom mode matches the default (but is still custom), you still can't click the context menu item to hook it into the pref again.
The patch is surely enough for me for alpha 2 though.

Comment 30

[Philip Chee]

Comment on attachment 350563 [details] [diff] [review]
Patch 1.0: Listen to the global preference

obsoleting patch. Non-primary toolbars shouldn't get/set custom_mode. Also:

> What I found though is that when the custom mode matches the default
> (but is still custom), you still can't click the context menu item
> to hook it into the pref again.

Comment 31

[[e-mail]]

I'm also worried about how this will work w.r.t. customise window, which iirc will try to set or reset the mode without knowing that it's watching the pref.

Comment 32

[asmpgmr]

Can you add a checkbox along the lines of what was suggest in post #24 to always use the global pref ? I always want all primary toolbars set to small icons, pictures only and NEVER want to change them separately. I honestly can't imagine why you would want a primary toolbar to appear one way in one component and differently in another.

Also shouldn't this change have been blocked by the fact that all of the small icons (mailnews) are done yet ? As it stands I have to use a userChrome.css hack to force consistent behavior across all components, this isn't progress.

Comment 33

[Robert Kaiser]

Let's not discuss additional UI prefs here and make a separate bug for that, please (and I'm not yet convinced on the small icon UI pref, but let's also discuss that there). Let's solved the issue of a global UI pref that doesn't apply here for alpha 2, and leave the other things to followup bugs.

We have small icons for browser and mailnews for the default theme, which is the most important issue. Modern is incomplete is multiple ways and for alpha we consider it enough if it's not actually broken.

Comment 34

[Philip Chee]

Attachment: Patch 1.2: Don't touch non-primary toolbars.

Changes since the last patch:
1. Don't add the "custom_mode" attribute to non-primary toolbars.
2. Don't disable the "Use default settings" menu item if the "custom_mode" attribute is "true" (or rather non-null).

> I'm also worried about how this will work w.r.t. customise window, which iirc
> will try to set or reset the mode without knowing that it's watching the pref.

Fear not O Fearless Leader. The toolkit customize toolbar code has a toolboxChanged() callback. It would be a matter of extreme simplicity to enhance this currently generic callback to return the most desirous of information and thence forthwith apply to our custom attributes as the situation demands.

Comment 35

[[e-mail]]

Comment on attachment 350740 [details] [diff] [review]
Patch 1.2: Don't touch non-primary toolbars.

>+                  !this.getAttribute("custom_mode")
>+                 ) {
>+                this.setAttribute("mode", style);
>+              }
Was discussed on IRC, but in fact you don't need the {}s at all, do you?

>+  var primarycustom = /toolbar-primary/.test(toolbar.getAttribute("class")) &&
>+                      toolbar.getAttribute("custom_mode");
How can custom_mode be true on a non-primary toolbar?

>+        toolbar.setAttribute("custom_mode", "");
has/removeAttribute (if it works with document.persist, which it should)

>+        toolbar.setAttribute("custom_mode", "true");
I'm not too keen on this attribute name but "ignorepref" was my best idea.

Comment 36

[Philip Chee]

Attachment: [checked in] Patch 1.3: nits fixed.

carrying forward r+=KaiRo/sr+=Neil

> Was discussed on IRC, but in fact you don't need the {}s at all, do you?
Fixed.

>>+  var primarycustom = /toolbar-primary/.test(toolbar.getAttribute("class")) &&
>>+                      toolbar.getAttribute("custom_mode");
> How can custom_mode be true on a non-primary toolbar?
Fixed.

>>+        toolbar.setAttribute("custom_mode", "");
> has/removeAttribute (if it works with document.persist, which it should)
Fixed.

>>+        toolbar.setAttribute("custom_mode", "true");
> I'm not too keen on this attribute name but "ignorepref" was my best idea.
Fixed - using "ignoremodepref"

Comment 37

[Mark Banner (:standard8)]

Comment on attachment 350783 [details] [diff] [review]
[checked in] Patch 1.3: nits fixed.

http://hg.mozilla.org/comm-central/rev/d1a28c00868b

Comment 38

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b3pre) Gecko/20081202 SeaMonkey/2.0a3pre] (nightly) (W2Ksp4)

V.Fixed