[SCSS] Fixes issues with URIs when handling interp and unary #217

bgriffith · 2016-08-20T14:49:57Z

This PR rewrites the way we handle URI nodes by allowing either a raw uri or non-raw uri that can be made up of multiple nodes (interps, numbers, idents, unary etc)

It solves issues highlighted in #85

Only implement in SCSS but if approved I'll add it into Sass too.

tonyganch

Can you please explain to me what's the proposed difference between "raw" and "non-raw" types, and what "raw" is supposed to mean now? Comments for checkUri1 and checkUri2 functions seem to be the same :)

tonyganch · 2016-10-13T18:51:52Z

src/scss/parse.js

+
+  // Store the opening parenthesis token as we will reference it's `right`
+  // property to determine when the parentheses close
+  token = tokens[i];


You don't reassign token anywhere later so you can move let from line 4032 here and rename variable to something like leftParenthesis

bgriffith · 2016-10-26T16:20:28Z

@tonyganch From a quick look checkUri1 is looking for non-string url url(image.png) and checkUri2 is looking for string based urls url('image.png')

bgriffith · 2016-10-29T17:55:31Z

@tonyganch Updated the code. Any further thoughts?

tonyganch · 2016-10-31T02:55:18Z

@bgriffith, I'm researching grammar and use cases, will get back to you shortly

tonyganch · 2016-10-31T14:53:50Z

src/scss/parse.js

+    case TokenType.LeftSquareBracket:
+    case TokenType.RightSquareBracket:
+    case TokenType.LowLine:
+    case TokenType.Tilde:


I believe there should be a different set of valid tokens.
According to css spec, any of the characters from this regexp are valid: [!#$%&*-\[\]-~]. Note that there are two groups, so the full list of valid characters is: !, #, $, %, &, *, +, ,, -, ., /, :, ;, <, =, >, ?, @, [, ], ^, _, {, |, }, plus backtick, digits and letters. Both " and ' are invalid.

tonyganch · 2016-10-31T14:55:53Z

src/scss/parse.js

-    raw = newNode(NodeType.RawType, joinValues(pos, pos + l), token.ln,
-        token.col);
+    if (l = checkIdent(i)) i += l;
+    else if (l = checkNumber(i)) i += l;


I think both ident and number checks should be moved to checkUriRawCharacters, since all those characters are just part of raw and I see not much sense in trying to parse them differently from other 26 token types.

tonyganch · 2016-10-31T15:00:43Z

src/scss/parse.js

+    else if (l = checkNumber(i)) i += l;
+    else if (l = _checkUriRawCharacters(i)) i += l;
+    // As comments are not allowed within the uri token, include as part of node
+    else if (l = checkCommentML(i)) i += l;


This check for comment should be removed and more tokens should be added to checkUriRawCharacters instead: not every comment is valid inside uri, for example, this is invalid code according to css spec: url(a/*(*/) (note ( inside the comment).

tonyganch · 2016-10-31T15:03:15Z

src/scss/parse.js


-    uri.push(raw);
+  return i - start;


You can store position of final token as uri_end, for example, in which case getUriRaw will not need to loop through characters again to find it.

tonyganch · 2016-10-31T15:11:10Z

src/scss/parse.js

+ * Check for a raw URI
+ * (1) `url(http://foo/bar.png)`
+ * (2) `url(http://foo/#{bar}.png)`
+ * (3) `url(#{foo}/bar.png)`


Could you please either update examples or clarify description, that "raw" in that case means "without quotes" and that we only check inner part, without url and parentheses?

tonyganch · 2016-10-31T15:16:50Z

src/scss/parse.js

+  if (l = checkSC(i)) i += l;
+
+  while (i < tokensLength) {
+    if (checkVariable(i) || checkFunction(i)) return 0;


I think this line can be removed. We don't need to check for function because parentheses are invalid raw tokens. Plus we should not break on variables because url($a) is a perfectly valid piece of css code where $a is just part of uri and not a variable.

@tonyganch - So are you ok with parsing the $a in url($a) always as raw and not variable?

For css syntax = definitely :) What will compiler assume? That this is variable in any case?

Thinking out loud - #{$a} would always parse as raw too right?

Yeah guess it HAS to be, incase we end up breaking up a totally legit url into many incorrect nodes.

I checked with both css validator and sassmeister. CSS validator treats url(#{$a}) as a valid url thus parsing it as "raw", and sassmeister interprets it strongly as interpolation.
So I guess the logic for css and scss/sass should be different here.

I'll have to check but I think the current implementation for the CSS parser is problem free.

So on the SCSS/Sass front, where would you stop? Would you parse url($a) as variable?

Yes, for SCSS/Sass those should be variables and interpolations.

So I've removed checkFunction but left checkVariable due to the above.

tonyganch · 2016-10-31T15:24:29Z

src/scss/parse.js

+  // Determine the type of URI
+  if (checkUri1(i)) tokens[start].uriType = 1; // Raw based URI
+  else if (checkUri2(i)) tokens[start].uriType = 2; // Non-raw based URI
+  else return 0;


Current implementation doesn't guarantee that those checkUri checks will go through all characters between parentheses because we don't compare token position with a matching right parenthesis anywhere. In checkUri1 for example we check for i < tokensLength in while loop. I suppose that will parse the following example as valid even though it is not: url(abc(()

tonyganch · 2016-10-31T15:27:55Z

src/scss/parse.js

+  if (l = checkSC(i)) i += l;
+
+  // Check if URI contains a raw node
+  if (!isRaw) return 0;


Do we actually need that? Consider url(#{abc}), even though there is no raw node besides a lonely interpolation, it still can be parsed inside this check without need to call checkUri2

tonyganch · 2016-10-31T15:29:43Z

src/scss/parse.js

+    if (l = checkSC(i)) i += l;
+    else if (l = checkString(i)) i += l;
+    else if (l = checkFunction(i)) i += l;
+    else if (l = checkUnary(i)) i += l;


Unary operator is the one that gives sign to a number, for example, -3 or +8. I think it can't be applied to uris but I guess there isn't anything more suitable in the code now :)

Haha yeah I know, just wasn't one for just a plus sign 😛

tonyganch · 2016-10-31T20:45:29Z

Related issue which should be fixed with this PR: #210

bgriffith · 2016-10-31T23:02:01Z

Awesome feedback @tonyganch - thanks a lot! I'll go through it and implement the changes.

bgriffith · 2016-11-04T16:25:21Z

@tonyganch Updated the PR! Let me know what you think - if it's all good I'll port the changes to Sass syntax

bgriffith · 2016-11-04T16:31:06Z

Few issues have cropped up and I noticed some bugs in my code! Hold yer horses 🐴

bgriffith · 2016-11-06T14:29:51Z

@tonyganch Ok! So I've cleaned up the loops, and fixed a few issues. Give me a shout if there's anything not quite right.

tonyganch · 2016-11-18T08:37:07Z

I'm okay with this PR, can you please port the changes to Sass and merge it?

bgriffith · 2016-11-18T14:47:31Z

@tonyganch Ported and merged! Glad to get that one in! 😁

tonyganch · 2016-11-18T14:50:14Z

Cool, thanks, will publish it tonight or tomorrow.

On 18 Nov 2016, at 15:47, Ben Griffith [email protected] wrote:

Ported and merged 😁

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

tonyganch added the pr status: waiting for tonyganch label Aug 23, 2016

DanPurdy mentioned this pull request Aug 25, 2016

Two issues with @font-face src property. sasstools/sass-lint#839

Closed

tonyganch reviewed Oct 13, 2016

View reviewed changes

tonyganch removed the pr status: waiting for tonyganch label Oct 13, 2016

tonyganch requested changes Oct 31, 2016

View reviewed changes

bgriffith self-assigned this Nov 2, 2016

tonyganch mentioned this pull request Nov 10, 2016

Spaces before and after variable throws validity error #242

Closed

tonyganch approved these changes Nov 18, 2016

View reviewed changes

bgriffith force-pushed the feature/uri branch from 410d1a8 to 157ae64 Compare November 18, 2016 14:35

bgriffith added 2 commits November 18, 2016 14:40

[sass] Improve URI node

c6bfbf6

[scss] Improve URI node

ca34795

bgriffith force-pushed the feature/uri branch from 157ae64 to ca34795 Compare November 18, 2016 14:41

bgriffith merged commit 2c3c49d into tonyganch:dev Nov 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SCSS] Fixes issues with URIs when handling interp and unary #217

[SCSS] Fixes issues with URIs when handling interp and unary #217

bgriffith commented Aug 20, 2016

tonyganch left a comment

tonyganch Oct 13, 2016

bgriffith commented Oct 26, 2016

bgriffith commented Oct 29, 2016

tonyganch commented Oct 31, 2016

tonyganch Oct 31, 2016

tonyganch Oct 31, 2016

tonyganch Oct 31, 2016

tonyganch Oct 31, 2016

tonyganch Oct 31, 2016

tonyganch Oct 31, 2016

bgriffith Nov 3, 2016 •

edited

Loading

tonyganch Nov 3, 2016

bgriffith Nov 3, 2016

bgriffith Nov 3, 2016

tonyganch Nov 3, 2016

bgriffith Nov 3, 2016 •

edited

Loading

tonyganch Nov 3, 2016

bgriffith Nov 4, 2016

tonyganch Oct 31, 2016

tonyganch Oct 31, 2016

tonyganch Oct 31, 2016

bgriffith Nov 4, 2016

tonyganch commented Oct 31, 2016

bgriffith commented Oct 31, 2016

bgriffith commented Nov 4, 2016

bgriffith commented Nov 4, 2016 •

edited

Loading

bgriffith commented Nov 6, 2016

tonyganch commented Nov 18, 2016

bgriffith commented Nov 18, 2016 •

edited

Loading

tonyganch commented Nov 18, 2016

[SCSS] Fixes issues with URIs when handling interp and unary #217

[SCSS] Fixes issues with URIs when handling interp and unary #217

Conversation

bgriffith commented Aug 20, 2016

tonyganch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgriffith commented Oct 26, 2016

bgriffith commented Oct 29, 2016

tonyganch commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgriffith Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgriffith Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyganch commented Oct 31, 2016

bgriffith commented Oct 31, 2016

bgriffith commented Nov 4, 2016

bgriffith commented Nov 4, 2016 • edited Loading

bgriffith commented Nov 6, 2016

tonyganch commented Nov 18, 2016

bgriffith commented Nov 18, 2016 • edited Loading

tonyganch commented Nov 18, 2016

bgriffith Nov 3, 2016 •

edited

Loading

bgriffith Nov 3, 2016 •

edited

Loading

bgriffith commented Nov 4, 2016 •

edited

Loading

bgriffith commented Nov 18, 2016 •

edited

Loading