Closed
Bug 22056
Opened 25 years ago
Closed 22 years ago
Show toolbars as text/icons/both
Categories
(SeaMonkey :: Preferences, enhancement, P3)
SeaMonkey
Preferences
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: bugzilla, Assigned: neil)
References
(Blocks 1 open bug)
Details
(Keywords: helpwanted, Whiteboard: [br])
Attachments
(2 files, 27 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Biesinger
:
review+
dveditz
:
superreview+
|
Details | Diff | Splinter Review |
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?
Bulk move of all Pref UI component bugs to new Preferences component. Pref UI
component will be deleted.
Component: Pref UI → Preferences
Comment 5•25 years ago
|
||
Reassigning for Don
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]
Comment 9•24 years ago
|
||
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
Comment 10•24 years ago
|
||
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•24 years ago
|
||
geez, this fell through the cracks (ack). nominating for beta3 (hope against
hope)...
Reporter | ||
Comment 12•24 years ago
|
||
my eyes were momentarily blind to the enhancement and future fields. removing
beta3 nomination.
Keywords: nsbeta3
Comment 13•24 years ago
|
||
*** Bug 51141 has been marked as a duplicate of this bug. ***
Comment 14•24 years ago
|
||
*** Bug 52021 has been marked as a duplicate of this bug. ***
Comment 15•24 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•24 years ago
|
||
paul/ben, how feasible would this be nowadays?
Whiteboard: se-radar
Comment 17•24 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•24 years ago
|
||
Jesse, file a separate RFE for that.
Comment 19•24 years ago
|
||
*** Bug 60458 has been marked as a duplicate of this bug. ***
Comment 20•24 years ago
|
||
*** Bug 60693 has been marked as a duplicate of this bug. ***
Comment 21•24 years ago
|
||
*** Bug 60974 has been marked as a duplicate of this bug. ***
Comment 22•24 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•24 years ago
|
||
adding this phrase for future queries: http://www.macintouch.com/netscape6.html
Comment 24•24 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.
Comment 25•24 years ago
|
||
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•24 years ago
|
||
Filed bug 62444 for the ability to choose the size of the images on the buttons.
Comment 27•24 years ago
|
||
*** Bug 67199 has been marked as a duplicate of this bug. ***
Comment 28•24 years ago
|
||
Nominating for nsbeta1: seven dups, and also one of the most frequently asked-
for features on newsgroups.
Keywords: nsbeta1
Comment 29•24 years ago
|
||
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
Comment 30•24 years ago
|
||
*** Bug 69767 has been marked as a duplicate of this bug. ***
Comment 31•24 years ago
|
||
*** Bug 70848 has been marked as a duplicate of this bug. ***
Comment 33•24 years ago
|
||
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•24 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•24 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•24 years ago
|
||
Assignee | ||
Comment 37•24 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•24 years ago
|
||
cool
Comment 39•24 years ago
|
||
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
Comment 40•24 years ago
|
||
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•24 years ago
|
||
Assignee | ||
Comment 42•24 years ago
|
||
Assignee | ||
Comment 43•24 years ago
|
||
Assignee | ||
Comment 44•24 years ago
|
||
Assignee | ||
Comment 45•24 years ago
|
||
Assignee | ||
Comment 46•24 years ago
|
||
Assignee | ||
Comment 47•24 years ago
|
||
Reporter | ||
Updated•24 years ago
|
Whiteboard: se-radar
Assignee | ||
Comment 48•24 years ago
|
||
Comment 49•24 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•24 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•24 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•24 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•24 years ago
|
||
What's the likelihood of getting this resolved by the mozilla0.9.1 deadline?
.Angela...
Assignee | ||
Comment 54•24 years ago
|
||
Comment 55•24 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•24 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•24 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•24 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?
Comment 59•24 years ago
|
||
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•24 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•24 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•24 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•23 years ago
|
||
toolbar-holder is in the anonymous content of the toolbar bindings
Comment 64•23 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•23 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•23 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•23 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•23 years ago
|
||
*** Bug 88647 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 69•23 years ago
|
||
Assignee | ||
Comment 70•23 years ago
|
||
Assignee | ||
Comment 71•23 years ago
|
||
Assignee | ||
Comment 72•23 years ago
|
||
Comment 73•23 years ago
|
||
Would be neat with some screenshots attached...
Assignee | ||
Comment 74•23 years ago
|
||
Comment 75•23 years ago
|
||
The screenshots from neil@parkwaycc.co.uk are exactly the way I hope it will
work once released.
Comment 76•23 years ago
|
||
Nitpick: the "Stop" text doesn't appear to be centered under the image in the
screenshot.
Assignee | ||
Comment 77•23 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•23 years ago
|
||
Comment 79•23 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•23 years ago
|
||
Comment 81•23 years ago
|
||
Heh. Ok, what is the change to brand.css in the latest patch? :)
Assignee | ||
Comment 82•23 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•23 years ago
|
||
Assignee | ||
Comment 84•23 years ago
|
||
Comment 85•23 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)?
Comment 86•23 years ago
|
||
neil: are you chasing review for this patch?
Gerv
Assignee | ||
Comment 87•23 years ago
|
||
The patch is about to suffer major bitrot because of the new <toolbarbutton>s :-(
Reporter | ||
Comment 88•23 years ago
|
||
*** Bug 100671 has been marked as a duplicate of this bug. ***
Comment 89•23 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•23 years ago
|
||
Marlon, there is already a patch attached to fix this bug. Are you saying you're
gonna reinvent the wheel? :)
Comment 91•23 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•23 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.
Comment 93•23 years ago
|
||
*** Bug 103618 has been marked as a duplicate of this bug. ***
Comment 94•23 years ago
|
||
*** Bug 107123 has been marked as a duplicate of this bug. ***
Comment 95•23 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•23 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.
Comment 97•23 years ago
|
||
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•23 years ago
|
||
*** Bug 123793 has been marked as a duplicate of this bug. ***
Comment 99•23 years ago
|
||
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•23 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•23 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.
Comment 102•23 years ago
|
||
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•23 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•23 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...
Comment 105•23 years ago
|
||
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•23 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...
Comment 107•23 years ago
|
||
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•23 years ago
|
||
*** Bug 127319 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 109•23 years ago
|
||
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
Comment 110•23 years ago
|
||
we have a patch... Anyone want to review this so we can get this checked in ?
Comment 111•23 years ago
|
||
Scott, please email reviewers@mozilla.org for a R=/SR= is kinda the way to get
someone to review a patch.
Thanks,
Dennis
Comment 112•23 years ago
|
||
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•23 years ago
|
||
nsbeta1- per ADT triage. This can still be checked in with drivers approval.
Comment 114•23 years ago
|
||
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•23 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
Comment 116•23 years ago
|
||
*** Bug 130839 has been marked as a duplicate of this bug. ***
Comment 117•23 years ago
|
||
*** Bug 131565 has been marked as a duplicate of this bug. ***
Comment 118•23 years ago
|
||
adding self to cc list
Comment 119•23 years ago
|
||
*** Bug 141240 has been marked as a duplicate of this bug. ***
Comment 120•23 years ago
|
||
Neil: do you think that the patch is ready for reviewing (and checking in)? Any
progress?
Assignee | ||
Comment 121•23 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•23 years ago
|
||
*** Bug 143197 has been marked as a duplicate of this bug. ***
Comment 123•23 years ago
|
||
*** Bug 144127 has been marked as a duplicate of this bug. ***
Comment 124•22 years ago
|
||
*** Bug 133447 has been marked as a duplicate of this bug. ***
Comment 125•22 years ago
|
||
*** Bug 149960 has been marked as a duplicate of this bug. ***
Comment 126•22 years ago
|
||
*** Bug 150355 has been marked as a duplicate of this bug. ***
Updated•22 years ago
|
Assignee | ||
Comment 127•22 years ago
|
||
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•22 years ago
|
||
Attachment #90339 -
Attachment is obsolete: true
Comment 129•22 years ago
|
||
> + <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•22 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 :-)
Assignee | ||
Comment 131•22 years ago
|
||
Attachment #91083 -
Attachment is obsolete: true
Comment 132•22 years ago
|
||
*** Bug 163012 has been marked as a duplicate of this bug. ***
Comment 133•22 years ago
|
||
> -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•22 years ago
|
||
> > - 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 135•22 years ago
|
||
Comment on attachment 98426 [details] [diff] [review]
Added comment to all.js
sr=bzbarsky
Attachment #98426 -
Flags: superreview+
Comment 136•22 years ago
|
||
The work I've done on customizable toolbars is a superset of this...
Comment 137•22 years ago
|
||
I still have a few bugs to work out before I can land that work on the trunk
though. :)
Comment 138•22 years ago
|
||
Hyatt, what's your point? That we shouldn't check this in?
Comment 139•22 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•22 years ago
|
||
Sounds reasonable to me.
Comment 141•22 years ago
|
||
Ew, _more_ prefs for our already over-packed prefs panel?
This must get module owner approval before being checked in. Ben?
Comment 142•22 years ago
|
||
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•22 years ago
|
||
Fine with me all if you want to check this in...
Attachment #98426 -
Flags: review+
Comment 144•22 years ago
|
||
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.
Comment 146•22 years ago
|
||
> 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 147•22 years ago
|
||
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+
Comment 148•22 years ago
|
||
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•22 years ago
|
||
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•22 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•22 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.
Comment 152•22 years ago
|
||
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•22 years ago
|
||
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
Comment 154•22 years ago
|
||
If you've got it down to within 0.4%, that's fine with drivers. Ship it.
Comment 155•22 years ago
|
||
So? Anyone forgot about this one? Who will chech this in?
Comment 156•22 years ago
|
||
the patch ahs neither r= nor sr= and can therefore not be checked in.
Comment 157•22 years ago
|
||
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+
Comment 158•22 years ago
|
||
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•22 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•22 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+
Comment 161•22 years ago
|
||
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•22 years ago
|
QA Contact: sairuh → claudius
Comment 162•22 years ago
|
||
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.
Comment 163•22 years ago
|
||
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.
Comment 164•22 years ago
|
||
bz: but maybe I have just not understood his explanation =)
Comment 165•22 years ago
|
||
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.
Comment 168•22 years ago
|
||
>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.
Comment 169•22 years ago
|
||
Uhh. This got checked in and it doesn't work.
Comment 170•22 years ago
|
||
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•22 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•22 years ago
|
||
Attachment #100533 -
Flags: review+
Attachment #100523 -
Attachment is obsolete: true
Assignee | ||
Comment 174•22 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 175•22 years ago
|
||
Comment on attachment 100533 [details] [diff] [review]
File accidentally left out of patch, sorry
sr=dbaron
Attachment #100533 -
Flags: superreview+
Comment 176•22 years ago
|
||
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•22 years ago
|
||
Attachment #99254 -
Attachment is obsolete: true
Attachment #100533 -
Attachment is obsolete: true
Comment 178•22 years ago
|
||
Comment on attachment 100597 [details] [diff] [review]
fixup prefservice call
r=biesi
looks much better now, thanks
Attachment #100597 -
Flags: review+
Comment 179•22 years ago
|
||
Comment on attachment 100597 [details] [diff] [review]
fixup prefservice call
r=biesi
looks much better now, thanks
Comment 180•22 years ago
|
||
Comment on attachment 100597 [details] [diff] [review]
fixup prefservice call
r=biesi
looks much better now, thanks
Comment 181•22 years ago
|
||
Comment on attachment 100597 [details] [diff] [review]
fixup prefservice call
r=biesi
looks much better now, thanks
Comment 182•22 years ago
|
||
Comment on attachment 100597 [details] [diff] [review]
fixup prefservice call
r=biesi
looks much better now, thanks
Comment 183•22 years ago
|
||
Comment on attachment 100597 [details] [diff] [review]
fixup prefservice call
r=biesi
looks much better now, thanks
Comment 184•22 years ago
|
||
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 185•22 years ago
|
||
Comment on attachment 100597 [details] [diff] [review]
fixup prefservice call
sr=dveditz
Attachment #100597 -
Flags: superreview+
Comment 186•22 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
Closed: 22 years ago
Resolution: --- → FIXED
Comment 187•22 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•22 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•22 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•22 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•22 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•22 years ago
|
||
Ever since "fixed", text-only in Modern works in mailnews but not browser. IOW,
never has been "fixed".
Assignee | ||
Comment 193•22 years ago
|
||
Modern has only ever supported icon only buttons in navigator.
Changing that is beyond the scope of this bug.
Comment 194•22 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•22 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).
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•