Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow SVG images in favicons #1551

Merged
merged 2 commits into from
Mar 23, 2020
Merged

Allow SVG images in favicons #1551

merged 2 commits into from
Mar 23, 2020

Conversation

Toflar
Copy link
Member

@Toflar Toflar commented Mar 23, 2020

Fixed restricting favicons to .ico only.
See https://caniuse.com/#feat=link-icon-svg

@Toflar Toflar self-assigned this Mar 23, 2020
@Toflar Toflar added the bug label Mar 23, 2020
@Toflar Toflar added this to the 4.9 milestone Mar 23, 2020
@leofeyer leofeyer merged commit fe5729c into contao:4.9 Mar 23, 2020
@leofeyer
Copy link
Member

Thank you @Toflar.

@Toflar Toflar deleted the bugfix/svg-favicon branch March 23, 2020 12:26
@aschempp
Copy link
Member

Are you sure this is correct? Browsers always request favicon.ico from the root site. Browsers might support a <meta tag with an SVG icon, but our root page icon is only for the fallback route, and that route cannot return an SVG icon imho?

@Toflar
Copy link
Member Author

Toflar commented Mar 25, 2020

The way I understood the spec is that in case the link meta tag is missing, the browser shall request /favicon.ico and use that. Browsers require Content-Type: image/svg+xml to be present to handle the SVG accordingly which should happen automatically due to our mime type guesser. Hmmm, maybe that guesses image/svg though, so we might want to hardcode image/svg-xml...
We could also add a Vary: Content-Type although I don't see this super critical as our reverse proxy is going to be invalidate the cache entry anyway when you change the selection and the browsers likely cache whatever they want to anyway^^

@aschempp
Copy link
Member

On caniuse.com, the Reference section links Specification to https://html.spec.whatwg.org/multipage/links.html#rel-icon, which is titled 4.6.6.8 Link type "icon". That section says

In the absence of a link with the icon keyword, for Document objects whose URL's scheme is an HTTP(S) scheme, user agents may instead run these steps in parallel:

  1. Let request be a new request whose url is the URL record obtained by resolving the URL "/favicon.ico" against the Document object's URL, client is the Document object's relevant settings object, destination is "image", synchronous flag is set, credentials mode is "include", and whose use-URL-credentials flag is set.

  2. Let response be the result of fetching request.

  3. Use response's unsafe response as an icon as if it had been declared using the icon keyword.

So maaayyybeee that actually works …

@leofeyer
Copy link
Member

and that route cannot return an SVG icon imho?

That's a valid point IMHO. Our favicon.ico route cannot return an SVG icon, can it? AFAIK, you need a link tag such as <link rel="icon" type="image/svg+xml" href="favicon.svg">.

@ausi /cc

@Toflar
Copy link
Member Author

Toflar commented Mar 25, 2020

Of course it can? It does now?

@leofeyer
Copy link
Member

But doesn't the browser expect image/x-icon if the file is called favicon.ico?

@Toflar
Copy link
Member Author

Toflar commented Mar 25, 2020

No, that‘s the whole idea of this spec :)

@leofeyer
Copy link
Member

I have tested this in Chrome, Firefox and Safari on Mac OS.

  1. It does not work in any browser with the guessed MIME type image/svg.

  2. Once I had overwritten the MIME type, Chrome and Firefox correctly loaded the SVG favicon. Safari did render the favicon when I requested /favicon.ico in the URL, however, it refused showing it in the tab bar.

  3. Safari also refused to show the SVG favicon if if was added via <link rel="icon", so I guess that is ok.

So we definitely have to overwrite the MIME type with image/svg+xml to make this work.

@Toflar
Copy link
Member Author

Toflar commented Mar 26, 2020

See #1554

leofeyer pushed a commit that referenced this pull request Mar 27, 2020
Description
-----------

| Q                | A
| -----------------| ---
| Fixed issues     | #1551

Commits
-------

d5d58f8 Make sure the favicon controller sends the correct content-type header
leofeyer pushed a commit to contao/core-bundle that referenced this pull request Mar 27, 2020
Description
-----------

| Q                | A
| -----------------| ---
| Fixed issues     | contao/contao#1551

Commits
-------

d5d58f80 Make sure the favicon controller sends the correct content-type header
@leofeyer leofeyer changed the title Allow svg in favicons Allow SVG images in favicons Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants