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(issue:3766) add support for container queries #3811

Merged
merged 4 commits into from
Aug 5, 2023

Conversation

puckowski
Copy link
Contributor

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.

* Add support for CSS Container Queries
* Add tests for CSS Container Queries
@puckowski
Copy link
Contributor Author

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
Copy link
Contributor Author

puckowski commented Jul 22, 2023

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
Copy link
Member

@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
Copy link
Contributor Author

Thank you @iChenLei for your assistance!

@visuallization
Copy link

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

@puckowski
Copy link
Contributor Author

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.

margin: 0.5em 0 0 0;
}
}
@container (width >= 500px) and (height >= 500px) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@matthew-dean
Copy link
Member

@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
Copy link

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

@puckowski
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

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;
    }
  }
}

* Add support for Media Queries Level 4.
* Add tests for Media Queries Level 4.
@puckowski
Copy link
Contributor Author

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

* Fix regex used for Media Queries Level 4 syntax parsing.
@puckowski
Copy link
Contributor Author

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
Copy link
Member

@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?

}

@media (example, all,), speech {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@puckowski puckowski Aug 5, 2023

Choose a reason for hiding this comment

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

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

}

@media &test, speech {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@matthew-dean
Copy link
Member

@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
Copy link
Member

@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.)

* Fix parsing of invalid CSS for media queries to be consistent with
  Less.js version 4.1.3 handling.
@puckowski
Copy link
Contributor Author

puckowski commented Aug 5, 2023

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
Copy link
Member

@puckowski Thanks so much!!

@matthew-dean matthew-dean merged commit 012d549 into less:master Aug 5, 2023
@matthew-dean matthew-dean mentioned this pull request Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for recently added CSS functionality container-queries
5 participants