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

url() bug #807

Closed
WraithKenny opened this issue May 15, 2012 · 10 comments
Closed

url() bug #807

WraithKenny opened this issue May 15, 2012 · 10 comments

Comments

@WraithKenny
Copy link

While using the browser, not node, in dev mode:
.test { background: url(); background: url('test'); background: url(''); }
when "manually invoke the parser and compiler" via JavaScript will output:
.test { background: url(); background: url('test'); background: url([object Object]); }
but when compiled via the 'rel set to “stylesheet/less”' method:
.test { background: url(http://www.example.com/less/); background: url('http://www.example.com/less/test');
with the last declaration causing a FATAL ERROR: b.value.charAt is not a function

This might be related to or explain a number of reported issues about "url()" and "string interpolation"

@neonstalwart
Copy link
Contributor

@WraithKenny let me help you with that list #111, #120, #132, #134, #177, #258, #294, #302, #325, #331, #361, #380, #409, #439, #465, #471, #483, #542, #559, #569, #598, #621, #636

i could be wrong about a few of those but that was my findings based on a quick look at titles.

@neonstalwart
Copy link
Contributor

also @WraithKenny you might reconsider if you really want to be working with a library that has a list like that which hasn't been addressed in over 2 years.

@jasonkeene
Copy link

The problem I ran into with url() has to do with absolute strings:

@my_absolute_url_fragment: '/path/to/something';
.test {
    background: url('@{my_absolute_url_fragment}/my_something');
}

while compiling in the browser it gives you:

.test {
    background: url('http://www.example.com/less//path/to/something/my_something');
}

but if you have:

@my_absolute_url_fragment: 'path/to/something';
.test {
    background: url('/@{my_absolute_url_fragment}/my_something');
}

it comes out fine. I'm guesing url checks if the first character is '/' before
interpolating the string.

@WraithKenny
Copy link
Author

@neonstalwart The point of this issue was to present a reduced test case which is reproducible. The other issues I linked seemed to only be reproducible under somewhat specific scenarios (which might explain why they haven't been addressed: they are seemingly not reproducible) or might be related in some other way, but regardless, I do like this library a lot, and thought this might help improve it and close some outstanding issues.

@neonstalwart
Copy link
Contributor

@WraithKenny #331 is a fairly simple test case that demonstrates the problem. if it would just be merged...

@WraithKenny
Copy link
Author

@jasonkeene Yes, though that's only reproducible using the rel=less method. If you compile by calling less's parse method in JavaScript the expected results are returned without an error. But I take your point about the leading slash, that does fix a certain test case in the rel=less environment.

To illustrate:
.test { background: url(); background: url('test'); background: url('/test'); }
is output exactly the same in the JavaScript call version, but returns when used via rel=less:
.test { background: url(http://www.example.com/less/); background: url('http://www.example.com/less/test'); background: url('/test'); }

The variable interpolation isn't related I don't think :-).

General testing note: I'm running less in the browser both via the method and using my own WordPress plugin called Scripts n Styles, which compiles LESS via JavaScript and shows the output in real-time (makes testing easier).

@WraithKenny
Copy link
Author

I would assume compiling in a node.js environment would show no problems at all and thus hide the issue.

@neonstalwart
Copy link
Contributor

compiling in node has its own set of issues too. paths in general are broken when doing relative paths. you'll also notice that there are 0 browser based tests. the typical situation has been that "fixes" have introduced regressions in the browser due to the lack of tests.

@dash-
Copy link

dash- commented Sep 1, 2012

I don't want to join development or put out a patch, but I do have an explanation and a fix (version 1.3.0).

Permanent fix (Non-Minified Source) Line 3013 (function tree.URL):

if (typeof(window) !== 'undefined' && !/^(?:https?://|file://|data:|/|@)/.test(val.value) && paths.length > 0) {

Explanation:

@base-url: "http://assets.fnord.com";
.thing{ background-image: url("@{base-url}/images/bg.png"); }

During parsing, url("@{base-url}/images/bg.png") matches as a url and currently gets compared to the regular expression: /^(?:https?://|file://|data:|/)/

"@{base-url}/..." doesn't match (since the variable hasn't been interpolated yet).

So its considered a relative path. So it gets turned into: url("http://localhost:3000/path/to/stylesheets/@{base-url}/images/bg.png").

Then variable interpolation takes place, turning it into: url("http://localhost:3000/path/to/stylesheets/http://assets.fnord.com/images/bg.png");

The fix is to include @ as an initial dis-qualifier of relative paths (as I've done in my fix above). I think it will still be interpolated and then re-ran through the tree.URL transform. If the interpolated value is relative, I think it will then be prefixed with the absolute url; if it is absolute, it will be left alone.

I hope someone else takes this minor fix and puts the energy in to merge it into the code base. (Cuz I'm too busy to do it).

@lukeapage
Copy link
Member

thanks, but that only half fixes the problem. There is a better patch (I think) to move the calculation after evaluation (after variables have been assessed). I will be looking into this next week.

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

5 participants