Fix #273 and escaped selector sass-spec test #274
Conversation
…e detailed CompilerException
…unescaped in the output, leading to unvalid selectors that the parser itself is refusing as valid. For now we are keeping the escaped sequence in the selector, in a normalize way, to not take the risk to produce an unvalid selector The target and safer should be to have the same way of managing escpaing as in string and keyword: store the real char in a non escaped form in the selector representation, and escape in the output what should be escaped only. But this require a better structure for representation of selectors out of the parser, otherwise this would more or less implie to reparse in the compiler to know what to escape, which would be a shame (ie for instance escape non alpha first chars of identifiers and non allowed chars in identifier, but without escaping attributes content matching)
src/Compiler.php
Outdated
| if ($parser->parseSelector($buffer, $newSelectors, true)) { | ||
| try { | ||
| $isValid = $parser->parseSelector($buffer, $newSelectors, true); | ||
| } catch (\Exception $e) { |
There was a problem hiding this comment.
This should catch only ParserException IMO
| .\31 u\24 { clear: right;} | ||
| .a\31 u\24 { clear: both;} | ||
| $x: '\\28' + 'xlarge' + '\\29'; | ||
| .\31 2u#{$x}, .\31 2u\24#{$x} { width: 100%; clear: none; margin-left: 0; } |
There was a problem hiding this comment.
I suggest that we also contribute upstream to have tests covering that in sass-spec, instead of covering them only in our own testsuite (see #179)
There was a problem hiding this comment.
Done with sass/sass-spec#1600
Let's see how the processus is working.
There was a problem hiding this comment.
@Cerdic any plan to take their review into account ?
…uence in this case, see sass/sass-spec#1600
|
I made the corrections, and I had to correct the patch to be fully compliant on the draft spec I sent as a PR on sass-spec , as dart-sass unescape numbers if not at the first char. Again, the fix is not a definitive fix, and I'm quite sure that pushing a bit on combination of escape sequence and position of the escape sequence you could find other cases where scssphp will keep the escape sequence and not dart-sass. But at least, this is a safe fix as the final selector should always be as valid as the input selector - If I did no mistake. |
|
keeping more escape sequences is OK, if the output is equivalent CSS |
Escaped chars in selectors should not been systematically unescaped in the output, leading to unvalid selectors that the parser itself is refusing as valid.
For now we are keeping the escaped sequence in the selector, in a normalize way, to not take the risk to produce an unvalid selector
The target and safer should be to have the same way of managing escaping as in string and keyword: store the real char in a non escaped form in the selector representation, and escape in the output what should be escaped only.
But this require a better structure for representation of selectors out of the parser, otherwise this would more or less implie to reparse in the compiler to know what to escape, which would be a shame
(ie for instance escape non alpha first chars of identifiers and non allowed chars in identifier, but without escaping attributes content matching)