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

Whitespace Can Break Checks for Plain CSS #1444

Closed
WillsterJohnson opened this issue Aug 4, 2021 · 2 comments · Fixed by #1483
Closed

Whitespace Can Break Checks for Plain CSS #1444

WillsterJohnson opened this issue Aug 4, 2021 · 2 comments · Fixed by #1483
Assignees
Labels

Comments

@WillsterJohnson
Copy link

sass lang documentation says that;

"If a min() or max() function call is valid plain CSS, it will be compiled to a CSS min() or max() call. “Plain CSS” includes nested calls to calc(), env(), var(), min(), or max(), as well as interpolation. As soon as any part of the call contains a SassScript feature like variables or function calls, though, it’s parsed as a call to Sass’s core min() or max() function instead."

Which (as far as I can tell) means that as long as I write valid CSS when using min or max, including "nested calls to calc(), env(), var(), min(), or max()", there will be no intervention by the sass compiler.

This is true, actually, and works great for things like min(10px, calc(1vmin - 2rem)).
So why am I raising an issue for something that works? Because it only works under a specific condition, when you don't format it in a way that sass deems "correct", which I'll talk more about below.
First, let's see sass fail to detect that I have written valid plain CSS;


My Code

global.scss

$font-size-text-small:       max(  0.75rem, min( calc( 0.625vmin +  0.75rem ), 1.125rem ) );
$font-size-text-medium:      max( 0.875rem, min( calc( 0.625vmin + 0.875rem ),  1.25rem ) );
$font-size-text-large:       max(     1rem, min( calc( 0.625vmin +     1rem ), 1.375rem ) );
$font-size-heading-smallest: max(     1rem, min( calc( 0.625vmin +     1rem ), 1.375rem ) );
$font-size-heading-small:    max( 1.125rem, min( calc(  1.25vmin + 1.125rem ), 1.875rem ) );
// etc

My Compiler

npm i -g sass

Command & Output

.../project $ sass global.scss out.css
Error: min(calc( 0.625vmin + 0.75rem ), 1.125rem) is not a number.
  ╷
1 │ $font-size-text-small:       max(  0.75rem, min( calc( 0.625vmin +  0.75rem ), 1.125rem ) );
  │                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵  

Sanity Check

It works in the browser, and as expected changing the viewport size also scales the font size.
FireFox:
Screenshot from 2021-08-04 23-14-12
Chrome:
Screenshot from 2021-08-04 23-37-32
I don't have any other browsers installed, but my guess would be that, like CSS itself, they don't care about whitespace.


Now to be fair, max(0.75rem, min(calc(0.625vmin + 0.75rem), 1.125rem)) works just fine, sass seems to be interpreting whitespace as a "SassScript feature like variables or function calls". I don't know why it would do that.
Note that it's not any particular whitespace, its everything that doesn't directly follow a comma or is adjacent to a math operator. In that one line, sass would find 9 characters that it believes denote that the expression is SassScript, all 9 of which are whitespace.

So why should anyone care that whitespace breaks sass's ability to distinguish CSS from SassScript?
"@willster277 just dont use whitespace" no, I want to use whitespace, more importantly I want to write code in a visual format that suits me.
The only other places I've seen inline whitespace (not leading) effect the code's behaviour is in a string or a <pre /> tag.
JS, CSS, HTML, Python (again, not leading), anything this side of 1990 doesn't care that you put one space or ten, and a lot of languages the other side of 1990 follow the same attitude to whitespace.
So if I want to put in some whitespace so that my code better suits me and the people I work with, then I should be able to. The guy who introduced me to this visual formatting style has been doing it for over 35 (thirty five) years in around a dozen programming languages in that time, why should he be excluded from sass for using a practice that existed a decade before even CSS?

I'd assume this is a bug that wormed it's way into the release because nobody tested putting whitespace in, which is fair.
But regardless, I'd love to see this fixed, hopefully not too far in the future. Currently our solution is... we dont have one. There is just no option for us which doesn't break our style guide, or puts everything in a string and getting rid of color formatting in our editors.


Thanks for taking the time to read this, apologies for it being so long.
And if you didn't read it, here's the TL:DR;
inserting whitespace in a min(), max(), calc(), and potentially other vanilla CSS functions anywhere that isn't directly after a comma or around a math operator causes an error. I've checked and those whitespaces are valid vanilla CSS, but sass believes them to denote SassScript.

@jathak jathak transferred this issue from sass/sass Aug 18, 2021
@jathak jathak added the bug label Aug 18, 2021
@mirisuzanne
Copy link

mirisuzanne commented Aug 19, 2021

I was able to reproduce this. It seems to be specifically an issue with adding whitespace before the first argument of min() or max(). Remove those spaces, and the rest compiles fine:

.test {
  // the original breaks compilation:
  font-size-text-small:       max(  0.75rem, min( calc( 0.625vmin +  0.75rem ), 1.125rem ) );

  // removing only the whitespace at the start of min() and max():
  font-size-text-small:       max(0.75rem, min(calc( 0.625vmin +  0.75rem ) , 1.125rem ) );
}

@DenisLanz
Copy link

DenisLanz commented Sep 14, 2021

I've a similar issue with calc e.g.

// results in an error
left: calc( var(--sah_gutter) * ( var(--circle-scale) / 2 ) - 2px );
// no error
left: calc( var(--sah_gutter) * (var(--circle-scale) / 2 ) - 2px );

Removing the whitespace fixes the issue. worked without any issues before and now throws Error: Expected number, variable, function, or calculation.

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

Successfully merging a pull request may close this issue.

6 participants