Closed Bug 366324 Opened 17 years ago Closed 9 years ago

SVG site icons (favicons, shortcut icons) support

Categories

(Core :: Graphics: ImageLib, enhancement, P5)

enhancement

Tracking

()

VERIFIED FIXED
mozilla41
Tracking Status
firefox41 --- fixed
relnote-firefox --- 41+

People

(Reporter: Aleksej, Assigned: Dolske)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat, testcase, Whiteboard: [see comment 14, comment 24, comment 29 for possible solutions])

Attachments

(5 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1) Gecko/20061208 Firefox/2.0.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a2pre) Gecko/20070108 Minefield/3.0a2pre

Firefox should support SVG favicons. They could then be shown scaled up in Page Info, and be more useful at higher resolutions.

Reproducible: Always

Steps to Reproduce:
1. Load the testcase (see the URL or the attachment, if available).
2. Look at the tab and at the location bar, where a favicon should be.
Actual Results:  
The "no favicon" image is shown

Expected Results:  
The SVG favicon should be shown, as specified in the testcase.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #250856 - Attachment description: Testcase v1 → Testcase v1 Should be accessed from a web server; copy murka.svg to /favicon.svg
Attachment #250856 - Attachment mime type: application/x-zip-compressed → application/x-7z-compressed
Attachment #250856 - Attachment is obsolete: true
This depends on bug 276431, since the favicon in the url bar is a <xul:image/>.
Depends on: 276431
Keywords: testcase
This testcase can be viewed online, because it uses the data uri to embed the svg.
Antialiasing results in blurriness at low resolutions. Current wisdom among icon designers is that icons below a certain size (certainly at 32x32 and below) need to be specially optimised for the size they are to be used at.

While I can't see that there would be any problems with using SVG for favicons under the same constraints as for img tags, it doesn't follow that it would be correct to scale these up for any other purpose. If such a use was established, designers/developers would want an alternative way of configuring the larger icon.

For a 16x16 icon, SVG is also likely to be a much larger file than a raster image.
Summary: SVG favicons (shortcut icons) support → SVG site icons (favicons, shortcut icons) support
Blocks: 120352
Product: Core → SeaMonkey
Product: SeaMonkey → Firefox
QA Contact: tabbed-browser → tabbed.browser
Not sure this should be wontfixed outright, but absolutely not a priority.
Priority: -- → P5
Target Milestone: --- → Future
My opinion is that SVG favicons should be supported like any other format. The "scale up" thingy was just a suggestion, but this is about implementing SVG support for favicons.
I'd like to see this feature in SeaMonkey as well.  Is it necessary to file a separate report?
Not sure if there's any Firefox specific change needed to make this work. This might essentially just be a duplicate of bug 276431, in which case a separate bug for SeaMonkey wouldn't make a lot of sense.
If there's nothing Firefox-specific about this, perhaps the product should be changed from Firefox to Core.
No, in that case this would actually be duplicate of bug 276431.
This "just works" with my current patch for bug 276431. (using testing build linked in bug 276431 comment 77, which is built from m-c + patches from http://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/file/20d4eae2b924 )

Here's a screenshot of this bug's testcase, working, in that testing build.

I think this should just be a duplicate of bug 276431, as hinted in comment 10 & comment 12.
Attachment #446032 - Attachment description: screenshot with WIP svg_images patch → screenshot of testcase working, with tryserver build from bug 276431
Comment 13 isn't entirely correct. bug 276431 (which just landed - yay!) does the bulk of the work required for this, but favicons still don't quite work reliably.

The current behavior (with bug 276431) is that the favicon displays correctly the first time, but then the next time (when it's retrieved from a cache), it's not displayed.

I think all we need here is to get imgLoader::GetMimeTypeFromContent* working for SVG content. Basically, given a big character-buffer of our SVG content, we need to infer that it's an SVG file.

I used to have a hack in my bug 276431 patch-queue that simply checked whether the first four characters of our buffer matches "<svg", but that's clearly not sensitive enough to catch all (or even most) SVG files.

I don't know whether it'd be best to just search through our content for a <svg> root node, or instead to rework the imgLoader code so it caches the mimetype together with the data. (so GetMimeTypeFromContent wouldn't even be required anymore)  Need to look into this more.

* http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/imgLoader.cpp#1872
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Actually, I only saw a black square in the favicon area instead of the cat...

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100926 Firefox/4.0b7pre
Ah yes.  You actually are seeing the image -- you're just seeing the upper-left 16x16 chunk of it. :) That's because the first testcase didn't use a viewBox on its SVG content, which means it's not requesting to be scaled up/down to fit its viewport.  (You'll find that you get the same results in Firefox, Opera, & Webkit if you view that SVG image in a 16px-by-16px <object>, <embed> tag, or browser-window -- they'll all just crop out a 16-by-16 black rectangle from the upper-left corner).

FWIW, it worked in the screenshot that I'd posted because an earlier of my patch handled heights/widths a bit differently.

I'm attaching an updated version of the testcase, including a viewBox on the SVG content.  That fixes the problem.

Side note: It may be that, for SVG content that has pixel-valued width & height attributes, we want to effectively synthesize a viewBox using that width/height.  No spec currently calls for that, but it's what Opera does for <img> and for favicons, and Erik filed an issue on it in the SVG WG tracker:
  http://www.w3.org/Graphics/SVG/WG/track/issues/2258

At the moment, though, SVG content needs to have a viewBox attribute, to tell us what region should be scaled up/down, in order for Firefox to auto-size it to fit a favicon-box or an <img> (or any other container).
Attachment #255775 - Attachment description: testcase → testcase (not using viewBox)
Requesting blocking status for this.  Right now SVG-as-a-favicon is kind of broken -- it only loads the first time, but then doesn't show up when we load it from cache, as noted in comment 14.
blocking2.0: --- → ?
Broken is bad, but how often is this actually used? And does this represent a regression? No debate from me on fixing it, but holding the release is a higher bar.
Blocking- for now, but feel free to re-nom with answers to comment 18 if you think I'm underestimating the bustage.
blocking2.0: ? → -
Not a regression, & not used frequently -- both of which are because SVG-as-an-image is a new feature in Gecko.

I'd rather not ship with this un-fixed, but I can understand the blocking- reasoning.  Hoping fix this & land with approval+, I guess.
blocking2.0: - → ?
gah, sorry, didn't meant to renom. bugzilla preserving-old-flag-state-across-page-reload fail.
blocking2.0: ? → ---
(In reply to comment #15)
> Actually, I only saw a black square in the favicon area instead of the cat...
(In reply to comment #16)
> Ah yes.  You actually are seeing the image -- you're just seeing the upper-left
> 16x16 chunk of it. :) That's because the first testcase didn't use a viewBox

BTW, this behavior changed in Bug 614649, so now both testcases (with- and without-viewbox) look right on the first load.

(favicon still doesn't show up on subsequent loads, though, per comment 17)
Moving to Core|ImageLib, since (per comment 14) that's where the code is that needs fixing.
Component: Tabbed Browser → ImageLib
Product: Firefox → Core
QA Contact: tabbed.browser → imagelib
Target Milestone: Future → ---
Given bug 111373 comment 49 (basically: shifting away from allowing/supporting animated favicons), it might be simpler to fix this bug here by rasterizing the SVG's first rendered frame, storing that in a PNG (or whatever's convenient), and sticking *that* in the favicon cache.

That would work around the issue brought up in Comment 14. (trying to detect "this is definitely SVG" given a buffer of data from the favicon cache)
Depends on: 650556
What’s left for this, is it just a mime check required in GetMimeTypeFromContent to fix the cache issue?
Putting some rudimentary checks into that function does solve it, though I wonder how best to test for ‘<?xml…<svg’ without wasting users or firefox’s time.
Not to mention if there's a huge <!-- ... --> comment before the <svg> that you have to sniff through.

Ideally we should just redesign the cache to store the mimetype as well as the image data.  Sniffing through possible-XML to try to guess whether it's SVG is kind of an ugly hack.
If all browsers don't support SVG favicons sometime soon we're going to have some damn horrible favicons on high PPI screens.
Oh the horrors of bitmap icons on retina displays.
Depends on: 878922
We should have the same pipeline as for <img>. Meaning we should check if the content's MIME type is image/svg+xml and use an SVG codepath in that case and just hand the content (ignoring its MIME type) to the image decoder otherwise.
(In reply to Anne (:annevk) from comment #35)
> We should have the same pipeline as for <img>.

We do, in general.

> Meaning we should check if
> the content's MIME type is image/svg+xml and use an SVG codepath in that
> case and just hand the content (ignoring its MIME type) to the image decoder
> otherwise.

That's what we do the first time.

The problem is, we don't have the mimetype on *subsequent* loads of the favicon (because we pull the data from the favicon cache on subsequent loads, which only stores the raw data and assumes that's enough).

See comment 14 / comment 29.
(and comment 24 has an alternate solution I'd forgotten about: rasterize once and stick that in the favicon cache.)

[I forgot this was assigned to me -- unassigning, since I'm not actively working on this at the moment, so that someone else can take it if they like. I want to see this fixed and will hopefully have time to circle back to it eventually, so I'm tagging myself with "needinfo" so that I don't forget about it entirely.]
Flags: needinfo?(dholbert)
Whiteboard: [see comment 14, comment 24, comment 29 for possible solutions]
Assignee: dholbert → nobody
Status: ASSIGNED → NEW
Any news on this?
It's possible (though the matter needs greater study) that the best solution here is to get rid of the favicon cache entirely, and just artificially boost the expiration time of favicons in our normal caches. We could really benefit from a simpler model here; if this problem existed in our normal (necko/imagelib) caches, we'd have fixed it long ago, but since it's in obscure code that nobody ever touches, this bug is still open after 7 years.
Sorry for introducing imprecise terminology up above -- for the record, I believe the mentions of "favicon cache" on this bug are really talking about the "Favicon Service" aka "nsFaviconService", which is accessed (in toolkit/places code) via nsFaviconService::SetAndFetchFaviconForPage():
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsFaviconService.cpp?rev=96a566fa1599#211

I believe the relevant troublesome mimetype sniffing is happening via a call to NS_SniffContent(), inside of AsyncFetchAndSetIconFromNetwork::OnStopRequest():
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/AsyncFaviconHelpers.cpp?rev=8340783e3145#638
Flags: needinfo?(dholbert)
From 1172342 -- I see both Twitter and Pinterest replying with an explicit "Content-Type: image/svg+xml" -- so it seems like at least for this case there shouldn't need to be any content type sniffing going on?
Flags: needinfo?(dholbert)
(In reply to Seth Fowler [:seth] - PTO until 6/12 from comment #39)
> It's possible (though the matter needs greater study) that the best solution
> here is to get rid of the favicon cache entirely

I had similar thoughts while looking at 1172342. Maybe not _entirely_ (since a cache is useful for, say, the history menu or the awesomebar). But it does seem a little unusual that we do this extra caching/checking for just the favicon specified by a page we're currently loading.
(In reply to Justin Dolske [:Dolske] from comment #43)
> From 1172342 -- I see both Twitter and Pinterest replying with an explicit
> "Content-Type: image/svg+xml" -- so it seems like at least for this case
> there shouldn't need to be any content type sniffing going on?

IIRC, we handle that correctly the first time the favicon is loaded. But then, we stick it in the favicon cache (which *only* stores the data, and assumes it can sniff the content-type from that later on as-needed). So on subsequent loads, we pull the favicon out of the cache & fail to sniff the content-type.
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #45)
> (In reply to Justin Dolske [:Dolske] from comment #43)
> > From 1172342 -- I see both Twitter and Pinterest replying with an explicit
> > "Content-Type: image/svg+xml" -- so it seems like at least for this case
> > there shouldn't need to be any content type sniffing going on?
> 
> IIRC, we handle that correctly the first time the favicon is loaded. But
> then, we stick it in the favicon cache (which *only* stores the data, and
> assumes it can sniff the content-type from that later on as-needed). So on
> subsequent loads, we pull the favicon out of the cache & fail to sniff the
> content-type.

Well, hmm, there is a mimetype column in the favicon cache:

CREATE TABLE moz_favicons (  id INTEGER PRIMARY KEY, url LONGVARCHAR UNIQUE, data BLOB, mime_type VARCHAR(32), expiration LONG, guid TEXT);

Except it ends up blank for SVG. :| I wonder if something just isn't hooked up properly, or it's just harder than one would think to get it from point A to point B... *looks*
Attached patch sniff-less workaround? (obsolete) — Splinter Review
This isn't a complete fix, but seems like an incremental improvement? Sites explicitly setting the content-type for SVG would work (as Twitter/Pinterest seem to do at the moment, intentional or not).

I suppose one downside is that native menus on OS X don't seem to like SVG images, and so the History/Bookmarks menu will be missing a favicon when SVG is used.
Attachment #8616488 - Flags: feedback?(mak77)
Interesting! I wasn't aware that the favicon cache stores the mimetype. (or intends to store it, at least)

Anyway, as I recall, comment 45 describes basically what happened for all favicons, when I was poking at this back in 2010; favicon image-data is treated as a local blob whose content-type needs to be sniffed on subsequent loads.  It sounds like we can do better by pulling out the mimetype, though (as long as it's been set) perhaps. It's also possible I misunderstood what was going on.
(In reply to Justin Dolske [:Dolske] from comment #47)
> This isn't a complete fix, but seems like an incremental improvement? Sites
> explicitly setting the content-type for SVG would work (as Twitter/Pinterest
> seem to do at the moment, intentional or not).

I think this is a reasonable thing to depend on (sites manually sending the correct mimetype), FWIW.  I'm pretty sure we depend on that for SVG-as-an-image in other contexts (e.g. <img>, backgrounds), too.
Yes, sites MUST set the MIME type correctly for SVG content. We don't want to ever sniff for SVG.
Comment on attachment 8616488 [details] [diff] [review]
sniff-less workaround?

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

FWIW, the reason the favicon service exists, even if we already have a cache, is because we have views showing thousands of favicons, but we clearly don't want to open thousands of file handles. It's very useful for perf reasons to have all blobs stored in a single file. It also enables other interesting facts like allowing to set custom icons for bookmarks, but we don't expose those in Firefox UI (some add-ons do) so this is a minor fact. The important thing is that we have views showing multiple favicons at once.

The approach looks sane to me, if you tested it working, it only needs a small xpcshell-test (in toolkit/components/places/tests/unit/) that tries to setAndFetch an SVG icon and checks it with getFaviconData. (I shortened API names here, just look at mozIAsyncFavicons.idl)
Attachment #8616488 - Flags: feedback?(mak77) → feedback+
Attached patch Patch v.2 (obsolete) — Splinter Review
Added the xpcshell test.
Attachment #8616488 - Attachment is obsolete: true
Attachment #8621304 - Flags: review?(mak77)
QA Contact: dolske
Assignee: nobody → dolske
QA Contact: dolske
Comment on attachment 8621304 [details] [diff] [review]
Patch v.2

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

::: toolkit/components/places/tests/unit/test_svg_favicon.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

no more needed (the default is PD if no header exists)

@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +function run_test() {
> +  run_next_test();
> +}

no more needed as well, if you use add_task

@@ +12,5 @@
> +    uri: pageuri,
> +    transition: TRANSITION_LINK,
> +    visitDate: Date.now() * 1000
> +  }).then(run_next_test());
> +});

please use add_task and yield PlacesTestUtils.addVisits

I'd like to see the whole test in a single add_task call

@@ +16,5 @@
> +});
> +
> +// Test adding the favicon
> +add_test(function test_setFavicon() {
> +  function onSetComplete(aURI, aDataLen, aData, aMimeType) {

yield new Promise(resolve => {
  ...whatever...
  resolve();
});

@@ +30,5 @@
> +});
> +
> +// Test fetching the favicon
> +add_test(function test_getFavicon() {
> +  function onGetComplete(aURI, aDataLen, aData, aMimeType) {

let data = yield PlacesUtils.promiseFaviconData(pageuri);

data is { uri, dataLen, data, mimeType }
Attachment #8621304 - Flags: review?(mak77) → review+
Attached patch Patch v.3Splinter Review
Fixed nits.
Attachment #8621304 - Attachment is obsolete: true
Just to summarize what got fixed here:

SVG favicons already kinda-sorta worked, but only for the first time a page was loaded in a session. Prior to this patch, Places wasn't able to properly cache the favicon so it got treated as a bad/corrupt icon, and wouldn't show up on future page loads (and always failed for places that only use the favicon cache, such as menus). See bug 1172342 comment 0 (which got duped here) for exactly what was happening.
(In reply to Justin Dolske [:Dolske] from comment #47)

> I suppose one downside is that native menus on OS X don't seem to like SVG
> images, and so the History/Bookmarks menu will be missing a favicon when SVG
> is used.

Filed bug 1174284 to get SVG images working in native OS X menus.
It's great that this bug seems to finally get a patch after all these years. So when, or in which release, do you think this will arrive?
Flags: needinfo?(dolske)
(In reply to Simen Mangseth from comment #58)
> It's great that this bug seems to finally get a patch after all these years.
> So when, or in which release, do you think this will arrive?

This is landing in Firefox 41, and that's what it will ship with unless it gets uplifted to an earlier release. See https://wiki.mozilla.org/RapidRelease/Calendar for more details.
Flags: needinfo?(dolske)
(In reply to :Gijs Kruitbosch from comment #59)
> (In reply to Simen Mangseth from comment #58)
> > It's great that this bug seems to finally get a patch after all these years.
> > So when, or in which release, do you think this will arrive?
> 
> This is landing in Firefox 41, and that's what it will ship with unless it
> gets uplifted to an earlier release. See
> https://wiki.mozilla.org/RapidRelease/Calendar for more details.

Thanks for the information. However, I just tried Firefox 41a1 from the Nightly channel, and in both sites like Twitter and in my own testcases, the SVG favicon still seems to appear only on the first page load - am I doing something wrong or is it just not fixed in 41 yet?
(In reply to Simen Mangseth from comment #60)
> (In reply to :Gijs Kruitbosch from comment #59)
> > (In reply to Simen Mangseth from comment #58)
> > > It's great that this bug seems to finally get a patch after all these years.
> > > So when, or in which release, do you think this will arrive?
> > 
> > This is landing in Firefox 41, and that's what it will ship with unless it
> > gets uplifted to an earlier release. See
> > https://wiki.mozilla.org/RapidRelease/Calendar for more details.
> 
> Thanks for the information. However, I just tried Firefox 41a1 from the
> Nightly channel, and in both sites like Twitter and in my own testcases, the
> SVG favicon still seems to appear only on the first page load - am I doing
> something wrong or is it just not fixed in 41 yet?

It's not fixed until this bug is marked fixed and the change lands on mozilla-central instead of just fx-team. And then of course, you'd still need to wait for the first nightly build that includes the change. It's the weekend now, and it's not merged to m-c yet, so you'll have to wait for a later build - maybe tomorrow's, maybe Monday, maybe even Tuesday, depending on circumstances. All assuming the change sticks and doesn't cause problems that require it to be backed out.

If you have more questions about the lifecycle of patches/bugs, please just email me - this bug is not really the best place to have this discussion.
https://hg.mozilla.org/mozilla-central/rev/ed791601d8e7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
On today's Nightly, Twitter favicon is always black!
Twitter is providing a black icon [1]. I think this is related to Safari 9 pinned tabs implementation [2]. They provide a <meta name="theme-color" content="#55acee"> for Safari. I don't know if websites can provide SVG icons with anything else than black to get the same result in Safari but a normal icon elsewhere. We should reach out to Twitter and Apple here, before Safari 9 ships.

[1] https://abs.twimg.com/a/1434065683/img/t1/favicon.svg
[2] https://developer.apple.com/library/safari/releasenotes/General/WhatsNewInSafari/Articles/Safari_9.html#//apple_ref/doc/uid/TP40014305-CH9-SW20
Yelp has the same issue (providing a black SVG favicon, with <meta name="theme-color"> to get it colorized in Safari I guess). Filed bug 1174548.
I spun off bug 1174552 to cover Tech Evang for Twitter (comment 63 / comment 64).
This bugfix probably merits mentioning in the release notes for Firefox 41 --> adding relnote keyword. (I think that's the right way to remind ourselves to do that?)
Keywords: relnote
Release Note Request (optional, but appreciated)
[Why is this notable]: New feature (or rather, fixing an existing feature to reliably work in a case where it previously was mostly-broken; see comment 56.)

[Suggested wording]: SVG images can be used as favicons. (Fixed a bug that made them disappear after first pageload.)

[Links (documentation, blog post, etc)]: none yet, AFAIK
relnote-firefox: --- → ?
Keywords: relnote
Depends on: 1181681
Keywords: site-compat
Depends on: 1218041
I have reproduced this bug with Firefox 3.0a2pre on Windows 7(64-bit) with the instructions from comment 0.

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9a2pre) Gecko/20070108 Minefield/3.0a2pre

Verified as fixed with Latest Firefox Aurora(Build ID : 20151211004013)

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0

[bugday-20151211]
Thanks! Marking as "Verified".
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.