Skip to content

CSS scroll state container queries #4311

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

puckowski
Copy link
Contributor

What:

Add Less.js support for container queries to style descendants of containers based on their scroll state.

Why:

Container query scroll state support is available in Chrome 133 (https://developer.chrome.com/blog/chrome-133-beta). Getting ahead of broader support. Without this PR the Less.js output of scroll-state is incorrect.

Checklist:

  • Documentation
  • Added/updated unit tests
  • Code complete

* Add support for CSS container query scroll-state.
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jan 17, 2025
@iChenLei iChenLei requested a review from matthew-dean January 19, 2025 12:30
@matthew-dean
Copy link
Member

@puckowski What's the state of this? Where you going to refactor it?

* Fixes issue with scroll-state function not being detected by regex.
@puckowski
Copy link
Contributor Author

Thank you for the ping.

Pulled latest master to avoid CI issues and applied fixes (no more space between scroll-state and parenthesis (). I believe this is in a good position to merge now.

@matthew-dean

@matthew-dean
Copy link
Member

matthew-dean commented Apr 12, 2025

@puckowski I ultimately still don't agree with the code present here. I don't understand why scroll-state needs to be specially whitelisted. It's just a function, and at-rules can have functions in their prelude. Why do we need the extra code here?

Meaning, if Less was sticking an extra space after parens or something, then the at-rule was getting incorrectly parsed, but that has nothing to do with needing to add this very specific function name as a whitelisted case. That's adding net code to circumvent a bug (in one very very narrow case) instead of fixing the bug. What happens if it was any other function after @container? If other functions do not have an extra space, then it's a parsing bug. If all functions after @container have an extra space, then it's a parsing bug. So either way it's a parsing bug.

If the problem is that functions aren't generally accepted in an at-rule, then that's a parsing bug. If the problem is that what's accepted after @container is much narrower than other at-rules, then that's a parsing bug. Any way you slice it would be a parsing bug IMO.

import Variable from '../tree/variable';
import Anonymous from '../tree/variable';

const scrollStateExpression = function (args) {
Copy link
Member

Choose a reason for hiding this comment

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

There have been a few PRs like this (I forget from whom) that to me, seem far too specific and as a result, are probably a bit bloated. The syntax of container queries should be mostly generic and aligned with media queries. That is, they should accept any property or custom property declaration. Scroll state is not special and should not be treated specially. Otherwise there would end up being n files like this related to every CSS property. Less is not a linter, and does not check to see if properties are valid, only that syntax is valid.

Copy link
Member

Choose a reason for hiding this comment

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

Further to that, there should, ideally, be only one AtRule type for @container, @media, and @supports (and maybe more). Something that acts as a generic with roughly the same structure that has the same bubbling behavior. They wouldn't necessarily be parsed the same, but they would be stored the same (with the individual options specifying which actual at-rule it represents).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants