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

VERIFIED FIXED in mozilla1.4alpha

Status

VERIFIED FIXED
16 years ago
11 years ago

People

(Reporter: jasonb, Assigned: shliang)

Tracking

({helpwanted, relnote})

Trunk
mozilla1.4alpha
helpwanted, relnote

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

16 years ago
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.
(Reporter)

Updated

16 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 1

16 years ago
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

16 years ago
cc-ing yourself makes for easy searching though.
(Reporter)

Comment 3

16 years ago
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

16 years ago
not preferences
Component: Preferences → Skinability
QA Contact: sairuh → pmac

Comment 5

16 years ago
whoops
Assignee: ben → shliang
Component: Skinability → Themes
(Reporter)

Comment 6

16 years ago
> 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.
Component: Themes → Preferences
(Reporter)

Comment 7

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

-> Themes again.  (?)
Component: Preferences → Themes

Comment 8

16 years ago
In the classic theme and "picture only", the search button still
has both picture and text.
(Reporter)

Comment 9

16 years ago
Please open a separate bug on Classic theme issues.

Updated

16 years ago
Keywords: mozilla1.2

Comment 10

16 years ago
Created attachment 103875 [details]
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

16 years ago
Comment on attachment 103875 [details]
Proposed fix

>PK
Attachment #103875 - Attachment is patch: false

Comment 12

16 years ago
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

16 years ago
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

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

Comment 15

16 years ago
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?
OS: Windows XP → All
Summary: Fix and hookup toolbars as text/icons/both under Win32/Modern. → Fix and hookup toolbars as text/icons/both in Modern
Keywords: nsbeta1
Hardware: PC → All

Comment 16

16 years ago
Nav triage team: nsbeta1-
Keywords: nsbeta1 → nsbeta1-
renominating. this feature is exposed in the prefs UI (Appearance panel) and
does not work.
Keywords: nsbeta1- → nsbeta1
Keywords: relnote
chatted w/samir: not enough resources to hide the UI, so will relnote (and
adding helpwanted).
Keywords: nsbeta1 → helpwanted, nsbeta1-
redux...
Keywords: nsbeta1- → nsbeta1

Comment 20

16 years ago
Nav triage team: nsbeta1+/adt2
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt2]
(Assignee)

Updated

16 years ago
Target Milestone: --- → mozilla1.4alpha
(Assignee)

Comment 21

16 years ago
Created attachment 117886 [details] [diff] [review]
patch
(Assignee)

Comment 22

16 years ago
Created attachment 117888 [details]
screenshots

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

Updated

16 years ago
Attachment #117886 - Flags: superreview?(jaggernaut)
Attachment #117886 - Flags: review?(varga)
QA Contact: pmac → gbush
(Reporter)

Comment 23

16 years ago
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?
(Assignee)

Comment 24

16 years ago
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)
(Reporter)

Comment 25

16 years ago
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)?
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?
(Assignee)

Comment 27

16 years ago
Created attachment 117896 [details]
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)
(Reporter)

Comment 28

16 years ago
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?)
Summary: Fix and hookup toolbars as text/icons/both in Modern → Fix and hookup toolbars as text/icons/both in Modern.
(Assignee)

Comment 29

16 years ago
yes, that makes sense (breaking the issues apart)  
(Reporter)

Comment 30

16 years ago
Filed bug 198476, and modifying summary to be explicit on what this bug's goals are.
Blocks: 198476
Summary: Fix and hookup toolbars as text/icons/both in Modern. → Fix and hookup toolbars (based on bug 22056) as text/icons/both in Modern.
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

16 years ago
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.
(Assignee)

Comment 33

16 years ago
Created attachment 118163 [details] [diff] [review]
patch
Attachment #117886 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #118163 - Flags: superreview?(jaggernaut)
Attachment #118163 - Flags: review?(varga)
(Assignee)

Updated

16 years ago
Attachment #117886 - Flags: superreview?(jaggernaut)
Attachment #117886 - Flags: review?(varga)

Comment 34

16 years ago
Comment on attachment 118163 [details] [diff] [review]
patch

r=varga
Attachment #118163 - Flags: review?(varga) → review+
Comment on attachment 118163 [details] [diff] [review]
patch

sr=jag
Attachment #118163 - Flags: superreview?(jaggernaut) → superreview+
(Assignee)

Comment 36

16 years ago
resolving
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 37

16 years ago
looks good on Win and Linux, waiting for Mac build

Comment 38

16 years ago
Mac build 2003032803
Status: RESOLVED → VERIFIED
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.