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

[gatsby-plugin-manifest] Default favicon set doesn't render well (Firefox, retina display) #19899

Closed
cycleseven opened this issue Dec 1, 2019 · 18 comments · Fixed by #23077
Closed
Labels
status: awaiting author response Additional information has been requested from the author type: bug An issue or pull request relating to a bug in Gatsby

Comments

@cycleseven
Copy link
Contributor

Description

When using the default favicon set produced by gatsby-plugin-manifest, the icon used by Firefox looks low-res (on my retina Macbook).

Firefox

Screenshot 2019-12-01 at 10 41 38

Screenshot 2019-12-01 at 10 41 38

Chrome

Screenshot 2019-12-01 at 14 14 30

Screenshot 2019-12-01 at 14 14 30

Steps to reproduce

This affects any site using gatsby-plugin-manifest's automatic icon generation, including Gatsby's own. You could build that project to reproduce this.

Expected result

Favicon looks sharp in both browsers.

Actual result

Favicon looks low-res in Firefox.

Environment

  System:
    OS: macOS 10.14.6
    CPU: (8) x64 Intel(R) Core(TM) i7-4850HQ CPU @ 2.30GHz
    Shell: 3.2.57 - /bin/bash
  Binaries:
    Node: 12.11.1 - /var/folders/hp/53td47vs5bq5pcx9j918qsfc0000gn/T/yarn--1575197307021-0.11396977243068873/node
    Yarn: 1.19.2 - /var/folders/hp/53td47vs5bq5pcx9j918qsfc0000gn/T/yarn--1575197307021-0.11396977243068873/yarn
    npm: 6.13.1 - /usr/local/bin/npm
  Languages:
    Python: 3.7.3 - /Users/owen/.pyenv/shims/python
  Browsers:
    Chrome: 78.0.3904.108
    Firefox: 70.0.1
    Safari: 13.0.3
  npmPackages:
    gatsby: 2.15.36 => 2.15.36
    gatsby-image: 2.2.27 => 2.2.27
    gatsby-plugin-manifest: 2.2.21 => 2.2.21
    gatsby-plugin-offline: 3.0.14 => 3.0.14
    gatsby-plugin-react-helmet: 3.1.11 => 3.1.11
    gatsby-plugin-sharp: 2.2.29 => 2.2.29
    gatsby-plugin-styled-components: 3.1.9 => 3.1.9
    gatsby-plugin-typography: 2.3.12 => 2.3.12
    gatsby-source-filesystem: 2.1.31 => 2.1.31
    gatsby-transformer-remark: 2.6.32 => 2.6.32
    gatsby-transformer-sharp: 2.2.21 => 2.2.21

Proposed fix

Adding the 32x32px version to the default icons config fixes this, and looks good in other browsers I tested too (Chrome, Safari, iOS Safari).

Happy to submit a PR with this change, unless there's a known reason to avoid including the 32x32 resolution.

@moonmeister
Copy link
Contributor

Strange that adding a lower resolution image fixes this. Have you found any information on why this fixes the problem?

@moonmeister moonmeister added topic: plugins type: bug An issue or pull request relating to a bug in Gatsby status: awaiting author response Additional information has been requested from the author labels Dec 2, 2019
@cycleseven
Copy link
Contributor Author

My original description of "looks low-res" isn't a good one actually. "Jagged" is a much better way to put it: it's from poor downscaling rather than upscaling (eg. some kind of nearest-neighbour thing going on).

16x16: that classic "blurry" appearance of low-res being scaled up

Screenshot 2019-12-02 at 21 56 37

32x32: lovely

Screenshot 2019-12-02 at 21 57 22

48x48: jagged

Screenshot 2019-12-02 at 22 01 19

512x512: more jagged still

Screenshot 2019-12-02 at 22 07 00

As for why 32x32 is the fix: I didn't find any sort of official spec from Mozilla on 32x32 being preferred. It's pretty empirical, just inspected the favicon loading for a few sites in the wild.

Github, Gitlab, Stack Overflow, MDN all look sharp, and they all load a 32x32 favicon. Some are png, some are ico, so the format doesn't seem to matter.

Screenshot 2019-12-02 at 22 38 33

Screenshot 2019-12-02 at 22 19 39

Screenshot 2019-12-02 at 22 38 57

Screenshot 2019-12-02 at 22 19 57

Wikipedia, Gatsby's own site and Amazon have that jagged appearance, and a 48x48 icon.

Screenshot 2019-12-02 at 22 38 02

Screenshot 2019-12-02 at 22 37 45

Interestingly (depending on what kind of things you find interesting 😄) the icons in the address bar suggestions seem to get downscaled perfectly fine. It's just the icon in the tab itself. So I imagine Firefox will fix this from their end one day (but after trying out their nightly build, that day has not come yet). I'll look into opening a bug with Firefox if one doesn't exist already.

Screenshot 2019-12-02 at 22 23 31

So I guess the question is whether there's a downside to including the 32x32 to work around this.

@moonmeister
Copy link
Contributor

thanks for the detailed information. I did a quick look and learned you can actually have multiple icon tags with different sizes. https://www.emergeinteractive.com/insights/detail/The-Essentials-of-FavIcons/

Right now the "favicon" support in the plugin just takes the smallest icon passed in and uses it for the favicon. If we want to fix this the better option would probably to have it scale that to 32x32 or add multiple tags with different size favicons.

Upon further review it seems the "sizes" property on rel="icon" is not actually supported in any browser. so Generating a 32px size may be the best option. It seems it'd be ideal if the icon was in the "ico" format to containing 16, 32, and 48 pixel sizes, but alas sharp doesn't support that.

Resources:
https://realfavicongenerator.net/blog/favicon-why-youre-doing-it-wrong/
https://github.com/audreyr/favicon-cheat-sheet
https://realfavicongenerator.net/favicon_checker

@cycleseven
Copy link
Contributor Author

cycleseven commented Dec 8, 2019

This is definitely a bit of a rabbit hole. Real Favicon Generator even goes as far as adding in extra image processing steps to make sure iOS/Android homescreen icons have solid backgrounds and appropriate padding, etc.

After reading a bit more, the important points to me are:

  • Even today, 32 and 16 still look like they're the standard favicon sizes loaded by desktop browsers [1]. I see 48 being recommended as a resolution that should be included inside favicon.ico too, if using that. Apparently that recommendation came straight from Microsoft. But I can't find any actual examples of browsers actually preferring to make use of the 48 size.
  • favicon.ico is only needed for older IE versions (6-10): otherwise the PNGs are fine.

Since gatsby-plugin-manifest doesn't currently produce the ICO format, maybe a good first step could just be to fix the favicon sizes to [32, 16] when include_favicon is true, and add those <link> tags to the head (so untying the favicon sizes from the manifest/apple-touch-icon sizes).

<link rel="icon" href="/icons/icon-32x32.png">
<link rel="icon" href="/icons/icon-16x16.png">

Sounds like you agree?

the better option would probably to have it scale that to 32x32 or add multiple tags with different size favicons

...Generating a 32px size may be the best option

[1] 32 is described here as "standard for most desktop browsers". Real Favicon Generator only includes 16 and 32 resolutions for the PNG favicons.

For reference, an example of Real Favicon Generator's HTML output:

<link rel="apple-touch-icon" sizes="180x180" href="/apple-touch-icon.png">
<link rel="icon" type="image/png" sizes="32x32" href="/favicon-32x32.png">
<link rel="icon" type="image/png" sizes="16x16" href="/favicon-16x16.png">
<link rel="manifest" href="/site.webmanifest">
<meta name="msapplication-TileColor" content="#da532c">
<meta name="theme-color" content="#ffffff">

@lifeiscontent
Copy link

lifeiscontent commented Dec 22, 2019

839FB6DE-7537-48A8-BEB2-CA555BBDCDC8
FWIW I just updated my site to use Gatsby. Here's the source image: https://github.com/lifeiscontent/lifeiscontent.net/blob/master/src/images/icon.svg

and here's a screen shot of how the icon gets generated when being saved to home screen.

@moonmeister
Copy link
Contributor

@lifeiscontent Is this what you'd expect? Did you have a question? I don't mean to be rude, just trying to understand.

@moonmeister
Copy link
Contributor

@cycleseven I agree we should probably scale it back to 32px. I see clear indications from spec that the sizes attribute on link tags is completely unsupported by browsers, so we should probably just keep what we're doing and change the resolution. Yes, RFG can be a rabbit hole, I also think it's one that's not correct at times, I've read to much spec to trust everything it does (though I haven't always done real world testing). I'll play the code with this and see how it goes.

@lifeiscontent
Copy link

@moonmeister hey, thanks for your reply. If you look at the link I send below the image, the source image is square as required by the gatsby favicon feature if I use a PNG instead of an SVG it results in a correct image output.

So the issue is when going from PNG to PNG it is correct, but when going from SVG to PNG it produces undesired results.

@moonmeister
Copy link
Contributor

@lifeiscontent I still don't fully understand the issue. From the sound of it, this is related to manifest icons and not the favicon, more specifically the conversion of SVGs. I'd suggest opening your own issue and include a side by side screenshot of correct vs. incorrect.

@lifeiscontent
Copy link

@moonmeister given this key in the gatsby config:

https://github.com/lifeiscontent/lifeiscontent.net/blob/756f7d56a9505077f40508e0157c49acdf4bbbd4/gatsby-config.js#L29

if I put the same exact image (except an SVG version)

the icons that are generated from gatsby are off-center and cut off from the bottom.

the image processing (crop, position) should yield the same results I would assume. But they do not.

after I update my code from an SVG to a PNG the results that got processed are now correct as you can see here: https://lifeiscontent.net/icons/icon-96x96.png?v=a5fe6232036f625a4049535fb204c378

however, if I used the SVG variant of the icon, it would be processed as I demonstrated in the previous screen shot of my phone.

@lifeiscontent
Copy link

lifeiscontent commented Jan 3, 2020

also I noticed when I give a transparent PNG as the source, the background is black, while if I give a transparent SVG it is white. so there are some really weird inconsistencies.

I'd expect the background of the icon to be the color of background_color in the config.

or throw an error specifying that an icon needs to not be transparent.

@github-actions

This comment has been minimized.

@github-actions github-actions bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Jan 24, 2020
@moonmeister moonmeister added not stale and removed stale? Issue that may be closed soon due to the original author not responding any more. labels Jan 24, 2020
@lifeiscontent
Copy link

@moonmeister if there is any other information I can provide, please let me know, I'd love to help figure out how to move forward with this 👍

@moonmeister
Copy link
Contributor

You're welcome to submit a PR. I may have time, I may not...Zombie apocalypse and all...

@lifeiscontent
Copy link

@moonmeister can you point me to the code that might be relevant for a PR? I haven't contributed before.

@moonmeister
Copy link
Contributor

@lifeiscontent Alright, here is the link where we simply extract the first and theoretically smallest icon and use it for the favicon.

https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-plugin-manifest/src/gatsby-ssr.js#L33

In the gataby-node.js file in that same package we need to explicitly create the 32px variant if the favicon generation is happening.

@cycleseven
Copy link
Contributor Author

Hey @moonmeister, the zombie apocalypse actually turned out to be a decent time for me to give a PR a shot 😄 (#23077). It's my first time attempting to contribute code to Gatsby, so imagine it will need some work. But maybe can get the ball rolling on this issue again at least.

@lifeiscontent I also think your issue is different to my original one, I doubt this PR will help with it unfortunately. Maybe it's worth opening a separate issue for the SVG problem so it doesn't get lost?

@pieh
Copy link
Contributor

pieh commented Apr 21, 2020

Published gatsby-plugin-manifest@2.3.6

cycleseven added a commit to cycleseven/cy7.io that referenced this issue Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting author response Additional information has been requested from the author type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants