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

Increase strictness about whitespace around minus sign #1912

Open
5 of 7 tasks
akaScooter opened this issue Nov 24, 2015 · 15 comments
Open
5 of 7 tasks

Increase strictness about whitespace around minus sign #1912

akaScooter opened this issue Nov 24, 2015 · 15 comments
Labels
blocked Waiting on another issue to be fixed enhancement New feature or request requires deprecation Blocked on a deprecation cycle

Comments

@akaScooter
Copy link

akaScooter commented Nov 24, 2015

Deprecation

Removal


The following code:

$value : 12px;

.my-class {
    margin: 0 auto -$value;
}

results in the following CSS output:

.my-class {
    margin: 0 auto-12px; }

with no space between the auto and the -12px values. My browser (Firefox v42.0) doesn't recognize auto-12px as a valid value and ignores the rule.

Is this an issue where Sass's rules for compressing the output aren't taking the combination of auto (or any keyword string value) with a numeric value into account?

(The following is an example of the work-around I am using in these instances, but there should really be a space left between those values)

$value : 12px;

.my-class {
    margin: 0 auto;
    margin-bottom: -$value;
}

[Sass 3.4.19 (Selective Steve)]

@HamptonMakes
Copy link
Member

ooof! That's pretty not-fun to have to track down in production code.

Does that happen in compressed mode, or just standard, like you show in your example?

@cimmanon
Copy link

If you add parentheses, it doesn't do that:

$value: 10px;

.my-class {
    margin: 0 auto (-$value);
}

Note that you should be doing this to prevent the expression from being evaluated as subtraction rather than negation.

Without parentheses

$value: 10px;

.my-class {
    margin: 0 -$value;
}

Output:

.my-class {
  margin: -10px;
}

With parentheses

$value: 10px;

.my-class {
    margin: 0 (-$value);
}

Output:

.my-class {
  margin: 0 -10px;
}

@davidkpiano
Copy link

@cimmanon Yes, but that's counter-intuitive - you shouldn't need to add parentheses.

These should all produce the same result:

$value : 12px;
$neg-value: -$value;

.my-class {
  margin: 0 auto -$value;
  margin: 0 auto (-$value);
  margin: 0 auto $neg-value;
  margin: 0 auto -12px;
}

@akaScooter
Copy link
Author

@hcatlin : it happens in standard, just like I showed. :(

@cimmanon : thx! that does work as well. However, I still consider it a bug because Sass is not treating auto as either a value or a variable — the problem may not come from Sass trying to perform an evaluation (as in your example), because no subtraction was performed.

@akaScooter
Copy link
Author

@cimmanon and I was just about to point out what @Darvishzadeh just wrote about the counter-intuitive-ness of it. Syntactically, I don't think you should need the parenthesis.

@darvi-sh
Copy link

@akaScooter, I think you meant @davidkpiano

@cimmanon cimmanon mentioned this issue Nov 26, 2015
@chriseppstein
Copy link

The fact that there is no space between the minus operator and the variable is irrelevant to the Sass parser. whitespace is optional except where the dash would be interpreted as part of the identifier. So Sass sees 0 auto - $value.

Very early in Sass's development we went out of our way to define all binary operators for all types of operands even where the implementation is a bit of a stretch (This is why we have color math. E.g. white / 2 => #808080). For strings, this operator is defined as <string> - <value> => <string>-<value as a string> which you can see in the interactive script parser:

$  scss -i
>> asdf - foo
"asdf-foo"
>> asdf - 2px
"asdf-2px"
>> asdf - #F00
"asdf-#F00"

Seen another way, what if auto was stored in a variable? 0 $side-margin -$bottom-margin. The parsing of this vs. the bare keyword should always be the same (for refactoring purposes). Also, keep in mind that we don't know what is stored in variables or what their names mean, so our parse tree has to always be the same here as when you mean to have subtraction occur. I think a reasonable people could argue that this expression could be interpreted as a two item list where the second item is a difference or a three item list where the last item is negated.

So we were left with the following choices:

  1. forcing you to distinguish negation from subtraction with parenthesis around negation in a space delimited list (this is what we picked)
  2. forcing you to distinguish subtraction from lists containing negation with parenthesis around the subtraction.
  3. Make the no-space version of unary minus distinct from binary minus by disallowing negation with a space in between the minus sign and the operand in all of SassScript and forcing spaces to surround all binary operators (because +, -, *, /, and % should follow the same syntactic rules).

Forcing infix operators to be surrounded by a space is not how any other language works (that I'm aware of) in their expression syntax so the 3rd choice here seemed very wrong early on, but we may have been wrong about that, given the peculiarities of CSS.

This is a common issue that trips people up, but changing the way expressions are parsed is not something we take lightly. The benefit at this point would need to be extremely clear and there would need to be a way to silence all deprecation warnings by adjusting the source which would mean we may need to force unambiguous syntax via parens (around intended subtraction) during the deprecation period -- which would certainly be frustrating for a lot of people.

TL;DR spaces are a really terrible delimiter for values in a data structure because it is a delimiter in expressions. This pattern in CSS has other negative consequences in Sass -- for instance, it is not possible to define a space-delimited list containing a single value syntactically which is one of the reasons all of our list functions have an optional delimiter argument and must treat single values as lists of one element wherever a list is expected.

@chriseppstein chriseppstein changed the title Issue with mixing 'auto' and negative values in CSS property declarations Two elements of a space delimited list are combined when one is negated. Dec 1, 2015
@chriseppstein
Copy link

@nex3 Thoughts on changing the parsing of binary operators to require a surrounding space and forcing unary operands to never allow a space at 4.0? We can have sass-convert take care of the expressions and re-parenthesize them during the upgrade process.

@nex3
Copy link
Contributor

nex3 commented Dec 4, 2015

@chriseppstein That sounds good if we can come up with a close-to-bulletproof strategy for deprecation warnings. Can you work on that?

@chriseppstein
Copy link

@nex3 I can take a swing at it, I'm still learning my way around the nitty gritty aspects of the script parser code.

@chriseppstein chriseppstein self-assigned this Dec 11, 2015
@chriseppstein chriseppstein added the requires deprecation Blocked on a deprecation cycle label Dec 11, 2015
@nex3
Copy link
Contributor

nex3 commented Dec 11, 2015

If you work on a prose description of the deprecation first, that shouldn't require too much parser delving (although probably non-zero). That may also make it easier to code later on, too.

@nex3
Copy link
Contributor

nex3 commented Jun 15, 2022

We have a proposal out for this now. I'll give it a full month for feedback, since it is a breaking change, then mark it as accepted if there are no substantive changes or challenges.

nex3 added a commit to sass/sass-site that referenced this issue Jun 16, 2022
@nex3 nex3 self-assigned this Jun 22, 2022
@nex3 nex3 added the blocked Waiting on another issue to be fixed label Aug 2, 2022
@nex3
Copy link
Contributor

nex3 commented Aug 2, 2022

This is blocked on @jathak having time to implement the migrator plugin for it.

@nex3
Copy link
Contributor

nex3 commented Sep 13, 2022

Plenty of time has passed and the migrator plugin has landed so I'm going to mark this accepted and get to work on the implementation.

@nex3
Copy link
Contributor

nex3 commented Mar 2, 2023

This is now just waiting on Dart Sass 2.0.0

asaf400 pushed a commit to asaf400/ass-site that referenced this issue Apr 18, 2024
asaf400 pushed a commit to asaf400/ass-site that referenced this issue Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Waiting on another issue to be fixed enhancement New feature or request requires deprecation Blocked on a deprecation cycle
Projects
None yet
Development

No branches or pull requests

7 participants