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

update url parsing logic to match current browsers #103

Open
skratchdot opened this issue Sep 25, 2019 · 1 comment
Open

update url parsing logic to match current browsers #103

skratchdot opened this issue Sep 25, 2019 · 1 comment

Comments

@skratchdot
Copy link

it would be nice if url parsing worked like latest firefox/chrome.

for instance, running the following snippet in those browsers:

(() => {
  var div = document.createElement('div');
  div.style.backgroundImage = '   url(http://some/url/here1.png)   , URL(2)  ';
  console.log(`backgroundImage:${div.style.backgroundImage};`);
})();

prints:

backgroundImage:url("http://some/url/here1.png"), url("2");

i've searched the code a bit, and think we can achieve this by modifying the following function:

cssstyle/lib/parsers.js

Lines 210 to 247 in f2db1eb

exports.parseUrl = function parseUrl(val) {
var type = exports.valueType(val);
if (type === exports.TYPES.NULL_OR_EMPTY_STR) {
return val;
}
var res = urlRegEx.exec(val);
// does it match the regex?
if (!res) {
return undefined;
}
var str = res[1];
// if it starts with single or double quotes, does it end with the same?
if ((str[0] === '"' || str[0] === "'") && str[0] !== str[str.length - 1]) {
return undefined;
}
if (str[0] === '"' || str[0] === "'") {
str = str.substr(1, str.length - 2);
}
var i;
for (i = 0; i < str.length; i++) {
switch (str[i]) {
case '(':
case ')':
case ' ':
case '\t':
case '\n':
case "'":
case '"':
return undefined;
case '\\':
i++;
break;
}
}
return 'url(' + str + ')';
};

we can also add some better unit tests here:

  • describe('parseUrl', () => {
    it.todo('test');
    });
  • test('url parsing works with quotes', () => {
    var style = new CSSStyleDeclaration();
    style.backgroundImage = 'url(http://some/url/here1.png)';
    expect(style.backgroundImage).toEqual('url(http://some/url/here1.png)');
    style.backgroundImage = "url('http://some/url/here2.png')";
    expect(style.backgroundImage).toEqual('url(http://some/url/here2.png)');
    style.backgroundImage = 'url("http://some/url/here3.png")';
    expect(style.backgroundImage).toEqual('url(http://some/url/here3.png)');
    });

if i make the "formatted" string look like url("foo.png") instead of the current url(foo.png), then I could see that breaking people's existing tests (but it would "match" modern browsers more closely). If I just allow trailing/closing whitespace, and multiple urls, then that would be "more" backwards compatible I think.

I can work on this in the next few days and submit a PR. I have quite a few ideas on how to get it to work.

Thanks!

@jsakas
Copy link
Member

jsakas commented Oct 25, 2019

@skratchdot thanks for the info. The spec calls for support of whitespace, single quotes, or double quotes in the URL field https://www.w3.org/TR/CSS2/syndata.html#uri.

It looks like chrome, ff, safari all return URLs in double quotes. I think if we did the work to support advanced syntax for this property it would make sense to make it match the return format also. That would be a backwards compatible API change and would have to be tagged accordingly.

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

2 participants