fix(issue:3766) add support for container queries

[puckowski]

What:

This pull request resolves issue 3766 by adding support for CSS Container Queries (https://www.w3.org/TR/css-contain-3/).

Why:

Users of Less.js may wish to use newer features of CSS supported by newer versions of most browsers.

Checklist:

• Documentation
• Added/updated unit tests
• Code complete

Performance:

I have found performance is roughly the same, fluctuating slightly between runs of the benchmark based on whatever the host is doing.

There are a couple of quick checks added for at rules, and if the checks pass a small amount of additional code is run relative to the default branch of execution. In all, performance should be level or only slightly slower when evaluating many at rules.

Pull request branch:

----------------------
Parsing
----------------------
Min. Time: 6 ms
Max. Time: 10 ms
Total Average Time: 8 ms (12369 KB/s)
+/- 44%

----------------------
Evaluation
----------------------
Min. Time: 16 ms
Max. Time: 37 ms
Total Average Time: 19 ms (5057 KB/s)
+/- 108%

----------------------
Render Time
----------------------
Min. Time: 23 ms
Max. Time: 45 ms
Total Average Time: 26 ms (3589 KB/s)
+/- 81%

master branch:

----------------------
Parsing
----------------------
Min. Time: 7 ms
Max. Time: 9 ms
Total Average Time: 8 ms (12309 KB/s)
+/- 33%

----------------------
Evaluation
----------------------
Min. Time: 16 ms
Max. Time: 36 ms
Total Average Time: 19 ms (5026 KB/s)
+/- 102%

----------------------
Render Time
----------------------
Min. Time: 23 ms
Max. Time: 43 ms
Total Average Time: 27 ms (3569 KB/s)
+/- 72%

Tests:

I evaluated the CSS Container Query specification add added appropriate tests, which are passing and have been manually reviewed. All the original tests pass.

- Test Sync _main/import: OK

All Passed 213 run
- Test Sync _main/plugin: OK

All Passed 214 run
- Test Sync math/strict/css: OK

All Passed 215 run

Bundle size:

Less.js 4.1.3 Official minified: 146,335 bytes
Less.js 4.1.3 Pull Request minified: 149,014 bytes
Delta: 2,679 bytes

Lines of code delta: 521 added (including tests)

Behavior of media at-rule with invalid query in parens:

Query in parens syntax comes from CSS Container Query specification, but I do not believe media at rules support them. I searched online and did some tests in a CodePen.

Both master and the pull request branch result in the following when media at rule tries to use query in parens:

ParseError: Missing closing ')'

Extensibility:

The pull request lays the groundwork for adding scope at rules with minimal additional effort.

[puckowski]

You can experiment with a build of Less.js that supports CSS Container Queries here: Edit JavaScript Less.js Container Queries

Alternatively, download a build here: https://github.com/puckowski/less.js/releases/tag/4.1.3-container-queries-2

[puckowski]

I see Nodejs Test for node14 failed due to:

 1) original-less:test-less-calc should match the expected output
 1 failing
  1) less.js main tests
       original-less:test-less-calc should match the expected output:
     Error: Timeout of 2500ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

I am not sure my code changes would impact CSS generation for usages of calc(), but either way I took a closer look.

I have

<link id="original-less:test-less-calc" title="test-less-calc" rel="stylesheet/less" type="text/css" href="../../calc.less">
<link id="expected-less:test-less-calc" rel="stylesheet" type="text/css" href="../../calc.css">

in test-runner-main.html which runner-main-spec.js tests when doing 'less.js main tests' and I see the following output for calc:

✓ original-less:test-less-calc should match the expected output

For what it is worth, I am using:

node v18.15.0
npm 9.5.0
grunt-cli v1.4.3
grunt v1.2.1
Chrome Version 114.0.5735.199 (Official Build) (64-bit)

Additionally,

node .\test\index.js 

produces something like:

- D:\a\less.js\less.js\packages\test-data\less\_main\calc: 

Do any maintainers have access to the CSS differences CI is reporting for calc?

[iChenLei]

@puckowski The timeout issue has always been a defect in our CI system, and it has nothing to do with your PR. I reran the CI, and now all the checks have passed.

[puckowski]

Thank you @iChenLei for your assistance!

[visuallization]

This is great! Thanks for adding this feature. Any ideas when this will be released?

[puckowski]

Any updates on the code review? @matthew-dean @iChenLei

Once this implementation is finalized and merged, I plan on submitting a subsequent PR that extends this PR with support for scope at-rules.

[matthew-dean]

Oh this is great! Does this add support for Media Queries Level 4 or did you only add Level 4 parsing for container queries?

[matthew-dean]

Ope, no, this doesn't add support. Seems like it would be trivial with these changes?

[matthew-dean]

@puckowski Can you add support with this change for Media Queries Level 4? I know it expands the scope a bit, but you're already parsing them for @container

Example:

@media (200px <= width <= 500px) {
    .test-range-syntax {
        padding: 0;
    }
}

[rejhgadellaa]

Hmm I'd rather have container query support land as fast as possible, then add MQ Level 4 support later?

[puckowski]

@matthew-dean @rejhgadellaa
I will experiment with support for MQ Level 4 as part of this pull request sometime today (08/02/2023). Will let you know an estimate of time to delivery.

[matthew-dean]

Thanks! I'll try to review as soon as I can if you can get that. Otherwise tests / everything looks good, and I like how you refactored this.

[puckowski]

I pulled examples from Media Queries Level 4 and tweaked the implementation during lunch break.

Tests passing. New bundle size minified: 151,827 bytes.

Will push to https://github.com/puckowski/less.js later today after I review.

This pull request will add support for CSS Container Queries and Media Queries Level 4.

New supported syntax examples:

@media screen and (color), projection and (color) {
  .selector {
    color: #eee;
  }
}

@media not (width <= -100px) {
  body { 
    background: green; 
  }
}

@media (height > -100px) {
  body { 
    background: green; 
  }
}

@media not (resolution: -300dpi) {
  body { 
    background: green; 
  }
}

@media (example, all,), speech { 
  body { 
    background: green; 
  }
}

@media &test, speech {
  body { 
    background: green; 
  }
}

@media (min-orientation:portrait) {
  body { 
    background: green; 
  }
}

@media print and (min-resolution: 118dpcm) {
  body { 
    background: green; 
  }
}

@media (200px <= width <= 500px) {
  .test-range-syntax {
      padding: 0;
  }
}

.selector {
  color: #eee;

  @media (200px <= width <= 500px) {
    .test-range-syntax {
        padding: 0;
    }
  }
}

[puckowski]

Pushed support for Media Queries Level 4 (https://www.w3.org/TR/mediaqueries-4/).
Added tests for Media Queries Level 4.

Updated bundle size minified: 149,493 bytes.

All tests passing locally.

@matthew-dean

[puckowski]

Tests were passing locally, but I had one bad character in a regex that caused CI to fail.

I pushed the fix.

I believe CI tests should pass now.
@iChenLei @matthew-dean

[matthew-dean]

@iChenLei This morning, this PR said that the CI checks would not run without approval from a maintainer. Was that a change you made, or I made? Either way, do you know how to revert this so that CI checks will run upon changes?

[matthew-dean]

Was this already allowed for Less media queries? This would be against the CSS spec AFAIK.

[puckowski]

Fixed my pull request to be consistent with Less.js version 4.1.3. Thank you for reviewing. See latest comment.

[matthew-dean]

Similar to above, was this already allowed for Less media queries? This isn't a valid identifier.

[puckowski]

Fixed my pull request to be consistent with Less.js version 4.1.3. Thank you for reviewing. See latest comment.

[matthew-dean]

@iChenLei

The timeout issue has always been a defect in our CI system

I'm not sure it always has been. Regardless, is this something we can fix / circumvent?

[matthew-dean]

@puckowski Had some questions about invalid media queries. If Less was already parsing these before, that's fine. But as a general rule, if something isn't valid CSS (like a media query), it shouldn't be valid Less. There are a number of existing cases in the parser (and int tests) where that's not the case, but I'd prefer we didn't add more.

Incidentally, I just tried both in the current Less Preview, and the first one passes and gets re-written, so that's fine. The second doesn't pass, so it should continue to not pass. Specifically, this should fail to parse:

@media &test, speech {
  body { 
    background: green; 
  }
}

(Although the error message it gives: media definitions require block statements after any features is somewhat inaccurate in this case.)

[puckowski]

I apologize, I was overzealous in supporting syntax from examples listed in https://www.w3.org/TR/mediaqueries-4/

The two examples you highlighted were from the error handling section and not valid CSS. Thank you for reviewing.

I updated the pull request.

The following:

@media (example, all,), speech { 
  body { 
    background: green; 
  }
}

gets transformed to:

<style type="text/css" id="less:styles">@media (example, all), speech {
  body {
    background: green;
  }
}
</style>

This syntax:

@media &test, speech {
  body { 
    background: green; 
  }
}

results in the following error:

SyntaxError: media definitions require block statements after any features
in [styles.less](http://192.168.56.1:8000/styles.less) on line 1, column 8:

1@media &test, speech {
2  body {

This is now consistent with Less.js 4.1.3.

I removed the invalid CSS from the tests, and all tests are passing locally.

@matthew-dean

[matthew-dean]

@puckowski Thanks so much!!