Last Comment Bug 839855 - Implement new favicon styling
: Implement new favicon styling
Status: RESOLVED FIXED
ui-hackathon
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 19 Branch
: Other Android
: -- normal (vote)
: Firefox 23
Assigned To: :Margaret Leibovic
:
Mentors:
Depends on: 867249 867627 869634 870858 873146
Blocks: 837392
  Show dependency treegraph
 
Reported: 2013-02-10 01:34 PST by Peter Retzer (:pretzer)
Modified: 2013-05-20 10:38 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
affected
affected
affected


Attachments
patch (4.97 KB, patch)
2013-04-24 18:58 PDT, :Margaret Leibovic
bnicholson: review+
Details | Diff | Review
WIP - Use dominant color as background for small favicons (5.11 KB, patch)
2013-04-24 19:04 PDT, :Margaret Leibovic
bnicholson: feedback+
Details | Diff | Review
WIP - screenshot of awesomescreen (122.23 KB, image/png)
2013-04-24 19:05 PDT, :Margaret Leibovic
no flags Details
WIP - screenshot of tabs from last time (126.10 KB, image/png)
2013-04-24 19:06 PDT, :Margaret Leibovic
no flags Details
(Part 1) Update tabs from last time favicons to match awesomescreen favicons (18.91 KB, patch)
2013-04-25 17:20 PDT, :Margaret Leibovic
no flags Details | Diff | Review
(Part 2 WIP) Use dominant color as background for small favicons (3.74 KB, patch)
2013-04-25 17:24 PDT, :Margaret Leibovic
no flags Details | Diff | Review
(Part 3) Fix margin on favicon in tabs from last time section (1.47 KB, patch)
2013-04-25 17:26 PDT, :Margaret Leibovic
bnicholson: review+
Details | Diff | Review
(Part 2) Use dominant color as background for small favicons (3.94 KB, patch)
2013-04-26 11:40 PDT, :Margaret Leibovic
wjohnston2000: review+
Details | Diff | Review
screenshot of awesomescreen (80.77 KB, image/png)
2013-04-26 11:41 PDT, :Margaret Leibovic
no flags Details
screenshot of tabs from last time (122.08 KB, image/png)
2013-04-26 11:42 PDT, :Margaret Leibovic
no flags Details
(Part 1 - v2) Update tabs from last time favicons to match awesomescreen favicons (27.36 KB, patch)
2013-04-26 14:10 PDT, :Margaret Leibovic
bnicholson: review+
Details | Diff | Review

Description Peter Retzer (:pretzer) 2013-02-10 01:34:37 PST
STR:
1) Go to http://en.m.wikipedia.org/wiki/Firefox
2) Make sure Firefox is shut down completely (via Android task manager) 

Expected: 
Favicon in the 'Tabs from last time' section on about:home and favicon in the history tab look the same

Actual:
Favicon in history shows a white border around the actual icon (the new style for small favicons) and the favicon on about:home stretches out over the whole area (the old style)
Comment 1 Aaron Train [:aaronmt] 2013-02-10 08:22:23 PST
This likely needs a UX call; CC'ing 'the favicon bunch'.
Comment 2 Ian Barlow (:ibarlow) 2013-02-11 08:58:41 PST
These should indeed be styled the same way (the new Awesomebar way)
Comment 3 :Margaret Leibovic 2013-04-23 15:31:29 PDT
It it still worth doing this if we're going to be redesigning about:home soon. Should we just start moving towards the even newer favicon design?
Comment 4 Ian Barlow (:ibarlow) 2013-04-24 05:48:15 PDT
That would make sense, yes. See bug 837392
Comment 5 :Margaret Leibovic 2013-04-24 11:30:01 PDT
(In reply to Ian Barlow (:ibarlow) from comment #4)
> That would make sense, yes. See bug 837392

Cool, I'll also take this bug. I actually started bug 837392 by playing around with the favicons :)
Comment 6 :Margaret Leibovic 2013-04-24 18:58:40 PDT
Created attachment 741632 [details] [diff] [review]
patch

This doesn't do the dominant color stuff (yet) but it updates the tabs from last time favicons to match to the current awesomescreen favicons.

I'm contemplating whether or not we should factor out some of this code, maybe put it in Favicons.
Comment 7 :Margaret Leibovic 2013-04-24 19:04:28 PDT
Created attachment 741635 [details] [diff] [review]
WIP - Use dominant color as background for small favicons

This WIP just sets the background of the favicons to a transparent dominant color. I'm wondering if it's bad to call getDominantColor on the main thread. Wes, how performant do you think that method is?

I also haven't tackled dealing with the border yet :)
Comment 8 :Margaret Leibovic 2013-04-24 19:05:52 PDT
Created attachment 741636 [details]
WIP - screenshot of awesomescreen
Comment 9 :Margaret Leibovic 2013-04-24 19:06:18 PDT
Created attachment 741637 [details]
WIP - screenshot of tabs from last time
Comment 10 Ian Barlow (:ibarlow) 2013-04-24 19:38:08 PDT
Nice, this is looking good so far.

I know we're changing the background colour of our in-content UI to white soon, but for the sake of my own OCD is there any way to set the favicon background colours to be opaque, so they don't pick up some of the light blue list background? Also for tabs from last time if we could nudge the favicon and text up so they're centered vertically in the row :)
Comment 11 Wesley Johnston (:wesj) 2013-04-24 20:19:05 PDT
I do not think you should call getDominantColor on the main thread. We basically loop over every pixel of the image and count the colors in different bins. Its not going to be performant.
Comment 12 Brian Nicholson (:bnicholson) (on PTO through June 3) 2013-04-24 20:20:32 PDT
Comment on attachment 741632 [details] [diff] [review]
patch

(In reply to :Margaret Leibovic from comment #6)
> Created attachment 741632 [details] [diff] [review]
> patch
>
> I'm contemplating whether or not we should factor out some of this code,
> maybe put it in Favicons.

You could also maybe create a FaviconView that extends ImageView. You could override its setBitmap() method to handle the scaling code, and then set the width, height, scaleType, background, etc. in the view's constructor so it doesn't have to be duplicated everywhere in XML.
Comment 13 Brian Nicholson (:bnicholson) (on PTO through June 3) 2013-04-24 22:43:23 PDT
Comment on attachment 741635 [details] [diff] [review]
WIP - Use dominant color as background for small favicons

Yeah, I'd do this work in an AsyncTask. Depending on the performance, it might be better to just use regular AsyncTasks instead of UiAsyncTasks so we don't clog our background thread. If you do, it shouldn't be a problem since you're already on the UI thread.
Comment 14 Wesley Johnston (:wesj) 2013-04-25 13:02:20 PDT
Comment on attachment 741635 [details] [diff] [review]
WIP - Use dominant color as background for small favicons

Review of attachment 741635 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/awesomebar/AwesomeBarTab.java
@@ +110,5 @@
>              faviconView.setBackgroundResource(0);
>          } else {
>              faviconView.setImageBitmap(bitmap);
> +            // XXX put this in a uiAsyncTask?
> +            int c = BitmapUtils.getDominantColor(bitmap);

yes please.

@@ +112,5 @@
>              faviconView.setImageBitmap(bitmap);
> +            // XXX put this in a uiAsyncTask?
> +            int c = BitmapUtils.getDominantColor(bitmap);
> +            int backgroundColor = Color.argb(30, Color.red(c), Color.green(c), Color.blue(c));
> +            faviconView.setBackgroundColor(backgroundColor);

The better way to do this is probably to create a background bitmap that we like (transparent white (or maybe gray) with a whiter border), and then create a drawable using it. Then on the drawable we can call setColorFilter:

http://developer.android.com/reference/android/graphics/drawable/Drawable.html#setColorFilter%28android.graphics.ColorFilter%29

which will tint the whole thing. Potentially you could use a shapedrawable to do this without a bitmap. Or you COULD event create the ShapeDrawable in code...:

https://developer.android.com/guide/topics/resources/drawable-resource.html#Shape
Comment 15 :Margaret Leibovic 2013-04-25 17:20:46 PDT
Created attachment 742104 [details] [diff] [review]
(Part 1) Update tabs from last time favicons to match awesomescreen favicons

Here's a new version of this patch that factors out the shared code into a new FaviconView class.
Comment 16 :Margaret Leibovic 2013-04-25 17:24:02 PDT
Created attachment 742107 [details] [diff] [review]
(Part 2 WIP) Use dominant color as background for small favicons

I moved the getDominantColors call into an AsyncTask, and I started using setColorFilter instead of setting the color directly.

I still need to figure out how to make a border for this shape drawable, and we also need to handle the case of a blank favicon, since right now it will just show a transparent white square.

Also, we should find a way to make sure the blue background under the favicon doesn't bleed through.
Comment 17 :Margaret Leibovic 2013-04-25 17:26:05 PDT
Created attachment 742109 [details] [diff] [review]
(Part 3) Fix margin on favicon in tabs from last time section

Fixing ibarlow's request from comment 10. It seems like these layouts could be cleaned up a bit, but we can deal with that when we go through to update things for the about:home redesign.
Comment 18 :Margaret Leibovic 2013-04-25 17:51:33 PDT
I was able to get a border (and avoid the bleed through issue) with this change to favicon_bg.xml:

      <solid android:color="#33FFFFFF"/>
+     <stroke android:width="1dp" android:color="#FFFFFF" />
+     <padding android:left="1dp" android:top="1dp" android:right="1dp" android:bottom="1dp" />
 </shape>

Unfortunately, it looks like using solid white makes the border too saturated when the color filter is applied. I tried using a transparent color for the stroke, but that made the color filter fail to apply properly. Time for some more experimentation...
Comment 19 :Margaret Leibovic 2013-04-26 11:40:12 PDT
Created attachment 742480 [details] [diff] [review]
(Part 2) Use dominant color as background for small favicons

This approach makes a drawable with a solid #FFFFFF background and a solid #DDDDDD border, then applies the dominant color with alpha 80 as a color filter. This seems to work pretty well, although there may be a more efficient way to do this, and ibarlow may have some nits about the exact colors. I'll upload some screenshots.
Comment 20 :Margaret Leibovic 2013-04-26 11:41:52 PDT
Created attachment 742481 [details]
screenshot of awesomescreen
Comment 21 :Margaret Leibovic 2013-04-26 11:42:35 PDT
Created attachment 742482 [details]
screenshot of tabs from last time
Comment 22 Wesley Johnston (:wesj) 2013-04-26 11:45:07 PDT
Comment on attachment 742480 [details] [diff] [review]
(Part 2) Use dominant color as background for small favicons

Review of attachment 742480 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/widget/FaviconView.java
@@ +44,5 @@
> +            // Otherwise show a dominant color background.
> +            new AsyncTask<Void, Void, Integer>(){
> +                @Override
> +                public Integer doInBackground(Void... params) {
> +                    return BitmapUtils.getDominantColor(bitmap);

I'm a little worried this will be slow and we should cache it, but we're already doing all this OMT, so maybe we're fine. Just something to keep an eye out for.

@@ +47,5 @@
> +                public Integer doInBackground(Void... params) {
> +                    return BitmapUtils.getDominantColor(bitmap);
> +                }
> +                @Override
> +                public void onPostExecute(Integer dc) {

Use a better name than dc.
Comment 23 Ian Barlow (:ibarlow) 2013-04-26 12:35:13 PDT
(In reply to :Margaret Leibovic from comment #20)
> Created attachment 742481 [details]
> screenshot of awesomescreen

Love it. We might want to go a little lighter on the fill colours, but let's land it and tweak from there if we have to.
Comment 24 :Margaret Leibovic 2013-04-26 14:10:23 PDT
Created attachment 742554 [details] [diff] [review]
(Part 1 - v2) Update tabs from last time favicons to match awesomescreen favicons

Updated the patch to use the FaviconView type where FaviconViews are used.
Comment 25 Peter Retzer (:pretzer) 2013-04-28 04:46:09 PDT
Updating summary to reflect the new scope. 

The screenshots look great! :-)
What's up with the tiny AMO favicon in the 'Your tabs from last time' section, though?
Comment 26 Lucas Rocha (:lucasr) 2013-04-29 05:47:41 PDT
(In reply to Ian Barlow (:ibarlow) from comment #23)
> (In reply to :Margaret Leibovic from comment #20)
> > Created attachment 742481 [details]
> > screenshot of awesomescreen
> 
> Love it. We might want to go a little lighter on the fill colours, but let's
> land it and tweak from there if we have to.

I agree, the fill colours look slightly too bright. I suggest tweaking the fill colours before pushing just avoid extra follow-ups just to get these details right. I assume it's simple to try different options?
Comment 27 Brian Nicholson (:bnicholson) (on PTO through June 3) 2013-04-29 10:44:57 PDT
Comment on attachment 742554 [details] [diff] [review]
(Part 1 - v2) Update tabs from last time favicons to match awesomescreen favicons

Review of attachment 742554 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/resources/layout/awesomebar_suggestion_row.xml
@@ +17,5 @@
> +                       android:layout_centerVertical="true"
> +                       android:minWidth="@dimen/favicon_bg"
> +                       android:minHeight="@dimen/favicon_bg"
> +                       android:scaleType="center"
> +                       android:background="@drawable/favicon_bg"/>

You could probably simplify the XML a bit and reduce redundancy by moving some of these parameters to FaviconView's constructor.
Comment 28 :Margaret Leibovic 2013-04-29 17:11:32 PDT
I also updated awesomebar_folder_row.xml to use FaviconView. A push to try revealed that neglecting to do that caused a crash, but with that update it's green on try: 
https://tbpl.mozilla.org/?tree=Try&rev=dfcacf6e9503

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5502bd10396
https://hg.mozilla.org/integration/mozilla-inbound/rev/48ba682136ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/f11dcc98df2e

I also made the color a little lighter before landing. We can file a follow-up to tweak, and I think we should also have a follow-up on a updated no-favicion icon.
Comment 29 :Margaret Leibovic 2013-04-29 17:12:42 PDT
(In reply to Peter Retzer (:pretzer) from comment #25)
> Updating summary to reflect the new scope. 
> 
> The screenshots look great! :-)

Thanks!

> What's up with the tiny AMO favicon in the 'Your tabs from last time'
> section, though?

That's the favicon that AMO serves us. It's only large before you actually visit the site, since the icon we ship with the default bookmark is larger. wesj filed an AMO bug to get that fixed.

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