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

Fix icons on firefox by decoding svg imgs #2378

Merged
merged 1 commit into from
May 30, 2017
Merged

Fix icons on firefox by decoding svg imgs #2378

merged 1 commit into from
May 30, 2017

Conversation

evenstensberg
Copy link
Contributor

@evenstensberg evenstensberg commented May 27, 2017

Before:

skjermbilde 2017-05-27 kl 18 11 41

After:

skjermbilde 2017-05-27 kl 18 10 38

@paulirish
Copy link
Member

paulirish commented May 28, 2017

It's nice having these in utf8 svg as they remain very editable.

I looked into this issue and found these articles which affirm my belief this shouldn't be a crossbrowser issue:

then after debugging the .lh-debug::before triangle icon a bit i saw this:
image

Interestingly, the error message they print is pointing to the wrong thing. the real issue is that the fill attribute doesn't finish.

And why? Well, Taylor's post mentions that #'s need to be escaped. :) And we've got #s for our hex colors. I tried manually changing the color from hex to hsl and everything is good.

So shall we use HSL instead? :)

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx.
let's revert this encoding change and go from hex colors to hsl colors.

http://mothereffinghsl.com y'all.

@ebidel
Copy link
Contributor

ebidel commented May 28, 2017

The fill are definitely it. We ran into that back in #1124 (comment).

@evenstensberg
Copy link
Contributor Author

I thought it was the issue too, but people on the googs explained as if the problem wasn't just the hashes being invalidated.

💯 Agree about utf 8 being most convenient , will change this ✌️

@evenstensberg
Copy link
Contributor Author

Ups, reverted this as I did git rebase -i HEAD and dropped the latest commit, hold on ..

@evenstensberg evenstensberg reopened this May 29, 2017
@evenstensberg
Copy link
Contributor Author

This is ready

@paulirish
Copy link
Member

honestly, this little bug is probably worth a lighthouse audit in its own right: "SVG background-images are cross-browser" :)

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. thanks!

@paulirish paulirish merged commit 9feb62b into GoogleChrome:master May 30, 2017
@paulirish
Copy link
Member

fixes (partiallly) #2320

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants