Last Comment Bug 735702 - The "Show Accounts" button doesn't have an icon
: The "Show Accounts" button doesn't have an icon
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 14.0
Assigned To: Andreas Nilsson (:andreasn)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-14 08:54 PDT by Siddharth Agarwal [:sid0] (inactive)
Modified: 2012-04-23 13:30 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
chat accounts icon for all platforms (49.36 KB, patch)
2012-03-24 07:34 PDT, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
patch (v2) (49.72 KB, patch)
2012-03-24 08:20 PDT, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
patch v3 (52.01 KB, patch)
2012-03-24 09:06 PDT, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
patch (v4) (52.00 KB, patch)
2012-03-24 09:23 PDT, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
unbitrotted patch (52.12 KB, patch)
2012-03-30 08:02 PDT, Andreas Nilsson (:andreasn)
mconley: review-
mconley: ui‑review+
Details | Diff | Splinter Review
screenshot to ease review (windows) (12.98 KB, image/png)
2012-03-30 08:41 PDT, Andreas Nilsson (:andreasn)
no flags Details
screenshot to ease review (mac) (23.05 KB, image/png)
2012-03-30 09:06 PDT, Andreas Nilsson (:andreasn)
no flags Details
screenshot to ease review (linux) (6.43 KB, image/png)
2012-03-30 10:11 PDT, Andreas Nilsson (:andreasn)
no flags Details
fixed patch (52.07 KB, patch)
2012-04-18 05:56 PDT, Andreas Nilsson (:andreasn)
mconley: review+
bugs: ui‑review+
mozilla: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Siddharth Agarwal [:sid0] (inactive) 2012-03-14 08:54:06 PDT
On Windows 7/Aero, the Show Accounts button seems to be missing an icon.
Comment 1 Florian Quèze [:florian] [:flo] 2012-03-14 09:00:30 PDT
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.
Comment 2 Andreas Nilsson (:andreasn) 2012-03-14 09:16:17 PDT
Will fix as soon as I figure out what to draw for this :)
Comment 3 Andreas Nilsson (:andreasn) 2012-03-24 07:34:50 PDT
Created attachment 608993 [details] [diff] [review]
chat accounts icon for all platforms

settled for the (i) icon
Comment 4 Richard Marti (:Paenglab) 2012-03-24 07:56:17 PDT
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);
}
Comment 5 Andreas Nilsson (:andreasn) 2012-03-24 08:20:53 PDT
Created attachment 608997 [details] [diff] [review]
patch (v2)

Good catch! Too little coffee. :)
Comment 6 Andreas Nilsson (:andreasn) 2012-03-24 08:21:40 PDT
Comment on attachment 608993 [details] [diff] [review]
chat accounts icon for all platforms

marking as obsolete
Comment 7 Andreas Nilsson (:andreasn) 2012-03-24 08:24:11 PDT
Comment on attachment 608997 [details] [diff] [review]
patch (v2)

Just noted a issues with cutoff of the icon on the mac, so cancelling review
Comment 8 Andreas Nilsson (:andreasn) 2012-03-24 09:06:42 PDT
Created attachment 609005 [details] [diff] [review]
patch v3

Just needs testing on windows before review
Comment 9 Andreas Nilsson (:andreasn) 2012-03-24 09:23:48 PDT
Created attachment 609010 [details] [diff] [review]
patch (v4)

Some stupid numbers fixed
Comment 10 Ludovic Hirlimann [:Usul] 2012-03-28 06:02:23 PDT
Andreas no review requests ?
Comment 11 Andreas Nilsson (:andreasn) 2012-03-30 08:02:28 PDT
Created attachment 610883 [details] [diff] [review]
unbitrotted patch

Unbitrotted and took care of the issue with small toolbar icons.
Comment 12 Andreas Nilsson (:andreasn) 2012-03-30 08:41:44 PDT
Created attachment 610896 [details]
screenshot to ease review (windows)
Comment 13 Andreas Nilsson (:andreasn) 2012-03-30 09:06:00 PDT
Created attachment 610906 [details]
screenshot to ease review (mac)
Comment 14 Andreas Nilsson (:andreasn) 2012-03-30 10:11:11 PDT
Created attachment 610925 [details]
screenshot to ease review (linux)
Comment 15 Mike Conley (:mconley) 2012-03-30 11:22:57 PDT
Andreas:

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

-Mike
Comment 16 Mike Conley (:mconley) 2012-03-30 11:25:19 PDT
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 17 Mike Conley (:mconley) 2012-04-05 11:25:03 PDT
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.
Comment 18 Andreas Nilsson (:andreasn) 2012-04-18 05:56:37 PDT
Created attachment 616095 [details] [diff] [review]
fixed patch

This takes care of the issues Mike found in his code review.
Comment 19 Mike Conley (:mconley) 2012-04-18 07:30:54 PDT
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.
Comment 20 Florian Quèze [:florian] [:flo] 2012-04-23 07:44:39 PDT
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 21 Florian Quèze [:florian] [:flo] 2012-04-23 07:51:36 PDT
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.
Comment 22 Mark Banner (:standard8, afk until Dec) 2012-04-23 13:30:39 PDT
Looks like this was pushed earlier today:

http://hg.mozilla.org/releases/comm-aurora/rev/b07948bdab69

Note You need to log in before you can comment on or make changes to this bug.