Last Comment Bug 738241 - Filelink account setup providers menulist should contain their respective icons
: Filelink account setup providers menulist should contain their respective icons
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Preferences (show other bugs)
: 13 Branch
: All All
: -- normal (vote)
: Thunderbird 14.0
Assigned To: Mike Conley (:mconley) - (Needinfo me!)
:
Mentors:
Depends on:
Blocks: BigFiles
  Show dependency treegraph
 
Reported: 2012-03-22 07:09 PDT by Jb Piacentino
Modified: 2012-03-26 06:39 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
WIP Patch 1 (5.41 KB, patch)
2012-03-23 10:33 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Patch v1 (10.43 KB, patch)
2012-03-23 10:53 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Patch v2 (11.35 KB, patch)
2012-03-23 12:08 PDT, Mike Conley (:mconley) - (Needinfo me!)
bwinton: review+
bwinton: ui‑review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review
Provider icons in Windows 7 (25.17 KB, image/png)
2012-03-23 13:43 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details
Provider icons in Ubuntu (28.21 KB, image/png)
2012-03-23 13:48 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details
Provider icons in OSX (25.56 KB, image/png)
2012-03-23 13:51 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details

Description Jb Piacentino 2012-03-22 07:09:37 PDT
Currently, the list only makes reference to the providers name. 
However, once a provider is selected, its icon is displayed.
Comment 1 Mike Conley (:mconley) - (Needinfo me!) 2012-03-23 10:33:14 PDT
Created attachment 608756 [details] [diff] [review]
WIP Patch 1

Got it working for Windows, stashing my work here to make sure it works on OSX and Ubuntu...
Comment 2 Mike Conley (:mconley) - (Needinfo me!) 2012-03-23 10:53:16 PDT
Created attachment 608767 [details] [diff] [review]
Patch v1

Ok, I've copied the CSS for both pinstripe and gnomestripe.

I still don't see the icons on Ubuntu - but I think this is a toolkit limitation.  Firefox does the same thing in the Feed Subscribe page.  Go to http://www.mikeconley.ca/blog/feed for example, and click on the "Subscribe to this feed using"...no icons there either on Ubuntu, but they exist on OSX and Windows.  So I think we might have to punt there.

I'm just going to test this patch on OSX (which originally worked, but I'm copying the CSS over for consistency).
Comment 3 Mike Conley (:mconley) - (Needinfo me!) 2012-03-23 11:01:31 PDT
Comment on attachment 608767 [details] [diff] [review]
Patch v1

Andreas:

Ok, here's my solution.  Does it make sense?  Also, I noticed we've got some styles specific to the addAccountDialog stuffed into preferences.css that we should probably move into addAccountDialog.css.  I've filed bug 738699 about that.

-Mike
Comment 4 Chris Coulson 2012-03-23 11:49:48 PDT
Sounds like you probably need the menuitem-with-favicon class if you want to see the icons on Ubuntu

see http://hg.mozilla.org/mozilla-central/file/d41503780635/toolkit/content/xul.css#l367
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2012-03-23 12:02:32 PDT
Comment on attachment 608767 [details] [diff] [review]
Patch v1

Chris Coulson just told me how I might get icons to display in Ubuntu - going to try that, and will update patch if it works.
Comment 6 Mike Conley (:mconley) - (Needinfo me!) 2012-03-23 12:08:43 PDT
Created attachment 608810 [details] [diff] [review]
Patch v2

This patch works across all three platforms.  Woo!
Comment 7 Mike Conley (:mconley) - (Needinfo me!) 2012-03-23 13:41:43 PDT
Comment on attachment 608810 [details] [diff] [review]
Patch v2

Screenshots en-route
Comment 8 Mike Conley (:mconley) - (Needinfo me!) 2012-03-23 13:43:42 PDT
Created attachment 608852 [details]
Provider icons in Windows 7
Comment 9 Mike Conley (:mconley) - (Needinfo me!) 2012-03-23 13:48:33 PDT
Created attachment 608854 [details]
Provider icons in Ubuntu
Comment 10 Blake Winton (:bwinton) (:☕️) 2012-03-23 13:51:20 PDT
Comment on attachment 608810 [details] [diff] [review]
Patch v2

>+++ b/mail/components/cloudfile/content/addAccountDialog.js
>@@ -95,17 +95,17 @@ let addAccountDialog = {
>       if (provider.iconClass) {
>-        menuitem.setAttribute("class", "menuitem-iconic");
>+        menuitem.setAttribute("class", "menuitem-iconic menuitem-with-favicon");
>         menuitem.setAttribute("image", provider.iconClass);
>       }

Huh, so we were always setting the image, but it wasn't showing up?  Cool.

In either case, it looks good, so r=me, and ui-r=me.

Thanks,
Blake.
Comment 11 Mike Conley (:mconley) - (Needinfo me!) 2012-03-23 13:51:58 PDT
Created attachment 608855 [details]
Provider icons in OSX

Here's the last one.
Comment 12 Mike Conley (:mconley) - (Needinfo me!) 2012-03-23 13:54:24 PDT
Comment on attachment 608810 [details] [diff] [review]
Patch v2

We'll want this for 13.
Comment 13 Mike Conley (:mconley) - (Needinfo me!) 2012-03-23 13:59:18 PDT
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/c2e492105d6d
Comment 14 Mike Conley (:mconley) - (Needinfo me!) 2012-03-26 06:39:53 PDT
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/b93d8d423cc0

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