Closed Bug 839855 Opened 11 years ago Closed 11 years ago

Implement new favicon styling

Categories

(Firefox for Android Graveyard :: General, defect)

19 Branch
Other
Android
defect
Not set
normal

Tracking

(firefox18 unaffected, firefox19 affected, firefox20 affected, firefox21 affected)

RESOLVED FIXED
Firefox 23
Tracking Status
firefox18 --- unaffected
firefox19 --- affected
firefox20 --- affected
firefox21 --- affected

People

(Reporter: pretzer, Assigned: Margaret)

References

Details

(Whiteboard: ui-hackathon)

Attachments

(5 files, 6 obsolete files)

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)
This likely needs a UX call; CC'ing 'the favicon bunch'.
These should indeed be styled the same way (the new Awesomebar way)
Whiteboard: ui-hackathon
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?
That would make sense, yes. See bug 837392
(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 :)
Assignee: nobody → margaret.leibovic
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #741632 - Flags: review?(bnicholson)
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 :)
Attachment #741635 - Flags: feedback?(wjohnston)
Attachment #741635 - Flags: feedback?(bnicholson)
Attached image WIP - screenshot of awesomescreen (obsolete) —
Attached image WIP - screenshot of tabs from last time (obsolete) —
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 :)
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 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.
Attachment #741632 - Flags: review?(bnicholson) → review+
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.
Attachment #741635 - Flags: feedback?(bnicholson) → feedback+
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
Attachment #741635 - Flags: feedback?(wjohnston)
Here's a new version of this patch that factors out the shared code into a new FaviconView class.
Attachment #741632 - Attachment is obsolete: true
Attachment #742104 - Flags: review?(bnicholson)
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.
Attachment #741635 - Attachment is obsolete: true
Attachment #742107 - Flags: feedback?(wjohnston)
Attachment #742107 - Flags: feedback?(bnicholson)
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.
Attachment #742109 - Flags: review?(bnicholson)
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...
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.
Attachment #742107 - Attachment is obsolete: true
Attachment #742107 - Flags: feedback?(wjohnston)
Attachment #742107 - Flags: feedback?(bnicholson)
Attachment #742480 - Flags: review?(wjohnston)
Attachment #741636 - Attachment is obsolete: true
Attachment #741637 - Attachment is obsolete: true
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.
Attachment #742480 - Flags: review?(wjohnston) → review+
(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.
Updated the patch to use the FaviconView type where FaviconViews are used.
Attachment #742104 - Attachment is obsolete: true
Attachment #742104 - Flags: review?(bnicholson)
Attachment #742554 - Flags: review?(bnicholson)
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?
Summary: [about:home] Tabs from last time still use old favicon styling → Implement new favicon styling
Blocks: 837392
No longer blocks: new-about-home
(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 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.
Attachment #742554 - Flags: review?(bnicholson) → review+
Attachment #742109 - Flags: review?(bnicholson) → review+
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.
(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.
Depends on: 867249
Depends on: 867627
Depends on: 869634
Depends on: 870858
Depends on: 873146
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.