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

Fix #273 and escaped selector sass-spec test #274

Merged
merged 4 commits into from Nov 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/Compiler.php
Expand Up @@ -15,6 +15,7 @@
use ScssPhp\ScssPhp\Base\Range;
use ScssPhp\ScssPhp\Compiler\Environment;
use ScssPhp\ScssPhp\Exception\CompilerException;
use ScssPhp\ScssPhp\Exception\ParserException;
use ScssPhp\ScssPhp\Exception\SassScriptException;
use ScssPhp\ScssPhp\Formatter\Compressed;
use ScssPhp\ScssPhp\Formatter\Expanded;
Expand Down Expand Up @@ -1807,7 +1808,13 @@ protected function evalSelectors($selectors)
$buffer = $this->collapseSelectors($selectors);
$parser = $this->parserFactory(__METHOD__);

if ($parser->parseSelector($buffer, $newSelectors, true)) {
try {
$isValid = $parser->parseSelector($buffer, $newSelectors, true);
} catch (ParserException $e) {
throw $this->error($e->getMessage());
}

if ($isValid) {
$selectors = array_map([$this, 'evalSelector'], $newSelectors);
}
}
Expand Down
82 changes: 74 additions & 8 deletions src/Parser.php
Expand Up @@ -3030,6 +3030,11 @@ protected function string(&$out, $keepDelimWithInterpolation = false)
return false;
}

/**
* @param string $out
* @param bool $inKeywords
* @return bool
*/
protected function matchEscapeCharacter(&$out, $inKeywords = false)
{
$s = $this->count;
Expand Down Expand Up @@ -3456,6 +3461,55 @@ protected function selector(&$out, $subSelector = false)
return true;
}

/**
* parsing escaped chars in selectors:
* - escaped single chars are kept escaped in the selector but in a normalized form
* (if not in 0-9a-f range as this would be ambigous)
* - other escaped sequences (multibyte chars or 0-9a-f) are kept in their initial escaped form,
* normalized to lowercase
*
* TODO: this is a fallback solution. Ideally escaped chars in selectors should be encoded as the genuine chars,
* and escaping added when printing in the Compiler, where/if it's mandatory
* - but this require a better formal selector representation instead of the array we have now
*
* @param string $out
* @param bool $keepEscapedNumber
* @return bool
*/
protected function matchEscapeCharacterInSelector(&$out, $keepEscapedNumber = false)
{
$s_escape = $this->count;
if ($this->match('\\\\', $m)) {
$out = '\\' . $m[0];
return true;
}

if ($this->matchEscapeCharacter($escapedout, true)) {
if (strlen($escapedout) === 1) {
if (!preg_match(",\w,", $escapedout)) {
$out = '\\' . $escapedout;
return true;
} elseif (! $keepEscapedNumber || ! \is_numeric($escapedout)) {
$out = $escapedout;
return true;
}
}
$escape_sequence = rtrim(substr($this->buffer, $s_escape, $this->count - $s_escape));
if (strlen($escape_sequence) < 6) {
$escape_sequence .= ' ';
}
$out = '\\' . strtolower($escape_sequence);
return true;
}
if ($this->match('\\S', $m)) {
$out = '\\' . $m[0];
return true;
}


return false;
}

/**
* Parse the parts that make up a selector
*
Expand Down Expand Up @@ -3516,9 +3570,14 @@ protected function selectorSingle(&$out, $subSelector = false)
continue 2;
}

if ($char === '\\' && $this->match('\\\\\S', $m)) {
$parts[] = $m[0];
continue;
// handling of escaping in selectors : get the escaped char
if ($char === '\\') {
$this->count++;
if ($this->matchEscapeCharacterInSelector($escaped, true)) {
$parts[] = $escaped;
continue;
}
$this->count--;
}

if ($char === '%') {
Expand Down Expand Up @@ -3661,7 +3720,7 @@ protected function selectorSingle(&$out, $subSelector = false)
continue;
}

if ($this->restrictedKeyword($name)) {
if ($this->restrictedKeyword($name, false, true)) {
$parts[] = $name;
continue;
}
Expand Down Expand Up @@ -3714,10 +3773,11 @@ protected function variable(&$out)
*
* @param string $word
* @param boolean $eatWhitespace
* @param boolean $inSelector
*
* @return boolean
*/
protected function keyword(&$word, $eatWhitespace = null)
protected function keyword(&$word, $eatWhitespace = null, $inSelector = false)
{
$s = $this->count;
$match = $this->match(
Expand All @@ -3744,7 +3804,12 @@ protected function keyword(&$word, $eatWhitespace = null)
$this->count < $send
&& $char === '\\'
&& !$previousEscape
&& $this->matchEscapeCharacter($out, true)
&& (
$inSelector ?
$this->matchEscapeCharacterInSelector($out)
:
$this->matchEscapeCharacter($out, true)
)
) {
$escapedWord[] = $out;
} else {
Expand Down Expand Up @@ -3775,14 +3840,15 @@ protected function keyword(&$word, $eatWhitespace = null)
*
* @param string $word
* @param boolean $eatWhitespace
* @param boolean $inSelector
*
* @return boolean
*/
protected function restrictedKeyword(&$word, $eatWhitespace = null)
protected function restrictedKeyword(&$word, $eatWhitespace = null, $inSelector = false)
{
$s = $this->count;

if ($this->keyword($word, $eatWhitespace) && (\ord($word[0]) > 57 || \ord($word[0]) < 48)) {
if ($this->keyword($word, $eatWhitespace, $inSelector) && (\ord($word[0]) > 57 || \ord($word[0]) < 48)) {
return true;
}

Expand Down
10 changes: 10 additions & 0 deletions tests/inputs/selectors.scss
Expand Up @@ -303,3 +303,13 @@ $end: ' ';
#{$selector} a .tocnumber #{$end} {
min-width: 100%;
}


// escaped
.\31u\$ {clear: left;}
.\31 u\$ {clear: left;}
.\31u\24 { clear: right;}
.\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; }
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done with sass/sass-spec#1600
Let's see how the processus is working.

Copy link
Member

Choose a reason for hiding this comment

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

@Cerdic any plan to take their review into account ?

20 changes: 20 additions & 0 deletions tests/outputs/selectors.css
Expand Up @@ -388,3 +388,23 @@ ul ul, ol ul, ul ol, ol ol {
ul li a .tocnumber {
min-width: 100%;
}
.\31 u\$ {
clear: left;
}
.\31 u\$ {
clear: left;
}
.\31 u\$ {
clear: right;
}
.\31 u\$ {
clear: right;
}
.a1u\$ {
clear: both;
}
.\31 2u\(xlarge\), .\31 2u\$\(xlarge\) {
width: 100%;
clear: none;
margin-left: 0;
}
1 change: 0 additions & 1 deletion tests/specs/sass-spec-exclude.txt
Expand Up @@ -1233,7 +1233,6 @@ non_conformant/extend-tests/237_extend_with_universal_selector_different_namespa
non_conformant/extend-tests/238_unify_root_pseudoelement
non_conformant/extend-tests/compound-unification-in-not
non_conformant/extend-tests/does_not_move_page_block_in_media
non_conformant/extend-tests/escaped_selector
non_conformant/extend-tests/extend-loop
non_conformant/extend-tests/extend-result-of-extend
non_conformant/extend-tests/extend-self
Expand Down