Using the latest Drupal core (7.22) and Shiny, the Open Sans font fails to load when CSS Aggregation is turned on. This seems to be due to a longstanding core bug that drops @import tags when the files aren't local when aggregation is turned on.

CommentFileSizeAuthor
#5 font_url_aggregation-1975138-2.patch1012 bytestimaholt
#2 font_url_aggregation-1975138.patch356 bytestimaholt

Comments

timaholt’s picture

timaholt commented
Status: Needs review » Active

Realized this was just because the path started with // instead of http:// The // was making Drupal think it was a local file, and the @import tag was dropped when it couldn't find it. Forcing a full http:// allows it to work, but breaks https:// sites.

timaholt’s picture

timaholt commented
Status: Active » Needs review
StatusFileSize
new font_url_aggregation-1975138.patch356 bytes

Patch attached.

timaholt’s picture

timaholt commented
Status: Active » Needs work

Patch works for http but not for https. Rethinking this.

Ignigena’s picture

Ignigena commented

The easiest fix for this might be to just hardcode in the https value. It will ensure no insecure content warnings on sites with SSL whilst not breaking the CSS aggregation.

Don't know if this is the best practice and I know that SSL sometimes has a bit of additional negotiating overhead which can add to the total load time. But running a test on Pingdom Tools it seems like the difference is minimal:

HTTP version (NY): 44ms
HTTPS version (NY): 74ms

Otherwise we'll need to go the PHP route and embed the OpenSans CSS within the document head rather than using an @import statement within the CSS file.

If I'm making no sense feel free to call me out. It's the end of a long week!

timaholt’s picture

timaholt commented
Status: Needs work » Needs review
StatusFileSize
new font_url_aggregation-1975138-2.patch1012 bytes

Actually this patch takes a different approach, and loads the protocol agnostic css file via a drupal_add_css in the preprocess_html hook. This should work on both, and also it works with aggregation turned on!

Ignigena’s picture

Ignigena commented
Version: 7.x-1.1 » 7.x-1.x-dev
Status: Needs review » Reviewed & tested by the community

Yes, that would be the "proper" method :)

Tested on my installation and everything seems to apply correctly to the 7.x-1.x development branch. Changing this to RTBC.

damien tournoud’s picture

damien tournoud commented

Before committing, let's add a comment explaining why we are doing that. Also, please open a core issue. The bug is in drupal_load_stylesheet_content() (that uses a regexp (!) to exclude external includes).

timaholt’s picture

timaholt commented

I filed a core bug here: http://drupal.org/node/1978794

dudenhofer’s picture

dudenhofer commented
Status: Reviewed & tested by the community » Fixed

I applied the patch, added the comment, and replicated the update in the scss file as well. This has been committed now, so I'm marking this as fixed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.