Implement new favicon styling

RESOLVED FIXED in Firefox 23

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: pretzer, Assigned: Margaret)

Tracking

19 Branch
Firefox 23
Other
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: ui-hackathon)

Attachments

(5 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
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)
(Reporter)

Updated

4 years ago
Whiteboard: ui-hackathon
(Assignee)

Comment 3

4 years ago
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
(Assignee)

Comment 5

4 years ago
(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
(Assignee)

Comment 6

4 years ago
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.
Attachment #741632 - Flags: review?(bnicholson)
(Assignee)

Comment 7

4 years ago
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 :)
Attachment #741635 - Flags: feedback?(wjohnston)
Attachment #741635 - Flags: feedback?(bnicholson)
(Assignee)

Comment 8

4 years ago
Created attachment 741636 [details]
WIP - screenshot of awesomescreen
(Assignee)

Comment 9

4 years ago
Created attachment 741637 [details]
WIP - screenshot of tabs from last time
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)
(Assignee)

Comment 15

4 years ago
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.
Attachment #741632 - Attachment is obsolete: true
Attachment #742104 - Flags: review?(bnicholson)
(Assignee)

Comment 16

4 years ago
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.
Attachment #741635 - Attachment is obsolete: true
Attachment #742107 - Flags: feedback?(wjohnston)
Attachment #742107 - Flags: feedback?(bnicholson)
(Assignee)

Comment 17

4 years ago
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.
Attachment #742109 - Flags: review?(bnicholson)
(Assignee)

Comment 18

4 years ago
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...
(Assignee)

Comment 19

4 years ago
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.
Attachment #742107 - Attachment is obsolete: true
Attachment #742107 - Flags: feedback?(wjohnston)
Attachment #742107 - Flags: feedback?(bnicholson)
Attachment #742480 - Flags: review?(wjohnston)
(Assignee)

Comment 20

4 years ago
Created attachment 742481 [details]
screenshot of awesomescreen
Attachment #741636 - Attachment is obsolete: true
Attachment #741637 - Attachment is obsolete: true
(Assignee)

Comment 21

4 years ago
Created attachment 742482 [details]
screenshot of tabs from last time
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.
(Assignee)

Comment 24

4 years ago
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.
Attachment #742104 - Attachment is obsolete: true
Attachment #742104 - Flags: review?(bnicholson)
Attachment #742554 - Flags: review?(bnicholson)
(Reporter)

Comment 25

4 years ago
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?
Blocks: 862793
Summary: [about:home] Tabs from last time still use old favicon styling → Implement new favicon styling
(Reporter)

Updated

4 years ago
Blocks: 837392
No longer blocks: 862793
(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+
(Assignee)

Comment 28

4 years ago
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.
(Assignee)

Comment 29

4 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/d5502bd10396
https://hg.mozilla.org/mozilla-central/rev/48ba682136ec
https://hg.mozilla.org/mozilla-central/rev/f11dcc98df2e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
(Assignee)

Updated

4 years ago
Depends on: 867249
(Assignee)

Updated

4 years ago
Depends on: 867627
(Assignee)

Updated

4 years ago
Depends on: 869634
(Assignee)

Updated

4 years ago
Depends on: 870858
(Assignee)

Updated

4 years ago
Depends on: 873146
You need to log in before you can comment on or make changes to this bug.