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

Support for recently added CSS functionality container-queries #3766

Closed
sneridagh opened this issue Dec 11, 2022 Discussed in #3759 · 17 comments · Fixed by puckowski/less.js#1 or #3811
Closed

Support for recently added CSS functionality container-queries #3766

sneridagh opened this issue Dec 11, 2022 Discussed in #3759 · 17 comments · Fixed by puckowski/less.js#1 or #3811

Comments

@sneridagh
Copy link

As pointed out by @treponat, it seems support for @container queries should be implemented in less.

By tomorrow (12/12/2022) Firefox will release 108 with support for container queries, completing all the major browsers support for the feature.

Discussed in #3759

Originally posted by treponat November 3, 2022
Recently some of the browser started to support a new feature that would allow to create responsive design based on the parent element rather than client's viewport.
https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Container_Queries
It would be wonderful if LESS would also start to fully support this feature.
Currently you can use container functionality in .less files, but the hierarchical structure of css blocks is not working as expected for the container syntax.
for example

.widget.discoverresults, .widget.repositoriesresults {
        container-type: inline-size;
        @container (max-width: 350px) {           
                    .cite {
                        .wdr-authors {
                            display: none;
                         }
                }
         }
}
    

would produce this css blocks

.widget.discoverresults,
.widget.repositoriesresults {
  container-type: inline-size;
}
@container (max-width: 350px) {
  .cite .wdr-authors {
    display: none;
  }
}

instead of

.widget.discoverresults,
.widget.repositoriesresults {
	container-type: inline-size;
}

@container (max-width: 350px) {

	.widget.discoverresults,
	.widget.repositoriesresults {
		.cite .wdr-authors {
			display: none;
		}
	}
}
@iChenLei
Copy link
Member

cc @matthew-dean

@levito
Copy link

levito commented Feb 2, 2023

I found a workaround with saving the selector to a variable using the plugin https://github.com/seven-phases-max/less-plugin-reflections as suggested here: #3053 (comment)

.container(@from, @rules) {
  .container(@from, @to: 0, @rules);
}
.container(@from, @to, @rules) {
  @selector: current-selector();

  .return(@selector) when (@from = 0) {
    @container (max-width: @to) {
      @{selector} {
        @rules();
      }
    }
  }
  .return(@selector) when (@to = 0) {
    @container (min-width: @from) {
      @{selector} {
        @rules();
      }
    }
  }
  .return(@selector) when (default()) {
    @container (min-width: @from) and (max-width: @to) {
      @{selector} {
        @rules();
      }
    }
  }
  .return(@selector);
}

Usage:

.deeply .nested .selector {
  .container(300px, {
    content: "container min width of 300px";
  }
  .container(0, 700px, {
    .going .deeper {
      content: "container max width of 700px";
    }
  }
}

levito added a commit to levito/tt-rss-feedly-theme that referenced this issue Feb 2, 2023
@matthew-dean
Copy link
Member

Oh I see, you're expecting @container to act like @media or @supports? That's reasonable.

@sneridagh
Copy link
Author

@matthew-dean yeah, that's the point :) BTW, I forgot to mention that SCSS has already support for this behavior.

@rejhgadellaa
Copy link

rejhgadellaa commented Feb 16, 2023

Just ran into this issue. Is there any timeframe on how long this could take to land this in a release? I'm trying to decide if I should wait a few weeks or do everything the old fashioned way for now and then refactor in a couple of months or so?

@abhishek-kumar-91
Copy link

I want to work on this issue, please assign me.

@matthew-dean
Copy link
Member

@abhishek-kumar-91 Go for it!

@matthew-dean
Copy link
Member

matthew-dean commented Apr 8, 2023

@abhishek-kumar-91

I presume that what you'll want to do is extend from (or refactor) the Media tree type. Then you'll probably want to alter the media parser to generalize it for @container queries.

This is called within the atrule parser early, for @import, @plugin, and @media, as they have special handling rules, so I presume the line will end up looking like:

value = this['import']() || this.plugin() || this.mediaOrContainer();

And then, of course, writing css / less tests to verify input / output. Make sense?

@levito
Copy link

levito commented Apr 25, 2023

How are things going with this? When working on this, it would be awesome to also support container style queries, which I think should be pretty straightforward since it's syntactically very similar to named container queries.

@tatof
Copy link

tatof commented Jun 20, 2023

Any news on the @container css media queries?

@puckowski
Copy link
Contributor

I have a fork of Less.js 4.1.3 that supports container queries. Passes all tests, plus additional tests for container queries.

This:

.widget.discoverresults, .widget.repositoriesresults {
        container-type: inline-size;

        @container (max-width: 350px) {           
            .cite {
                .wdr-authors {
                    display: none;
                }
            }
        }
}

becomes this:

.widget.discoverresults, 
.widget.repositoriesresults {
    container-type: inline-size;
}

@container (max-width: 350px) {           
    .widget.discoverresults .cite .wdr-authors,
    .widget.repositoriesresults .cite .wdr-authors {
        display: none;
    }
}

I plan on making the fork public sometime later today (07/19/23) and creating a pull request sometime within the next couple of days.

Size of the minified bundle goes up by around 2KB.

@levito
Copy link

levito commented Jul 19, 2023

Awesome, @puckowski, this is really great news!

@tatof
Copy link

tatof commented Jul 19, 2023

yeah great @puckowski 👍

@matthew-dean
Copy link
Member

Nice!

@puckowski
Copy link
Contributor

I've used Less.js before, but I am new to the codebase as of yesterday.

I added support for container queries and tests report: All Passed 215 run
Test Less here: https://github.com/puckowski/less.js/blob/3c26737190b8690c1908bb05fad7eb55ff14806f/packages/test-data/less/_main/container.less

Tomorrow I plan on reviewing the container query specification and adding more tests.

I also need to polish my forked pull request, with an eye for:

  • Easy ability to add support for the scope at-rule in the future
  • As few lines of code added as possible
  • Performance
  • Testability

Ran out of time today, but I hope to get a pull request to the Less.js official repository within the next couple of days.

You can track the progress here:
puckowski#1

@puckowski
Copy link
Contributor

Will review everything one last time tomorrow (07/21/23) and then submit a pull request to this repository.

I created a build of Less.js 4.1.3 with support for CSS Container Queries here:
https://github.com/puckowski/less.js/releases/tag/4.1.3-container-queries-1

If you test the above build and find an issue, let me know.

I think I added enough tests here: https://github.com/puckowski/less.js/pull/1/files#diff-9880faf95a44167ced4666a6eb1c9259f577483ce79944aa8b356e033a9cb239

puckowski#1

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: 11 ms
Total Average Time: 8 ms (12070 KB/s)
+/- 67%

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

----------------------
Render Time
----------------------
Min. Time: 23 ms
Max. Time: 44 ms
Total Average Time: 27 ms (3554 KB/s)
+/- 79%

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: 148,260 bytes
Delta: 1,925 bytes

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

#3811 adds support for CSS Container Queries to Less.js.

I updated the build here: https://github.com/puckowski/less.js/releases/tag/4.1.3-container-queries-2 (there was an issue with the first release in my fork). If you download, test, and find any issues, please let me know.

Will refactor pull 3811 as needed until it gets merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment