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

CSS resource url path rewrite works incorrectly in 0.8.1 & 0.8.1.1 #2106

Closed
bobmonsour opened this issue May 2, 2014 · 40 comments
Closed

CSS resource url path rewrite works incorrectly in 0.8.1 & 0.8.1.1 #2106

bobmonsour opened this issue May 2, 2014 · 40 comments

Comments

@bobmonsour
Copy link

After updating to Meteor 0.8.1 and then also to 0.8.1.1, when referencing Font Awesome icons, the icons no longer display correctly.

In the js console, the following message is displayed (not seen prior to 0.8.1):

"Resource interpreted as Font but transferred with MIME type text/html..."

Reproduction: see https://github.com/bobmonsour/meteor-issue-2106

Two images below show the output of the repro app alongside the same html when rendered without Meteor.

Expected:
correct display

Actual:
incorrect display

bobmonsour added a commit to bobmonsour/meteor-issue-2106 that referenced this issue May 2, 2014
@aaronjudd
Copy link

just a comment, I'm using font-awesome with 0.8.1+ and it's fine, the difference might be in implementation. I'm doing

@import "//netdna.bootstrapcdn.com/font-awesome/4.0.3/css/font-awesome.min.css";

might be something to consider if you can't get the local files working.

@emgee3
Copy link
Contributor

emgee3 commented May 2, 2014

I didn't actually check but probably related to

Convert relative URLs to absolute URLs when merging CSS files.

in https://github.com/meteor/meteor/blob/devel/History.md#meteor-command-line-tool

@raix
Copy link
Contributor

raix commented May 2, 2014

Just updated my app to 0.8.1.1 and its not serving *.svg / *.png / *.ttf / *.woff?

Seems as if path prefixes gone bad:

http://localhost:3000/client/css/image/svg/qs_tab_9_12.svg
should be:
http://localhost:3000/image/svg/qs_tab_9_12.svg

http://localhost:3000/client/fonts/kimmy_design_-_lunchbox-webfont.woff
should be:
http://localhost:3000/fonts/kimmy_design_-_lunchbox-webfont.woff

@ghost
Copy link

ghost commented May 2, 2014

I had the same problem. I use Less and Bootstrap and have the following code in my main.less file:

@import "../../public/components/bootstrap/less/bootstrap.less";

In bootstrap/less/variables.less is a variable: @icon-font-path: "../fonts/";.
So I just needed to overwrite it in my main.less with @icon-font-path: "/fonts/"; and the font loading works again.

@mquandalle
Copy link
Contributor

Author of the relative URLs rewriting feature here. @bobmonsour In your reproduction do you expect files in client/fonts to be served as static resources? That seems wrong to me, Meteor expects such static resources to be located in the public root directory.

@raix Where is your CSS file?

@bobmonsour
Copy link
Author

@mquandalle I see what you mean about the public directory for static assets. I followed the font awesome convention of placing css and fonts directories at the same level in the project. This worked fine prior to 0.8.1.

@bobmonsour
Copy link
Author

@mquandalle I just edited the repro app locally, moving the fonts directory to /public and changing the relative path in the css file and I get the same (incorrect) result along with the console messages.

@ghost
Copy link

ghost commented May 2, 2014

I have the problem again with another 3rd party CSS dependency. I created a repro: https://github.com/Sanjo/css-paths-repro
The load-dependencies package is my package where I list all my 3rd party dependencies that should get loaded. The example dependency nice-background has CSS with a relative url.
The url is transformed to public/components/nice-background/background.jpg. But it should transform to /components/nice-background/background.jpg.

@raix
Copy link
Contributor

raix commented May 2, 2014

@mquandalle havent checked, I'm using less so the css is added using addStylesheet in the source handler. But havent checked paths, on a really tight schedule.

matfin added a commit to matfin/cv.mattfinucane.ie that referenced this issue May 3, 2014
This is to address a bug raised in release 0.8.8.1 - see
meteor/meteor#2106
@pookguy88
Copy link

@aaronjudd which file did you put that line in?

@aaronjudd
Copy link

@pookguy88 you should be able to add that in your primary css file. Just using a traditional CSS import, like this:

/* CSS declarations go here */
@import "//netdna.bootstrapcdn.com/font-awesome/4.0.3/css/font-awesome.min.css";
@import "//netdna.bootstrapcdn.com/bootstrap/3.0.0/css/bootstrap-glyphicons.css";
@import url(http://fonts.googleapis.com/css?family=Roboto);

Just my thoughts, we tend to get overly excited about using packages sometimes, when the classic methods still work ;-)

@pookguy88
Copy link

@aaronjudd yup, just tried it right before you replied and works... I disabled the FA .lessimport in my main.less and just did the CSS import. I don't know why changing paths in the variables.less/lessimport file doesn't work though

@aaronjudd
Copy link

@pookguy88 I use this in my LESS files, works great. See: https://github.com/ongoworks/reaction-core/tree/master/client/themes for my full config - I include the import.less files twice, once in the package.js, and once here.

@pookguy88
Copy link

@aaronjudd hmm, k I'll have to look into that..

this is weird, so I think I spoke too soon, that CSS import fix worked in Chrome but in Firefox FA still isn't working, I'm getting weird .woff and .ttf errors (in console) [downloadable font: rejected by sanitizer (font-family: "FontAwesome" style:normal weight:normal stretch:normal src index:1)
source: http://localhost:3000/client/fonts/fontawesome-webfont.woff?v=4.0.3]

@mquandalle
Copy link
Contributor

@sanjo I consider that your code repro is indeed a bug (but not introduced in 0.8.1, it didn't work with previous versions either). I'm not sure how we should fix it. @Slava, is that expected that CSS stylesheets inside the public directory are "bundled" (ie css merged+minified)?

@ghost
Copy link

ghost commented May 6, 2014

@mquandalle I rechecked my app. It worked for me before 0.8.1 because I had a copy of the image directly in the public folder (for some reason I don't remember). So it didn't work before 0.8.1.
I think this is a special case for the public folder. Removing "public/" from the begin of the resolved URLs would fix it, I think.

var publicPrefix = 'public/';
if (publicPrefix === absolutePath.substr(0, publicPrefix.length)) {
   absolutePath = absolutePath.substr(publicPrefix.length);
}

after line https://github.com/meteor/meteor/blob/devel/packages/minifiers/minifiers.js#L113?

@mquandalle
Copy link
Contributor

Yes it worked iff you had an image copy at the public root, as a kind of side effect of CSS minification in one file served in the web root. But otherwise relative links like that never worked before.

About the fix, having this special case for public/ seems hacky. I think that the more Meteor consistent way to solve this issue is to not include stylesheets from the public directory into the big merged minified CSS file, but instead include it manually in the <head> block, as it's just a static resource and relative links will work.

@ghost
Copy link

ghost commented May 6, 2014

I'd like to keep the minfication and concatenation in production (-> when bundeling). This is currently not done when I include the stylesheet in the head. The stylesheets I reference with the package are all third party components that included via Bower. I will try the Bower package instead of my own package.

EDIT: The Bower smart package works fine for my use case.

@Slava Slava changed the title Bug - Font Awesome icons not displayed correctly in 0.8.1 & 0.8.1.1 CSS resource url path rewrite works incorrectly in 0.8.1 & 0.8.1.1 May 7, 2014
@Slava
Copy link
Contributor

Slava commented May 7, 2014

I updated the title to reflect the problem better. The CSS url path rewriting part doesn't handle several cases and this problem is not Font Awesome specific.

@Slava
Copy link
Contributor

Slava commented May 9, 2014

@bobmonsour Hi there, I ran your repro with 0.8.0 (meteor --release 0.8.0) and I still see squares (i.e. fonts were not served). I suspect your browser just had these fonts cached, try to run your reproduction on 0.8.0 and a clean browser state.

Please, post an update if I missed something or when you have a new repro.

@Slava
Copy link
Contributor

Slava commented May 9, 2014

@mquandalle CSS files in '/public' folder should not be merged and served as app's CSS. If it is a thing that is going on, please open a separate issue with a reproduction for that particular bug.

@bobmonsour
Copy link
Author

@Slava Will do. I am traveling now without my laptop, butt will try your suggestion when I return late next week.

@raix
Copy link
Contributor

raix commented May 14, 2014

Hmm, so my issue is a change in the way Meteor handles relative paths

.icon {
  /*
    This url is converted into /client/css/image/png/icon.png
    since the less file is placed in /client/css
  */
  background: url(image/png/icon.png);
}

I currently workaround setting it to /image/png/icon.png - But I still consider it an issue, since /client/css is not public.

@bobmonsour
Copy link
Author

@Slava You are correct. I ran the repro with 0.8.0 and still have the problem. I must have an incorrect path somewhere. Thanks for your sleuthing.

@bobmonsour
Copy link
Author

Fixed it...my solution was to change one line in variables.import.less of the less-fontawesome-4 pacakge from @fa-font-path: "../fonts"; to @fa-font-path: "/fonts"; and to put the fonts directory into the top-level meteor public directory. I think it's time to close this one.

@raix
Copy link
Contributor

raix commented May 15, 2014

Not sure if this issue is resolved, relative paths are still broken? If relative paths are not allowed we should throw a warning to help the user debug imho.

Having image/png/icon.png turn into /client/css/image/png/icon.png is not logical unless we did actually have images there and turned them public.

@bobmonsour
Copy link
Author

You're likely correct @raix. I forgot that @Slava had changed the title to reflect what the real issue was. Also, not likely my place to close this.

@bobmonsour bobmonsour reopened this May 15, 2014
@frozeman
Copy link

+1 this breaks a lot of font icons and images referenced in CSS and other files.

@Slava
Copy link
Contributor

Slava commented May 22, 2014

@frozeman submit a reproduction.

@raix
Copy link
Contributor

raix commented May 23, 2014

@Slava I know I'm not @frozeman but I've created a reproduction https://github.com/raix/reproduce-meteor-issue-2106

I'm currently building a cordova app - and this issue means that I have to do search and replace on the css bundle every time I want to test the app on a device. cordova handles absolute urls differently

@frozeman
Copy link

I thought the reproduction of the thread creator, show the problem already, right?

@Slava
Copy link
Contributor

Slava commented May 23, 2014

@raix I will look at it later, thanks, it is still arguable what is/is not expected, you didn't provide any of the information that looks unexpected

@frozeman see the whole thread. The repro of OP didn't work in the original case on pre-css-refactoring as well

@frozeman
Copy link

If i got time i will create a repro. Thanks @Slava

@raix
Copy link
Contributor

raix commented May 23, 2014

@Slava I maybe missing out on something,

Its unexpected that the relative css paths gets modified.
The current way Meteor allows us to organize our code is eg. by placing it in sub folders.

We have organized our css in the /client/css folder.
All my images and fonts are placed in the /public Should we put less files into the public folder?

I would argue that Meteor should not try to fix the relative paths - relative to what?

The issue for me is that all relative paths are prefixed with /client/css but how does that relate to the public folder. Is the future pattern to create eg. /public/client/icon.png

Does this make more sense? :)

@bobmonsour
Copy link
Author

@raix, here's my understanding of things. While Meteor allows us to put our files just about anywhere, it seems that it's perhaps not a best practice to rely on the locations of the folders we create to organize our code. The "public" directory appears to be much more like the root of a typical website. As a result, when I place my font-awesome fonts directory in the public directory, my reference to them is /fonts. Given that, I'd see no reason to place less files in the public directory as Meteor will process them and combine them for us.

The key elements in the Meteor docs, in my opinion are (from http://docs.meteor.com/#structuringyourapp):

"CSS files are gathered together as well: the client will get a bundle with all the CSS in your tree (excluding the server, public, and private subdirectories)."

and...

"Lastly, the Meteor server will serve any files under the public directory, just like in a Rails or Django project. This is the place for images, favicon.ico, robots.txt, and anything else."

So, if you want to target a specific directory location for files like fonts and images, I think that the public directory is the right place, treating it as the root of the site.

I hope that makes sense.

@raix
Copy link
Contributor

raix commented May 23, 2014

@bobmonsour thanks, yeah well this wasn't the case pre 0.8.0 - so I may be missing out here or am I talking about an other issue #2127?

I don't actually store less/css in public I just tried to prove a point dont think it would work, public is excluded from source handlers?

This issue would make sense if we store images etc. in the /client/ folder - otherwise relative paths would have no relations to how they should be resolved to absolute paths. (a more traditional approach)

But having a reference image/icon.png in /client/css/style.css become ``/client/css/image/icon.pngseems strange / unexpected - since the image is placed in/public/image/icon.png`

We did actually "solve" our problem, its a bit ugly / messy imho - we place the less files in top / app root they are resolved "correctly". without involving the inaccessible/non-existing /client folder

So I guess this change is related to the future server-side rendering / router?

If not an issue I guess we have to think in a different pattern / best practice.

Well, thanks for taking the time to explain, I may be a slow runner on this one,

@frozeman
Copy link

i figured that the paths only break, if i use relative paths. absolut paths seem to work.

@Slava Slava closed this as completed in 1e4838c Jun 5, 2014
@Slava
Copy link
Contributor

Slava commented Jun 5, 2014

I pushed a fix, still need to write tests. Reproduction from @raix now seem to work. Post here if you have a new reproduction with the mentioned css urls rewrite working incorrectly (describing the expected result too).

@Slava
Copy link
Contributor

Slava commented Jun 5, 2014

The fix was: distinguish between CSS code coming from packages vs CSS code coming from the application code.

  • for the packages, every asset is served at the /packages/nameOfPackage/original/path/on/fs/asset.png, i.e. just the filesystem path prefixed by /packages/nameOfPackage/. Every CSS file gets merged into one big CSS file but assets are served relative to the original location
  • for the application folders, every asset is placed in /public/ folder is served at root /. CSS files on the other hand are can be located in any folder and the relative paths rewrite doesn't make much sense, so in the linked commit 1e4838c it is disabled for the application code.

@raix
Copy link
Contributor

raix commented Jun 5, 2014

@Slava Just want to confirm that app css relative paths its solved on devel - I haven't tested paths in packages - Thanks!! :)

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

No branches or pull requests

8 participants