Skip to content

css cascade layers polyfill #244

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

Merged
merged 43 commits into from
May 12, 2022
Merged

Conversation

oluoluoxenfree
Copy link
Contributor

#105

Writing a polyfill for cascade layers using :is() and :not().

@romainmenke

This comment was marked as outdated.

@oluoluoxenfree

This comment was marked as outdated.

@romainmenke

This comment was marked as outdated.

@oluoluoxenfree

This comment was marked as outdated.

@romainmenke

This comment was marked as outdated.

@oluoluoxenfree

This comment was marked as outdated.

@romainmenke

This comment was marked as outdated.

@oluoluoxenfree
Copy link
Contributor Author

@romainmenke thank you!

…-slow-worm-8ac9f7ce8c

cascade layers tests
@romainmenke

This comment was marked as outdated.

@oluoluoxenfree

This comment was marked as outdated.

@romainmenke
Copy link
Member

do you want me to change the branch name on github?

Whatever works best for you :)
You can merge the one branch into the other on your end and then everything will end up here, or rename.


i wanted to sense check my approach with you as it seems different to the way other people are doing it

I had something like this in mind :

  1. replace anonymous layers with named layers. (a random ID is fine, this is just to simplify the plugin as anonymous layers do not have specific behaviour other than being unique, correct me if I am wrong).
  2. walk the entire AST and build a data model.
  3. walk the entire AST again, remove @layer rules and adding :not(<N times "#foo">)

It might not be needed to walk twice, but maybe it's easier to start with two walks so that the analysis and transforms are separated in code?

The main challenge I think is walking the AST and building the correct data model.
Knowing which layer name corresponds with a certain specificity is non-trivial because they can also be nested.

The actual transform is much simpler :

this bit was not tested and is oversimplified, it won't handle nested @layer correctly

const atRuleClone = atRule.clone();
const atRuleClone.nodes.forEach((node) => {
  const modifiedSelectors = node.selectors.map((selector) => {
    return `${selector}:not(<N times #foo>)`;
  });
  atRule.parent.insertBefore(atRule, node.clone({ selectors: modifiedSelectors }));
});

@import should not be handled by this plugin.

@oluoluoxenfree
Copy link
Contributor Author

oluoluoxenfree commented Mar 8, 2022

@romainmenke would you be able to explain more about what walk() is doing, or are there any resources beyond the API guide to work it out? I'm not sure what you mean by a data model here either? Are any other modules doing similar things?

@romainmenke
Copy link
Member

romainmenke commented Mar 8, 2022

Hi @oluoluoxenfree!

@oluoluoxenfree said :

would you be able to explain more about what walk() is doing

@romainmenke said :

walk the entire AST and build a data model.
walk the entire AST again, remove @layer rules and adding :not(<N times "#foo">)

walk in this context refers to root.walkAtRules((atRule) => { ...

This function and callback will go over each @foo statement in the source CSS file.
Allowing you to make edits if they are @layer statements.

In my mind it would be clearest (not most efficient) to have something like this :

const state = ....;

root.walkAtRules((atRule) => {
  // analyse the source CSS and store in "state"
  //
  // you want to have a structure or mechanic that enables this :
  // give it a layer name : "foo" or "foo.bar"
  // get a number back that indicates how much specificity needs to be added
});

root.walkRules((rule)) => {
 // modify unlayered styles.
 //
 // these need to get the highest possible specificity compared to layered styles
 // rule.selector = ...
});

root.walkAtRules((atRule) => {
  // modify the layered CSS based on what is stored in "state"
  //
  // move all CSS out of the "atRule" and remove the now empty "atRule"
  //   https://postcss.org/api/#container-insertbefore
  //
  // modify the selectors so that they have the correct specificity
});

At the moment there is no other plugin doing something like this.


@oluoluoxenfree said :

I'm not sure what you mean by a data model here either

Very simplistic it could be something like this :

const state = {
  "foo": 1,
  "bar": 2
}

Corresponding to this :

@layer foo {
  a {
    order: 1;
  }
}

@layer bar {
  a {
    order: 2;
  }
}

a {
  order: 3;
}
a:not(#foo) { /* 1 -> #foo */
  order: 1;
}

a:not(#foo#foo) { /* 2 -> #foo#foo */
  order: 2;
}

a:not(#foo#foo#foo) { /* unlayered -> 3 -> #foo#foo#foo */
  order: 3;
}

Building this state separately should make it easier to handle nested @layer and other tricky bits.

I hope this makes it a bit clearer :)
Feel free to ask any questions!

@sanajaved7
Copy link
Contributor

Hi @romainmenke and @oluoluoxenfree - I have an approach to handle when layers have both nested layers and unlayered styles and I'd love to get your feedback on it.

Here is a draft PR I just opened and also a forked AST explorer to help visualize my thinking.

In a scenario like this:

@layer A {
	target {
		color: red;
	}

	@layer Z {
		target {
			color: yellow;
		}
	}
} 

The rule inside layer A but outside of layer Z (i.e. target { color: red } } ) should have higher priority since it is outside of the nested layer. Here's a tiny codepen to see this live in case that is helpful.

When we do our first walkthrough and build out the data model, I wanted to ensure that our state reflects this priority correctly when there is layer nesting.

Here is how I am thinking to handle it:

  • In the first walkthrough (root.walkAtRules((atRule)), look within the atRule for any nested layers and rules outside of those nested layers
  • When that condition is met, create a new nested layer. Append this new layer at the end as the final node.
  • Clone any outside rules and place them into the new final layer. Remove original nodes from the parent atRule.

This means that after the first walkthrough our example above would now look like:

@layer A {
	@layer Z {
		target {
			color: yellow;
		}
	}
	@layer A.last {
		target {
			color: red;
		}
	}
}

And since layer A.last is after layer Z, it will have the higher priority. And when we build out the data model, it should resemble something like this:

const state = {
  "A": 1,
  "Z": 2,
  "A.last": 3,
}

❓The issue I am having with this approach is that after the first walk, when I am doing the second walk to build out the data model, I don't see the new layer. The AST explorer and postcss-tape tests show the insertion is working correctly but I don't think the root is showing the new insertions. Any advice on how to tackle this? 🤔

Also, please feel free to share any thoughts on this approach or feedback. I am happy to answer any questions, too 🙏🏼

@romainmenke
Copy link
Member

romainmenke commented Mar 18, 2022

@sanajaved7 Can you try with this code? :

if (hasNestedLayers && hasUnlayeredStyles) {
	const implicitLayer = atRule.clone({ params: `${atRule.params}.implicit` });
	implicitLayer.each((node) => {
		node.remove();
	});

	//create final layer
	atRule.append(implicitLayer);

	// go through the unlayered rules, clone, and delete from top level atRule
	atRule.each((node) => {
		if (node.type == 'rule') {
			implicitLayer.append(node.clone());
			node.remove();
		}
	});
}

Want to give a slightly longer reply later, but hoping this gets you unstuck :)

@sanajaved7
Copy link
Contributor

@romainmenke Awesome, that worked! Thanks for such a quick reply! I'll keep an eye out for your longer reply as well. Thanks again!

Sana Javed and others added 2 commits March 22, 2022 09:15
@sanajaved7
Copy link
Contributor

@romainmenke @oluoluoxenfree I started on handling revert-layer this week but after chatting with Miriam, I think it might make more sense for revert-layer handling to be grouped with the browser transformations for layers that need to happen (like layers in media queries). What Miriam pointed out and that I didn't realize is that as we go through the other layers looking for a selector match, there could be other rules that impact that selector that we won't know or have access to at compile time. Let me know if you have any questions but for now, I'll hold off on adding any code here for revert-layer.

@romainmenke
Copy link
Member

What Miriam pointed out and that I didn't realize is that as we go through the other layers looking for a selector match, there could be other rules that impact that selector that we won't know or have access to at compile time.

@sanajaved7 Do you maybe have an example?

If possible we prefer not to have a client side component to this polyfill as this will cause a flash of incorrectly styled content and make presentation dependant on javascript.

At the moment the only part of the specification that we can not handle is re-ordering of layers in @media / @supports. I think this is an ok trade off.

I am however curious if we can not handle revert-layer in PostCSS.
I haven't had time yet to read up on how it is supposed to work, so I have no idea if it is feasible.

@romainmenke
Copy link
Member

Have read up a bit on revert and revert-layer and I am unsure if we can provide a good enough fallback/polyfill for this :/

My best idea at the moment :

  • assign each property/value pair to a custom property as --<property>-<layer>: <the-value>
  • keep track of the created custom properties for each layer
  • anywhere revert-layer is used we insert var(--<property>-<previous-layer>)

This has many issues :)

  • shorthands won't work well when mixed with longhand
  • logical properties and values won't work well when mixed with non-logical
  • needs more complexity to handle multiple layers
Basic example (click to expand)

<!DOCTYPE html>
<html lang="en">
<head>
	<meta charset="UTF-8">
	<meta http-equiv="X-UA-Compatible" content="IE=edge">
	<meta name="viewport" content="width=device-width, initial-scale=1.0">
	<title>Document</title>

	<style>
		@layer base, special;

		@layer special {
			.item {
				--color-special: red;
				color: red;
			}
			.feature {
				color: var(--color-base);
				color: revert-layer;
			}
		}

		@layer base {
			.item {
				--color-base: blue;
				color: blue;
			}
			.feature {
				--color-base: green;
				color: green;
			}
		}
	</style>
</head>
<body>
	<p>This example contains a list.</p>

	<ul>
		<li class="item feature">Item one</li>
		<li class="item">Item two</li>
		<li class="item">Item three</li>
	</ul>
</body>
</html>


Is this something that can be handled better client side with Javascript?

Although I understand the usefulness of revert-layer in the specification, I do wonder if it is blocking for this plugin?

Does this plugin work well enough without any support for revert-layer?

@mirisuzanne
Copy link

mirisuzanne commented May 6, 2022

Hi all!

Yeah, I think there a few features here that would need to be handled client-side, and revert-layer is one of them. In order to polyfill revert-layer we would basically need to understand exactly how the styles cascade on each individual element. And it's not possible for us to know in a build step what selectors will apply to any given element.

We'll have a similar issue if people want to use layers inside media queries. If you put @layer inside @media then the layer is only added to the layer order if the media query is true (which we can't know in a build step. We might be able to get away with just documenting the workarounds, and telling people not to do it:

  • There's no issue if the layer rule is outside the media rule, and it otherwise means the same thing.
  • There's no issue if the layer order is established up front, so that the media-nested layers wouldn't change the order anyway.

We might just want to throw a warning if people do it anyway.

(I don't think either of these should be blocking - we should just make sure the limitations are well-documented)

@romainmenke
Copy link
Member

Hi @mirisuzanne!

We might just want to throw a warning if people do it anyway.

I think this is the right way to go at the moment.
This way we help people write CSS that will work the same with or without the plugin.

We can adjust later if there is a good client-side polyfill.


I did still have some questions about !important handling as WPT tests seemed inclusive and I don't fully understand how !important behaves in layered styles.

Also opened an issue about this on WPT : web-platform-tests/wpt#33767

Do you have any guidance on this aspect?

if this is resolved I think we can do a last review on this plugin and finalise documentation and examples

@mirisuzanne
Copy link

Do you have any guidance on this aspect?

@romainmenke I can answer the question about proper behavior, but not the question about what other tests to look at. I replied on the WPT thread with the proper behavior (layer ordering is reversed in important layers). I also pinged some of the Chrome devs who wrote those tests, so hopefully they can speak to the other tests that exist.

@sanajaved7
Copy link
Contributor

@romainmenke @oluoluoxenfree I was looking at layer importing and realized there is a comment from earlier that @import shouldn't be handled by the plugin but does that mean that we should use the preexisting postcss import plugin for when layers are imported? Or do we not need to additionally handle layer importing at all?

@oluoluoxenfree
Copy link
Contributor Author

@romainmenke are we good to merge everything in without the wpt tests or are they blocking?

@romainmenke
Copy link
Member

I was looking at layer importing and realized there is a comment from earlier that @import shouldn't be handled by the plugin but does that mean that we should use the preexisting postcss import plugin for when layers are imported? Or do we not need to additionally handle layer importing at all?

You would need to use the postcss import plugin if you want to use @layer with @import.
Maybe this also needs a warning or a mention in the README?

are we good to merge everything in without the wpt tests or are they blocking?

I do not think we need to wait on the wpt tests.
As long as the current handling of !important (in the feature branch) seems good to everyone.

Would be nice to have a few final touches :

  • add contributors from oddbird in package.json in the contributors field
  • add a reference to oddbird in the README

@sanajaved7
Copy link
Contributor

@romainmenke I've got a draft PR where I'm adding a warning and info the README for using the @import at-rule - could you take a look and let me know if I'm on the right track? I also had two questions:

  1. Do we need to check if the postcss-import plugin is being used before throwing the warning? Is that possible?
  2. When the tests run, the import warning is placed after the at-rule in the CSS - is that expected/correct? I see the others have the warning above but I'm not sure why this warning comes after. Any advice?
  3. Does the language in the README make sense?

@romainmenke
Copy link
Member

  1. Do we need to check if the postcss-import plugin is being used before throwing the warning? Is that possible?

This is not needed. In practice people who use postcss-import wont see this notice because any @import will have been removed by that plugin before this plugin runs.

If any @import is visible to this plugin it means that the user isn't using postcss-import.

  1. When the tests run, the import warning is placed after the at-rule in the CSS - is that expected/correct? I see the others have the warning above but I'm not sure why this warning comes after. Any advice?

This is expected but maybe we need to make the sorting of root nodes a bit more strict 🤔.
@charset and @import are shifted up if needed, which in practice should be harmless, but people are very creative with PostCSS.

We can improve this later, after an initial release.

  1. Does the language in the README make sense?

This looks great!

@sanajaved7
Copy link
Contributor

@romainmenke thanks so much! I've updated that test and think the PR should be ready to review whenever you have a chance. Thanks in advance!

Docs changes and warning for @import at-rule
@romainmenke
Copy link
Member

@oluoluoxenfree @sanajaved7 I think the whole feature is now ready to be merged?

@sanajaved7
Copy link
Contributor

@romainmenke yes, I think so! Should we just flip it to ready for review? Are there any other things needed as well?

@romainmenke romainmenke marked this pull request as ready for review May 12, 2022 10:40
@romainmenke
Copy link
Member

@sanajaved7 There seems to be a merge conflict, most likely package-lock.json.
Can you pull in the latest changes from main?

That should be the last step 🎉

@romainmenke
Copy link
Member

@sanajaved7 @oluoluoxenfree and everyone at oddbird thank you for all this!
Has been awesome to work with you all on this 🎉

We will merge this in and perform a few last housekeeping steps before releasing this.
Keeping you posted on the release status!

@romainmenke romainmenke merged commit 74b1c29 into csstools:main May 12, 2022
@Antonio-Laguna
Copy link
Member

Antonio-Laguna commented May 12, 2022

Even if I haven't been personally involved I've been keeping an eye and all I can say is great work you all! It's been awesome to see it take shape and can't wait to get it out.

Kudos! @sanajaved7 @oluoluoxenfree (and oddbird! 🐥) 🎉

@sanajaved7
Copy link
Contributor

Thank you @romainmenke and @Antonio-Laguna, this has been super fun to work on with you and we appreciate all of your help along the way! 🚀 🎉

@romainmenke
Copy link
Member

romainmenke commented May 12, 2022

@sanajaved7 @oluoluoxenfree This has just been released : https://www.npmjs.com/package/@csstools/postcss-cascade-layers by @Antonio-Laguna

We always wait a little bit before adding it to postcss-preset-env but plan to do so next week.

Did you have anything in mind for an announcement for this?
We usually do this on twitter with some examples and docs.
But would be nice to have your info so that we can give you credit while doing so (if this sounds good) :)

I also keep forgetting to mention that we are on Discord : https://discord.gg/bUadyRwkJS
Which can be handy!

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

Successfully merging this pull request may close these issues.

6 participants