calc() fix - fixes #974 #3162
calc() fix - fixes #974 #3162
Conversation
lib/less/parser/parser.js
Outdated
| @@ -421,7 +421,11 @@ var Parser = function Parser(context, imports, fileInfo) { | |||
| } | |||
|
|
|||
| parserInput.forget(); | |||
| return new(tree.Call)(name, args, index, fileInfo); | |||
| var call = new(tree.Call)(name, args, index, fileInfo); | |||
| if (name === 'calc') { | |||
There was a problem hiding this comment.
Should not this code (i.e. testing if the func name is calc and setting temporary math-on for args) be paced right into Call.prototype.eval below?
There was a problem hiding this comment.
That's a good point, let me see if I can simplify it.
lib/less/tree/call.js
Outdated
| if (this.mathOff) { | ||
| context.mathOn = true; | ||
| } | ||
| var result, funcCaller = new FunctionCaller(this.name, context, this.getIndex(), this.fileInfo()); | ||
|
|
There was a problem hiding this comment.
Mmm, (sorry I can't really test it - so this is abstract code reading from me), the call should set mathOn=false, evaluate args and then restore prev. mathOn value. I.e. in summary (counting my other coment above), pseudocode:
Call.prototype.eval = function (context) {
...
var prevMath = context.mathOn;
if (funcName === "calc")
context.mathOn = false;
var args = this.args.map(function (a) { return a.eval(context); });
context.mathOn = prevMath;
...
}And the test to ensure:
foo: 1 + 2 calc(3 + 4) 5 + 6; // -> 3 calc(3 + 4) 11;There was a problem hiding this comment.
You're absolutely right.
There was a problem hiding this comment.
Fixed and test added!
- Simplify mathOn switch
|
Okay to merge? |
| @@ -7,6 +7,7 @@ var Node = require("./node"), | |||
| var Call = function (name, args, index, currentFileInfo) { | |||
| this.name = name; | |||
| this.args = args; | |||
| this.mathOn = name === 'calc' ? false : true; | |||
There was a problem hiding this comment.
No, we can't set it to true if the name is not calc - this will break the sm: on mode.
It should be only:
if (name === `calc`)
context.mathOn = false;logic like I suggested earlier. The way it goes in this commit every non-calc function will aways be evaluated with mathon regardless of the main -sm fag.
Or in other words, you can't have precalcuated
this.mathOn = name === 'calc' ? false : true;
because it must be
this.mathOn = name === 'calc' ? false : context.mathOn
but there's no context at this point (and even if it was it may be not equal to the context in the Call.prototype.eval). Thus:
Call.prototype.eval = function (context) {
...
if (this.name === "calc")
context.mathOn = false;
...
}There was a problem hiding this comment.
No, we can't set it to true if the name is not calc - this will break the sm: on mode.
Not so. It still has to pass the strictMath test. Setting to true doesn't override this. Relevant code is:
contexts.Eval.prototype.isMathOn = function () {
if (!this.mathOn) {
return false;
}
return this.strictMath ? (this.parensStack && this.parensStack.length) : true;
};There are still tests to make sure strict math is working. These changes for calc() disable math, but enabling does not override strict math rules.
== Less_Tree_Operation ==
In Less_Tree_Operation::operate(), we call Dimension::operate() with
argument $b (named $other there), which Dimension expects to be a
number with a unit, i.e. something we can perform math on.
Change Operation::operate() to not call this if argument $b is
something else, which avoids the reported PHP warning of $other->unit
being undefined. Noting that the only class with a 'unit' property
is Less_Tree_Dimension.
As a side-effect, this immediately fixes another bug, namely that the
"var()" right-hand side of `calc(100% - var(foo))` was completely
ignored previously when it wasn't a number. For example, it can be
a CSS keyword or CSS function like var() that the browser handles
client-side. Noting that CSS also natively supports math. LESS should
only handle math when the input uses PHP variables or literals, and
thus effectively has a constant outcome.
Previously, Dimention::operate() would implicitly read $other->value
as NULL when $other value wasn't a numerical Dimension object, and
thus the math operation silently failed before this commit, e.g.:
> "100% - var(foo)"
> "100 - NULL" (as %)
> "100%"
With the warning avoided/fixed, we now correctly preserve the
equation in its entirely when $b is e.g. a var() call.
== Less_Tree_Call ==
The above change on its own is insufficient, as that elevates the
warning to a fatal error. The added test case exposes this,
where `calc(100% - var(foo) - var(bar))` is parsed as:
Call (calc)
\-- Expression
\-- Operation (-)
|-- Operation (-_
| |- Dimension (100%)
| |- Call (var foo)
|-- Call (var bar)
The above change, makes the first Operation pair no longer compile
Dimension+Call down to a Dimension, instead preserving Operation as
literal CSS output. This means the second Operation will now see a
left-hand value of Operation, which doesn't have an "operate" method,
and thus correctly triggers the `throw` statement that I cleaned up,
because that's not something we can compute server-side.
Fix this by recognising `calc()` higher up as something that
server-side LESS math should not be performed on in the first place.
This effectively backports less/less.js#3162
(less/less.js@a48c24c4dd3c13e00a20ece803237)
for the similarly reported issue upstream at
less/less.js#974.
That bugfix was originally released with less.js 3.0.0, but the
part of that I'm backporting here doesn't change behaviour of any
of our less.js 2.5.3 compliance tests, thus safe to backport.
We already threw a fatal error when one of the arguments wasn't a
Dimension or Color (no other class has the 'operate' method), so this
only adds non-fatal outcomes to previously fatal outcomes.
Bug: T331688
Change-Id: Idd443a76372682de4edddeb64ab5a65b23bbd3ce
I also flipped strictMath off for the root tests folder since that's the default option so that tests are a little more realistic.