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 min and max function calls that hold CSS variables fail with "Operation on an invalid type" #3777

Open
rjgotten opened this issue Feb 8, 2023 · 24 comments

Comments

@rjgotten
Copy link
Contributor

rjgotten commented Feb 8, 2023

To reproduce:

With strict units enabled, perform a calc-like expression inside a min or max function
and use a CSS variable. E.g.

prop : min(100% - var(--some-var), 10px);

Current behavior:
Compiler will throw "Operation on an invalid type" error.

Expected behavior:
Compiler knows the arguments to min and max can be calc-like and doesn't throw, but retains the arguments as-is.

Environment information:

  • less version: 4.1.3
  • nodejs version: 14
  • operating system: any

Having dug around a bit, it appears that the root of the issue is the fact that Operation is visited and flattened before passing to min and max, which means the var() node trips the compiler error as it afaict does not implement an operate method.

This problem can be avoided by setting evalArgs : false for both functions and performing custom lazy evaluation inside the functions themselves, where it can be wrapped in a try-catch and cause the function to go inert (and be treated as the CSS function proper) when incompatible types are present.

@rjgotten rjgotten added the bug label Feb 8, 2023
@shindodkar
Copy link

can i work on this bug?

@matthew-dean
Copy link
Member

@shindodkar I don't see why not!

@debmalya37
Copy link

can I contribute to it ??

@matthew-dean
Copy link
Member

@GitsOfVivek @debmalya37 Anyone can do this at any time. @shindodkar asked about it March 31st, but nothing has been produced yet.

@matthew-dean
Copy link
Member

One way to prevent having to do this several times in several places would be to, rather than creating a try / catch block within each function, would be to create an abstraction that outputs certain functions as-is.

In other words, a number of functions have had try / catch blocks added within the function statement, and outputs the nodes as-is instead of throwing an error if it can't evaluate something, but that gets repetitive to add per function.

A downside of doing this is that if the stylesheet author intended Less to evaluate the statement, and there is an error in the expression, rather than showing an error, it will just output the function.

@matthew-dean
Copy link
Member

@GitsOfVivek I'm not sure what more can be said about it. min / max are Less functions. They are also CSS functions. So, passing a var() into it is invalid if you intended it to be evaluated as a Less function at compile time, but valid if you intended it to be evaluated as a CSS function at runtime.

@VIRAT9358
Copy link

Can I be assigned this @matthew-dean

@VIRAT9358
Copy link

prop: clamp(10px, calc(100% - var(--some-var)), 100%);
Can we use clamp() function has relatively good browser support but may not work in older or less commonly used browsers

@Cyddharth-Gupta
Copy link

Cyddharth-Gupta commented Jul 10, 2023

I have been pretty active on github for the last few days, I'll be quick in fixing this.
@matthew-dean can I take up this issue? There hasn't been any progress since the last 2 assignments

@matthew-dean
Copy link
Member

@Cyddharth-Gupta 👍

@Cyddharth-Gupta
Copy link

thanks @matthew-dean

@Cyddharth-Gupta
Copy link

Cyddharth-Gupta commented Jul 12, 2023

Hey @matthew-dean I tried implementing the abstraction in the src/less/functions/number.js

`function evaluateWithFallback(func, context) {
return function (...args) {
try {
return func.apply(context, args);
} catch (e) {
// Return the original node as-is if evaluation fails
return new Anonymous(func.toCSS(context));
}
};
}

export default {
min: evaluateWithFallback(function (...args) {
return minMax.call(this,false, args);
}, this),
max: evaluateWithFallback(function (...args) {
return minMax.call(this, true, args);
}, this),

convert: function (val, unit) {
return val.convertTo(unit.value);
},
pi: function () {
return new Dimension(Math.PI);
},
mod: function(a, b) {
return new Dimension(a.value % b.value, a.unit);
},
pow: function(x, y) {
if (typeof x === 'number' && typeof y === 'number') {
x = new Dimension(x);
y = new Dimension(y);
} else if (!(x instanceof Dimension) || !(y instanceof Dimension)) {
throw { type: 'Argument', message: 'arguments must be numbers' };
}

    return new Dimension(Math.pow(x.value, y.value), x.unit);
},
percentage: function (n) {
    const result = mathHelper(num => num * 100, '%', n);

    return result;
}

};
`
I have added evaluateWithFallback() and make some changes in the export object in min and max.

the following test cases are failing: min(1, 4ex, 2pt) and max(5m, 3em);

Your expertise in this matter would be highly appreciated.
Thanks

@matthew-dean
Copy link
Member

@Cyddharth-Gupta Can you start a draft PR I can look at?

@matthew-dean
Copy link
Member

the following test cases are failing: min(1, 4ex, 2pt) and max(5m, 3em);

I'm guessing Less tries to eagerly normalize them to the same unit type?

@Cyddharth-Gupta
Copy link

@Cyddharth-Gupta Can you start a draft PR I can look at?

sure, will do that.

@Cyddharth-Gupta
Copy link

@matthew-dean i created a Draft PR a couple of days ago, please have a look at it.

@iChenLei
Copy link
Member

iChenLei commented Jul 15, 2023

@Cyddharth-Gupta CI failed , Before starting the PR review, make sure CI passes at least. Thanks

@Cyddharth-Gupta
Copy link

Cyddharth-Gupta commented Jul 15, 2023

@iChenLei the CI test that are failing are in accordance with the conversation with @matthew-dean , that's why Matthew asked me to start a draft PR, please go through the conversation above.
Thanks

@matthew-dean
Copy link
Member

@Cyddharth-Gupta it says you closed the PR?

@Cyddharth-Gupta
Copy link

Cyddharth-Gupta commented Jul 17, 2023

@matthew-dean my apologies, didn't get a reply for some days since I created the pr, so I thought it might not be an active issue. I will create a new one by tomorrow.

@lubber-de
Copy link

Workaround: use string escaping

x {
   prop: e("min(100% - var(--some-var), 10px)");
}

results in

x {
   prop: min(100% - var(--some-var), 10px);
}

vibhaw1904 added a commit to vibhaw1904/less.js that referenced this issue Aug 25, 2023
@vibhaw1904 vibhaw1904 mentioned this issue Aug 25, 2023
3 tasks
@Cyddharth-Gupta Cyddharth-Gupta removed their assignment Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants