fix: strip // comments from unknown at-rule preludes - #4544
dchaudhari7177 wants to merge 2 commits into
Conversation
An unknown at-rule's prelude is scanned as raw text by $parseUntil, which
handled /* */ but had no case for //. A line comment inside `@supports
selector(...)` was therefore copied into the CSS, taking the rest of the line
with it -- and any quote inside the comment was picked up as a real string,
mangling the output further.
Skip from // to the end of the line, but only where whitespace precedes it.
That is what separates a comment from a URL: the scanner absorbs comments
while skipping whitespace, which is why `http://host` and a protocol-relative
`url(//host/x.png)` are not comments, and both must keep working.
Gate it on a new stripLineComments flag rather than applying it to every
permissive value. A custom property's value is preserved verbatim, so a //
inside `--this: () => { ... }` is content, not a comment.
Closes less#3527
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: less/less.js/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughUnknown at-rule parsing now strips recognized ChangesUnknown at-rule line comment handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Unknown at-rule line comments are removed while supported URL forms and block comments remain preserved. The change is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| } else if (stripLineComments && | ||
| input.charAt(i + 1) === '/' && | ||
| /\s/.test(input.charAt(i - 1))) { |
There was a problem hiding this comment.
The preceding-whitespace check cannot reliably distinguish comments from URLs. For example, a valid protocol-relative URL with whitespace after url(, such as @supports (background: url( //cdn.example/x)), meets this condition, so the scanner removes the URL and the rest of the line, including its closing delimiters. Conversely, a Less comment directly adjacent to prelude content, such as @supports (display: grid)// comment, fails the condition and remains in the generated CSS. Unknown at-rule preludes are therefore corrupted for both realistic forms.
There was a problem hiding this comment.
Both cases were real, thanks. Fixed in 3c7bf86. The whitespace test is gone. A // is now left alone only inside url(, url-prefix( or domain( (the value parser also stops absorbing comments there), or right after a : (a scheme). url( //cdn.example.com/y.png) keeps its address, and (display: flex)// comment is stripped. Both are in the fixture now, and the previous parser fails them. grunt test:node passes.
Requiring whitespace before `//` kept `url( //host/x.png)` wrong (the URL was stripped) and let `(display: flex)// comment` through. Track whether the scanner is inside `url(`, `url-prefix(` or `domain(`, where the value parser also stops absorbing comments, and treat a `//` after a `:` as a scheme. Every other `//` in an unknown at-rule prelude is a comment.
Closes #3527.
An unknown at-rule's prelude is scanned as raw text by
parserInput.$parseUntil, which has a case for/* */but none for//. Soa line comment inside
@supports selector(...)was copied straight into theCSS:
The
"inside the comment also got picked up by the quote handling as a realstring, which is what mangles the rest of the line.
Telling a comment from a URL
The obvious fix — skip from
//to end of line — breaks URLs, and thatturned out to be a live concern:
So
(cannot be treated as a comment-start context either.So a
//is not a comment in two places, and is one everywhere else in the prelude:url(,url-prefix(,domain(). The value parser turns comment absorption off insideurl()too, so this matches how Less already reads the same text in a declaration. It coversurl(//cdn…)andurl( //cdn…).:, which is a scheme, as in a barehttp://host.(An earlier revision required whitespace before the
//. The review bot pointed out two holes in that:url( //cdn…)lost its address, and(display: flex)// commentleaked. Both are now fixture cases.)Scoped to at-rule preludes
My first attempt applied this to every permissive value, and the existing
permissive-parsefixture caught it:That is a custom property, whose value is preserved verbatim — the
//there is content, not a comment. So the behaviour is gated behind a new
stripLineCommentsflag that onlyatruleUnknownpasses.Verification
New fixture
tests-unit/at-rules-unknown-line-commentscovers the issue'scase, a trailing comment before the block,
http://in both@supportsand@document url-prefix, a protocol-relativeurl(//…)with and without a space after(, a//with no space before it, and a/* */commentthat must survive.
grunt test:node— exit 0, zero failures.One thing I deliberately did not change: the issue's expected output shows
@supports selector(:focus-visible)with the whitespace collapsed. Lesspreserves prelude whitespace for unknown at-rules even when no comment is
involved, so the remaining newline is existing behaviour and not part of this
bug. Output here is
@supports selector(\n :focus-visible ) {.Summary by CodeRabbit
Bug Fixes
http://and protocol-relative//URLs.Tests