171013 - Fix and hookup toolbars (based on bug 22056) as text/icons/both in Modern.

Status: VERIFIED FIXED

(SeaMonkey :: Themes, defect)

Description

[Jason Bassford]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20020926
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20020926

From the last comment in bug 22056:

"I think that we can fix each of those in other bugs, the core is fixed:
* modern navigator looks odd because the throbber shrinks"

So - I'm opening a bug on the specific issue.  (I checked over the last two
days, but could find nothing else.)

I don't know if the above is the reason that the toolbar fix doesn't work under
Win32/Modern (I would have assumed it would have been checked in anyway and a
bug would have been opened on the "shrinking throbber" issue specifically).

In any case, I see the preferences in existence under Preferences -> Appearance
but they don't do anything.

Reproducible: Always

Steps to Reproduce:
1. Preferences -> Appearances.
2. Change the radio box between "Pictures and text", "Pictures only", and "Text
only".

Actual Results:  
Toolbar UI should change.

Expected Results:  
Nothing happens.

Comment 1

[José Jeria]

Jason, just a tip, you dont need to cc yourself to the bug. You are the 
reporter so you will get the mails.

Comment 2

[Sander]

cc-ing yourself makes for easy searching though.

Comment 3

[Jason Bassford]

Thanks for the tip - I know.  I do it so that I can do a simple Bugzilla query
of every bug I'm currently following - if I want a bug off of my radar I take
myself off the CC list.

> cc-ing yourself makes for easy searching though.

Oops!  Mid-air collision.   Exactly. <grin>  Now back to the actual bug...

Comment 4

[timeless]

not preferences

Comment 5

[timeless]

whoops

Comment 6

[Jason Bassford]

> not preferences

Odd.  I just used the same component that the original bug 22056 had been filed
against - assuming it would be the same.  Oh, well.

Comment 7

[Jason Bassford]

What the ...?  I didn't change it back to Preferences.

-> Themes again.  (?)

Comment 8

[Wenqing Jiang]

In the classic theme and "picture only", the search button still
has both picture and text.

Comment 9

[Jason Bassford]

Please open a separate bug on Classic theme issues.

Comment 10

[aaron spuler]

Attachment: Proposed fix

Edited navigator.css to show toolbar button text, but the only problem now is
that the little down arrow to go forward sometimes covers up the "rd" of the
"Forward" text.

Comment 11

[aaron spuler]

Comment on attachment 103875 [details]
Proposed fix

>PK

Comment 12

[aaron spuler]

Disregard the last comment, and the proposed fix attachment.  Here's what I
changed:  (skins\modern\navigator\navigator.css)

/* Hides text below the above buttons */
.toolbarbutton-1 > stack > .toolbarbutton-menubutton-button 
  > .toolbarbutton-text,
.toolbarbutton-1 > .toolbarbutton-text {
  display: all;                                /* original value display: none */
}

/* ..... dropmarker box ..... */

#back-button > .toolbarbutton-menubutton-stack
    > .toolbarbutton-menubutton-dropmarker,
#forward-button > .toolbarbutton-menubutton-stack
    > .toolbarbutton-menubutton-dropmarker
{
  margin: 30px 0px 15px 34px;              /*original values 30px 0px 0px 34px */
}

/* ::::: navbar-inner - the grooved area around the urlbar ::::: */

#nav-bar-inner {
  -moz-box-align: center;
  margin: 7px 0px 7px 5px;                  /* original values 7px 0px 0px 5px */
  border: 2px solid;
  -moz-border-top-colors: #A2AFBD #EBF4FF;
  -moz-border-right-colors: #D1D9E0 #A2AFBD;
  -moz-border-bottom-colors: #CFD7DE #939EAA;
  -moz-border-left-colors: #9FABB9 #D2DAE1;
  -moz-border-radius: 3px;
  padding: 0px 3px;
  min-width: 0px;
}


That's it.  I also embedded a new preview.gif with the updated icons.  

Comment 13

[aaron spuler]

To download my modified modern.jar file, go to http://www.cs.swt.edu/~as1130/new/

and download either modern11.zip for mozilla 1.1 or modern12b.zip for mozilla1.2b.

Comment 14

[Felix Miata]

*** Bug 175409 has been marked as a duplicate of this bug. ***

Comment 15

[Felix Miata]

Text doesn't work in Linux or OS/2 either. -> OS=All

1.2 milestone is long gone. Does this need to be reassigned to nobody?

Comment 16

[Samir Gehani]

Nav triage team: nsbeta1-

Comment 17

[sairuh (rarely reading bugmail)]

renominating. this feature is exposed in the prefs UI (Appearance panel) and
does not work.

Comment 18

[sairuh (rarely reading bugmail)]

chatted w/samir: not enough resources to hide the UI, so will relnote (and
adding helpwanted).

Comment 19

[sairuh (rarely reading bugmail)]

redux...

Comment 20

[Samir Gehani]

Nav triage team: nsbeta1+/adt2

Comment 21

[shliang]

Attachment: patch

Comment 22

[shliang]

Attachment: screenshots

remove groove around urlbar when text-only to save vertical space, adjusted
spacing in various other places

Comment 23

[Jason Bassford]

Is there a screenshot of text and icons together?  (You've got what looks like
two of icons only, although they're oddly different if you look at the search
button, and one of text only - which looks good.)  Or - is search the only
element that will actually combine text with an icon?

Comment 24

[shliang]

text and icons would be the first one in the screenshots (what it looks like 
currently - the modern skin does not normally have text under the toolbar 
buttons in navigator)

Comment 25

[Jason Bassford]

In that case I've completely misunderstood this bug (and bug 22056). <grin>

I thought that the summary was pretty clear - that you should be able to have
text / icons / or both text and icons with the Modern theme.  Perhaps part of
the confusion was that, despite the summary, my comment 0 indicated that it was
just extending bug 22056 into the Modern theme.

Should the summary of this bug be changed to read "Fix and hookup search
function as text/icon/both in Modern?"  (It looks now as if that's the only
element to which this summary actually applies.)  Because, given the current
summary, this patch isn't going to solve what I thought I'd originally filed...

Should I file a new bug on actually having "Preferences -> Show toolbars as"
make sense with the Modern theme (i.e. putting text under the toolbar buttons)?

Comment 26

[sairuh (rarely reading bugmail)]

shliang's right, the browser window in modern doesn't have text under the icons.

however, other windows (mailnews 3pane, composer, mail compose, address book,
aim compose (nscp only)) in modern do have icon+text. would we be able to
provide icon-only and text-only versions toolbars for those windows as well?

Comment 27

[shliang]

Attachment: mail toolbars

ok. i thought this bug was to make the text/pictures pref do something
sensible, given the current design of modern, which is what i did.

this feature was always working but appeared not to be in navigator because
there is no text under the buttons. so it did work in other windows, i just did
a little bit of cleanup there. 

i am not sure i want to add the option of having text below the toolbarbuttons
because there is no text there by design, which is the default, default being
the "text and pictures" option. then the way it currently looks would turn into
the "pictures only" option, which i don't think we want.

then there are the other windows to consider, right now there is a combination
of "pictures only" in navigator and "text and pictures" in everything else
which doesn't fit into one option. we would have to pick the same one for
everything then which would probably mean that the default for navigator in
modern would have both text and pictures, which would change the original
design, which i can't do without approval etc (and also doesn't look that good
visually)

Comment 28

[Jason Bassford]

Right.  My real problem is the same as what sairuh indicates in comment 17.

The Preferences UI indicates that you can see the toolbars as icons, text, or
both.  The normal expectation of people selecting "both" would be to see both
everywhere, and not just in one or two places. <grin>  (Such an expectation is
borne out by many other interfaces in which *all* elements change based on such
a preference.)

Don't get me wrong - the implementation of being able to switch between just
text and just icons (which this patch will do) is a *big* improvement, and
should definitely be checked in here - at the very least as a partial fix. 
(Then, at least, the preference would do *something*, even if not *everything*
one might expect.)

If text cannot be added to existing icons (where it doesn't currently exist) an
alternative solution to this bug (in addition to the suggested patch) would be
to simply remove the "Both" option from the preferences UI - assuming that it's
even possible to do that, since it should only be removed if Modern is being used.

However, that also presents other difficulties, as mentioned in comment 27,
because Both *IS* currently the default, and anybody who sees toolbars other
than those used by Navigator, or who has the Search button displayed, currently
will be used to Both in those cases - and would complain quite strongly if they
were suddenly forced to see only text or icon representation.

FWIW: I've always been assuming that I'd only been seeing icons in my toolbars -
because I don't have the Search button displayed, and I only use Navigator.  It
was only when I (just now) explicitly looked at my settings that I discovered
it had been set to Both.  The behaviour the Navigator toolbar currently exhibits
for both (in my case) led me to believe that Icons was the default.

Given all of this, what I'm tempted to do here is to leave this bug exactly as
shliang assumed it to be - so that if the proposed patch is checked in, this bug
will be resolved.  However, I think I should *also* open a separate bug, setting
this bug as a dependency, for adding text to the Navigator buttons in Modern
(and any other toolbar element that doesn't currently have it) so that the
existing preference actually make sense for anybody who uses "Both" as their
preference setting.

Any comments / objections to doing this?  (Does anybody think that it should all
be covered in this bug?  Or is it simpler to break these two issues apart?)

Comment 29

[shliang]

yes, that makes sense (breaking the issues apart)  

Comment 30

[Jason Bassford]

Filed bug 198476, and modifying summary to be explicit on what this bug's goals are.

Comment 31

[jag (Peter Annema)]

Comment on attachment 117886 [details] [diff] [review]
patch

I just remembered another way of making nav-bar-inner sync up with nav-bar:

<hbox id="nav-bar-inner" flex="1">
  <observes element="nav-bar" attribute="buttonstyle"/>
  <!-- rest of nav-bar-inner -->

Comment 32

[Felix Miata]

Re the screenshots in comment 22: there is much too much horizontal whitespace
between main "buttons" in text-only. What resolution were those shots made
using? A (the?) reason to choose text-only is to minimize space consumption by
the toolbar buttons.

Comment 33

[shliang]

Attachment: patch

Comment 34

[Jan Varga [:janv]]

Comment on attachment 118163 [details] [diff] [review]
patch

r=varga

Comment 35

[jag (Peter Annema)]

Comment on attachment 118163 [details] [diff] [review]
patch

sr=jag

Comment 36

[shliang]

resolving

Comment 37

[Grace Bush]

looks good on Win and Linux, waiting for Mac build

Comment 38

[Grace Bush]

Mac build 2003032803