Resolve urls in css files instead of simply joining them

[tommedema]

• Fixes issue described below
• Includes test case
• All tests pass

I noticed a bug where paths to resources specified in css files are resolved incorrectly when they are absolute root paths, such as /image.png instead of image.png or ./image.png. This is because you are using path.join instead of path.resolve or url.resolve. You have to resolve paths, not join them.

Currently there is a bug in this scenario:

• basePath (relativeTo) is http://www.example.com/
• example.com/index.html has a stylesheet link at assets/main.css: <link href="assets/css-root-path.css" rel="stylesheet">
• assets/main.css has a background image with url(/image.png) (notice the prefix of /)

The background image is now resolved to http://www.example.com/assets/image.png whereas obviously it should resolve to the root domain i.e. http://www.example.com/image.png due to the preceding /.

I've resolved this in this PR. However, I am curious about your feedback on my solution for these reasons:

• I had to prepend a / to path.resolve in css.js, and then remove it again, to deal with the path library always returning absolute urls with path.resolve, while css.js is not aware of the page's root url (and therefore it expects relative urls):

var resolved = path.resolve( '/', settings.rebaseRelativeTo, src ).substring( 1 )

This works reliably, and all tests pass, but it still feels a little strange. It might be better if we don't return relative paths in css.js at all? Now we rebase relative paths in css.js to new relative paths, and then make them absolute in html.js.

• there is another call to path.join whereas this might have to be path.resolve too in html.js at line 89:

rebaseRelativeTo: path.relative( settings.relativeTo, settings.rebaseRelativeTo || path.join( settings.relativeTo, args.src, ".." + path.sep ) )

This line is hard for me to understand. Do you think this path.join should also be path.resolve? If so, we are missing a test case for these cases (root paths like /img.png), because all tests are passing atm. Generally, resolving makes more sense than joining.

So I think either we need to merge this, or we need to add more test cases if we feel like something is not entirely fixed it with this solution.

@jrit would appreciate your input. I kind of need this working asap so any input would be great.

[jrit]

I'm going to sit down with a cup of tea and try and sort through this, hopefully today. 1) I want to make sure I understand the change since its been a while since I was looking in that area 2) if tests pass here and in npm/juice then I'd have to accept this behavior was basically "undocumented" to begin with and whatever we decide is fine, though it may require a major version bump (which is fine)

[tommedema]

@jrit great.

Also, if you don't like the implementation with resolve, you could just copy the test form this PR, and then have it pass in another way. :)

[tommedema]

@jrit while all tests pass, I just discovered another untested case where this doesn't work

if rebaseRelativeTo starts with a sublevel (../something, e.g. because relativeTo is example.com/en and a link was found at example.com/assets/main.css) then the initial / in .resolve will cause the first .. to be disregarded since it is already at the root.

So, we still need to resolve rather than doing a naive join (this issue is not fixed), but my approach is not a fits-all solution. We need to find a better solution that also works in the above case. I will try to find one.