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 container queries in editor CSS #49915

Merged
merged 7 commits into from
Aug 10, 2023
Merged

Conversation

tomjn
Copy link
Contributor

@tomjn tomjn commented Apr 19, 2023

What?

Support for container queries! If you use a container query the entire stylesheet breaks. This PR adds container query parsing support to the existing CSS parsing.

Why?

Container queries break stylesheets in the editor ( #49862 and #46277 ).

While #49483 is in progress this PR aims to satisfice a simpler solution until the more difficult and challenging work is completed

How?

It replicates the processing for other @ rules

Testing Instructions

  1. Add an editor stylesheet to the theme
  2. Use a container query in that stylesheet
  3. load the editor
  4. If successful the container query and all other styles in the editor stylesheet work as expected

If the issue has not been fixed, then no styles from the editor stylesheet will be applied, even those not in a container query.

Testing Instructions for Keyboard

This PR doesn't change the UI

@tomjn tomjn marked this pull request as ready for review April 19, 2023 08:32
@tomjn tomjn requested a review from ellatrix as a code owner April 19, 2023 08:32
@tomjn tomjn changed the title Support container queries in parse.js Support container queries when parsing CSS in JS Apr 19, 2023
@tomjn
Copy link
Contributor Author

tomjn commented Apr 19, 2023

I would note that there's a lot of duplication between the various at rule functions, with some work it could be consolidated down to a generic rule that would be more futureproof, but that's beyond the scope of this PR

@kadamwhite
Copy link
Contributor

It'd be valuable to unblock using modern syntax with something like this while the more holistic change from the higher PR is still in-progress -- +1 for the quick fix

@octoxan
Copy link

octoxan commented Jun 5, 2023

Is there any guess on when this can get merged? Trying to use modern CSS is taking down the whole site

@jackmakesthings
Copy link

Any movement on this?

@michaelbourne
Copy link

Would love to see the quick fix in place.

@fabiankaegy fabiankaegy added [Type] Bug An existing feature does not function as intended [Package] Block editor /packages/block-editor labels Jun 19, 2023
Copy link
Member

@fabiankaegy fabiankaegy left a comment

Choose a reason for hiding this comment

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

I don't have enough familiarity with the parser to approve the changes. But in my manual testing, this works as advertised and doesn't cause any new issues I could find with large existing stylesheets.

Thank you for working on this fix while a larger effort for reworking this is underway :)

@ndiego If possible, I think this should get included in WordPress 6.3 because it is a real bummer that container queries, now supported in all modern browsers, don't work in regularly loaded editor styles.

@tomjn
Copy link
Contributor Author

tomjn commented Jun 19, 2023

Noting that @layer support could be done using a very similar and slightly smaller PR, but I'm not sure how using @layer would work with the block editor so didn't want to touch that or expand the scope beyond @container since its semantics and syntax work the same way media queries do

@youknowriad youknowriad requested a review from jsnajdr June 21, 2023 09:45
@tomjn tomjn changed the title Support container queries when parsing CSS in JS Support container queries in editor CSS Aug 8, 2023
@tomjn
Copy link
Contributor Author

tomjn commented Aug 8, 2023

I'm wondering how to gain more traction on this PR, it should be straight forward to test but it's been several months and 6.3 is iminent. If there are changes needed I'm not aware of them, to my knowledge the PR just needs a review and merge

@tomjn
Copy link
Contributor Author

tomjn commented Aug 8, 2023

Rebased on to latest trunk and force pushed branch/commits

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This is working great in my testing. It also might not be the only thing that we don't support in the latest CSS spec. That said, this file is copied as is from https://github.com/reworkcss/css (we were not able to use directly due to an import issue).

That said, it's been some time that we want to move away from this library. There are on going efforts to try several approaches #49521 #49483

In the meantime, I'm personally ok with this change. Maybe we should add a unit test or two to the "transforms" test to at least cover this if/when we replace the library.

@youknowriad
Copy link
Contributor

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Assuming the tests pass, this is looking good to me. Thanks :)

@youknowriad youknowriad enabled auto-merge (squash) August 9, 2023 16:50
auto-merge was automatically disabled August 9, 2023 17:03

Head branch was pushed to by a user without write access

@youknowriad
Copy link
Contributor

Would you mind doing a rebase of the PR, I believe the "bundle size" failure is due to the node upgrade on trunk.

@tomjn
Copy link
Contributor Author

tomjn commented Aug 9, 2023

Ofcourse, the test changes I've made were browser based edits but when I get back to my machine I can do some git time travel and make everything nice

@tomjn
Copy link
Contributor Author

tomjn commented Aug 9, 2023

hmm static analysis wasn't failing before, looks like a package lock issue, but I haven't touched that file, things should have been rebased on to the latest trunk

@Mamaduka
Copy link
Member

SA checks should be fixed now (#53499) but will require another rebase.

@elliottmangham
Copy link

Hey @tomjn, this appears to be an issue for me still (using latest version of WP). Are @container queries supported in custom CSS files loaded in via enqueue_block_editor_assets as well, rather than styles defined in block.json?

We disable block-specific styles, as we have a global CSS file that encompasses everything. This contains an extensive amount of @container queries which do not work. When we replace them with @media they work just fine.

Thank you!

@tomjn
Copy link
Contributor Author

tomjn commented Oct 23, 2024

@elliottmangham I have no idea, the code that parsed the CSS and then rewrote it to add the editor wrapper rules didn't know what container query syntax was, this PR fixed that, but I have no idea what else is involved, or if this code has been replaced etc. I've not touched this or the stuff that triggered it in at least a year

I'd raise a new issue as a bug

@elliottmangham
Copy link

Cheers @tomjn, will raise a new bug.

@jsnajdr
Copy link
Member

jsnajdr commented Oct 24, 2024

@elliottmangham The custom CSS parser that this PR was enhancing no longer exists, we switched to the PostCSS parser a year ago in #49521. So, we'll need to figure out why PostCSS is failing to parse the container queries. Maybe it needs to be upgraded or some plugin has to be added.

@elliottmangham
Copy link

Hey @jsnajdr I think it was due to how I was enqueuing the CSS files. I tried another approach and it appears to be working now, so no issues for me at the moment 😊

@jsnajdr
Copy link
Member

jsnajdr commented Oct 24, 2024

@elliottmangham That's good to hear! In the meantime I found out we have a unit test for parsing container queries, so they should work seamlessly even with the new PostCSS parser.

@elliottmangham
Copy link

Thanks for the insight, that is very good to know! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants