Patryk Adamczyk [:patryk] UX Reporter
	Description • 13 years ago

It appears that the favicons the browser downloads from a website are 16x16 pixels and then the browser scales them up to a size optimized for the device resolution. This leads to fuzzy favicons in the URL bar and awesome screen, see screen shot.

	Doug Turner (:dougt)
	Comment 1 • 13 years ago

fabrice, anything we can do?

	[:fabrice] Fabrice Desré
	Comment 2 • 13 years ago

We used to retrieve hi-res favicons when they are available but it looks like we don't anymore.
(I still had the hi res one for http://www.bbc.co.uk/ before browsing the site again)

I'm wondering if http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1279 should not be:

if (rel.indexOf("icon") != -1) {

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 3 • 13 years ago

(In reply to Fabrice Desré [:fabrice] from comment #2)
> We used to retrieve hi-res favicons when they are available but it looks
> like we don't anymore.
> (I still had the hi res one for http://www.bbc.co.uk/ before browsing the
> site again)
> 
> I'm wondering if
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.
> java#1279 should not be:
> 
> if (rel.indexOf("icon") != -1) {

Bug 696433 changed the data sent to Java. We now split the rel="" data in browser.js into an array, wrap each item in "[ ]" and join the items into a single string again. That way we know "[icon]" is matching just "icon" and  not "rubicon".

If we need more checks in the Java to look for other data, we can add that.

	[:fabrice] Fabrice Desré
	Comment 4 • 13 years ago

Simple patch that also uses the apple specific hi res favicons if they are available.

The issue with this patch is that if a rel="icon" appears after the rel="apple-touch-icon" it will override it.

In the xul fennec we kept track of the favicon size and only updated when getting a better one.

I'm not familiar with the java code here but it looks that this would be implemented in the Tab class.

Adding a parameter to updateFaviconURL(url) that specify the size of the icon should be enough.

We would to updateFaviconURL(url, 16) for rel=icon and updateFaviconURL(url, 64) for res="apple-touch-icon"

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 7 • 13 years ago

Comment on attachment 586202 [details] [diff] [review]
wip

The plan sounds pretty good to me:
* Look for and send the "size" data from JS to Java
* Change the method to Tab.updateFaviconUrl(url, size)
* Keep track of the "best" size in the Tab class

	Peter Retzer (:pretzer)
	Comment 8 • 13 years ago

Can the priority of this bug be re-evaluated, please? 
In my eyes, this is a major flaw, that should be fixed in the initial NativeUI release, especially since there already is a solid proposal, how to do that. 
Thank you for your consideration!

	Brian Nicholson (:bnicholson)
	Comment 9 • 13 years ago

apple-touch-icons are intended to be used for adding bookmarks to the homescreen (http://en.wikipedia.org/wiki/Favicon#Device_support). There's a couple of problems using them for favicons:

1) We'd be replacing the expected standard favicon (either the one given in link rel or favicon.ico). No other browser--including the mobile stock browser, Dolphin HD, Firefox on desktop, or Google Chrome--uses the apple-touch-icon as a favicon.

2) More importantly, many sites simply don't provide apple-touch-icons--so if the goal is to remove blurry favicons, this won't be a very effective approach. The fundamental problem is that many sites serve a 16x16px favicon, and we're blowing them up to be approximately 62x62px (on Galaxy Nexus), resulting in either a pixelated image (no anti-aliasing) or a blurry one (with anti-aliasing). A more robust alternative might be to just shrink the favicons in the AwesomeScreen (we could make them 32x32 without anti-aliasing, like we did in XUL Fennec). Chrome for Android actually avoids this problem by removing favicons completely.

Thoughts?

	Brian Nicholson (:bnicholson)
	Comment 10 • 13 years ago

To give an idea of how common apple-touch-icons are, consider the attached screenshot (https://bugzilla.mozilla.org/attachment.cgi?id=585843). nytimes.com, ca.msn.com, and google.com do *not* provide apple-touch-icons; CNN is the only one that does (incidentally, CNN also includes a 48x48 version in its favicon.ico file, which is why it looks better than the others).

	Brian Nicholson (:bnicholson)
	Comment 11 • 13 years ago

CC'ing madhava in case he has any suggestions.

	Aaron Kaluszka
	Comment 12 • 13 years ago

Perhaps use a better upsampling algorithm for this use case? e.g. http://research.microsoft.com/en-us/um/people/kopf/pixelart/paper/pixel.pdf

	Ian Barlow (:ibarlow)
	Comment 13 • 12 years ago

Soft blocking nom?

Would be great to have favicons looking as good as possible.

	:Margaret Leibovic Assignee
	Comment 14 • 12 years ago

This patch just does what XUL fennec did, but I also added apple-touch-icon-precomposed, since that got me more images:
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/browser-ui.js#1023

I'm not sure how I feel about using apple-touch-icons. Just testing a few sites, I noticed that they often give their apple-touch-icons a background color (Twitter and Wikipedia both do this), and it makes the favicon look bad, even though it's higher resolution. I agree with Brian that developers specify these icons with certain expectations of how they'll be used. I think that if we want to use them for the "Add to Home Screen" functionality, that would suit their purpose, but using them in the awesomescreen seems wrong.

We could make this patch just about adding logic for the sizes attribute, but that's not going to give us much of a win until developers start actually using it (almost no websites do). More than anything, I think we just really need evangelism about websites serving up larger icons.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 15 • 12 years ago

We do need to be careful with the types of images we use. Apple touch icons come in different resolutions (low and high) for different devices (iPhone and iPad). The "precomposed" variety allow the designer to add their own effects to the image, otherwise the OS will add the rounded corners and glare. See:
http://developer.apple.com/library/ios/#DOCUMENTATION/AppleApplications/Reference/SafariWebContent/ConfiguringWebApplications/ConfiguringWebApplications.html
http://developer.apple.com/library/ios/#DOCUMENTATION/UserExperience/Conceptual/MobileHIG/IconsImages/IconsImages.html#//apple_ref/doc/uid/TP40006556-CH14-SW11

Note that even the apple icons allow for a "sizes" attribute.

I don't think we want to use the "precomposed" variety at all. Those icons are designed for iOS desktops, not internal browser favicons or Android home screen shortcuts.

We know the folowing:
* Not many sites provide high res favicons
* Those that do lean heavily on the iOS spec and seem to use "precomposed" a lot.
* The iOS touch icons tend to look different than the normal favicon and might not look "right" as internal browser favicons.

Maybe we should just start by supporting the "sizes" variant and see what that gives us. I'd rather play it safer. We could also add non-precomposed apple-touch-icons to see how good or bad those are in the wild.

Madhava, Ian - Thoughts?

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 16 • 12 years ago

Comment on attachment 614961 [details] [diff] [review]
WIP

>+    void handleLinkAdded(final int tabId, String rel, final String href, int size) {
>+        if (rel.indexOf("[icon]") != -1 || rel.indexOf("[apple-touch-icon]") != -1 ||
>+            rel.indexOf("[apple-touch-icon-precomposed]") != -1) {

Let's drop support for "precomposed"

>+            if (rel.indexOf("apple") != -1)
>+                size = 57;

Keep in mind that "apple-touch-icon" can have a size attribute too. You might want to check and see if 'size' is not set first

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>--- a/mobile/android/chrome/content/browser.js
>+++ b/mobile/android/chrome/content/browser.js
>@@ -1832,23 +1832,37 @@ Tab.prototype = {
>           list = target.rel.toLowerCase().split(/\s+/);
>           let hash = {};
>           list.forEach(function(value) { hash[value] = true; });
>           list = [];
>           for (let rel in hash)
>             list.push("[" + rel + "]");
>         }
> 
>+        // We use the sizes attribute if available
>+        // see http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#rel-icon

Not sure if it matters but we could also add the apple docs here too:
http://developer.apple.com/library/ios/#DOCUMENTATION/AppleApplications/Reference/SafariWebContent/ConfiguringWebApplications/ConfiguringWebApplications.html

hmm, that URL is huge - let's not :)

>+        let size = 16;

But maybe we should default to 57 here for "apple-touch-icon"

>         let json = {
>           type: "DOMLinkAdded",
>           tabID: this.id,
>           href: resolveGeckoURI(target.href),
>           charset: target.ownerDocument.characterSet,
>           title: target.title,
>-          rel: list.join(" ")
>+          rel: list.join(" "),
>+          size: size
>         };
> 
>         // rel=icon can also have a sizes attribute
>         if (target.hasAttribute("sizes"))
>           json.sizes = target.getAttribute("sizes");

Do we want to keep this "sizes" JSON ?

	Ian Barlow (:ibarlow)
	Comment 17 • 12 years ago

Interesting. I didn't realize apple was adding the rounding and highlighting on the OS side for web clip icons, I thought they only did that for native apps. 

Sigh.

It would be great to see a build where we allow whatever highest quality icon we can get, whether it's large favicons, or web clip apps (both precomposed and un-precomposed), and see what it looks like. My sense is we will probably want to ditch the icons that are meant to get the apple corner and lighting effect treatment, because they won't look as good when they are just plain squares. 

Can we try a build like this, and possibly even get a small list of pages where we know what kind of favicion / web clip icon we're getting with them?

	:Margaret Leibovic Assignee
	Comment 18 • 12 years ago

I updated this patch to get rid of the apple icons. I tried testing a build with just apple-touch-icon (not apple-touch-icon-precomposed) and I found that at least one site (cnn.com) incorrectly uses a precomposed icon, and I found that other sites (e.g. wikipedia.com) use icons with colored backgrounds that don't really look good as favicons.

More than anything else, I noticed that not many sites even serve us apple-touch-icons, so I don't think it's worth it to try to support it. I think the real solution is that we just need to evangelize specifying larger icons using the standardized <link rel=icon> method.

	:Margaret Leibovic Assignee
	Comment 19 • 12 years ago

This patch just implements the sizes spec. I decided to just pass one size integer to Java for now, since that's all we're using in the UI. If eventually we want to keep track of multiple icons in Java, we can change this to pass an array of sizes.

Since no real websites use this sizes attribute, I made some test pages:
http://people.mozilla.com/~mleibovic/test/icon_sizes/

I also filed bug 751712 for the desktop part of this.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 21 • 12 years ago

Comment on attachment 620891 [details] [diff] [review]
patch


>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>-        tab.setContentType(contentType);
>-        tab.updateFavicon(null);
>-        tab.updateFaviconURL(null);
>+        tab.clearFavicon();

Did you mean to remove the | tab.setContentType(contentType); | ? clearFavicon() does not change the content type.

>+    void handleLinkAdded(final int tabId, String rel, final String href, int size) {

>+        // If tab is not loading and the favicon is updated, we
>+        // want to load the image straight away. If tab is still
>+        // loading, we only load the favicon once the page's content
>+        // is fully loaded (see handleContentLoaded()).

Off-topic, but I think we might want to stop waiting. I think if we updated the favicon here it would avoid the brief flash of "no favicon" image we see when the page loads. Can you file a new bug to investigate?

>diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java
>+    public void updateFaviconURL(String faviconUrl, int size) {
>+        // If we already have an "any" sized icon, don't update the icon.
>+        if (mFaviconSize == -1)
>+            return;

An "any" sized icon must explicitly use "any" right? A simple, regular <link rel="shortcut icon".../> is not considered an "any" ?

r+, but figure out the setContentType issue.

	:Margaret Leibovic Assignee
	Comment 22 • 12 years ago

(In reply to Mark Finkle (:mfinkle) from comment #21)

> >-        tab.setContentType(contentType);
> >-        tab.updateFavicon(null);
> >-        tab.updateFaviconURL(null);
> >+        tab.clearFavicon();
> 
> Did you mean to remove the | tab.setContentType(contentType); | ?
> clearFavicon() does not change the content type.

Good catch, that was a mistake.

> >+    void handleLinkAdded(final int tabId, String rel, final String href, int size) {
> 
> >+        // If tab is not loading and the favicon is updated, we
> >+        // want to load the image straight away. If tab is still
> >+        // loading, we only load the favicon once the page's content
> >+        // is fully loaded (see handleContentLoaded()).
> 
> Off-topic, but I think we might want to stop waiting. I think if we updated
> the favicon here it would avoid the brief flash of "no favicon" image we see
> when the page loads. Can you file a new bug to investigate?

Filed bug 759501. I agree that's something worth looking into.

> >diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java
> >+    public void updateFaviconURL(String faviconUrl, int size) {
> >+        // If we already have an "any" sized icon, don't update the icon.
> >+        if (mFaviconSize == -1)
> >+            return;
> 
> An "any" sized icon must explicitly use "any" right? A simple, regular <link
> rel="shortcut icon".../> is not considered an "any" ?

I assume so, since the spec talks about how "any" should basically just be used for scalable images that really will look right at any size. I would think a website that's using the sizes attribute would use it on all of the rel="icon" links, not just some of them, so I don't think this would be a problem (worst case, we just end up using the icon with the sizes attribute).

	:Margaret Leibovic Assignee
	Comment 23 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd363d3cfee8

	Ed Morley [:emorley]
	Comment 24 • 12 years ago

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

	Peter Retzer (:pretzer)
	Comment 25 • 12 years ago

This should be documented so site owners know they can use it and to help increase adoption, right?

	:Margaret Leibovic Assignee
	Comment 26 • 12 years ago

(In reply to pretzer from comment #25)
> This should be documented so site owners know they can use it and to help
> increase adoption, right?

Yes, good call. Right now it's mobile-only until bug 751712 gets fixed. It's also worth noting that it won't handle multiple images in one .ico/.icns file because we don't have an image decoder to handle that right now.

	Johnathan Nightingale [:johnath]
	Comment 27 • 12 years ago

Margaret, is this worth nomming for .N+? Or scary enough to ride the trains?

	:Margaret Leibovic Assignee
	Comment 28 • 12 years ago

(In reply to Johnathan Nightingale [:johnath] from comment #27)
> Margaret, is this worth nomming for .N+? Or scary enough to ride the trains?

I don't think we found any problems other than bug 760218, so it seems pretty safe. We'd just want to make sure to uplift that bug as well.

	Johnathan Nightingale [:johnath]
	Comment 29 • 12 years ago

(In reply to Margaret Leibovic [:margaret] from comment #28)
> (In reply to Johnathan Nightingale [:johnath] from comment #27)
> > Margaret, is this worth nomming for .N+? Or scary enough to ride the trains?
> 
> I don't think we found any problems other than bug 760218, so it seems
> pretty safe. We'd just want to make sure to uplift that bug as well.

Oooh, that's lovely news! Can you approval-beta nom a rollup patch?

	:Margaret Leibovic Assignee
	Comment 30 • 12 years ago

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: no support for HTML5 sizes attribute in <link rel=icon>
Testing completed (on m-c, etc.): been on m-c since 5/30
Risk to taking this patch (and alternatives if risky): could potentially break <link rel=icon> if web authors are putting weird things in the sizes attribute
String or UUID changes made by this patch: n/a

	Johnathan Nightingale [:johnath]
	Comment 31 • 12 years ago

Comment on attachment 634151 [details] [diff] [review]
Rollup patch (includes patch from bug 760218)

And after all that, we discussed it in triage and decided that 15 is probably soon enough for this, so no need for beta landing. Sorry for the makework, Maggie.