apache / incubator-pagespeed-ngx

Automatic PageSpeed optimization module for Nginx
http://ngxpagespeed.com/
Apache License 2.0
4.37k stars 363 forks source link

Broken web fonts with IPRO #893

Open nikolay opened 9 years ago

nikolay commented 9 years ago

I enabled IPRO and all web fonts stopped working due to Firefox complaining that the fonts are rejected by the sanitizer. More silently, Chrome and IE rejected some of the web fonts as well (specifically, FontAwesome, GitHub: https://github.com/FortAwesome/Font-Awesome). Previously, we were getting other issues with web fonts.

downloadable font: rejected by sanitizer (font-family: "FontAwesome" style:normal weight:normal stretch:normal src index:1) source: http://www3.vocativ.com/content/themes/vocativ/fonts/fontawesome-webfont.woff
downloadable font: rejected by sanitizer (font-family: "FontAwesome" style:normal weight:normal stretch:normal src index:2) source: http://www3.vocativ.com/content/themes/vocativ/fonts/fontawesome-webfont.ttf

Last, but not least, for IE web fonts to work, a trick applied is to add a #iefix to the web font URL. Unfortunately, the IPRO tries to "optimize" the .css file and shards the file with #iefix on a different shard. I think you should ignore the hash when calculating the shard number, otherwise, the same file might be referenced at two different shards.

nginx: v1.7.9 ngx_pagespeed: v1.8

jeffkaufman commented 9 years ago

This sounds like the problem you initially reported as #719. This was fixed in 1.9; could you see if this happens on pagespeed 1.9?

jeffkaufman commented 9 years ago

It sounds like the #iefix issue may be unrelated, and could still be a problem in 1.9. Could you give more details about that? For example, what does the font css look like before and after processing by PageSpeed?

nikolay commented 9 years ago

This is the file: http://www.vocativ.com/content/themes/vocativ/css/build/fonticons.css

IPRO actually translates the URLs like this:

But now I realize that the second one has an empty ?, which now makes sense.

Anyway, given ?#iefix is a common practice around archaic browsers (http://stackoverflow.com/questions/8050640/how-does-iefix-solve-web-fonts-loading-in-ie6-ie8), maybe PageSpeed can handle it differently, i.e. strip the whole ?#iefix - I'm not sure if an empty query string by RFC should be treated differently than without any query string.

jeffkaufman commented 9 years ago

Hmm. Could you try turning on PreserveUrlRelativity?

jeffkaufman commented 9 years ago

In general pagespeed shouldn't be ignoring that question mark; servers are allowed to treat /foo and /foo? differently so we shouldn't collapse them. Testing now I see:

<link rel=stylesheet href="css">
<link rel=stylesheet href="css?">

becomes:

<link rel=stylesheet href=A.css.pagespeed.cf.fMmK0P8uVk.css>
<link rel=stylesheet href="A.css,q.pagespeed.cf.fMmK0P8uVk.css">
jeffkaufman commented 9 years ago

Testing on my own, I can't get pagespeed to rewrite the urls in this at all:

@font-face {
  font-family: 'FontAwesome';
  src: url('../../fonts/fontawesome-webfont.eot');
  src: url('../../fonts/fontawesome-webfont.eot?#iefix') format('embedded-opentype'), url('../../fonts/fontawesome-webfont.woff') format('woff'), url('../../fonts/fontawesome-webfont.ttf') format('truetype'), url('../../fonts/fontawesome-webfont.svg#fontawesomeregular') format('svg');
  font-weight: normal;
  font-style: normal; }

Did you have to turn anything on to get pagespeed to touch these urls?

nikolay commented 9 years ago

The only setting that made a difference was turning IPRO on, but I am gonna re-test on v1.9 and confirm if it's still an issue. I actually was going to release with v1.9, but I got some issues (with exactly the same configuration I had working with v1.8, v1.9 wasn't merging assets) and had to revert back to v1.8, but now I have more time to test and report issues. Although I agree in general that ? should be treated differently by default, it makes sense to have a manual override option that will treat those identically or even an option to strip off all query parameters from known extensions (like static files).

oschaaf commented 9 years ago

"it makes sense to have a manual override option that will treat those identically or even an option to strip off all query parameters from known extensions (like static files)." I wonder, could this be achieved by changing ngx_pagespeed's input via nginx's url rewriting capabilities?

nikolay commented 9 years ago

@oschaaf Actually that's a good idea as I already strip off stuff like Google Analytics tags and other things that just love to bust caches, but the problem is that it won't work with IPRO, which can (and does) shard ?#iefix inconsistently (by design)!