Closed Bug 22056 Opened 25 years ago Closed 22 years ago

Show toolbars as text/icons/both

Categories

(SeaMonkey :: Preferences, enhancement, P3)

enhancement

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?
Status: NEW → ASSIGNED
Target Milestone: M14
This might be hooked up in m14 hopefully.
Target Milestone: M14 → M15
M15 ...
Bulk move of all Pref UI component bugs to new Preferences component. Pref UI component will be deleted.
Component: Pref UI → Preferences
Move to M16 for now ...
Target Milestone: M15 → M16
Reassigning for Don
Assignee: matt → ben
Status: ASSIGNED → NEW
Keywords: nsbeta2
Target Milestone: M16 → M17
*** Bug 25921 has been marked as a duplicate of this bug. ***
*** Bug 25921 has been marked as a duplicate of this bug. ***
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
geez, this fell through the cracks (ack). nominating for beta3 (hope against hope)...
Keywords: 4xp, helpwanted, nsbeta3
my eyes were momentarily blind to the enhancement and future fields. removing beta3 nomination.
Keywords: nsbeta3
*** Bug 51141 has been marked as a duplicate of this bug. ***
*** Bug 52021 has been marked as a duplicate of this bug. ***
[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
paul/ben, how feasible would this be nowadays?
Whiteboard: se-radar
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.
Jesse, file a separate RFE for that.
*** Bug 60458 has been marked as a duplicate of this bug. ***
*** Bug 60693 has been marked as a duplicate of this bug. ***
*** Bug 60974 has been marked as a duplicate of this bug. ***
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 → ---
adding this phrase for future queries: http://www.macintouch.com/netscape6.html
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
Filed bug 62444 for the ability to choose the size of the images on the buttons.
*** Bug 67199 has been marked as a duplicate of this bug. ***
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. ***
*** Bug 70848 has been marked as a duplicate of this bug. ***
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.
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.
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.
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 ...
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
Attached patch Patch to enable the pref panel (obsolete) (deleted) — Splinter Review
Attached patch Patch to Classic skin for Windows (obsolete) (deleted) — Splinter Review
Attached image animthrob_sm.gif (7K) (obsolete) (deleted) —
Attached image animthrob_sm_single.gif (<1K) (obsolete) (deleted) —
Whiteboard: se-radar
Attached patch Better patch for Modern dual menubuttons (obsolete) (deleted) — Splinter Review
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
cc'ing hewitt for modern 3 info: Joe, how do the patches in this bug look in relation to your work?
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.
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.
What's the likelihood of getting this resolved by the mozilla0.9.1 deadline? .Angela...
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]
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.
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.
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
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.
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
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.
toolbar-holder is in the anonymous content of the toolbar bindings
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
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.
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.
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.
*** Bug 88647 has been marked as a duplicate of this bug. ***
Attached patch Patch to Windows Classic skin (obsolete) (deleted) — Splinter Review
Would be neat with some screenshots attached...
Attached file screenshots and polished diff (obsolete) (deleted) —
The screenshots from neil@parkwaycc.co.uk are exactly the way I hope it will work once released.
Nitpick: the "Stop" text doesn't appear to be centered under the image in the screenshot.
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...
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?
Heh. Ok, what is the change to brand.css in the latest patch? :)
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.
Attached patch updated for typo (obsolete) (deleted) — Splinter Review
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
The patch is about to suffer major bitrot because of the new <toolbarbutton>s :-(
*** Bug 100671 has been marked as a duplicate of this bug. ***
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
Marlon, there is already a patch attached to fix this bug. Are you saying you're gonna reinvent the wheel? :)
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.
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. ***
Blocks: 104166
*** Bug 107123 has been marked as a duplicate of this bug. ***
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?
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
*** 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.
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.
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
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.
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.
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.
*** Bug 127319 has been marked as a duplicate of this bug. ***
Attached patch Patch for Modern and Windows Classic only (obsolete) (deleted) — Splinter Review
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
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
nsbeta1- per ADT triage. This can still be checked in with drivers approval.
Keywords: nsbeta1nsbeta1-
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.
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. ***
*** Bug 131565 has been marked as a duplicate of this bug. ***
adding self to cc list
*** Bug 141240 has been marked as a duplicate of this bug. ***
Neil: do you think that the patch is ready for reviewing (and checking in)? Any progress?
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>
*** Bug 143197 has been marked as a duplicate of this bug. ***
*** Bug 144127 has been marked as a duplicate of this bug. ***
*** Bug 133447 has been marked as a duplicate of this bug. ***
*** Bug 149960 has been marked as a duplicate of this bug. ***
*** Bug 150355 has been marked as a duplicate of this bug. ***
Blocks: 135457
Keywords: nsbeta1-nsbeta1
Attached patch Updated previous patch (obsolete) (deleted) — Splinter Review
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
Attached patch Oops missed a QI (obsolete) (deleted) — Splinter Review
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.
> 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 :-)
Blocks: 157199
Attached patch This must be the one surely? (obsolete) (deleted) — Splinter Review
Attachment #91083 - Attachment is obsolete: true
*** Bug 163012 has been marked as a duplicate of this bug. ***
Blocks: majorbugs
> -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!
Attached patch Added comment to all.js (obsolete) (deleted) — Splinter Review
> > - 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+
The work I've done on customizable toolbars is a superset of this...
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?
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...)
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...
Fine with me all if you want to check this in...
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.
Attached patch Improved perf (obsolete) (deleted) — Splinter Review
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
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.
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"?
Attached patch Best yet (obsolete) (deleted) — Splinter Review
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.
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?
>+ 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 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.
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.
Attached patch hookup classic (obsolete) (deleted) — Splinter Review
i'm still looking into modern, but classic was really easy.
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.
Attached patch File accidentally left out of patch, sorry (obsolete) (deleted) — Splinter Review
Attachment #100533 - Flags: review+
Attachment #100523 - Attachment is obsolete: true
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.
Attached patch fixup prefservice call (deleted) — Splinter Review
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+
- 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
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.
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; }
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?
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?
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.
Ever since "fixed", text-only in Modern works in mailnews but not browser. IOW, never has been "fixed".
Modern has only ever supported icon only buttons in navigator. Changing that is beyond the scope of this bug.
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
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).
verifying as fixed (as of some time ago...)
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
No longer blocks: majorbugs
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: