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

Improve url parsing (allow line-feeds) #1196

Merged
merged 1 commit into from
May 12, 2015

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented May 10, 2015

Fixes #1096

@mgreter mgreter self-assigned this May 10, 2015
@mgreter mgreter added this to the 3.2.4 milestone May 10, 2015
@@ -434,6 +434,9 @@ namespace Sass {
alternatives <
alnum,
exactly <'/'>,
exactly <'\\'>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a sequence?

sequence< exactly <'\\'>, alternatives< exactly <'\r'>, exactly <'\n'> > >

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really want to be that strict? Seems to work fine and not sure how ruby sass really does it ... this way it also accepts certain escape sequences (which I guess is correct). Feel free to create a more specific spec test ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As is stands I believe this would allow cases Ruby Sass does not, and that aren't in the CSS Spec (which is bad)

@import url("foo\
bar");
@import url("foo
bar");
@import url(foo
bar);
//Errors with Ruby Sass
@import url(foo\
bar);

Ruby Sass

@import url("foobar");
@import url("foo\a bar");
@import url(foo bar);

With this patch

@import url("foobar");
@import url("foo\abar");
@import url(foo
bar);
@import url(foobar);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it looks like Ruby Sass treats the new line in the quoted string as a space delimited list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I see you're point!

@@ -21,7 +21,7 @@ namespace Sass {
string comment_to_string(const string& text);
string normalize_wspace(const string& str);

string quote(const string&, char q = 0);
string quote(const string&, char q = 0, bool flag = false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does flag mean? Is there a better name that will describe what this is for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't yet come around to look more into it. But it's the reason why I added WIP again. Again I don't really know if we need to move the code part that uses this flag to another function. Will have another look after work 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok cool. I'm just catching up trying to prepare for a new node-sass release.

@xzyfer
Copy link
Contributor

xzyfer commented May 11, 2015

Update the specs sass/sass-spec#374

@mgreter
Copy link
Contributor Author

mgreter commented May 11, 2015

I renamed the flag to keep_linefeed_whitespace. I have no idea why ruby sass does what it does here. So I really can only describe what the flag is actually doing. It seems to be some discrepancy between interpolation, quoted string, strings to css or something else. And I'm out of clues and guesses.

@mgreter mgreter removed the Dev - WIP label May 11, 2015
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 80.17% when pulling 258e8b7 on mgreter:bugfix/issue_1096 into fb33a2a on sass:master.

@xzyfer
Copy link
Contributor

xzyfer commented May 12, 2015

Are just handling url values poorly? The Ruby Sass behaviour seems intuitive to me.

If the url value is a quoted string and the newline is escaped then it's removed.
If the url value is not a quoted string it's a Sass list.

@xzyfer
Copy link
Contributor

xzyfer commented May 12, 2015

I noticed another thing

@import url("foo
bar");

Produces a deprecation warning in Ruby Sass

DEPRECATION WARNING on line 2, column 13 of test.scss:
Unescaped multiline strings are deprecated and will be removed in a future version of Sass.
To include a newline in a string, use "\a" or "\a " as in CSS.

@import url("foo\a bar");

But not Libsass

@import url("foo\a bar");

Libsass does however produce the deprecation warning if there is more content after the @import

@import url("foo
bar");
// foo

@xzyfer
Copy link
Contributor

xzyfer commented May 12, 2015

Given this new information it would seem I was incorrect in thinking Ruby turned multi-line strings into lists. It appears to native support multi-line strings but normalises the newline to a space in the output phase.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 80.1% when pulling 9b8545b on mgreter:bugfix/issue_1096 into 8ca8816 on sass:master.

mgreter added a commit that referenced this pull request May 12, 2015
Improve url parsing (allow line-feeds)
@mgreter mgreter merged commit 928260d into sass:master May 12, 2015
@mgreter mgreter deleted the bugfix/issue_1096 branch May 12, 2015 17:51
@am11
Copy link
Contributor

am11 commented May 12, 2015

🎉
Thanks @mgreter! ,

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

Successfully merging this pull request may close these issues.

Permit multiline in URL
4 participants