Downscale awesome-bar row Favicons from 32dip to 16dip

VERIFIED FIXED in Firefox 19

Status

()

Firefox for Android
Awesomescreen
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: aaronmt, Assigned: wesj)

Tracking

Trunk
Firefox 19
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16 affected, firefox17 affected, firefox18 affected, firefox19 verified, firefox20 verified)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
Currently the [1] ImageView used for awesome bar_rows and [2] in the browser-toolbar are upscaling 16x16 Favicons to 32x12 resulting in really poorly prominently displayed graphics in-product. 

These look extremely bad on high-end high-resolution devices like the Galaxy Note II where no other browser or Android application has this issue.

I have built Fennec with this value change and it is much nicer.

Can we please downscale Favicons to 16dip?

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/awesomebar_row.xml#17

[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/browser_toolbar.xml.in#75
(Reporter)

Comment 1

6 years ago
Created attachment 678374 [details]
Nightly (11/05) - Galaxy Note II
I think we should avoid varying favicon sizes as it wouldn't look good, especially on the awesome screen. So, my suggestion is to always show favicons in a 32dip *area* but avoid scaling them up by adding a surrounding border using the dominant color as background (or something like this). Ian, what do you think?
I see a few options here.

1. Find a way to not anti-alias icons when we scale them up, so that even if we use 16px favicons we can still show sharp pixels, not blurry edges: http://cl.ly/image/161r311M2l2W. However I've been trying to make this happen for about a year and a half though, and we can't seem to do it for whatever reason ;)

2. As Lucas suggested, scale favicons down to 16dip, but keep them in a 32x32 area. We might consider styling this with some kind of neutral background or border.

3. Building on #2, use 16dip favicons, and place them on a dominant colour background, choosing the colour in the same way we choose the "android home screen bookmark" background colour. 

-- 

Each of these options have their own strengths and tradeoffs, so ideally we should see if we can prototype all 3 of them, and then make a call on what is the best fix.
(Reporter)

Comment 4

6 years ago
Whatever works :-)

I just really cant stand seeing Favicons as is (for many months) right now, especially since they're so in-your-face in-product.
(Assignee)

Comment 5

6 years ago
I want to play with this.
Assignee: nobody → wjohnston
(Assignee)

Comment 6

6 years ago
Created attachment 678838 [details]
Screenshot

Ian, what do you think of this? I'll post the patch in a sec, but I'm not sure this is great....
(Assignee)

Comment 7

6 years ago
Created attachment 678843 [details] [diff] [review]
Patch

Patch to scale favicons without filtering them (nearest neighbor). I also applied this to the urlbar favicon too. I think this looks better, but that we might be good to reduce their size in the awesomescreen as well. Right now they're 32dip. Maybe 24dip? Easier to fix with this patch. I'll try it and post a screenshot!
Attachment #678843 - Flags: review?(bnicholson)
(In reply to Wesley Johnston (:wesj) from comment #6)

> I'm not sure this is great....


Yeah. It's better, but not great either. Definitely still worth trying the options 2 and 3 from comment 3, where we see how 16dip favicons look.
(Assignee)

Comment 9

6 years ago
Hmm.. 24 isn't great either: https://dl.dropbox.com/u/72157/device-2012-11-06-123124.png

I'll play with some background things behind that.
I just opened the 24 one on my phone, and it actually looks half decent. Let's keep it on the table as one option.
Comment on attachment 678843 [details] [diff] [review]
Patch

+        if (image != null) {
+            if (mFaviconSize > 0) {
+                image = Bitmap.createScaledBitmap(image, mFaviconSize, mFaviconSize, false);
+            }

Do we need this > 0 check? The constructor should always initialize mFaviconSize, so unless getDimension() is somehow returning a negative value, I don't see how mFaviconSize could ever be negative.

+    protected void updateFavicon(ImageView faviconView, Bitmap bitmap) {
+        if (bitmap == null) {
+            faviconView.setImageDrawable(null);
+        } else {
+            if (mFaviconSize > 0) {
+                bitmap = Bitmap.createScaledBitmap(bitmap, mFaviconSize, mFaviconSize, false);
+            }

Same here. The could would be a bit simpler if we didn't have these checks (and we wouldn't need the extra updateFavicon() method you're adding).
(In reply to Brian Nicholson (:bnicholson) from comment #11)
> Same here. The could would be a bit simpler if we didn't have these checks

s/The could/The code/
(Assignee)

Comment 13

6 years ago
No we don't. I was just trying to use view.getWidth() for awhile, which is sometimes zero and will crash. So I left the safety checks in, but we shouldn't need them using the dimens approach.
(Assignee)

Comment 14

6 years ago
Playing with the homescreen background:

https://dl.dropbox.com/u/72157/home1.png - white background
https://dl.dropbox.com/u/72157/home2.png - no (well.. technically #3999)

We can reverse the overlay so that its below the icon too. For now its on top of the favicon.

Getting the dominant color is doable, but will take a bit more work to do right, we should calculate and store it when we first fetch the icon. Ideas ian?
Comment on attachment 678843 [details] [diff] [review]
Patch

r+ with the mFaviconSize checks removed.

One other nit:

According to http://source.android.com/source/code-style.html#follow-field-naming-conventions, static field names should start with "s", so I'd rename it to sFaviconSize.
Attachment #678843 - Flags: review?(bnicholson) → review+
I assume we are waiting for Ian to report back on the preferred option?
... and can someone update the summary of this bug?
(Assignee)

Comment 18

6 years ago
This seems like an improvement regardless. I can land it alone and we can leave this open for any further work we want to do?

Just backed up with patches that have bounced that I'm trying to sort out.
(In reply to Wesley Johnston (:wesj) from comment #14)
> Playing with the homescreen background:
> 
> https://dl.dropbox.com/u/72157/home1.png - white background
> https://dl.dropbox.com/u/72157/home2.png - no (well.. technically #3999)
> 
> We can reverse the overlay so that its below the icon too. For now its on
> top of the favicon.
> 
> Getting the dominant color is doable, but will take a bit more work to do
> right, we should calculate and store it when we first fetch the icon. Ideas
> ian?

Why not simply adding the borders and save the new image in the db instead of the original favicon? I like the white borders as a starting point btw. I'd probably only show it on smaller favicons though (16dip). No border is needed if the favicon can fill the 32dip space without scaling up.
(Reporter)

Comment 22

6 years ago
Created attachment 679154 [details]
Inbound (11/07) - Screenshot

Getting there. The filtering removal is good but the 32dp on all these 16x16's still look the same.
Hey guys -- I'm working on some simplified visual designs for the favicon backgrounds, will post when they're ready. 

In the mean time, of the screenshots so far, I am happiest with Wes's 24dip favicons with no background colour in comment 9. I think putting a background image behind them could work, but I agree with Lucas that I would use smaller 16dip icons in this case. 

Anyway designs are coming soon, just wanted to keep people up to date since there is a lot of discussion happening here.
(Assignee)

Comment 24

6 years ago
Created attachment 679174 [details] [diff] [review]
Use 24dip icons

This moves the icons to 24dip. It also changes the scaleType since we're now doing the scaling manually ourselves (I want to ensure that Android doesn't do extra work).
Attachment #679174 - Flags: review?(bnicholson)
(Assignee)

Comment 25

6 years ago
Created attachment 679175 [details] [diff] [review]
WIP Patch for backgrounds

I'm just putting this up to save it or in case someone else wants to play with it. It adds background behind the icons (but not when they are saved like Lucas suggested above).
(Reporter)

Updated

6 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

6 years ago
Keywords: reproducible
QA Contact: aaron.train
Hey folks, let's try this: I adapted the look we use in our add-ons and downloads panel, where we float an icon in the middle of a neutral coloured background - http://cl.ly/image/2f1J0m2P213G

favicons are 16dip in size, and the background colour is #e3e7ed

(I tried ones where we sampled the dominant colour, but it just looked too overwhelming)
Correction. After some deliberation in IRC, we are going to go with an alternate that I came up with earlier: http://cl.ly/image/2m143B0a3a2F

Still using 16dip favicons, but set inside a 32dip #fafbfc square with a 1dp corner radius.
(Assignee)

Comment 28

6 years ago
Created attachment 681110 [details] [diff] [review]
Patch

White background. Screenshot on an SIII:

https://dl.dropbox.com/u/72157/home3.png
Attachment #679175 - Attachment is obsolete: true
Attachment #681110 - Flags: review?(bnicholson)
(Assignee)

Comment 29

6 years ago
Just noticed that our search engine icons for some reason aren't resized.
before you change that, can you send me a build? I'd be interested to see if having a different sized icon for search is useful at all.
(In reply to Wesley Johnston (:wesj) from comment #29)
> Just noticed that our search engine icons for some reason aren't resized.

They use awesomebar_suggestion_row.xml, so you'll have to change that layout too.
(Assignee)

Comment 32

6 years ago
Created attachment 681150 [details] [diff] [review]
Patch 2/2

Fixes up search icons. I needed a bitmap, and it seems a bit like a waste to use a Drawable for these anyway? A build without this patch is at:

https://dl.dropbox.com/u/72157/fennec-favicons.apk
Attachment #679174 - Attachment is obsolete: true
Attachment #679174 - Flags: review?(bnicholson)
Attachment #681150 - Flags: review?(bnicholson)
Comment on attachment 681150 [details] [diff] [review]
Patch 2/2

Have you tried this with different sites using the "Add Search Engine" feature for text boxes? Without the try/catch here, I wonder if we'd hit any problems if the favicon is corrupt (or if there's no favicon at all for the search engine).
Attachment #681150 - Flags: review?(bnicholson) → review+
This is looking good.

Request from myself and Stephen Horlander (who has been thinking about favicons for far longer than I have) -- Is there a way we can use the 16dip size, but if we have larger favicons available, to scale those to 32dip? 

While I worry that it might make the list look janky, Stephen makes a good point that we will continue to want larger favicons from web providers, and that any kind of evangelism or support we can provide might compel more people to make them.
(Assignee)

Comment 35

6 years ago
Created attachment 682176 [details] [diff] [review]
Patch 3/2

Final fixup. This only scales up the icon if its bigger than 16px (not 16dip) and hides/shows the background image. I also added a try-catch around the search icon stuff because it was there before. Pulling and building to test that still.
Attachment #678374 - Attachment is obsolete: true
Attachment #678838 - Attachment is obsolete: true
Attachment #679154 - Attachment is obsolete: true
Attachment #682176 - Flags: review?(bnicholson)
Attachment #681110 - Flags: review?(bnicholson) → review+
Attachment #682176 - Flags: review?(bnicholson) → review+
Depends on: 813346
Depends on: 813810
(Reporter)

Updated

6 years ago
status-firefox20: --- → affected
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox19: affected → fixed
status-firefox20: affected → fixed
Keywords: reproducible, uiwanted
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → Firefox 19
The favicons are changed as expected on the latest Nightly and Aurora builds. Closing bug as verified fixed on:

Firefox 20.0a1 (2012-11-27)
Device: Galaxy Nexus
OS: Android 4.1.1
Status: RESOLVED → VERIFIED
status-firefox19: fixed → verified
status-firefox20: fixed → verified
You need to log in before you can comment on or make changes to this bug.