Closed
Bug 839855
Opened 11 years ago
Closed 11 years ago
Implement new favicon styling
Categories
(Firefox for Android Graveyard :: General, defect)
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)
1.47 KB,
patch
|
bnicholson
:
review+
|
Details | Diff | Splinter Review |
3.94 KB,
patch
|
wesj
:
review+
|
Details | Diff | Splinter Review |
80.77 KB,
image/png
|
Details | |
122.08 KB,
image/png
|
Details | |
27.36 KB,
patch
|
bnicholson
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
This likely needs a UX call; CC'ing 'the favicon bunch'.
Comment 2•11 years ago
|
||
These should indeed be styled the same way (the new Awesomebar way)
Reporter | ||
Updated•11 years ago
|
Whiteboard: ui-hackathon
Assignee | ||
Comment 3•11 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?
Comment 4•11 years ago
|
||
That would make sense, yes. See bug 837392
Assignee | ||
Comment 5•11 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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
Assignee | ||
Comment 9•11 years ago
|
||
Comment 10•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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 13•11 years ago
|
||
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 14•11 years ago
|
||
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
Updated•11 years ago
|
Attachment #741635 -
Flags: feedback?(wjohnston)
Assignee | ||
Comment 15•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 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•11 years ago
|
||
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•11 years ago
|
||
Attachment #741636 -
Attachment is obsolete: true
Attachment #741637 -
Attachment is obsolete: true
Assignee | ||
Comment 21•11 years ago
|
||
Comment 22•11 years ago
|
||
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+
Comment 23•11 years ago
|
||
(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•11 years ago
|
||
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•11 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: new-about-home
Summary: [about:home] Tabs from last time still use old favicon styling → Implement new favicon styling
Reporter | ||
Updated•11 years ago
|
Comment 26•11 years ago
|
||
(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•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #742109 -
Flags: review?(bnicholson) → review+
Assignee | ||
Comment 28•11 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•11 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.
Comment 30•11 years ago
|
||
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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•