Skip to content

postcss-logical : use variables #91

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

Conversation

romainmenke
Copy link
Member

@romainmenke romainmenke commented Dec 24, 2021

related issues :

The current implementation of postcss-logical modifies the selector by adding :dir(lrt|rtl).

This changes specificity and doesn't work for @keyframes.
It also breaks down in code that uses multiple dir values in a single document

<html dir="ltr">
 <body>
  <div dir"rtl">
    <p>example of "rtl" text</p>
  </div>
 </body>
</html>

I think that using CSS Variables would resolve most of these issues.
The tradeoff is that we can't have both ltr and rtl in a single document in older browsers that do not support CSS Variables.

https://caniuse.com/css-variables

Browsers that do not support variables would fallback to ltr.

The variable name used in the example below is a shortened md5 hash of the current "tree", combined with a simple incrementing counter.
This gives a unique variable name that is still consistent between multiple builds from the same source.


This change does not effect CSS output when the dir plugin option is provided.


Input CSS :

test-float-3 {
	float: inline-end;
}

@media screen and (min-width: 640px) {
	test-float-3 {
		float: center;
	}
}

Before this change :

test-float-3:dir(ltr) {
	float: right;
}

test-float-3:dir(rtl) {
	float: left;
}

@media screen and (min-width: 640px) {
	test-float-3 {
		float: center;
	}
}

After this change :

:dir(ltr) {
	--logical-b352c2-2: right;
}

:dir(rtl) {
	--logical-b352c2-2: left;
}

test-float-3 {
	float: right;
	float: var(--logical-b352c2-2);
}

@media screen and (min-width: 640px) {
	test-float-3 {
		float: center;
	}
}

Any thoughts on this?

@devongovett
Copy link

I tried to implement this strategy in @parcel/css as well - parcel-bundler/lightningcss#30. I ran into one problem though, which is that it doesn't really work if the value itself also contains variables. It only works if the variables are defined at or above the element with the dir attribute, not if the variable is defined somewhere within.

some-element {
  --radius: 4px;
  border-start-start-radius: var(--radius);
}

produces

[dir="ltr"] {
  --top-left-radius: var(--radius);
  --top-right-radius: ;
}

[dir="rtl"] {
  --top-left-radius: ;
  --top-right-radius: var(--radius);
}

some-element {
  --radius: 4px;
  border-top-left-radius: var(--top-left-radius);
  border-top-right-radius: var(--top-right-radius);
}

That doesn't work unless some-element itself has a dir attribute.

Unfortunately, I haven't found a workaround yet... 😞

@devongovett
Copy link

Ok, found an alternative approach: parcel-bundler/lightningcss#30 (comment)

@romainmenke
Copy link
Member Author

@devongovett Thank you for posting this here!
Your alternative looks very promising and will try to work this out here as soon as possible :)

@romainmenke romainmenke self-assigned this Feb 6, 2022
@devongovett
Copy link

FYI, discovered another problem with this approach.

.foo {
  border-radius: 20px;
}

.foo {
  border-start-start-radius: 0;
}

transforms to:

.foo {
  border-radius: 20px;
}

.foo {
  border-top-left-radius: var(--ltr, 0);
  border-top-right-radius: var(--rtl, 0);
}

[dir="ltr"] {
  --ltr: initial;
  --rtl: ;
}

[dir="rtl"] {
  --ltr: ;
  --rtl: initial;
}

The problem is that, for example in ltr, border-top-right-radius will resolve to " ". That's invalid for this property normally, but at the point variables are substituted, the cascaded value that would normally be a fallback during parsing has been thrown away. It'll just become the default value (0) instead. This is the invalid at computed value time part of the spec. AFAICT there is no way around this.

I'm thinking of changing Parcel CSS to use the :lang selector for this instead, with a list of languages that are normally written right-to-left. It's not 100% right, but I think it's the closest since it works with nested direction changes. Usually, when you use the dir attribute you also set the lang.

Using the dir attribute was also not quite right because direction changes can be made using the direction CSS property as well, and it can also be automatically inferred by the browser based on the content if no dir attribute is set. So there's no way to polyfill it 100% correctly, but I think lang is pretty close for most use cases. Curious if you have experience that suggests otherwise.

@romainmenke
Copy link
Member Author

romainmenke commented Mar 31, 2022

@devongovett Thank you for this! I always appreciate it so much when you come back here and share your learnings!

Using the dir attribute was also not quite right because direction changes can be made using the direction CSS property as well, and it can also be automatically inferred by the browser based on the content if no dir attribute is set. So there's no way to polyfill it 100% correctly, but I think lang is pretty close for most use cases. Curious if you have experience that suggests otherwise.

This is where I gave up my last attempt.
Reading through all the properties that can influence this and that this goes way beyond ltr and rtl.

I hope to pick this up again when I have time to take a deep dive.

I do wonder if maybe it's best to fallback everything to one base direction.
If a true polyfill can't be achieved we can still save authors time. They won't have to manually provide fallback declarations for each logical property.
The final result on the page will also be consistent and readable.

Maybe configurable which fallback is created?

@devongovett
Copy link

Yeah. My current idea is something like this:

.foo:not(:lang(ae, ar, arc, bcc, bqi, ckb, dv, fa, glk, he, ku, mzn, nqo, pnb, ps, sd, ug, ur, yi)) {
  /* ltr */
  border-top-left-radius: 0;
}

.foo:lang(ae, ar, arc, bcc, bqi, ckb, dv, fa, glk, he, ku, mzn, nqo, pnb, ps, sd, ug, ur, yi) {
  /* rtl */
  border-top-right-radius: 0;
}

This way, if no lang is set, the :not selector will apply and you'll get the LTR styles.

@romainmenke
Copy link
Member Author

That looks nice, and compression will make the overhead largely go away as it's always the same.

https://developer.mozilla.org/en-US/docs/Web/CSS/:lang#browser_compatibility

Didn't know that support for :lang was this good 🤔
Also makes sense to follow your approach on this.
Makes it easier for users to switch tooling without painful migrations.

@romainmenke
Copy link
Member Author

romainmenke commented Mar 31, 2022

Side note :

I should send a patch to fix the specificity display in VSCode.
In my last PR I didn't account for :lang() and I don't think it works this way.

Screenshot 2022-03-31 at 17 26 27


done : microsoft/vscode-css-languageservice#268

@devongovett
Copy link

yeah that seems weird. but it got me thinking. Pseudo elements including :lang and :dir raise the specificity of the selector by one element, so that means compiling the following wouldn't work correctly. 😢

.foo {
  border-radius: 20px;
}

.foo {
  border-start-start-radius: 0;
}

.foo {
  border-radius: 50px;
}

It could be possible to fix that using the :where selector to lower the specificity of the :lang to zero, but not sure it has enough browser support yet. Still, :lang might be the best we can do...

@romainmenke
Copy link
Member Author

romainmenke commented Mar 31, 2022

We can raise the specificity of everything else by [0, 1, 0] with :not(.something-unlikely-to-exist).

It's messy but it works.

I initially started this PR to fix specificity issues with [dir="..."] :D

@devongovett
Copy link

Ha, nice hack!

@nnn3d
Copy link

nnn3d commented Oct 11, 2022

Hello, any updates here? I have the same problem with selectors specifity.
Can i help with this PR somehow?

@romainmenke
Copy link
Member Author

Hi @nnn3d,

Unfortunately there isn't much you can do at this time for this issue.
The only way to really fix this plugin is with breaking changes and that means that it will be part of postcss-preset-env version 8 and more specifically this batch of changes : #530

@SalimBensiali
Copy link

Hi all,
I have been following this PR closely, and think I have something working that avoids the issues related to the invalid at computed value time part of the spec. My solution does however require a JS polyfill that leverages the MutationObserver API to add is-{ltr,rtl} classes to every DOM element on the page. Would that be acceptable for this plugin? I see some other postcss plugins do require the use of JS as well - Example: https://github.com/csstools/postcss-plugins/tree/main/plugins/css-blank-pseudo.

@romainmenke
Copy link
Member Author

Hi @SalimBensiali Thank you for suggesting this!
We however try to focus on CSS only transforms and fallbacks as these give developers a bit more flexibility. Only when absolutely needed do we create a JS polyfill.
In this case we feel a CSS only solution will be ok.

@romainmenke
Copy link
Member Author

To everyone following along here, I am closing this pull request because I will not continue working on this issue within the same approach to solve it.

If you want to be notified of updates related to this issue it is best to subscribe to this issue : #90

@romainmenke romainmenke closed this Nov 7, 2022
@romainmenke romainmenke deleted the postcss-logical-variables--persistent-common-buzzard-b4cb687e85 branch November 7, 2022 18:48
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.

4 participants