839923 - Many favicons look bad when upscaled for hidpi

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Favicons don't look very good on Retina displays right now. The long-term fix for this is stuff like bug 828508 -- if the site provides an icon better than 16x16, we should use that.

But as a a short term fix -- and for sites that provide ONLY a 16x16 icon -- I think we can still do better. The issue is that upscaling small icons to 32x32 (the native size favicons will be shown at) just doesn't work very well. The icons often end up as fuzzy blobs, and really stand out from the rest of browser chrome, which is generally quite sharp now on Retina displays.

Attached is a simple patch to set CSS's |image-rendering: -moz-crisp-edges| on the favicon. This results in simple nearest-neighbor upscaling, which looks much, well, crisper and fits in better with the general Retina feel. The blurryness is gone.

There is a downside -- this property also affects _downscaling_, so if a site provides a favicon larger than 32x32, or if we generate a favicon from viewing an image, it's possible the downscaling will not be pleasant. But from limited testing it seems ok.

(Screenshots coming)

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Screenshot

Screenshot of common sites with/without this patch.

After the red dino head are mozilla.com schneier.com, both of which are serving 32x32 favicons (and this patch does not change their display). The last two two tabs are viewing images directly (http://i.imgur.com/GCo3GVX.jpg and http://farm9.staticflickr.com/8202/8236404294_580850ed6c_c.jpg), to illustrate that how those are scaled.

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 712293 [details] [diff] [review]
Patch v.1

> @media (min-resolution: 2dppx) {
>   .tab-throbber,
>   .tab-icon-image {
>     list-style-image: url("chrome://mozapps/skin/places/[e-mail]");
>+    image-rendering: -moz-crisp-edges;
>   }
> 
>   .tab-throbber {
>     list-style-image: url("chrome://browser/skin/tabbrowser/[e-mail]");
>   }

It doesn't make sense that .tab-throbber is part of the first selector.

There are unrelated nsLoginManager.js changes in this patch...

Comment 3

[Frank Yan (:fryn)]

Comment on attachment 712293 [details] [diff] [review]
Patch v.1

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

(In reply to Dão Gottwald [:dao] from comment #2)
> It doesn't make sense that .tab-throbber is part of the first selector.

Whoops, that looks like a copy-paste mistake from an earlier HiDPI patch. Please remove it.

r+ only with the removal of the `.tab-throbber,` line and all the nsLoginManager.js changes.
(I'd rather avoid attachment churn.)

Please wait for Stephen's ui-review before pushing a patch for this.

Comment 4

[Stephen Horlander [:shorlander]]

Comment on attachment 712295 [details]
Screenshot

Looks cleaner and sharper. An improvement in most cases.

Comment 5

[Justin Dolske [:Dolske]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/324ef02e2161

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/324ef02e2161

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cccb528b3584