Page MenuHomePhabricator

Introduce commentStore and handle comments in skipWhitespace method
Closed, ResolvedPublic

Description

In 353131 we updated parser to consume/skip comments to prevent syntax errors when parsing directives. Depends on the case, tests expect the Parser to

  • leave comments in the same space (see /*{comment}*/
  • skip comments (see /* Safari */)
  • move comments (see /* and Chrome */)

For example


@-webkit-keyframes /* Safari */ hover /* and Chrome */ {
  0% {
    color: red;
  }
}
.bg {
  background-image: linear-gradient(#333 /*{comment}*/, #111);
}

should become

@-webkit-keyframes hover {
  /* and Chrome */
  0% {
    color: red;
  }
}
.bg {
  background-image: linear-gradient(#333 /*{comment}*/, #111);
}

To make it happen we need to match Less2 behaviour and start storing comments in the commentStore. Versions 2.5.3 and 3.13.1 handle comments in the same matter, therefore with this change we should reach the feature parity with 3.13.1.

The ParserInput.skipWhitespace method skips all white space, and it parses comments. Comments are not skipped/ignored but stored in the commentStore for later use.
This ticket targets fixes for parsing comments.less and comments2.less for both versions (2.5.3 and 3.13.1).

Definition of done:

  • lessjs-2.5.3/overrides/comments.css is removed
  • lessjs-2.5.3/less/comments2.less is passing, fixture removed from KNOWN_ERRORS
  • lessjs-3.13.1/less/comments.css is passing, fixture removed from KNOWN_ERRORS
  • lessjs-3.13/1/less/comments2.less is passing, fixture removed from KNOWN_ERRORS

Failing fixture:

... parse /test/Fixtures/lessjs-2.5.3/less/comments2.less Fail
--- actual
+++ /test/Fixtures/lessjs-2.5.3/css/comments2.css
@@ -1,7 +1,12 @@
-ParseError: unmatched `/*` in comments2.less on line 4, column 53
-2| @-webkit-keyframes hover /* Safari and Chrome */{  }
-3| .bg {
-4|   background-image: linear-gradient(#333 /*{comment}*/, #111);
-5| }
-6| #planadvisor,
-7| /*comment*//*comment*/
+@-webkit-keyframes hover {
+  /* Safari and Chrome */
+}
+.bg {
+  background-image: linear-gradient(#333 /*{comment}*/, #111);
+}
+#planadvisor,
+.first,
+.planning {
+  margin: 10px;
+  total-width: (1 * 6em * 12) + (2em * 12);
+}

Event Timeline

Krinkle renamed this task from Add support for comments between @keyframe and opening curly brace to [comments2] Add support for comments between @keyframe and opening curly brace.Mar 26 2024, 2:30 PM
pmiazga renamed this task from [comments2] Add support for comments between @keyframe and opening curly brace to Introduce ParserInput and handle comments in skipWhitespace method.Apr 12 2024, 9:44 AM
pmiazga updated the task description. (Show Details)

Change #1019269 had a related patch set uploaded (by Pmiazga; author: Pmiazga):

[mediawiki/libs/less.php@master] Proof of concept - ParserInput - Do not merge yet

https://gerrit.wikimedia.org/r/1019269

Change #1020283 had a related patch set uploaded (by Pmiazga; author: Pmiazga):

[mediawiki/libs/less.php@master] Store comments in commentStore

https://gerrit.wikimedia.org/r/1020283

pmiazga renamed this task from Introduce ParserInput and handle comments in skipWhitespace method to Introduce commentStore and handle comments in skipWhitespace method.Apr 19 2024, 12:42 PM
pmiazga updated the task description. (Show Details)
pmiazga changed the task status from Open to In Progress.Apr 23 2024, 2:09 PM

Change #1020283 merged by jenkins-bot:

[mediawiki/libs/less.php@master] Fix ParseError for comments in more places and preserve them in output

https://gerrit.wikimedia.org/r/1020283

Looks I moved this ticket unintentionally, It's solved now. For now we abandoned the idea of introducing the ParserInput class. I'm going to close the ticket, once we start working on versions 3 or 4 we might reconsider bringing back the ParserInput class.

Change #1019269 abandoned by Pmiazga:

[mediawiki/libs/less.php@master] Move input handling logic to separate ParserInput class

Reason:

Not a priority for now. There were plenty of changes in Parser, therefore if we decide to purse this, we will have to split code from scratch again.

https://gerrit.wikimedia.org/r/1019269