Show toolbars as text/icons/both

VERIFIED FIXED

Status

SeaMonkey
Preferences
P3
enhancement
VERIFIED FIXED
18 years ago
6 years ago

People

(Reporter: sairuh (rarely reading bugmail), Assigned: neil@parkwaycc.co.uk)

Tracking

(Blocks: 1 bug, {helpwanted})

Trunk
helpwanted
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [br])

Attachments

(2 attachments, 27 obsolete attachments)

3.36 KB, patch
Details | Diff | Splinter Review
822 bytes, patch
Biesinger
: review+
dveditz
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

18 years ago
Tested with 1999121608/9-m12 on all platforms. Selecting Pictures Only, Text
Only or Pictures and Text doesn't seem to have any affect on the toolbars. ie,
the Navigation Toolbar still uses pictures, the Personal Toolbar still uses
text, and the Taskbar still uses both pictures and text.

is this expected behavior, or is something[s] not yet hooked up? estimates as
to when this feature'll be functioning?

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: M14

Comment 1

18 years ago
This might be hooked up in m14 hopefully.

Updated

18 years ago
Target Milestone: M14 → M15

Comment 2

18 years ago
M15 ...

Comment 3

18 years ago
Bulk move of all Pref UI component bugs to new Preferences component.  Pref UI 
component will be deleted.
Component: Pref UI → Preferences

Comment 4

18 years ago
Move to M16 for now ...
Target Milestone: M15 → M16

Comment 5

18 years ago
Reassigning for Don
Assignee: matt → ben
Status: ASSIGNED → NEW
Keywords: nsbeta2
Target Milestone: M16 → M17

Comment 6

18 years ago
*** Bug 25921 has been marked as a duplicate of this bug. ***

Comment 7

18 years ago
*** Bug 25921 has been marked as a duplicate of this bug. ***

Comment 8

18 years ago
Putting on [nsbeta2+][5/16] radar.  This is a feature MUST complete work by 
05/16 or we may pull this feature for PR2.
Whiteboard: [nsbeta2+][5/16]
I will remove this element for nsbeta2, because a) this is a feature and it's 
past 5/16 (=) b) it requires a fair bit of code to get right (given that it has 
never worked right in the past) c) this is possible via skins/user stylesheet.

Fix for removal in hand, time to complete: 5 minutes to check in. Once that's 
done I'll mark this an RFE and shift it off some. 
Status: NEW → ASSIGNED
hiding of fieldset checked in. leaving open, reducing severity and shifting off 
for future.
Severity: major → enhancement
Keywords: nsbeta2
Whiteboard: [nsbeta2+][5/16]
Target Milestone: M17 → Future
(Reporter)

Comment 11

18 years ago
geez, this fell through the cracks (ack). nominating for beta3 (hope against 
hope)...
Keywords: 4xp, helpwanted, nsbeta3
(Reporter)

Comment 12

18 years ago
my eyes were momentarily blind to the enhancement and future fields. removing 
beta3 nomination.
Keywords: nsbeta3

Comment 13

18 years ago
*** Bug 51141 has been marked as a duplicate of this bug. ***

Comment 14

17 years ago
*** Bug 52021 has been marked as a duplicate of this bug. ***

Comment 15

17 years ago
[Resummarizing to minimize dups]

One thing that would be nice would be to allow the text to go on the right (or 
the left, for RTL-reading locales) of the icon. So the options would be

Show toolbar buttons as:
( ) pictures
( ) text
( ) pictures and text (horizontal)
(*) pictures and text (vertical)
Summary: Appearance > Show toolbars as not hooked up? → Show toolbars as text/icons/both
(Reporter)

Comment 16

17 years ago
paul/ben, how feasible would this be nowadays?
Whiteboard: se-radar

Comment 17

17 years ago
From bug 51695: it should be possible to choose the size of the buttons when 
text text is turned off.  Some users will want to keep makes the images larger 
so that the button is still easy to hit, and others will want smaller buttons 
so that the navigation toolbar takes up less space.

Comment 18

17 years ago
Jesse, file a separate RFE for that.

Comment 19

17 years ago
*** Bug 60458 has been marked as a duplicate of this bug. ***
*** Bug 60693 has been marked as a duplicate of this bug. ***

Comment 21

17 years ago
*** Bug 60974 has been marked as a duplicate of this bug. ***

Comment 22

17 years ago
removing milestone in the hopes that it might get retargeted for mozilla 1.0 :-)

from bug #60974--the height of the toolbar should be adjusted (shrink) when it is 
"text only"
Keywords: nsmac2
Target Milestone: Future → ---

Comment 23

17 years ago
adding this phrase for future queries:  http://www.macintouch.com/netscape6.html

Comment 24

17 years ago
Here's hoping that this will be a step forward towards being able to 'float' the
navigation toolbar next the to the main menu bar to acheive a minimalistic
browser UI as can be done in IE5. Adding self to cc.
Nominating as I don't think Moz should go out without this.  I prefer Modern and
icons, but others might not be so lucky.
Keywords: mozilla1.0, softui

Comment 26

17 years ago
Filed bug 62444 for the ability to choose the size of the images on the buttons.

Comment 27

17 years ago
*** Bug 67199 has been marked as a duplicate of this bug. ***

Comment 28

17 years ago
Nominating for nsbeta1: seven dups, and also one of the most frequently asked-
for features on newsgroups.
Keywords: nsbeta1
Moving mozilla nomination forward to 0.9, I think this might be a bit of effort
and 1.0 might be too late.
Keywords: mozilla0.9
*** Bug 69767 has been marked as a duplicate of this bug. ***

Comment 31

17 years ago
*** Bug 70848 has been marked as a duplicate of this bug. ***

Comment 32

17 years ago
Adding mostfreq keyword: 9 dups, 9 votes.
Keywords: mostfreq
It would be really nice if this was for all themes automatically, or at least at
was easy for a theme to do, but I guess anything's better than nothing.

The current situation with Modern MailNews & Composer having both and Modern
Browser having just pictures is just silly.

Comment 34

17 years ago
MPT suggested we should have the option of having the text labels below the
icons (as in netscape) or horizontal as is the default for IE these days.

If we do have the horizontal option we'll need to take the IE approach and only
do this for selected icons otherwise they won't all fit on the screen.

I don't think horizontal text labels would look good with any of the current
themes, both the classic and modern icons appear bigger than their IE
counterparts and I think it'd just look weird with the current themes.

My view is to have whether the text labels are horizontal or vertical as an
option for the currently active skin, therefore a skin like native.windows (the
IE lookalike) can have horizontal labels.

The main problem usability wise is making it clear to the user that not all
skins will support  all options, so some skin may not have any text labels, etc.

Comment 35

17 years ago
I think preferences on a per skin basis is a long, ugly road we shouldn't go down. More to
the point, this bug is about the ability to turn on/off the text labels or the icons themselves
for the navigation toolbar etc.(4.x style). That ability is a feature that should be supported by
all themes. Where to put the text sounds like a skin designer issue.
(Assignee)

Comment 36

17 years ago
Created attachment 28899 [details] [diff] [review]
patch to toolbox binding to maintain attribute based on toolbar pref
(Assignee)

Comment 37

17 years ago
This patch is not intended to be a complete solution. The style="display: none;"
needs to be removed from from appearance preferences. Also skins need CSS styles.

toolbox[tbstyle="pictures"] > .toolbar-primary ...
toolbox[tbstyle="text"] > .toolbar-primary ...
toolbox[tbstyle="both"] > .toolbar-primary ...

Comment 38

17 years ago
cool
Assignee: ben → neil
Status: ASSIGNED → NEW
Keywords: patch, review
neil@parkwaycc.co.uk - nice one, mate :-) Are you chasing some reviewers and 
super-reviewers for your patch?

Also, is that PrefObserver stuff documented anywhere? It looks really handy...

Gerv
Keywords: nsCatFood
Blocks: 74644
Neil, could you finish up your patch with the skins changes etc and then get
reviews and sr. This is a very useful feature to have. I'd be glad to help with
the reviewing. thanks, Vishy
(Assignee)

Comment 41

17 years ago
Created attachment 31451 [details] [diff] [review]
Updated tookit patch to remove dump statement :-)
(Assignee)

Comment 42

17 years ago
Created attachment 31453 [details] [diff] [review]
Patch to enable the pref panel
(Assignee)

Comment 43

17 years ago
Created attachment 31455 [details] [diff] [review]
Patch to modern skin that allows text to be turned off. Slight display problem with dual menubuttons. Can't work out how to turn images off if at all.
(Assignee)

Comment 44

17 years ago
Created attachment 31458 [details] [diff] [review]
Patch to Classic skin for Windows
(Assignee)

Comment 45

17 years ago
Created attachment 31459 [details] [diff] [review]
Patch to Classic for Mac that might or might not work...
(Assignee)

Comment 46

17 years ago
Created attachment 31462 [details]
animthrob_sm.gif (7K)
(Assignee)

Comment 47

17 years ago
Created attachment 31463 [details]
animthrob_sm_single.gif (<1K)
(Reporter)

Updated

17 years ago
Whiteboard: se-radar
(Assignee)

Comment 48

17 years ago
Created attachment 31571 [details] [diff] [review]
Better patch for Modern dual menubuttons

Comment 49

17 years ago
Modern 3 should eliminate a lot of the problems here.  It will have text under 
all buttons, or at least the space is there now.  What we need to do is make 
this text a child XUL element of the main toolbar that cn be displayed or not.  
Every skin would have to include CSS definitions for this text.  As a child 
element, it would react to the same mouse-overs that the image does.  The mouse-
over border-bottom for the image and the border-top for the text would be 
eliminated to make it look like one button.. There are pixel tricks that can be 
done to make tht happen

Comment 50

17 years ago
cc'ing hewitt for modern 3 info: Joe, how do the patches in this bug look in
relation to your work?

Comment 51

17 years ago
What needs to happen above all is that regardless of the skin someone is using,
regardless of which app it is, toolbar buttons can be text only, pictures only
or both.  Every skin has to be able to cope with these three possibilities, and
changing the pref should not change the skin, but instead change the XUL being
used.  That's the best way to structure this and minimize dependencies while
maintaining a logical object-oriented program.. I can't understand those patches
up there to save my life, I stopped learning after html4.  Are those patches
going to do what needs to be done? I'd learn XUL and fix this mess for you, but
I've got IB and AP exams coming up.
(Assignee)

Comment 52

17 years ago
In Classic to hide the text or images is a simple display style. Modern was only
harder because of its dual menubuttons. Much simpler than using pixel tricks.

Comment 53

17 years ago
What's the likelihood of getting this resolved by the mozilla0.9.1 deadline?
.Angela...
(Assignee)

Comment 54

17 years ago
Created attachment 33522 [details] [diff] [review]
Better patch to toolboxBindings.xml using inline object property

Comment 55

17 years ago
Neil, can you point out which of the patches here are final so we can review 
and get this in?  Also, please don't add the dump()s -- thanks.
Whiteboard: [br]
(Assignee)

Comment 56

17 years ago
Sorry about the dump statement. Do you want me to recreate the patch?
Attachment Status
========== ======
     28899 Superceded by 31451
     31451 As 28899 but removing dump. Final.
     31453 Final
     31455 Superceded by 31571
     31458 Final
     31459 Final
     31462 Final
     31463 Final
     31571 I think this was superceded by Modern 3 theme :-(
     33522 Alternative to 28899.

Comment 57

17 years ago
As much as I would love to land this, the Modern theme is not exactly ready for
this feature.  We'd still need to create a bunch of smaller toolbar button
images, and it's uncertain if we'll have to time to do that given our current
schedule.  So, I'm not sure if it's a good idea to land this before our themes
are ready for it.

Comment 58

17 years ago
Hewitt, a lot of us use the classic theme (and it's the default in mozilla.org
builds)... Much as I'd love for Modern to support this too, I'd still rather
have classic-and-third-party-theme-only support than none at all as is the case
right now.

From what I've seen of Modern3 (which is only screenshots; I only use the
milestones usually) it looks like this would be trivial to support for modern3
mail/news, but not for navigator which still uses the circular buttons with no
text under them. Is this correct?
It's easy to query the chrome registry to get the name of the current theme. We 
can add 5 lines of JS which remove (hide) the menu item if Modern is selected. 
We can then remove them when it supports this. No problem.

Gerv

Comment 60

17 years ago
We could, but that seems silly -- why special case Modern?  If we had to take 
care of every theme out there, and design our UI around them, we'd have a big 
problem.  Classic is the default anyways, so I think if we can get Classic 
ready, this is a go.

Comment 61

17 years ago
We make it a general policy to never use the descendant combinator ("toolbar
button { ... }") for performance reasons. Soo.. change your selectors that
currently say something like this:

toolbox[tbstyle="pictures"] > .toolbar-primary .button-toolbar-1

to use a string of child combinators, like this:

toolbox[tbstyle="pictures"] > .toolbar-primary > .toolbar-holder > .button-toolbar-1





(Assignee)

Comment 62

17 years ago
Joe, where did you get .toolbar-holder from?
In general I can't tell what XUL (if any) lives between a toolbar and a button.

Comment 63

17 years ago
toolbar-holder is in the anonymous content of the toolbar bindings

Comment 64

17 years ago
Chiming in with a quick note about mozilla.org's position on skins in our tree.
 A couple of the above comments suggested that Modern was not important to
Mozilla.  This is not the case.   Modern is included in cvs.mozilla.org and
built by default.  We care about it and breaking it is not an option.  This
patch has to _not_ break Modern before it can land.  It does not have to provide
feature parity for Modern but it cannot break Modern.  

--Asa
(Assignee)

Comment 65

17 years ago
All the patches are now out of date and I won't be able to update them until June.

The patches to the toolbox binding and pref panel won't break anything.
However they require help from the skins to have any visual effect.

Comment 66

17 years ago
FYI - I have implemented this in Komodo (only a no text option right now). It 
works well but is not implemented as elegantly as I would like.

What I did was essentially created a new binding for 'no text' buttons and to 
switch back and forth I just switch the class of the buttons. However, this 
turns out to be multiple class switching (and multiple bindings) if you have 
menubuttons or buttons with toggle behaviour.

So it would be nice if all the logic was contained in the implementation of a 
single binding, which could be called to make the switch.

If I investigate more, I'll post my findings here.
(Assignee)

Comment 67

17 years ago
Here are five of my alternatives:

1. New bindings for top buttons which individually watch the preference.
   Menubuttons internally contain a top button which will watch the pref.
   Throbbers can also be inherited from top buttons.
2. New binding for i. toolboxes or ii. primary toolbars which watch the pref.
   a. Loop for elements with tag name *button and set an attribute.
   b. Set an attribute on the toolbox/bar and use the descendant combinator.

I have 2.i.a. and b. working. I haven't tried option 1 yet.

Comment 68

17 years ago
*** Bug 88647 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 69

17 years ago
Created attachment 41048 [details] [diff] [review]
patch to new toolbar.xml for primary toolbars to set tbstyle on buttons and menubuttons
(Assignee)

Comment 70

17 years ago
Created attachment 41054 [details] [diff] [review]
Patch to Windows Classic skin
(Assignee)

Comment 71

17 years ago
Created attachment 41177 [details] [diff] [review]
Sorry for the spam. Combined patch for Windows Classic only. Still needs previously attached images.
(Assignee)

Comment 72

17 years ago
Created attachment 41178 [details] [diff] [review]
_really_ sorry for the spam. forgot to diff appearance pref.

Comment 73

17 years ago
Would be neat with some screenshots attached...
(Assignee)

Comment 74

17 years ago
Created attachment 41630 [details]
screenshots and polished diff

Comment 75

17 years ago
The screenshots from neil@parkwaycc.co.uk are exactly the way I hope it will
work once released.

Comment 76

17 years ago
Nitpick: the "Stop" text doesn't appear to be centered under the image in the
screenshot.
(Assignee)

Comment 77

17 years ago
Jesse Ruderman wrote:

> Nitpick: the "Stop" text doesn't appear to be centered under the image in the
> screenshot.

I hate to say this but the screenshot of images and text is the current look...
(Assignee)

Comment 78

17 years ago
Created attachment 42542 [details] [diff] [review]
updated patch because of bitrot and strict warnings

Comment 79

17 years ago
In your patch there are modifications to the folder pane, and lots of other
things in messenger. 

Is this patch supposed to implement text/icons/both for toolbars or is it more?
(Assignee)

Comment 80

17 years ago
Created attachment 42544 [details] [diff] [review]
So sorry for the SPAM, I attached the wrong file.

Comment 81

17 years ago
Heh. Ok, what is the change to brand.css in the latest patch? :)
(Assignee)

Comment 82

17 years ago
Håkan Waara wrote:

> Heh. Ok, what is the change to brand.css in the latest patch? :)

The patch to bug 73671 supercedes the vertical-align on the throbber box.
(Assignee)

Comment 83

17 years ago
Created attachment 42548 [details] [diff] [review]
Håkan Waara noticed I diffed wrongly so more spam I'm afraid
(Assignee)

Comment 84

17 years ago
Created attachment 43216 [details] [diff] [review]
updated for typo

Comment 85

16 years ago
I've been using a variation on this code in my userChrome.css and noticed that
it wasn't working right as of 0.9.4. The reason turned out to be a vbox added in
buttonBindings.xml around the .simplebutton-text item. Changing ">
.simplebutton-text" to "> vbox > .simplebutton-text" fixed my problem. I'm
mentioning this here just in case an equivalent change also needs to be made to
this patch. You'll probably also need to change .simplebutton-icon in the same way.

Btw is there any traction on getting this into the tree? Is the current patch in
a state that's worth looking for review on (after bitrot has been tidied up)?
neil: are you chasing review for this patch?

Gerv
(Assignee)

Comment 87

16 years ago
The patch is about to suffer major bitrot because of the new <toolbarbutton>s :-(
(Reporter)

Comment 88

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

Comment 89

16 years ago
someone correct me if i am wrong. i think we are going to try to implement this 
in 0.9.5- timeframe.  i am currently about half way through a spec which details 
this and other features related to toolbar customization UI.  there will be 
extensive changes to Modern to accommodate the features.  I believe Hyatt is 
working on some of its implementation

Comment 90

16 years ago
Marlon, there is already a patch attached to fix this bug. Are you saying you're
gonna reinvent the wheel? :)

Comment 91

16 years ago
They already did reinvent the wheel and it turned into an 'M' (or 'N' in
Netscape builds) :)
Sorry couldn't resist (I was talking about the taskbar icons BTW - bug 92018)

Wasn't the main reason this patch wasn't checked in because of issues with the
modern theme (although saying this bug predates modern 3 then the skin should
have been designed with this in mind). Is there any bugs detailing the work
Hyatt is doing on getting the modern theme ready for this feature? As a user of
the modern theme I hope to see this feature implemented soon. I'll be interested
to see Marlon's spec for toolbar customisation when it is ready because this is
one of the main things that Mozilla lacks.

Comment 92

16 years ago
sorry, i wasn't referring to this bug specifically.  what i meant was that the
design spec that addresses this bug is now part of a "toolbar customization" UI,
which includes some other features which Hyatt will be implementing. However, in
regard to this bug, there are issues with the design of modern which will limit
you from implementing it exactly the same as 4.x.  Essentially, we want the
freedom to allow each theme to determine it's own space saving method, or what
previously constituted "text, icons only, text&icons" for 4.x.  these options
might now be more like "micro (text only), small, default, large".  there will
probably be 4 levels (or, perhaps still 3) of toolbar sizing which will enable
theme designers to interpret these more generically.
*** Bug 103618 has been marked as a duplicate of this bug. ***

Updated

16 years ago
Blocks: 104166

Comment 94

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

Comment 95

16 years ago
Marlon, in comment 89, way back in September, you said you were half way
finished with a spec proposal for this. Patches have been sitting in this bug
for over 9 months and you're the bottleneck. Are you done with this spec
proposal yet?

Comment 96

16 years ago
the default for this (with the Little Mozilla theme) changed from 11/23/01 to
1/02/02. Not it has both pictures and text. It used to have text on the personal
toolbar and pictures on the selector toolbar (which I liked).  Now I have no
room. And there is NO GUI for setting this preference.
James, I dont think that has anything to do with this bug but rather something
you take up with the author of the theme

Comment 98

16 years ago
*** Bug 123793 has been marked as a duplicate of this bug. ***
How close are we to actually getting this working ? The Tree for 0.9.9 just
closed tonight and this bug has the Mozilla 1.0 keyword. Can this realistically
be accomplished before 1.0 if not we will need to push this back.

Comment 100

16 years ago
asa, appreciate your encouragement there.  i stopped working on this spec
because there are these really bossy people at work who keep bothering me.  we
call them, "bosses".  oddly enough, every month since i've been going into their
building  (this just baffles me) i've been receiving these huge checks in my
mail. I am beginning to think it's those "bosses" who keep sending me money!
what idiots! anyway, i don't want to ruin that little mixup.

Comment 101

16 years ago
marlon, there are other people that were waiting on your work. If you stopped
that work you should have said so. You told a rather large group of people in
comment 89 that you were half way done and you thought that it would be
happening in 0.9.5.  When you received information from your bosses that you
were not going to be working on it you should have let others know. Leaving a
bunch of people in limbo sucks and adding a comment to the bug wouldn't have
taken that much effort. 
Can we use some of the full screen work for this ? The small browser icons are
in. Maybe this will actually help someone with the implementation

Comment 103

16 years ago
Alright, so it seems like we're back on track again!  Neil, can you update your
patch?  CC:ing some more likely reviewers so we can get some traction on this
sucker, finally.
(Assignee)

Comment 104

16 years ago
Unfortunately I think the small button set only exists for navigator but I'll
see what I can do... I assume you do want to them used in pictures only mode...
Well we could do that, but now that I thought about it we might be able to se
some of the full screen code I guess. Since it replaces the navbar buttons and
resizes the navbar itself, which is what we want to do right ? The current
images in the browser are ok for pictures only, but we might want to use the
full screen ones for small pictures or something. I'm just spouting out ideas
right now, trying to shorten the time it takes to actually implement this feature.
(Assignee)

Comment 106

16 years ago
OK I've updated the chrome portion of my patch but I've got to look at the skin
portion again because the fullscreen code will have bitrotted that as well...
I'm curious as you can use the code from Full-screen toolbar, if you can just
like in classic use the toolbar code itself, if you hit a switch or key it would
show the full-screen toolbar in place of the navigation toolbar.. this should
be fairly trivial to use it like a drop in replacement.. ie: hit button show this
toolbar, hit button show the other toolbar.. etc.   In theory it should be
re-usable dop-in code throughout mozilla for this stuff.  Just like adding in
and out the toolbar buttons in preferences panel.

Comment 108

16 years ago
*** Bug 127319 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 109

16 years ago
Created attachment 71233 [details] [diff] [review]
Patch for Modern and Windows Classic only
Attachment #28899 - Attachment is obsolete: true
Attachment #31451 - Attachment is obsolete: true
Attachment #31453 - Attachment is obsolete: true
Attachment #31455 - Attachment is obsolete: true
Attachment #31458 - Attachment is obsolete: true
Attachment #31462 - Attachment is obsolete: true
Attachment #31463 - Attachment is obsolete: true
Attachment #31571 - Attachment is obsolete: true
Attachment #33522 - Attachment is obsolete: true
Attachment #41048 - Attachment is obsolete: true
Attachment #41054 - Attachment is obsolete: true
Attachment #41177 - Attachment is obsolete: true
Attachment #41178 - Attachment is obsolete: true
Attachment #41630 - Attachment is obsolete: true
Attachment #42542 - Attachment is obsolete: true
Attachment #42544 - Attachment is obsolete: true
Attachment #42548 - Attachment is obsolete: true
Attachment #43216 - Attachment is obsolete: true
(Reporter)

Updated

16 years ago
Keywords: nsbeta1
we have a patch... Anyone want to review this so we can get this checked in ?
Scott, please email reviewers@mozilla.org for a R=/SR= is kinda the way to get
someone to review a patch.

Thanks,
Dennis
Get r= from module owner or peer (ask around, look at bug ownership, cvs logs,
etc.).  Then, mail a particular super-reviewer from the list at
http://www.mozilla.org/hacking/reviewers.html, cc'ing reviewers@mozilla.org. 
This is all doc'd at that URL.

/be

Comment 113

16 years ago
nsbeta1- per ADT triage. This can still be checked in with drivers approval.
Keywords: nsbeta1 → nsbeta1-
Comment on attachment 71233 [details] [diff] [review]
Patch for Modern and Windows Classic only

I can't usefully comment on most of the CSS... My comments on the JS:

>+          prefs: Components.classes["@mozilla.org/preferences;1"].getService(Components.interfaces.nsIPref),

Please use nsIPrefBranch (and nsIPrefBranchInternal to register/deregister
observers).  nsIPref is deprecated.

>+          observe: function observe() {

nsIObserver.observe() takes arguments.	Please have them here, even if
you're ignoring them -- that makes it clear what you're doing.

>+          init: function init() {
>+            this.prefs.addObserver(this.pref, this, false);
>+            this.observe();
>+          },
>+          destroy: function destroy() {
>+            this.prefs.removeObserver(this.pref, this);
>+          }

Have you checked that you addObserver and removeObserver the same object?
Try setting some property in init() and then dump()'ing it in destroy().  It
looks to me like this.mPrefObserver will create a new object on each access...

>+      <radio group="toolbarStyle" value="2" label="&picsNtextRadio.label;" accesskey="&picsNtextRadio.accesskey;"/>

How about "picsAndTextRadio.label" instead of "picsNtextRadio.label"?  Same
for the accesskey.
(Assignee)

Comment 115

16 years ago
Boris, thanks for the review. My replies to your comments:
1. When the code was originally written nsIPrefBranchInternal didn't exist
2. The code for mPrefObserver is an initializer, not a getter
3. The existing entity names were taken from pref-appearance.dtd
*** Bug 130839 has been marked as a duplicate of this bug. ***

Comment 117

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

Comment 118

16 years ago
adding self to cc list
*** Bug 141240 has been marked as a duplicate of this bug. ***

Comment 120

16 years ago
Neil: do you think that the patch is ready for reviewing (and checking in)? Any
progress?
(Assignee)

Comment 121

16 years ago
I haven't checked it for bitrot, apart from bz's comment that nsIPref is now
deprecated in favour of nsIPrefBranch, and the missing args to observe().

      <field name="mPrefObserver" readonly="true">
        <![CDATA[
          ({
          domain: "browser.chrome.toolbar_style",
          styles: ["pictures", "text", "both"],
          toolbar: this,
          prefs: Components.classes["@mozilla.org/preferences-service;1"]
                           .getService(Components.interfaces.nsIPrefBranchInternal),
          observe: function observe(subject, topic, name) {
            if (topic != "nsPref:changed") return;
            if (name != this.domain) return;
            var style = this.styles[this.prefs.getIntPref(name)];
            this.toolbar.setAttribute("tbstyle", style);
            this.update("toolbarbutton", style);
            this.update("button", style);
          },
          update: function update(tag, style) {
            var elements = this.toolbar.getElementsByTagName(tag);
            for (var i = 0; i < elements.length; i++)
              elements[i].setAttribute("tbstyle", style);
          },
          init: function init() {
            this.prefs.addObserver(this.domain, this, false);
            this.observe();
          },
          destroy: function destroy() {
            this.prefs.removeObserver(this.domain, this);
          }
          })
        ]]>
      </field>

Comment 122

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

Comment 123

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

Comment 124

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

Comment 125

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

Comment 126

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

Updated

16 years ago
Blocks: 135457

Updated

16 years ago
Keywords: nsbeta1- → nsbeta1
(Assignee)

Comment 127

16 years ago
Created attachment 90339 [details] [diff] [review]
Updated previous patch

Not only do I use the new pref observer mechanism, I make the toolbar itself
observer the pref instead of using a helper object! bz, eat your heart out :-)
Attachment #71233 - Attachment is obsolete: true
(Assignee)

Comment 128

16 years ago
Created attachment 91083 [details] [diff] [review]
Oops missed a QI
Attachment #90339 - Attachment is obsolete: true
> +      <radio group="toolbarStyle" value="0" label="&picsOnlyRadio.label;"
accesskey="&picsOnlyRadio.accesskey;"/>
> +      <radio group="toolbarStyle" value="1" label="&textonlyRadio.label;"
accesskey="&textonlyRadio.accesskey;"/>

If this is the only place those DTD entries are used, could we make the entity
names consistent?  So "&textOnlyRadio.foo;" or "&picsonlyRadio.foo;".

The QIing in <field name="prefs"> looks odd to me; let me check on that...

> +            var style = styles[this.prefs.getIntPref(name)];

What if getIntPref throws an exception?

Are things already using the "tbstyle" attribute?  If not, could we name it
"toolbarstyle"?

Someone other than me should do the CSS stuff, I think... I don't know that part
of things very well.
(Assignee)

Comment 130

16 years ago
> If this is the only place those DTD entries are used, could we make the entity
> names consistent?  So "&textOnlyRadio.foo;" or "&picsonlyRadio.foo;".

The entity names already exist; I would have to rename them.

> The QIing in <field name="prefs"> looks odd to me; let me check on that...

XPConnect remembers that I have QI'd the prefs to both nsIPrefBranch and
nsIPrefBranchInternal when I got the service for when I use it.

> What if getIntPref throws an exception?

Makes it difficult to change the pref (pref window doesn't catch exceptions).

> Are things already using the "tbstyle" attribute?  If not, could we name it
"toolbarstyle"?

I just wanted to save some typing :-)

Updated

16 years ago
Blocks: 157199
(Assignee)

Comment 131

16 years ago
Created attachment 92407 [details] [diff] [review]
This must be the one surely?
Attachment #91083 - Attachment is obsolete: true

Comment 132

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

Updated

15 years ago
Blocks: 163993
>   -moz-binding:
url("chrome://communicator/skin/toolbar/toolbarBindings.xml#toolbar-primary");
> -  min-height: 45px;

Why was this min-height there? Have you tested that you aren't regressing
whatever prompted its addition?

Add a comment to all.js saying what the various values of the
browser.chrome.toolbar_style pref mean?

With that, sr=bzbarsky.  Looks good!
(Assignee)

Comment 134

15 years ago
Created attachment 98426 [details] [diff] [review]
Added comment to all.js

> > -  min-height: 45px;
> Why was this min-height there? Have you tested that you aren't regressing
> whatever prompted its addition?

I can't tell why it was added, but the patch is unusable without the removal.
Comment on attachment 98426 [details] [diff] [review]
Added comment to all.js

sr=bzbarsky
Attachment #98426 - Flags: superreview+

Comment 136

15 years ago
The work I've done on customizable toolbars is a superset of this...

Comment 137

15 years ago
I still have a few bugs to work out before I can land that work on the trunk 
though. :)
Hyatt, what's your point? That we shouldn't check this in?

Comment 139

15 years ago
If Hyatt's patch is "better" but it still "needs work" then why not check in the
existing patch for now and back it out later when Hyatt's is ready?  (Assuming
that the existing patch does not do the job as well...)

Comment 140

15 years ago
Sounds reasonable to me.
Ew, _more_ prefs for our already over-packed prefs panel?

This must get module owner approval before being checked in. Ben?
Of course.  I _did_ specifically request (over irc) that every effort be made to
contact blake or ben for comment on this patch...  (and I'm sending them mail
myself).

As a note, the "appearance" panel (where this is going based on the current
patch) is currently pretty empty...

Comment 143

15 years ago
Fine with me all if you want to check this in...

Updated

15 years ago
Attachment #98426 - Flags: review+
So what are we waiting for? Check this in now! I'm too excited to wait,
especially now that we have approval. We may make it before this bug gets 3
years old! :)
> So what are we waiting for? Check this in now!

Well, we aren't waiting for you.  This did get checked in already, over 4 hours
ago (read the checkin logs next time, maybe?).  However don't start jumping for
joy.  It might get backed out since it appears to have caused a regression in
both Ts (startup) and Txul (Page load) times.
> So what are we waiting for?

We were waiting for the closed-for-1.2alpha tree to reopen.

Like caillon said, this regressed window open by what looks like 5% or so, and
regressed startup by about 1.5%.  Both are pretty unacceptable; I've filed
blocker bug 168029
Comment on attachment 98426 [details] [diff] [review]
Added comment to all.js

I've backed out all but the changes to all.js.	(which was a useful
comment-only change).

I don't have time to debug this in detail, but I suspect that using
xbl:inherits to inherit toolbarmode or buttonstyle (whichever is relevant) down
into the bindings (see http://www.mozilla.org/xpfe/goodcss.html) would make the
patch more performant...
Attachment #98426 - Flags: needs-work+
Interestingly, it looks like only half the Txul hit was due to the CSS changes.
 The other half was the XBL changes (see this morning's times on comet as I
backed out the pieces separately).  The Ts was _entirely_ due to the XBL changes.
(Assignee)

Comment 149

15 years ago
Created attachment 98935 [details] [diff] [review]
Improved perf

Well, I started checking Txul times and got from 17s to 16.5s (3%!) but after
fixing the CSS rules to work with full screen mode my PC hung for 3 minutes
before deciding that Txul was now 5s so I would appreciate independent testing!
Attachment #92407 - Attachment is obsolete: true
Attachment #98426 - Attachment is obsolete: true
(Assignee)

Comment 150

15 years ago
Oh, and you'll want to know what I did... I added xbl:inherits for buttonstyle
and toolbarmode in various places to reduce the CSS rules, and I also changed
the default attribute to null which speeds up that case slightly.
(Assignee)

Comment 151

15 years ago
Figured out my low Txul times - my XUL cache was off. So, without the XUL cache,
the new patch is 5% faster than the old patch.
Really?  5% was about the Txul hit of the old patch, no?  That sounds
encouraging.. ;) How does "new patch" compare to "unpatched"?
(Assignee)

Comment 153

15 years ago
Created attachment 99254 [details] [diff] [review]
Best yet

Txul with this patch is within 0.4% compared to without this patch.
Some of my styles in the last patch were wrongly converted so I've fixed them.
Attachment #98935 - Attachment is obsolete: true
If you've got it down to within 0.4%, that's fine with drivers.  Ship it.

Comment 155

15 years ago
So? Anyone forgot about this one? Who will chech this in?
the patch ahs neither r= nor sr= and can therefore not be checked in.
Comment on attachment 99254 [details] [diff] [review]
Best yet

I have had no chance to test this (no tree), but looks like the right thing to
do.  sr=bzbarsky
Attachment #99254 - Flags: superreview+
urg

+        Components.classes["@mozilla.org/preferences-service;1"]
+                  .getService(Components.interfaces.nsIPrefBranchInternal)
+                  .QueryInterface(Components.interfaces.nsIPrefBranch)

no. don't do that. do not QI the pref service to prefbranch. and if you did, you
should do it directly, rather than via nsIPrefBranchInternal.
so write that above as:

Components.classes["@mozilla.org/preferences-service;1"]
          .getService(Components.interfaces.nsIPrefService)
          .getBranch(null)

why this change:
-  margin: 0 5px 0 0;
+  margin: 0 5px;

+.toolbarbutton-menubutton-dropmarker[buttonstyle="pictures"] {
+  margin: 20px 0px 0px 40px;

isn't 20px and 40px a bit excessive?

+  height: 0px;

0px?

+        Components.classes["@mozilla.org/preferences-service;1"]
+                  .getService(Components.interfaces.nsIPrefBranchInternal)
+                  .QueryInterface(Components.interfaces.nsIPrefBranch)

same as above

+              const style = styles[this.prefs.getIntPref(name)];
what if a) the pref is not set or b) this returns 2 (both) ?

+      <radio group="toolbarStyle" value="2" label="&picsNtextRadio.label;"
accesskey="&picsNtextRadio.accesskey;"/>

I don't see a .dtd file in the patch?
(Assignee)

Comment 159

15 years ago
>+        Components.classes["@mozilla.org/preferences-service;1"]
>+                  .getService(Components.interfaces.nsIPrefBranchInternal)
>+                  .QueryInterface(Components.interfaces.nsIPrefBranch)
>
>no. don't do that. do not QI the pref service to prefbranch. and if you did, you
>should do it directly, rather than via nsIPrefBranchInternal.
No, because I need PrefBranch to get the pref, but PrefBranchInteral to add the
observer.

>why this change:
>-  margin: 0 5px 0 0;
>+  margin: 0 5px;
The old figures were for full screen mode which doesn't have a URL bar, but for
text/icons mode you need some margin to stop the URL bar from jamming the print
button up against the throbber.

>+.toolbarbutton-menubutton-dropmarker[buttonstyle="pictures"] {
>+  margin: 20px 0px 0px 40px;
>
>isn't 20px and 40px a bit excessive?
I copied these figures from navigator.css, they seem to work there...

>+  height: 0px;
>
>0px?
Because of the way it flexes, I want to override the intrinsic height.

>+              const style = styles[this.prefs.getIntPref(name)];
>what if the pref is not set?
The pref must be set because it exists in all.js

>+      <radio group="toolbarStyle" value="2" label="&picsNtextRadio.label;"
>accesskey="&picsNtextRadio.accesskey;"/>
>
>I don't see a .dtd file in the patch?
The entities were never removed, so I don't have to add them back.

Comment 160

15 years ago
Comment on attachment 99254 [details] [diff] [review]
Best yet

ok, with ncompare i can see and understand the improvement between 98426 and
99254.

r=timeless
Attachment #99254 - Flags: review+
Neil, you did not reply to this statement by me:
+        Components.classes["@mozilla.org/preferences-service;1"]
+                  .getService(Components.interfaces.nsIPrefBranchInternal)
+                  .QueryInterface(Components.interfaces.nsIPrefBranch)

no. don't do that. do not QI the pref service to prefbranch. 
(Reporter)

Updated

15 years ago
QA Contact: sairuh → claudius
Biesi, this was already discussed in comment 129 and comment 130.  Do you have a
good reason not to do what Neil is doing?  If so, we're all ears.
bz: It was not.
I am complaining that he QIs the pref service to the pref branch. which I
believe is not guaranteed by anything to work.

that he QIs the BranchInternal to Branch to get all the method is not what I
complain about.
bz: but maybe I have just not understood his explanation =)
Oh, I see what you mean... This pattern is very commonly used in our code, and
the contractid in question is frozen.  So yes, you are guaranteed that anything
that fulfills that contract will implement nsIPrefBranch.  Of course there's no
explicit guarantee that it's the root branch...
And addressed again in comment 159.  I understand the need for this, but Neil,
would it make more sense to just have mPrefs only be an nsIPrefBranch?  That's
how everything else that uses prefs does it (unless I missed a usage).  The only
reasoning one might want to do that really is to be able to call .observe() or
something on it (as you do) but that seems kind of wrong.  Shouldn't you
propogate that stuff through the observer, so others who want to listen in on
that can?  It seems to me that creating a locally scoped object ( e.g.  {
domain: "foo", observe: function() { ... } } ) might be a good solution here. 
Plus ISTR that adding methods like this add bloat onto the binding.  A locally
scoped object to do your observing stuff here may alleviate that.
Look at how tabbrowser.xml does its progress listeners.  I think something like
that adapted for nsIObserver would work here.
>Oh, I see what you mean... This pattern is very commonly used in our code

urg, ok... 

personally, I would still use getBranch.

caillon: no it was not addressed there.
Uhh.  This got checked in and it doesn't work.

Comment 170

15 years ago
Created attachment 100523 [details] [diff] [review]
hookup classic

i'm still looking into modern, but classic was really easy.
Comment on attachment 100523 [details] [diff] [review]
hookup classic

r=glazman
Attachment #100523 - Flags: review+

Comment 172

15 years ago
ok, it turns out that modern doesn't affect navigator or mailnews. the urlbar in
navigator makes the height not worth it, i suspect it's the same deal w/ the
masthead in mailnews.  but using the manually applied patch, modern works in
addressbook and composer.

neil is going to attach a slightly more correct patch for classic probably to
communicator.css, although it probably will be equivalent to the patch i attached.
(Assignee)

Comment 173

15 years ago
Created attachment 100533 [details] [diff] [review]
File accidentally left out of patch, sorry

Updated

15 years ago
Attachment #100533 - Flags: review+

Updated

15 years ago
Attachment #100523 - Attachment is obsolete: true
(Assignee)

Comment 174

15 years ago
Oh I see, you wanted

<field name="prefs" readonly="true">
  Components.classes["@mozilla.org/preferences-service;1"]
            .getService(Components.interfaces.nsIPrefService).getBranch(null)
            .QueryInterface(Components.interfaces.nsIPrefBranchInternal)
</field>
Comment on attachment 100533 [details] [diff] [review]
File accidentally left out of patch, sorry

sr=dbaron
Attachment #100533 - Flags: superreview+
On windows with masthead pictures such as mail the masthead should shrink when
just as the throbber does in Communicator 4.x when you go to icon or text only.
You could make the case it should disappear entirely in the text-only mode.

Comment 177

15 years ago
Created attachment 100597 [details] [diff] [review]
fixup prefservice call
Attachment #99254 - Attachment is obsolete: true
Attachment #100533 - Attachment is obsolete: true
Comment on attachment 100597 [details] [diff] [review]
fixup prefservice call

r=biesi
looks much better now, thanks
Attachment #100597 - Flags: review+
Comment on attachment 100597 [details] [diff] [review]
fixup prefservice call

r=biesi
looks much better now, thanks
Comment on attachment 100597 [details] [diff] [review]
fixup prefservice call

r=biesi
looks much better now, thanks
Comment on attachment 100597 [details] [diff] [review]
fixup prefservice call

r=biesi
looks much better now, thanks
Comment on attachment 100597 [details] [diff] [review]
fixup prefservice call

r=biesi
looks much better now, thanks
Comment on attachment 100597 [details] [diff] [review]
fixup prefservice call

r=biesi
looks much better now, thanks
grrrrrr
I got a "timeout" message so I tried again...

looks like there was no timeout, but instead the comment was added multiple
times !"/§%
Comment on attachment 100597 [details] [diff] [review]
fixup prefservice call

sr=dveditz
Attachment #100597 - Flags: superreview+

Comment 186

15 years ago
- modern mail works

I think that we can fix each of those in other bugs, the core is fixed:
* modern navigator looks odd because the throbber shrinks
* I suspect mac-classic doesn't work
* third party themes (new-classic) won't work
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 187

15 years ago
I hate to complain about this fix, which works, but broke all the other themes).
However, the most important reason that I wanted it was to get rid of the folder
icons on the personal tool bar. Thise icons serve absolutely NO purpose and they
are still there. Only the name of the folder is needed to know what it is.

Comment 188

15 years ago
James, this patch has nothing to do with the personal toolbar. You can remove
the icons there with CSS. This is part of my userChrome.css file in my profile
to remove the icons on normal bookmarks. You can adapt this to remove folders
instead or also.

/* Kill icons on normal bookmarks */
toolbarbutton.bookmark-item:not(.bookmark-group):not([type="menu"])
> .toolbarbutton-icon {
        display: none;
}

toolbarbutton.bookmark-item {
        margin-left: 1px !important;
        margin-right: 1px !important;
}

/* Kill normal bookmark icons in the bookmarks menu */
menuitem.bookmark-item > .menu-iconic-left {
        display: none;
}

/* Kill only default tabbrowser icons (no site icon) */
.tabbrowser-tabs *|tab:not([image]) .tab-icon {
        display: none;
}

Comment 189

15 years ago
Timeless, why did you resolve this bug as fixed?  It's not apparent to me what happened, but in the interest of getting this implemented for Mozilla and Netscape, i'd like to reopen and attach some visual resources so that we can actually get it looking right in Modern.  Anyone know if the work  here is still useful, or is there another bug which now supersedes this?

Comment 190

15 years ago
Timeless, why did you resolve this bug as fixed?  It's not apparent to me what
happened, but in the interest of getting this implemented for Mozilla and
Netscape, i'd like to reopen and attach some visual resources so that we can
actually get it looking right in Modern.  Anyone know if the work  here is still
useful, or is there another bug which now supersedes this?

Comment 191

15 years ago
It's fixed because mozilla supports showing toolbars as text/icons/both.
Right now using 2003011908 w32talkback I have icons without text in classic in
navigator and messenger. Additionally, third party themes can support this feature.

If you need to improve the modern skin so that navigator supports this feature
then you should use a new bug. I'm not aware of bugs for it, I don't use modern
would leave filing and fixing those bugs to someone else.

Comment 192

15 years ago
Ever since "fixed", text-only in Modern works in mailnews but not browser. IOW,
never has been "fixed".
(Assignee)

Comment 193

15 years ago
Modern has only ever supported icon only buttons in navigator.
Changing that is beyond the scope of this bug.

Comment 194

15 years ago
ok thanks for the update. i think i wrongly assumed that this bug covered "small
buttons" feature as well.  Which is actually what i am after.  if anyone is
currently tracking a small buttons feature or know of a bug let me know,
otherwise i will create one. thanks

Comment 195

15 years ago
bug 171013 was opened for making this (specifically what happened in this bug
for classic) work in modern. There was some work in there by someone (which even
talks about updated icons), but it looks like he was new to mozilla, didn't
request review and only linked to his work rather than attaching it (and now it
doesn't exist anymore at the linked to place).

Comment 196

15 years ago
verifying as fixed (as of some time ago...)
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey

Updated

13 years ago
No longer blocks: 163993
You need to log in before you can comment on or make changes to this bug.