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

[1.3.4.2] CSS URLs without protocol (//url.ws/) being re-written incorrectly #7416

Closed
benlavalley opened this issue Jul 16, 2016 · 16 comments
Closed

Comments

@benlavalley
Copy link

benlavalley commented Jul 16, 2016

I noticed my site wasn't loading some images and custom fonts recently. After thinking it was an issue with my CDN, I did some digging and it appears a CSS minifier change introduced in 1.3.4.4 is stripping protocol-relative URL prefixes from CSS.

Perhaps this is OK as maybe it's something I shouldn't be doing anyway, but I wanted to get an issue submitted for it considering others might have run into it as well.

Reproducing the problem is pretty simple:

  1. meteor create csstest --release 1.3.4.1
  2. Paste the following into csstest/client/main.css:
    .blacklogo { background-size: contain; background-repeat: no-repeat; background-image: url('//d2c1utpwc9g9zq.cloudfront.net/images/nibbl_logo_new_2015new.png'); width: 115px; height: 40px; }
  3. Paste the following into the body tags or a template in csstest/client/main.html:
    <div class="blacklogo" href=#></div>
  4. cd csstest && meteor
  5. Open http://localhost:3000. Observe the amazing logo.
  6. Within the csstest meteor project folder, run: meteor update --release 1.3.4.4
  7. Open http://localhost:3000. The beautiful logo is nowhere to be seen :(

Upon inspecting the CSS, you will see the // prefix before the background URL link is gone and the browser isn't able to render the image.

While I suspect it isn't relevant, my dev box is OS X 10.11.5. with Node 0.10.46 installed.

@vlasky
Copy link
Contributor

vlasky commented Jul 16, 2016

I also wish to a report a problem which is likely to be related.

My meteor app is hosted in a subfolder "/live". The ROOT_URL has "/live" at the end.

I just updated a project from Meteor 1.3.3.1 to Meteor 1.4-beta.14 and now many css url()'s are resolving differently.

e.g. in /live/client/common.css I had this line which loaded correctly in version 1.3.3.1

background: url('/live/images/reports.jpg') no-repeat center top;

I had to change it to the following to get it to load correctly after upgrading to 1.4-beta.14.

background: url('/images/reports.jpg') no-repeat center top;

Without the above change, it would attempt to load the file from /live/live/images/reports.jpg which would of course fail.

Outside of the css url() function, all other references to the relative URL worked exactly as intended - I didn't need to remove the '/live' prefix from them.

@abernix
Copy link
Contributor

abernix commented Jul 16, 2016

I was able to confirm @benlavalley's issue beginning in 1.3.4.2, actually (don't even need to go all the way to 1.3.4.4).

@vlasky Could you test and see if your problem exists in Meteor 1.3.4.2 but works properly in Meteor 1.3.4.1? (I'd like to avoid the huge jump to 1.4-beta.14 in tracking this down).

@abernix
Copy link
Contributor

abernix commented Jul 16, 2016

Sorry, I used all the wrong versions in my last update. I meant 1.3.4 everywhere I said 1.3.3. (Edited now).

@abernix
Copy link
Contributor

abernix commented Jul 16, 2016

@vlasky Ok, I'm almost positive it's the same issue. Based on the code I'm finding, the change you're needing to do makes sense (though it probably should have been better documented as a breaking change, sorry!). This started with #5837 which was intended to make it unnecessary to have to prefix your URLs with the subfolder when running Meteor under a path (see PR for more info). I think the intention of this PR is actually good and it prevents you from having to change all your CSS if you simply want to deploy to a different path. Aside from the frustration of having to make the changes, do you agree?

@vlasky
Copy link
Contributor

vlasky commented Jul 16, 2016

@abernix - thanks for investigating.

In response to your final question, I strongly disagree and urge for the former behaviour to be retained.

Reason: all relative path references were previously consistent throughout our code base - in CSS url()'s, and in DOM attributes etc. Now they are an inconsistent and confusing mix - some places require the "/live" prefix and others don't.

Here is an example to demonstrate the ambiguity:

function setDriverImage(driverId, image) {
  var element = $("#driver-image-" + driverId.toString());

  if (image) {
    element.attr("style", 'background-image: url("data:image/jpeg;base64,'+imag$
  } else {
    element.css({"background-image": "url(/live/images/driver.png)"});
  }
}

Because the CSS URL is being set dynamically, the '/live' is still required in the url() unlike in the .css files where it isn't because of the minifier update in the new version.

@abernix
Copy link
Contributor

abernix commented Jul 16, 2016

@vlasky: You should be using Meteor.absoluteUrl in order to build your dynamic URLs which would take this into consideration and allow seamless deployment changes to new hosts/URLs by simply changing the ROOT_URL of the application.

element.css({"background-image": url(${Meteor.absoluteUrl("images/driver.png")})});

Ultimately, I think the goal was to have a single variable that could be changed and I think I agree with this.

@mquandalle Do you have any input on this topic?

@mquandalle
Copy link
Contributor

mquandalle commented Jul 16, 2016

Yes as @abernix pointed out, the idea is that you should be able to define your application ROOT_URL in a single place, and changing it shouldn’t introduce bugs in the application.

This principle wasn’t introduced by my PR, it existed since a very long time and is indeed behind the creation of the Meteor.absoluteUrl method. The problem my PR fixed was that the ROOT_URL subpath wasn’t handled in the already existing CSS rewriting function to merge the different stylesheets into one.

Protocol-relative URLs shouldn’t be rewritten though, so 👍 for e62e3ba fix. It also means that if for some reason, you really don’t want to write your URLs relatively to your application root you can use a double slash in your CSS file to indicate it:

background: url('//live/images/reports.jpg');

but I don’t see the rationale for doing that. I would strongly favor writing application assets URLs relative to the application root, and use Meteor.absoluteUrl in the JS code—that is also what is used under the hood by FlowRouter.

@abernix abernix changed the title CSS minifier update introduced in 1.3.4.4 strips protocol-relative URL prefix [1.3.4.2] CSS URLs without protocol (//url.ws/) being re-written incorrectly Jul 16, 2016
abernix added a commit to abernix/meteor that referenced this issue Jul 16, 2016
abernix added a commit to abernix/meteor that referenced this issue Jul 16, 2016
Fixes regression caused in 1.3.4.1 which caused network path reference URLs (i.e. //img.domain.com/path/font.eot) used in CSS `url()`'s' to be stripped of their leading slashes causing them to become relative URL paths.

Closes meteor#7416
abernix added a commit to abernix/meteor that referenced this issue Jul 16, 2016
This change should be mentioned as it those who were manually prefixing their relative CSS paths with the same path as their ROOT_URL will need to remove the path.

Relates to meteor#7416
abernix added a commit to abernix/meteor that referenced this issue Jul 16, 2016
@vlasky
Copy link
Contributor

vlasky commented Jul 17, 2016

@abernix @mquandalle - and what about URLs contained in attributes in .html files?

We can't use Meteor.absoluteUrl() in those.

At the moment, we still have to have the "/live" subfolder referenced in things like:

<img src="/live/images/connection-lost.png">

So it seems the inconsistency remains unless you have a solution for this too.

@abernix
Copy link
Contributor

abernix commented Jul 18, 2016

The same way you would call any JavaScript function from a .html template would apply – you would use a (global, if you want) template helper that uses Meteor.absoluteUrl().

Or, assuming you use FlowRouter with arillo:flow-router-helpers you could use the functions it provides (and other Router libraries have similar functions).

@vlasky
Copy link
Contributor

vlasky commented Jul 19, 2016

OK we'll try the global template helper approach. Thanks.

@abernix
Copy link
Contributor

abernix commented Jul 21, 2016

This is resolved in Meteor 1.4-rc.1 (meteor update --release 1.4-rc.1)

@antoinep92
Copy link
Contributor

Using a protocol relative URL forces to specify the host which can be complex when deploying to multiple hosts. Is there any other way to specify absolute (host-relative) paths in css urls and without them being rewritten ?

For exemple, currently url(/test.png) is rewritten to url(test.png), which in our case is absolutely not what we want.

I'm looking to a way to disable the rewritting, either globally (by adding/removing a package or editing a conf file, etc.) or locally (by using special characters or syntax when writing urls). Any help appreciated :)

@mquandalle
Copy link
Contributor

url(//test.png)

@antoinep92
Copy link
Contributor

Thanks for your answer!
Unfortunately protocol-relative urls are relative to the protocol, not thehost, and the domain nametest.png doesn't exist.
To make things clear, if I deploy to https://dom.com/root and am visiting https://dom.com/root/home

css url browser resolves as meteor rewrites as result
test.png https://dom.com/root/home/test.png no rewrite https://dom.com/root/home/test.png
/test.png https://domain.com/test.png test.png https://domain.com/root/home/test.png
//test.png https://test.png no rewrite https://test.png
//dom.com/test.png https://dom.com/test.png no rewrite https://dom.com/png

What I want is write something in my css that doesn't contain dom.com and will be resolved by the browser as https://domain.com/test.png. In "pure" CSS that's /test.png. How do I do that with meteor ?

@dominicarrojado
Copy link

Have the same problem as you @antoinep92 . Did you able to resolve this?

@janka
Copy link

janka commented Dec 18, 2018

#10247

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

No branches or pull requests

7 participants