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] bug when handling var(foo) #5407

Open
wangxd18 opened this issue Nov 8, 2018 · 7 comments
Open

[CSS] bug when handling var(foo) #5407

wangxd18 opened this issue Nov 8, 2018 · 7 comments
Labels
lang:css/scss/less Issues affecting CSS, Less or SCSS

Comments

@wangxd18
Copy link

wangxd18 commented Nov 8, 2018

Prettier 1.15.1
Playground link

--parser css

Input:

.a/root {
  transition: height var(transition-time)s;
}

Output:

.a/root {
  transition: height var(transition-time) s;
}

Note the extra space after var(transition-time).

@lydell lydell added type:bug Issues identifying ugly output, or a defect in the program priority:high Code is printed in a way that alters the AST, breaks syntax, or is a significant regression. Urgent! lang:css/scss/less Issues affecting CSS, Less or SCSS and removed priority:high Code is printed in a way that alters the AST, breaks syntax, or is a significant regression. Urgent! type:bug Issues identifying ugly output, or a defect in the program labels Nov 8, 2018
@vjeux
Copy link
Contributor

vjeux commented Nov 8, 2018

For context, @wangxd18 is a Facebook employee and it was found in the Facebook codebase.

@lydell
Copy link
Member

lydell commented Nov 8, 2018

The problematic thing here is that the example is invalid CSS.

  • It should be var(--transition-time), not var(transition-time) (missing --).
  • Putting a unit after var() is not valid (try it in a browser, it won't work) – it has to be part of the value itself, or added using for example calc(var(--foo) * 1s).

Why are you using invalid CSS?

@lydell lydell added the status:needs discussion Issues needing discussion and a decision to be made before action can be taken label Nov 8, 2018
@lydell
Copy link
Member

lydell commented Nov 8, 2018

See also: #4224 (duplicate?)

@lydell lydell added status:awaiting response Issues that require answers to questions from maintainers before action can be taken and removed status:needs discussion Issues needing discussion and a decision to be made before action can be taken labels Nov 8, 2018
@wangxd18
Copy link
Author

wangxd18 commented Nov 8, 2018

This might be a Facebook specific issue. FB has css preprocesser that will replace these vars before sending to browser.

What's the best way to handle this non-standard issue?

@no-response no-response bot removed the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Nov 8, 2018
@vjeux
Copy link
Contributor

vjeux commented Nov 8, 2018

@lydell I'm curious, since it's not valid CSS, would it make sense to keep it as it is written? This won't break anyone and help out Facebook. It also doesn't seem unreasonable to have it kept that way.

@lydell
Copy link
Member

lydell commented Nov 8, 2018

Yeah, I guess we could try to keep it as written. Just wanted to make sure this is something worth doing something about on Prettier's side first (it could have been a mistake).

(CSS is difficult since we support an arbitrary superset of the standard, which forces us to apply "garbage in, garbage out" instead of throwing parse errors sometimes, and it's not easy to to know what's garbage and what's not.)

@alexander-akait
Copy link
Member

It is hard to fix it, logic around spaces based on prev, current and next types (i.e. function/number with unit/variable and etc), it is allow add spaces for math operators in Less and Sass and in calc in CSS, better avoid invalid CSS, it can lead to such consequences in many places, we can try to close this case, but then we can break the normal syntax

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang:css/scss/less Issues affecting CSS, Less or SCSS
Projects
None yet
Development

No branches or pull requests

4 participants