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

CSS container queries "@container" breaks parser #6969

Closed
mitchray opened this issue Nov 30, 2021 · 28 comments · Fixed by #8275
Closed

CSS container queries "@container" breaks parser #6969

mitchray opened this issue Nov 30, 2021 · 28 comments · Fixed by #8275
Labels
compiler Changes relating to the compiler feature request

Comments

@mitchray
Copy link

Describe the bug

With CSS container queries on the way I wanted to flag this early

Currently using @container throws "RightParenthesis is expected"

Reproduction

https://svelte.dev/repl/c660c23146364ec3910534a89363801b?version=3

Logs

[!] (plugin svelte) ParseError: RightParenthesis is expected
src\views\album.svelte
156:     }
157:
158:     @container (min-width: 400px) {
                              ^
159:         .details {
160:             outline: 3px solid lime;
ParseError: RightParenthesis is expected
    at error (C:\git-projects\ample\node_modules\svelte\src\compiler\utils\error.ts:25:16)
    at Parser$1.error (C:\git-projects\ample\node_modules\svelte\src\compiler\parse\index.ts:101:3)
    at Object.read_style [as read] (C:\git-projects\ample\node_modules\svelte\src\compiler\parse\read\style.ts:31:11)
    at tag (C:\git-projects\ample\node_modules\svelte\src\compiler\parse\state\tag.ts:189:27)
    at new Parser$1 (C:\git-projects\ample\node_modules\svelte\src\compiler\parse\index.ts:53:12)
    at parse (C:\git-projects\ample\node_modules\svelte\src\compiler\parse\index.ts:218:17)
    at compile (C:\git-projects\ample\node_modules\svelte\src\compiler\compile\index.ts:93:14)
    at Object.transform (C:\git-projects\ample\node_modules\rollup-plugin-svelte\index.js:111:21)
    at ModuleLoader.addModuleSource (C:\git-projects\ample\node_modules\rollup\dist\shared\rollup.js:19504:30)
    at ModuleLoader.fetchModule (C:\git-projects\ample\node_modules\rollup\dist\shared\rollup.js:19560:9)

System Info

System:
    OS: Windows 10 10.0.19042
    CPU: (8) x64 AMD Ryzen 7 3700X 8-Core Processor
    Memory: 9.25 GB / 16.00 GB
  Binaries:
    Node: 14.13.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.5 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
    npm: 6.14.8 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Spartan (44.19041.1266.0), Chromium (96.0.1054.34)
    Internet Explorer: 11.0.19041.1202
  npmPackages:
    rollup: ^2.52.3 => 2.52.3
    svelte: ^3.42.5 => 3.42.5

Severity

annoyance

@Conduitry Conduitry added compiler Changes relating to the compiler feature request labels Nov 30, 2021
@tanhauhau
Copy link
Member

Pending upstream support csstree/csstree#174

@georgecrawford
Copy link

Out of interest, Svelte is currently using css-tree@v1.1.2, and yet the latest release is v2.1.0. Is there any guidance from the Svelte maintainers about whether support for @container syntax, when added to csstree, would be easy to pull in to Svelte?

Follow-up question - is anyone aware of a hack to get Container Query syntax working in Svelte at the moment? I'd ideally drop in the de-facto polyfill and do something in either a preprocessor or an action to bypass csstree for now. Any thoughts?

@mitchray
Copy link
Author

mitchray commented Apr 3, 2022

My workaround is to have container queries in separate CSS files, and bring them in with svelte:head

https://github.com/mitchray/ample/blob/b37d70e73be00b7fd506fd73274371bdae52bfe4/src/views/album.svelte#L27

@georgecrawford
Copy link

That's an option, for sure. I'd definitely miss the idea of co-locating the CQ with the Svelte file in question though.

I've been playing around with this pattern: https://svelte.dev/repl/1a52f9bfad9c41729338200a5d552ef2?version=3.46.6. It works well, but is jumping through hoops just because the CSS can't be parsed, which is frustrating! But super cool to have CQs working in a component with a simple action.

AaronMaywood added a commit to AaronMaywood/ThreeStyledComponent that referenced this issue Apr 7, 2022
この方法は
sveltejs/svelte#6969 (comment)
による。

仕組み
1. Svelte では@container はSyntax Error となる
2. 外部CSS まではSvelte は関与しないので、そこに@container を書く
    ただし、グローバルCSS での提供となり、クラス名に配慮しなければならない
    また、src/Product.svelte と public/product.css と二元管理になってしまう
3. container-query-polyfill を読み込む
    https://github.com/GoogleChromeLabs/container-query-polyfill

小中大の3サイズが確認できる。
@benmccann
Copy link
Member

Here's a PR to upgrade to the latest css-tree which would probably be the first step: #7572

@rookiepsi
Copy link

rookiepsi commented Oct 1, 2022

We can use container queries (temporarily) in Svelte by creating a .css file and importing it into the <script> tag.

Example

src/routes/+page.svelte

<script>
  import "./page.css";
</script>

<div>
  <h1>Welcome to SvelteKit</h1>
  <p>Visit <a href="https://kit.svelte.dev">kit.svelte.dev</a> to read the documentation</p>
</div>

<style>
  div {
    container-type: inline-size;
  }
</style>

src/routes/page.css

@container (min-width: 500px) {
  div h1 {
    font-size: 5cqi;
  }
}

PS I like to create a folder for each component in src/lib and create .svelte and .css files inside that folder to keep the code more organized.

@typhonrt
Copy link
Contributor

typhonrt commented Nov 27, 2022

I have written a first pass at adding container queries to csstree and have verified that things easily can be enabled in Svelte with a single line modification once support is officially released in csstree. I'm going to work w/ the csstree maintainer and try and get this going ASAP as it's blocking my Svelte UI framework development.

You can see my extensive comment detailing the parser support I've added to my csstree fork here:
csstree/csstree#174 (comment)


I have a temporary release of a fork of Svelte 3.53.1 w/ container query support that you can install as a dependency in package.json as follows: "svelte": "github:typhonjs-svelte/svelte#3.53.1-cq"

Hopefully I'll be able to reach the maintainer of csstree in a timely manner to finish off mainline support, but otherwise I'll publish my Svelte fork on NPM in the coming couple of weeks and keep it updated w/ the mainline Svelte releases until csstree can be updated.

@secretgspot

This comment was marked as off-topic.

@typhonrt
Copy link
Contributor

typhonrt commented Dec 20, 2022

any update on this? Would be nice to utilize @container in sveltekit now that it is production ready

The maintainer of css-tree is working on at-rule / queries updates currently. It is a bigger refactor including query range support for media queries and a few other things I imagine besides just container query support. There is a branch of css-tree / queries where the work is being done w/ last commit ~4 days ago.

I've already offered lending a hand for any testing or anything else that could help in the related css-tree issue, but haven't heard anything, so it's a bit of a waiting game.

I gather the core Svelte maintainers, can't speak for them, will want to wait for the css-tree official mainline update as it should be a simple version bump and minor one line change to support things in svelte.

Container queries are amazing and really help making compound / complex components. I've developed a very advanced color picker w/ my CQ Svelte fork.

@AxelBriche

This comment was marked as spam.

@typhonrt
Copy link
Contributor

Hi @benmccann / @tanhauhau...

I'd like to continue discussion on a solution that I have just implemented for container query (CQ) support that I believe will be palatable to Svelte maintainers. The last correspondence from Roman Dvornov was December 5th in the linked css-tree issue filed. The queries branch where presumably official container query support is being added to css-tree has stalled at December 15th. Chrome has had support since ~August '22 and Firefox 110 is releasing on Feb 14th w/ CQ support. At this time further waiting for css-tree to catch up is entering what I consider not ideal timing.

I made a hard fork of css-tree and implemented CQ support early December and verified that it is trivial to enable support in svelte. It is not of course ideal to depend on my hard fork. However, I just implemented css-tree extension through the mechanism brought up by Roman in this post: csstree/csstree#174 (comment). I was able to take the css-tree node extensions from my hard fork and implement them directly in the svelte repo using the css-tree fork capability. My CQ implementation does full parsing and verification of @container prelude and is 100% working and tested in non-trivial CQ usage in my separate UI framework. I have not added tests yet to svelte, but would like either of you to be involved in a code review.

Changes

src/compiler/parse/read/style.ts: Switch to locally forked css-tree w/ CQ support.

src/compiler/parse/read/css-tree-cq/css_tree_parse.ts: fork css-tree w/ local extension.

src/compiler/parse/read/css-tree-cq/node: css-tree new CQ nodes.

src/compiler/compile/css/Stylesheet.ts: Trivial one line change to enable CQ support in Svelte.

You can view a diff of my changes here:
master...typhonjs-svelte:svelte:master

Impact

No downsides that I'm aware of regarding these changes. Simply that Svelte can now ship 3.56.0 w/ CQ support. I have thoroughly tested the code and would be glad to add specific tests to the svelte repo. I updated to the latest css-tree version 2.3.1. Because I'm using the fork capability of css-tree to extend it the css-tree version can keep being bumped if new versions come out without CQ support. However, when CQ support is added my local extensions can be removed and an easy swap back to the official css-tree parse capability is trivial.

On my side I'm releasing the next version of my UI framework imminently and there is non-trivial CQ usage for a complex color picker component. It would really be great to have a version of Svelte out that supports CQ.

@dummdidumm
Copy link
Member

I think this approach makes sense given that container queries are available for a long time now and there doesn't seem another short term solution in sight. I haven't looked at the code closely though.

@typhonrt
Copy link
Contributor

typhonrt commented Jan 28, 2023

I think this approach makes sense given that container queries are available for a long time now and there doesn't seem another short term solution in sight. I haven't looked at the code closely though.

@dummdidumm it would be great to move forward on a code review of my added code. A good part of it though of course is internal implementation details to css-tree that is probably not familiar to any Svelte reviewer.

I'd like to get an idea of what tests could be added. In an ideal world of course css-tree is tested independently. I haven't spent all too much time evaluating or working with adding tests to svelte. An idea would be to setup a test file / component with all valid permutations of container query usage. Make sure that compiles. Then provide a negative test file / component w/ invalid usage and verify that compilation / css-tree usage produces an error. In the latter error conditions you'd need separate tests per error. Perhaps just providing the positive / all permutation valid test is enough to satisfy testing requirements.

If you or any other Svelte maintainer can respond with a testing goal / requirement I will then add it and make a clean 1-commit PR.


Keeping this section here for reference. Found solution...

Unfortunately, I have run into one more snag with css-tree in the process of verifying a 100% production build of Svelte that fully bundles in all resources . With svelte when not running a production build it doesn't bundle in local node_modules including css-tree marking a few packages as external. When running a production build and bundling in all dependencies the fork capability of css-tree has a code path that uses createRequire attempting to load a JSON file local to css-tree and this causes the production build to not work correctly due to the missing JSON data from the build (IE it is external to the Rollup process). This relates to this Rollup / CommonJS issue: rollup/rollup#4274

I'm spending quite a bit of time now continuing to investigate css-tree issues. It very much looks like the fork capability has not been built with bundling in mind / works fine in a Node environment, but not friendly to Rollup / bundling. There is no particular reason the data loaded from these JSON files needs to be in a local JSON file and easily could be moved to a js file exporting that data. I'll know more in a few hours / over the weekend what may be required to work around or fix this.. The worst case though is that this may be a final snag that will prevent this solution from working without input from Roman / new css-tree version to move that data from JSON to js files inside css-tree. I'm glad I decided to test a production build of Svelte and ran into this now while the code review is pending. It would have been a bummer to achieve consensus and get this in then run into this last snag for a production build. CQ support is so close...


OK... We are back on... The problem described above was solved in css_tree_parse.ts by not importing fork from the css-tree Node package, but importing it from the css-tree browser distribution bundle...

Changing this:
import { fork } from 'css-tree';

to:

import { fork } from '../../../../../node_modules/css-tree/dist/csstree.esm.js';


I have temporarily published to NPM @typhonjs-svelte/svelte for those that would like to try out Svelte w/ CQ support. I have only tested this with direct Svelte builds, so it might not work with SvelteKit.

You can try out CQ support in Svelte by removing svelte from dependencies in package.json

and adding an overrides section like this:

  "overrides": {
    "svelte": "npm:@typhonjs-svelte/svelte@3.55.1-cq"
  },

@mitchray
Copy link
Author

You can try out CQ support in Svelte by removing svelte from dependencies in package.json

and adding an overrides section like this:

  "overrides": {
    "svelte": "npm:@typhonjs-svelte/svelte@3.55.1-cq"
  },

I've tested the override in my Svelte project and can confirm it works!

@finnhvman
Copy link

finnhvman commented Feb 8, 2023

You can try out CQ support in Svelte by removing svelte from dependencies in package.json

and adding an overrides section like this:

  "overrides": {
    "svelte": "npm:@typhonjs-svelte/svelte@3.55.1-cq"
  },

For me the solution was to simply set your release as the svelte version in the devDependencies and I'm using SvelteKit. Here's my full devDependencies for reference:

	"devDependencies": {
		"@sveltejs/adapter-static": "2.0.0",
		"@sveltejs/kit": "1.5.0",
		"@typescript-eslint/eslint-plugin": "5.51.0",
		"@typescript-eslint/parser": "5.51.0",
		"eslint": "8.33.0",
		"eslint-plugin-svelte3": "4.0.0",
		"svelte": "npm:@typhonjs-svelte/svelte@3.55.1-cq",
		"svelte-check": "3.0.3",
		"tslib": "2.5.0",
		"typescript": "4.9.5",
		"vite": "4.1.1"
	},

Anyways, thanks for this special release, it really helps! 🙏

@typhonrt
Copy link
Contributor

typhonrt commented Feb 8, 2023

Awesome... Re: two confirmations above. I'm working on a PR later today to hopefully get this mainlined! :D

@typhonrt
Copy link
Contributor

As indicated above the PR is in and waiting on review + merge. #8275.

ryanfiller added a commit to ryanfiller/portfolio-svelte that referenced this issue Feb 24, 2023
ryanfiller added a commit to ryanfiller/portfolio-svelte that referenced this issue Feb 24, 2023
ryanfiller added a commit to ryanfiller/portfolio-svelte that referenced this issue Mar 1, 2023
@typhonrt
Copy link
Contributor

typhonrt commented Mar 3, 2023

An update on the PR. Given the follow on discussion in the PR I have added single value CSS function support to the associated PR supporting the usage of calc, clamp, min, max in container queries. This allows you to write query rules like:

@container test-container (calc(400px + 1px) <= width < calc(500px + 1px)) {}

As described above you can test out this functionality now as I have a temporary fork of Svelte w/ this PR available with this NPM package. You can load it in a Svelte project with the following in package.json:

"overrides": {
   "svelte": "npm:@typhonjs-svelte/svelte@3.55.1-cq2"
},

Waiting on Svelte maintainers for review / merge.

ryanfiller added a commit to ryanfiller/portfolio-svelte that referenced this issue Mar 4, 2023
ryanfiller added a commit to ryanfiller/portfolio-svelte that referenced this issue Mar 4, 2023
ryanfiller added a commit to ryanfiller/portfolio-svelte that referenced this issue Mar 4, 2023
ryanfiller added a commit to ryanfiller/portfolio-svelte that referenced this issue Mar 4, 2023
ryanfiller added a commit to ryanfiller/portfolio-svelte that referenced this issue Mar 7, 2023
@typhonrt
Copy link
Contributor

typhonrt commented Mar 14, 2023

Yet another minor update.. The PR is up to date w/ mainline Svelte as of this message. A new external version of Svelte 3.56.0 now 3.57.0 w/ the container query PR applied is available here with this NPM Package. You can load it in a Svelte project with the following in package.json:

"overrides": {
   "svelte": "npm:@typhonjs-svelte/svelte@3.57.0-cq"
},

Still waiting on Svelte maintainers to take a look.

@Jothsa
Copy link

Jothsa commented Mar 23, 2023

Really hope this gets implemented into mainline Svelte soon…

dummdidumm pushed a commit that referenced this issue Mar 27, 2023
Closes #6969

As discussed there, container query support is quite useful to add to Svelte as it is now broadly available with Firefox releasing support imminently w/ FF v110 this upcoming week (~Feb 14th). Chrome has had support since ~Aug '22. The central issue is that css-tree which is a dependency for CSS AST parsing is significantly lagging behind on adding more recent features such as container query support. Ample time has been given to the maintainer to update css-tree and I do have every confidence that in time css-tree will receive a new major version with all sorts of modern CSS syntax supported including container queries. This PR provides an interim solution for what Svelte needs to support container queries now.
@mitchray
Copy link
Author

Huge thanks to @typhonrt for making the dream a reality

@rob-balfre
Copy link
Contributor

Agreed! @typhonrt thanks so much 🙏

@finnhvman
Copy link

Yes, thank you @typhonrt for your efforts and persistence! 👏

@typhonrt
Copy link
Contributor

typhonrt commented Mar 29, 2023

Thanks @mitchray, @rob-balfre, and @finnhvman for the nod! I'm certainly excited to now ship some advanced CQ components. IMHO it is a massive game changer for responsive component design.

Even better, now that the CQ PR is merged and I can rely on the Svelte maintainers being OK w/ how the solution is implemented I was able to refactor the css-tree extension to add single value function support and range syntax support to media queries reusing the code between CQ / MQ bringing the media query support up to date. This is a follow up PR that brings home modern query syntax to Svelte. Hopefully that PR is merged as well.

The MQ PR is here: #8430
The associated MQ issue that it closes is here: #5876

@Conduitry
Copy link
Member

Thank you! This has been released in 3.58.0.

ryanfiller added a commit to ryanfiller/portfolio-svelte that referenced this issue May 9, 2023
ryanfiller added a commit to ryanfiller/portfolio-svelte that referenced this issue May 9, 2023
ryanfiller added a commit to ryanfiller/portfolio-svelte that referenced this issue May 11, 2023
ryanfiller added a commit to ryanfiller/portfolio-svelte that referenced this issue May 11, 2023
@akbar-idwise
Copy link

Amazing ❤️❤️❤️

@z-x
Copy link

z-x commented Dec 15, 2023

Hm, Svelte 4.2.8 and it seems that using @container (){} breaks all the following CSSes. Is this a regression or did I do something weird with my code?

EDIT: I've been mistaken obviously and the reason was me not fully understanding what setting an element as container does to the layout. Sorry!

@typhonrt
Copy link
Contributor

Hm, Svelte 4.2.8 and it seems that using @container (){} breaks all the following CSSes. Is this a regression or did I do something weird with my code?

If you can work up an example in the Svelte REPL that is helpful. Though I don't believe @container (){} is valid as there is no condition; at least from reading the specification.

It could be a situation where the custom parsing for @container can generate a better error or warning instead of possibly exhibiting the symptoms described.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler Changes relating to the compiler feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.