postcss / postcss-calc

PostCSS plugin to reduce calc()
MIT License
215 stars 33 forks source link

Operator precedence is not preserved #115

Closed coreyworrell closed 2 years ago

coreyworrell commented 4 years ago

Source:

:root {
    --fluid-min-width: 320;
    --fluid-max-width: 1140;
    --fluid-screen: 100vw;
    --fluid-bp: calc(
        (var(--fluid-screen) - ((var(--fluid-min-width) / 16) * 1rem)) /
        ((var(--fluid-max-width) / 16) - (var(--fluid-min-width) / 16))
    );
}

7.0.4 outputs:

:root {
    --fluid-min-width: 320;
    --fluid-max-width: 1140;
    --fluid-screen: 100vw;
    --fluid-bp: calc(
        (var(--fluid-screen) - var(--fluid-min-width) / 16 * 1rem) / 
        (var(--fluid-max-width) - var(--fluid-min-width)) / 16
    );
}

The output should instead be:

:root {
    --fluid-min-width: 320;
    --fluid-max-width: 1140;
    --fluid-screen: 100vw;
    --fluid-bp: calc(
        (var(--fluid-screen) - (var(--fluid-min-width) / 16) * 1rem)) / 
        ((var(--fluid-max-width) - var(--fluid-min-width)) / 16
    );
}

postcss-calc is removing the necessary parentheses and affecting the operator precedence.

mischnic commented 4 years ago

After some testing, I came up with this. But I'm not sure if it's even correct and not over engineered:

diff --git src/lib/stringifier.js src/lib/stringifier.js
index 40f92f5..c58f8a9 100644
--- src/lib/stringifier.js
+++ src/lib/stringifier.js
@@ -5,6 +5,30 @@ const order = {
   "-": 1,
 };

+var nonAssociative = {
+  '*': false,
+  '/': true,
+  '+': false,
+  '-': true
+}
+
+function needsParenthesis(op, childOp){
+  let opOrder = order[op];
+  let childOpOrder = order[childOp];
+  if (opOrder === childOpOrder){
+    // Chains of the same operation only need parenthesis if non-associative
+    if (op === childOp){
+      return nonAssociative[op];
+    } else {
+      // Same precedence but different operator: always
+      return true;
+    }
+  } else {
+    // Follow operator precendence
+    return order[op] < order[childOp];
+  }
+}
+
 function round(value, prec) {
   if (prec !== false) {
     const precision = Math.pow(10, prec);
@@ -19,7 +43,7 @@ function stringify(node, prec) {
       const {left, right, operator: op} = node;
       let str = "";

-      if (left.type === 'MathExpression' && order[op] < order[left.operator]) {
+      if (left.type === 'MathExpression' && needsParenthesis(op, left.operator)) {
         str += `(${stringify(left, prec)})`;
       } else {
         str += stringify(left, prec);
@@ -27,7 +51,7 @@ function stringify(node, prec) {

       str += order[op] ? ` ${node.operator} ` : node.operator;

-      if (right.type === 'MathExpression' && order[op] < order[right.operator]) {
+      if (right.type === 'MathExpression' && needsParenthesis(op, right.operator)) {
         str += `(${stringify(right, prec)})`;
       } else {
         str += stringify(right, prec);

This does fix your case but some other existing test cases change. For now, I don't have time to figure out why/if they should change

alexander-akait commented 4 years ago

@mischnic Can you send a PR and we will look what we break, I can help with fixing

philwolstenholme commented 4 years ago

I think this relates to #91 too

mischnic commented 4 years ago

Given that CSS variables are really just text replacements and they don't have implicit parenthesis around them (like variables in a mathematical sense), I think minification of expressions with var() need to be a lot less aggressive.

basher commented 4 years ago

I have same issue with 7.0.4 which comes with parcel@2.0.0-nightly.400.

I only spotted this today on production sites with our responsive iframe containers, where we set an inline HTML style attribute with correct iframe aspect ratio, and pass that to Sass to evaluate padding-top as per this CSS Tricks article:

<div class="responsive-media" style="--aspectRatio: 560/315;">
    <iframe width="560" height="315" ...></iframe>
</div>
.responsive-media {
    padding-top: calc(100% / (var(--aspectRatio)));
}

Output in DEV mode with Parcel start:

padding-top: calc(100% / (var(--aspectRatio)));

Output in PROD mode after with Parcel build:

padding-top: calc(100% / var(--aspectRatio));
dangelion commented 3 years ago

Same problem here. This bug is serious since it completely breaks calculation then the interface. Please someone can take a look to this? Thanks!

basher commented 3 years ago

I've given up waiting for a fix for this.

I'm now using the new standard CSS aspect-ratio property in supported browsers, and a hardcoded padding-top hack without calc() for older browsers.