-
Notifications
You must be signed in to change notification settings - Fork 357
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
Parse silent comments in _interpolatedDeclarationValue()
#2266
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -380,7 +380,8 @@ abstract class StylesheetParser extends Parser { | |
// Parse custom properties as declarations no matter what. | ||
var name = nameBuffer.interpolation(scanner.spanFrom(start, beforeColon)); | ||
if (name.initialPlain.startsWith('--')) { | ||
var value = StringExpression(_interpolatedDeclarationValue()); | ||
var value = StringExpression( | ||
_interpolatedDeclarationValue(silentComments: false)); | ||
expectStatementSeparator("custom property"); | ||
return Declaration(name, value, scanner.spanFrom(start)); | ||
} | ||
|
@@ -532,7 +533,8 @@ abstract class StylesheetParser extends Parser { | |
scanner.expectChar($colon); | ||
|
||
if (parseCustomProperties && name.initialPlain.startsWith('--')) { | ||
var value = StringExpression(_interpolatedDeclarationValue()); | ||
var value = StringExpression( | ||
_interpolatedDeclarationValue(silentComments: false)); | ||
expectStatementSeparator("custom property"); | ||
return Declaration(name, value, scanner.spanFrom(start)); | ||
} | ||
|
@@ -1550,7 +1552,7 @@ abstract class StylesheetParser extends Parser { | |
|
||
Interpolation? value; | ||
if (scanner.peekChar() != $exclamation && !atEndOfStatement()) { | ||
value = almostAnyValue(); | ||
value = _interpolatedDeclarationValue(allowOpenBrace: false); | ||
} | ||
|
||
AtRule rule; | ||
|
@@ -1575,7 +1577,7 @@ abstract class StylesheetParser extends Parser { | |
/// This declares a return type of [Statement] so that it can be returned | ||
/// within case statements. | ||
Statement _disallowedAtRule(LineScannerState start) { | ||
almostAnyValue(); | ||
_interpolatedDeclarationValue(allowEmpty: true, allowOpenBrace: false); | ||
error("This at-rule is not allowed here.", scanner.spanFrom(start)); | ||
} | ||
|
||
|
@@ -2748,13 +2750,11 @@ abstract class StylesheetParser extends Parser { | |
/// | ||
/// Differences from [_interpolatedDeclarationValue] include: | ||
/// | ||
/// * This does not balance brackets. | ||
/// * This always stops at curly braces. | ||
/// | ||
/// * This does not interpret backslashes, since the text is expected to be | ||
/// re-parsed. | ||
/// | ||
/// * This supports Sass-style single-line comments. | ||
/// | ||
/// * This does not compress adjacent whitespace characters. | ||
@protected | ||
Interpolation almostAnyValue({bool omitComments = false}) { | ||
|
@@ -2773,11 +2773,21 @@ abstract class StylesheetParser extends Parser { | |
buffer.addInterpolation(interpolatedString().asInterpolation()); | ||
|
||
case $slash: | ||
var commentStart = scanner.position; | ||
if (scanComment()) { | ||
if (!omitComments) buffer.write(scanner.substring(commentStart)); | ||
} else { | ||
buffer.writeCharCode(scanner.readChar()); | ||
switch (scanner.peekChar(1)) { | ||
case $asterisk when !omitComments: | ||
buffer.write(rawText(loudComment)); | ||
|
||
case $asterisk: | ||
loudComment(); | ||
|
||
case $slash when !omitComments: | ||
buffer.write(rawText(silentComment)); | ||
|
||
case $slash: | ||
silentComment(); | ||
|
||
case _: | ||
buffer.writeCharCode(scanner.readChar()); | ||
} | ||
|
||
case $hash when scanner.peekChar(1) == $lbrace: | ||
|
@@ -2794,12 +2804,17 @@ abstract class StylesheetParser extends Parser { | |
|
||
case $u || $U: | ||
var beforeUrl = scanner.state; | ||
if (!scanIdentifier("url")) { | ||
buffer.writeCharCode(scanner.readChar()); | ||
var identifier = this.identifier(); | ||
if (identifier != "url" && | ||
// This isn't actually a standard CSS feature, but it was | ||
// supported by the old `@document` rule so we continue to support | ||
// it for backwards-compatibility. | ||
identifier != "url-prefix") { | ||
buffer.write(identifier); | ||
continue loop; | ||
} | ||
|
||
if (_tryUrlContents(beforeUrl) case var contents?) { | ||
if (_tryUrlContents(beforeUrl, name: identifier) case var contents?) { | ||
buffer.addInterpolation(contents); | ||
} else { | ||
scanner.state = beforeUrl; | ||
|
@@ -2830,11 +2845,19 @@ abstract class StylesheetParser extends Parser { | |
/// | ||
/// If [allowColon] is `false`, this stops at top-level colons. | ||
/// | ||
/// If [allowOpenBrace] is `false`, this stops at top-level colons. | ||
/// | ||
/// If [silentComments] is `true`, this will parse silent comments as | ||
/// comments. Otherwise, it will preserve two adjacent slashes and emit them | ||
/// to CSS. | ||
/// | ||
/// Unlike [declarationValue], this allows interpolation. | ||
Interpolation _interpolatedDeclarationValue( | ||
{bool allowEmpty = false, | ||
bool allowSemicolon = false, | ||
bool allowColon = true}) { | ||
bool allowColon = true, | ||
bool allowOpenBrace = true, | ||
bool silentComments = true}) { | ||
// NOTE: this logic is largely duplicated in Parser.declarationValue. Most | ||
// changes here should be mirrored there. | ||
|
||
|
@@ -2854,7 +2877,22 @@ abstract class StylesheetParser extends Parser { | |
buffer.addInterpolation(interpolatedString().asInterpolation()); | ||
wroteNewline = false; | ||
|
||
case $slash when scanner.peekChar(1) == $asterisk: | ||
case $slash: | ||
switch (scanner.peekChar(1)) { | ||
case $asterisk: | ||
buffer.write(rawText(loudComment)); | ||
wroteNewline = false; | ||
|
||
case $slash when silentComments: | ||
silentComment(); | ||
wroteNewline = false; | ||
|
||
case _: | ||
buffer.writeCharCode(scanner.readChar()); | ||
wroteNewline = false; | ||
} | ||
|
||
case $slash when silentComments && scanner.peekChar(1) == $slash: | ||
buffer.write(rawText(loudComment)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this really parse a loud comment when finding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right that it's incorrect and also that it's dead code. I'll send out a PR to remove it. |
||
wroteNewline = false; | ||
|
||
|
@@ -2882,6 +2920,9 @@ abstract class StylesheetParser extends Parser { | |
scanner.readChar(); | ||
wroteNewline = true; | ||
|
||
case $lbrace when !allowOpenBrace: | ||
break loop; | ||
|
||
case $lparen || $lbrace || $lbracket: | ||
var bracket = scanner.readChar(); | ||
buffer.writeCharCode(bracket); | ||
|
@@ -2907,13 +2948,18 @@ abstract class StylesheetParser extends Parser { | |
|
||
case $u || $U: | ||
var beforeUrl = scanner.state; | ||
if (!scanIdentifier("url")) { | ||
buffer.writeCharCode(scanner.readChar()); | ||
var identifier = this.identifier(); | ||
if (identifier != "url" && | ||
// This isn't actually a standard CSS feature, but it was | ||
// supported by the old `@document` rule so we continue to support | ||
// it for backwards-compatibility. | ||
identifier != "url-prefix") { | ||
buffer.write(identifier); | ||
wroteNewline = false; | ||
continue loop; | ||
} | ||
|
||
if (_tryUrlContents(beforeUrl) case var contents?) { | ||
if (_tryUrlContents(beforeUrl, name: identifier) case var contents?) { | ||
buffer.addInterpolation(contents); | ||
} else { | ||
scanner.state = beforeUrl; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"not including the trailing newline" makes me wonder about what happens when there are consecutive lines with silent comments, but otherwise LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function will only consume one of them, but
whitespace()
will consume them all and the whitespace.