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

Add support for "retina" favicon in Firefox Mac #17

Open
finneas opened this Issue Jan 27, 2014 · 11 comments

Comments

Projects
None yet
4 participants
@finneas

finneas commented Jan 27, 2014

But first of all, thanks for this wonderful tool! :-)

@phbernard

This comment has been minimized.

Contributor

phbernard commented Jan 28, 2014

Thank you! :)

I suppose you have a Retina Mac, right? Could you first run the compatibility test with Firefox in order to spot the current RFG'code behavior with this platform and browser?

As far as I understand, FF for Retina Mac is using the 32x32 PNG picture, or else the 16x16 PNG picture. However this contradicts issue 751712. Do you have reference information about this topic?

@finneas

This comment has been minimized.

finneas commented Jan 29, 2014

Yes I do have a retina MBP. Compatibility test: done.
So maybe I should just change the loading order of the pictures – an move the 32x32 icon to the last position?

@phbernard

This comment has been minimized.

Contributor

phbernard commented Jan 30, 2014

I received the results of your first test. Ok, the 16x16 PNG picture is used, and it is blurry. Unfortunately, this result is not a surprise.

I would be glad if you could try the workaround you suggest: if you move the 32x32 declaration to the last position, what happens? Is the 32x32 icon used instead? (ie. not blurry anymore)

If the test is conclusive, I will update the generated code.

@finneas

This comment has been minimized.

finneas commented Jan 30, 2014

The test is conclusive: I moved the 32x32 icon declaration to the last position and now my favicon is as crisp as expected in Firefox :)

@phbernard

This comment has been minimized.

Contributor

phbernard commented Jan 31, 2014

Perfect. Next step: I test this change on several platforms, just to make sure that there are no unfortunate side effect.

@phbernard

This comment has been minimized.

Contributor

phbernard commented Feb 11, 2014

The fix was deployed a minute ago.

I would be glad if you could re-run the compatibility tests (the four of them; they look similar but each of them test different settings) and thus confirm the generated code works well in your configuration.

@finneas

This comment has been minimized.

finneas commented Feb 11, 2014

Compatests re-run. I confirm the generated code works well in my config. Let's hope there are no unwanted side effects with other configurations…

@phbernard

This comment has been minimized.

Contributor

phbernard commented Feb 12, 2014

Thanks! I re-run the tests on a bunch of devices and browsers, so I'm rather confident.

@phbernard phbernard closed this Feb 12, 2014

@davidklebanoff

This comment has been minimized.

davidklebanoff commented Mar 31, 2015

I just used version .10 and am experiencing this problem. I checked the outputted html and it has the 16x16 favicon listed last, so that's most likely the culprit.

Did this bug get reintroduced by a recent change?

@g10p

This comment has been minimized.

g10p commented Mar 31, 2015

I noticed that too. I had to put the favicon-32x32.png link after the 16x16 one.

@phbernard

This comment has been minimized.

Contributor

phbernard commented Mar 31, 2015

Ouch. That's right. The 16x16 and 32x32 PNG icons are inverted in the current version. Clearly a bug. It was introduced during the refactoring that took place in 2014Q4 when writing the non-interactive API. Thank you for reporting!

The fix itself it simple and I updated the compatibility tests to reflect the change, so this new version can be tested right now.

However I will have a very limited bandwidth this week so I won't be able to re-run the tests and deploy the fix.

@phbernard phbernard reopened this Mar 31, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment