386007 - Clean up toolbarbutton CSS

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Phil Ringnalda (:philor)]

We've got a mix of .toolbarbutton-1 and !important and redeclarations of the list-style-image in the #button-id selector to make toolbarbuttons work in both their own toolbar and in the customize toolbar dialog, as detailed in bug 380087 comment 4. We'd be better off picking one strategy, particularly with some comments to make it clear what we're doing.

Off the top of my head, since !important always winds up being annoying somehow and the repeated list-style-images feel inefficient, we could either have .toolbarbutton-nameofwindow, or keep .toolbarbutton-1 and add another per-window class for just the list-style-image. Not sure which would be better for themers and extension authors.

Comment 1

[Alfred Kayser]

Reaction from a themer (of LittleFox, Nautipolis, Walnut and others):

Please stick with minimal number of classes (.toolbarbutton-1) for the toolbar buttons, and use the #button-id selector to make each button unique, even across windows...

Generic styling (hover effects, borders, focus and such) can then be assigned to the class. 

Images can then be assigned to the specific #button-id.

Likewise, as a themer, I have learned to not assign images to the class and then to do -moz-image-region to the #id's, but to have all image assignments to the #id's only. This prevents problems with new buttons.

Comment 2

[Phil Ringnalda (:philor)]

I wouldn't want to fool anyone who might want to work on this into think that I was actually doing anything about it at the moment.

Comment 3

[Jim Porter (:squib)]

This sounds like something I could probably handle, assuming someone was willing to provide details of what needs to be fixed. It gather this is necessary for bug 519956, which I'd really like to see get into TB 3.0.

Comment 4

[Phil Ringnalda (:philor)]

It shouldn't be necessary for that, though it might be sufficient for it.

Fixing this bug just calls for going through all of http://mxr.mozilla.org/comm-central/search?string=.toolbarbutton-1&find=%2Fmail%2F making sure that the selector is never assigning a list-style-image, and when it is move that rule down into each #button-whatever selector, and for the #button-whatever selectors, make sure that there aren't bogus |!important|s that are working around this, like there are in, for example, qute/mail/compose/messengercompose.css for #button-contacts.

Getting rid of !important is a little more complicated for [disabled="true"], where as I remember it we use !important because foo:hover is more specific than foo[disabled="true"], so hovering a disabled button would give you the enabled hover image. Parts of toolkit and browser avoid that by using foo:not([disabled="true"]):hover and then foo[disabled="true"], though it's hard to say whether that added parsing load is worthwhile just to get rid of an !important.

Comment 5

[Joachim Herb]

Attachment: Cleanup list-style-image files for .toolbarbutton-1

I hope I didn't miss a button...

Comment 6

[Phil Ringnalda (:philor)]

Comment on attachment 405886 [details] [diff] [review]
Cleanup list-style-image files for .toolbarbutton-1

Not sure whether you missed any, but I'm sure that's way too many. For example, in:

> #button-getmsg {
>+  list-style-image: url("chrome://messenger/skin/icons/mail-toolbar.png");
>   -moz-image-region: rect(0px 32px 32px 0px);
> }
> 
> #button-getmsg:hover:active,
> #button-getmsg[open] {
>+  list-style-image: url("chrome://messenger/skin/icons/mail-toolbar.png");
>   -moz-image-region: rect(32px 32px 64px 0px);
> }
> 
> #button-getmsg[disabled] {
>+  list-style-image: url("chrome://messenger/skin/icons/mail-toolbar.png");
>   -moz-image-region: rect(64px 32px 96px 0px) !important;
> }
> 

the "#button-getmsg" selector also applies when any of the other selectors do, so for cases like that you only need to add the list-style-image to the first one, and for all the cases where you were only adding it to [disabled] or :hover:active, you need to not add it, just verify that it's already in the rules for the bare ID selector.

Comment 7

[Phil Ringnalda (:philor)]

Nice to have, but not a blocker.

Comment 8

[Joachim Herb]

Attachment: Next trail: Hopefully not to many new list-style-image

This time hopefully I added the list-style-image commands only where needed.

Comment 9

[Joachim Herb]

At bug 519956 comment #30 I posted links to try server builds containing the patches (https://bugzilla.mozilla.org/attachment.cgi?id=406106 and
https://bugzilla.mozilla.org/attachment.cgi?id=405913).

Comment 10

[Phil Ringnalda (:philor)]

Comment on attachment 405913 [details] [diff] [review]
Next trail: Hopefully not to many new list-style-image

r=me, but I'm undecided about a.

Two benefits, one of which is also a risk:

* makes life easier for bug 519956 (and anything else that ever again has to deal with the customization dialog, as long as we don't regress this)

* gets rid of the freaky "the button has every button's icon as an icon" effect when an extension screws up its overlay, since the toolbarbutton-1 class won't default to one of our images anymore

Risks:

* the usual, that I missed checking something while looking at icons in three windows in four themes in small and large and text below and text beside and no text and nothing but text, and we'll have a wrong or missing icon

* not having the "omg, I so broke something" effect when an extension screws up its overlay - somewhere, I saw a claim that Firefox does it intentionally, to make it obvious, which I don't entirely buy, but it does make for a difference between us and Fx, which is always a risk

Comment 11

[Philip Chee]

> * makes life easier for bug 519956 (and anything else that ever again has to
> deal with the customization dialog, as long as we don't regress this)

vs:

> * not having the "omg, I so broke something" effect when an extension screws up
> its overlay - somewhere, I saw a claim that Firefox does it intentionally, to
> make it obvious, which I don't entirely buy, but it does make for a difference
> between us and Fx, which is always a risk

The difference between Firefox on one hand and Thunderbird and SeaMonkey on the other is that the panda basically has only one window with customizable toolbars. Thunderbird has several, SeaMonkey even more. When I started adding customizable toolbars to SeaMonkey I initially went with the Firefox style; but as I converted more windows I rapidly ran into the wrong image/region problem. And since SeaMonkey has significantly more windows the risk/benefit ratio rapidly tilted away from the Firefox paradigm. Of course we had the luxury of doing this at a significant distance from the Betas and the Release Candidates so we had ample time to catch any regressions. To mitigate this, [generic] you might want to do a specific Bug Day focusing on smoke testing toolbar customization..

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Given that if you did miss something, it should be trivial to fix, and we still have two weeks until code freeze, I'd be inclined to take this.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 405913 [details] [diff] [review]
Next trail: Hopefully not to many new list-style-image

I think I'm with Dan, let's take this as its nice tidy up.

Comment 14

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/9df935b047eb

Is this fixed now? (if so please mark it as such).

Comment 15

[Phil Ringnalda (:philor)]

Yep, that patch was all I filed it for.