On Windows 7/Aero, the Show Accounts button seems to be missing an icon.

Andreas, we added this "Show Accounts" button during the last hours before landing, so it appeared after your work on the Chat toolbar. Sorry about that.

Will fix as soon as I figure out what to draw for this :)

settled for the (i) icon

On gnomestripe and qute you are using chat-toolbar-small.png also for big and aero icons. On Aero you are referring for the disabled state to a not existent image-region but Aero sets the opacity for disabled icons. I suppose to use this to override the XP image-regions: #button-add-buddy, #button-join-buddy[disabled] { list-style-image: url("chrome://messenger/skin/icons/chat-toolbar.png"); -moz-image-region: rect(0px 18px 18px 0px); } #button-join-chat, #button-join-chat[disabled] { list-style-image: url("chrome://messenger/skin/icons/chat-toolbar.png"); -moz-image-region: rect(0px 36px 18px 18px); } #button-chat-accounts, #button-chat-accounts[disabled] { list-style-image: url("chrome://messenger/skin/icons/chat-toolbar.png"); -moz-image-region: rect(0px 54px 18px 36px); }

Good catch! Too little coffee. :)

Comment on attachment 608993 [details] [diff] [review] chat accounts icon for all platforms marking as obsolete

Comment on attachment 608997 [details] [diff] [review] patch (v2) Just noted a issues with cutoff of the icon on the mac, so cancelling review

Just needs testing on windows before review

Some stupid numbers fixed

Andreas no review requests ?

Unbitrotted and took care of the issue with small toolbar icons.

Andreas: Am I crazy, or are the labels for gnomestripe/qute not vertical-align middle'd? -Mike

Comment on attachment 610883 [details] [diff] [review] unbitrotted patch Review of attachment 610883 [details] [diff] [review]: ----------------------------------------------------------------- Hey Andreas, Just a few simple formatting issues, nothing major. I'm holding off an r+ until I hear more about the vertical align of the labels for that button. -Mike ::: mail/themes/pinstripe/mail/chat.css @@ +67,5 @@ > -moz-image-region: rect(32px 32px 64px 0px); > } > > +#button-add-buddy[disabled] { > + list-style-image: url("chrome://messenger/skin/icons/chat-toolbar.png"); Lines 71-72 should be indented by two spaces, not four. @@ +77,5 @@ > -moz-image-region: rect(0px 64px 32px 32px); > } > > +#button-join-chat:hover:active { > + list-style-image: url("chrome://messenger/skin/icons/chat-toolbar.png"); Lines 81-82 should be indented by two spaces, not four. @@ +97,5 @@ > + -moz-image-region: rect(32px 96px 64px 64px); > +} > + > +#button-chat-accounts[disabled] { > + list-style-image: url("chrome://messenger/skin/icons/chat-toolbar.png"); Lines 101-102 should be indented by two spaces, not four. @@ +109,5 @@ > -moz-image-region: rect(0px 24px 24px 0px); > } > > +toolbar[iconsize="small"] #button-add-buddy:hover:active { > + list-style-image: url("chrome://messenger/skin/icons/chat-toolbar-small.png"); Lines 113-114 should be indented by two spaces, not four. @@ +124,5 @@ > -moz-image-region: rect(0px 48px 24px 24px); > } > > +toolbar[iconsize="small"] #button-join-chat:hover:active { > + list-style-image: url("chrome://messenger/skin/icons/chat-toolbar-small.png"); Lines 128-129 should be indented by two spaces, not four. @@ +144,5 @@ > + -moz-image-region: rect(24px 72px 48px 48px); > +} > + > +toolbar[iconsize="small"] #button-chat-accounts[disabled] { > + list-style-image: url("chrome://messenger/skin/icons/chat-toolbar-small.png"); Lines 148-149 should be indented by two spaces, not four. ::: mail/themes/qute/mail/chat-aero.css @@ +123,4 @@ > } > } > > +/* Default icons have a built-in glow, so they are 18*18px even in small mode, Good documentation. @@ +128,5 @@ > + rather than 18px. This will pick the correct size based on the image > + region. */ > +:-moz-any( > + #button-add-buddy, #button-join-chat, #button-chat-accounts > + ) .toolbarbutton-icon { Something about this is a little hard to read. I'd suggest unindenting line 132 by two spaces.

Comment on attachment 610883 [details] [diff] [review] unbitrotted patch Ok, Andreas and I determined over IRC that my eyes were playing tricks on me. ui-r+, but r- due to the issues I found.

This takes care of the issues Mike found in his code review.

Comment on attachment 616095 [details] [diff] [review] fixed patch Review of attachment 616095 [details] [diff] [review]: ----------------------------------------------------------------- I didn't apply / visually test this patch, but the code looks good.

Checked in as http://hg.mozilla.org/comm-central/rev/0eda2c044258 Note that the issue with small toolbar icons (mentioned here at comment 11) was already fixed with a different solution by a patch in bug 747165. To apply this patch, I had to back out the patch from bug 747165 first (I prefer the solution used here).

Comment on attachment 616095 [details] [diff] [review] fixed patch [Approval Request Comment] Not sure it matters at this point as IM won't be enabled by default in Tb13, but requesting approval-aurora anyway as there was a pending approval-aurora request in bug 747165.

Looks like this was pushed earlier today: http://hg.mozilla.org/releases/comm-aurora/rev/b07948bdab69