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.

Sign up for GitHub

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

Jump to bottom

calc() fix - fixes #974 #3162

Merged
merged 2 commits into from Feb 11, 2018
Merged

calc() fix - fixes #974 #3162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f3c7f70
calc() fix - fixes #974
matthew-dean Feb 10, 2018
e2242b9
Add support for nested functions in calc()
matthew-dean Feb 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 18 additions & 5 deletions dist/less.js
View file
Open in desktop
Expand Up @@ -1003,7 +1003,11 @@ contexts.Eval.prototype.outOfParenthesis = function () {
this.parensStack.pop();
};

contexts.Eval.prototype.mathOn = true;
contexts.Eval.prototype.isMathOn = function () {
if (!this.mathOn) {
return false;
}
return this.strictMath ? (this.parensStack && this.parensStack.length) : true;
};

Expand Down Expand Up @@ -4019,6 +4023,7 @@ var Parser = function Parser(context, imports, fileInfo) {
}

parserInput.forget();

return new(tree.Call)(name, args, index, fileInfo);
},

Expand Down Expand Up @@ -4194,7 +4199,7 @@ var Parser = function Parser(context, imports, fileInfo) {
}
},

// A property entity useing the protective {} e.g. @{prop}
// A property entity useing the protective {} e.g. ${prop}
propertyCurly: function () {
var curly, index = parserInput.i;

Expand Down Expand Up @@ -6331,6 +6336,7 @@ var Node = require("./node"),
var Call = function (name, args, index, currentFileInfo) {
this.name = name;
this.args = args;
this.mathOn = name === 'calc' ? false : true;
this._index = index;
this._fileInfo = currentFileInfo;
};
Expand All @@ -6353,8 +6359,16 @@ Call.prototype.accept = function (visitor) {
// The function should receive the value, not the variable.
//
Call.prototype.eval = function (context) {
var args = this.args.map(function (a) { return a.eval(context); }),
result, funcCaller = new FunctionCaller(this.name, context, this.getIndex(), this.fileInfo());

/**
* Turn off math for calc(), and switch back on for evaluating nested functions
*/
var currentMathContext = context.mathOn;
context.mathOn = this.mathOn;
var args = this.args.map(function (a) { return a.eval(context); });
context.mathOn = currentMathContext;

var result, funcCaller = new FunctionCaller(this.name, context, this.getIndex(), this.fileInfo());

if (funcCaller.isValid()) {
try {
Expand Down Expand Up @@ -8384,7 +8398,6 @@ module.exports = Property;

},{"./declaration":58,"./node":73}],77:[function(require,module,exports){
var Node = require("./node"),
JsEvalNode = require("./js-eval-node"),
Variable = require("./variable"),
Property = require("./property");

Expand Down Expand Up @@ -8441,7 +8454,7 @@ Quoted.prototype.compare = function (other) {
};
module.exports = Quoted;

},{"./js-eval-node":67,"./node":73,"./property":76,"./variable":85}],78:[function(require,module,exports){
},{"./node":73,"./property":76,"./variable":85}],78:[function(require,module,exports){
var Node = require("./node"),
Declaration = require("./declaration"),
Keyword = require("./keyword"),
Expand Down
10 changes: 5 additions & 5 deletions dist/less.min.js
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions lib/less/contexts.js
View file
Open in desktop
Expand Up @@ -73,7 +73,11 @@ contexts.Eval.prototype.outOfParenthesis = function () {
this.parensStack.pop();
};

contexts.Eval.prototype.mathOn = true;
contexts.Eval.prototype.isMathOn = function () {
if (!this.mathOn) {
return false;
}
return this.strictMath ? (this.parensStack && this.parensStack.length) : true;
};

Expand Down
3 changes: 2 additions & 1 deletion lib/less/parser/parser.js
View file
Open in desktop
Expand Up @@ -421,6 +421,7 @@ var Parser = function Parser(context, imports, fileInfo) {
}

parserInput.forget();

return new(tree.Call)(name, args, index, fileInfo);
},

Expand Down Expand Up @@ -596,7 +597,7 @@ var Parser = function Parser(context, imports, fileInfo) {
}
},

// A property entity useing the protective {} e.g. @{prop}
// A property entity useing the protective {} e.g. ${prop}
propertyCurly: function () {
var curly, index = parserInput.i;

Expand Down
13 changes: 11 additions & 2 deletions lib/less/tree/call.js
View file
Open in desktop
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    ...
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this._index = index;
this._fileInfo = currentFileInfo;
};
Expand All @@ -29,8 +30,16 @@ Call.prototype.accept = function (visitor) {
// The function should receive the value, not the variable.
//
Call.prototype.eval = function (context) {
var args = this.args.map(function (a) { return a.eval(context); }),
result, funcCaller = new FunctionCaller(this.name, context, this.getIndex(), this.fileInfo());

/**
* Turn off math for calc(), and switch back on for evaluating nested functions
*/
var currentMathContext = context.mathOn;
context.mathOn = this.mathOn;
var args = this.args.map(function (a) { return a.eval(context); });
context.mathOn = currentMathContext;

var result, funcCaller = new FunctionCaller(this.name, context, this.getIndex(), this.fileInfo());

if (funcCaller.isValid()) {
try {
Expand Down
12 changes: 0 additions & 12 deletions lib/less/tree/directive.js
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion lib/less/tree/quoted.js
View file
Open in desktop
@@ -1,5 +1,4 @@
var Node = require("./node"),
JsEvalNode = require("./js-eval-node"),
Variable = require("./variable"),
Property = require("./property");

Expand Down
12 changes: 0 additions & 12 deletions lib/less/tree/rule.js
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion test/browser/runner-browser-options.js
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@ var less = {
logLevel: 4,
errorReporting: "console",
javascriptEnabled: true,
strictMath: true
strictMath: false
};

// There originally run inside describe method. However, since they have not
Expand Down
1 change: 0 additions & 1 deletion test/browser/runner-main-options.js
View file
Open in desktop
Expand Up @@ -2,7 +2,6 @@ var less = {
logLevel: 4,
errorReporting: "console"
};
less.strictMath = true;
less.functions = {
add: function(a, b) {
return new(less.tree.Dimension)(a.value + b.value);
Expand Down
5 changes: 5 additions & 0 deletions test/css/calc.css
View file
Open in desktop
@@ -0,0 +1,5 @@
.no-math {
width: calc(50% + (25vh - 20px));
foo: 3 calc(3 + 4) 11;
bar: calc(1 + 20%);
}
2 changes: 1 addition & 1 deletion test/css/comments2.css
View file
Open in desktop
Expand Up @@ -8,7 +8,7 @@
.first,
.planning {
margin: 10px;
total-width: (1 * 6em * 12) + (2em * 12);
total-width: 96em;
}
.some-inline-comments {
a: yes /* comment */;
Expand Down
0 test/css/css.css → test/css/strict-math/css.css
View file
Open in desktop
File renamed without changes.
0 test/css/mixins-args.css → test/css/strict-math/mixins-args.css
View file
Open in desktop
File renamed without changes.
0 test/css/parens.css → test/css/strict-math/parens.css
View file
Open in desktop
File renamed without changes.
8 changes: 6 additions & 2 deletions test/index.js
View file
Open in desktop
Expand Up @@ -8,13 +8,17 @@ console.log("\n" + stylize("Less", 'underline') + "\n");
lessTester.prepBomTest();
var testMap = [
[{
strictMath: true,
strictMath: false,
relativeUrls: true,
silent: true,
javascriptEnabled: true,
// Set explicitly for legacy tests for >3.0
ieCompat: true
}],
[{
strictMath: true,
ieCompat: true
}, "strict-math/"],
[{strictMath: true, strictUnits: true, javascriptEnabled: true}, "errors/",
lessTester.testErrors, null],
[{strictMath: true, strictUnits: true, javascriptEnabled: false}, "no-js-errors/",
Expand Down Expand Up @@ -56,7 +60,7 @@ testMap.forEach(function(args) {
lessTester.runTestSet.apply(lessTester, args)
});
lessTester.testSyncronous({syncImport: true}, "import");
lessTester.testSyncronous({syncImport: true}, "css");
lessTester.testSyncronous({syncImport: true}, "strict-math/css");
lessTester.testNoOptions();
lessTester.testJSImport();
lessTester.finished();
7 changes: 7 additions & 0 deletions test/less/calc.less
View file
Open in desktop
@@ -0,0 +1,7 @@
.no-math {
@var: 50vh/2;
width: calc(50% + (@var - 20px));
foo: 1 + 2 calc(3 + 4) 5 + 6;
@floor: floor(1 + .1);
bar: calc(@floor + 20%);
}
4 changes: 2 additions & 2 deletions test/less/media.less
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@
@ratio_large: 16;
@ratio_small: 9;

@media all and (device-aspect-ratio: @ratio_large / @ratio_small) {
@media all and (device-aspect-ratio: ~"@{ratio_large} / @{ratio_small}") {
body { max-width: 800px; }
}

Expand Down Expand Up @@ -177,7 +177,7 @@ body {
}
}

@media (-webkit-min-device-pixel-ratio: 2), (min--moz-device-pixel-ratio: 2), (-o-min-device-pixel-ratio: 2/1), (min-resolution: 2dppx), (min-resolution: 128dpcm) {
@media (-webkit-min-device-pixel-ratio: 2), (min--moz-device-pixel-ratio: 2), (-o-min-device-pixel-ratio: ~"2/1"), (min-resolution: 2dppx), (min-resolution: 128dpcm) {
.b {
background: red;
}
Expand Down
0 test/less/css.less → test/less/strict-math/css.less
View file
Open in desktop
File renamed without changes.
0 test/less/mixins-args.less → test/less/strict-math/mixins-args.less
View file
Open in desktop
File renamed without changes.
0 test/less/parens.less → test/less/strict-math/parens.less
View file
Open in desktop
File renamed without changes.