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

Fixes #1421 - re-parses variable-interpolated elements to selectors #3217

Merged
merged 5 commits into from
Jun 16, 2018
Merged

Fixes #1421 - re-parses variable-interpolated elements to selectors #3217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c383326
WIP - partially solves 1241 but breaks other tests
matthew-dean Jun 2, 2018
15d662a
Fix element to selector list conversion, passing all tests!
matthew-dean Jun 3, 2018
d1ed1fe
Add passing test from #3098
matthew-dean Jun 3, 2018
94e7c50
Added passing test example from #1817
matthew-dean Jun 3, 2018
9cec20e
Allow lists to be re-evaluated as selectors (Fixes #1694)
matthew-dean Jun 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 28 additions & 23 deletions lib/less/parser/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,7 @@ var Parser = function Parser(context, imports, fileInfo) {
if (!e) {
break;
}
elem = new(tree.Element)(c, e, elemIndex, fileInfo);
elem = new(tree.Element)(c, e, false, elemIndex, fileInfo);
if (elements) {
elements.push(elem);
} else {
Expand Down Expand Up @@ -1097,7 +1097,7 @@ var Parser = function Parser(context, imports, fileInfo) {
}
}

if (e) { return new(tree.Element)(c, e, index, fileInfo); }
if (e) { return new(tree.Element)(c, e, e instanceof tree.Variable, index, fileInfo); }
},

//
Expand Down Expand Up @@ -1177,6 +1177,30 @@ var Parser = function Parser(context, imports, fileInfo) {
if (elements) { return new(tree.Selector)(elements, allExtends, condition, index, fileInfo); }
if (allExtends) { error("Extend must be used to extend a selector, it cannot be used on its own"); }
},
selectors: function () {
var s, selectors;
while (true) {
s = this.selector();
if (!s) {
break;
}
if (selectors) {
selectors.push(s);
} else {
selectors = [ s ];
}
parserInput.commentStore.length = 0;
if (s.condition && selectors.length > 1) {
error("Guards are only currently allowed on a single selector.");
}
if (!parserInput.$char(',')) { break; }
if (s.condition) {
error("Guards are only currently allowed on a single selector.");
}
parserInput.commentStore.length = 0;
}
return selectors;
},
attribute: function () {
if (!parserInput.$char('[')) { return; }

Expand Down Expand Up @@ -1228,34 +1252,15 @@ var Parser = function Parser(context, imports, fileInfo) {
// div, .class, body > p {...}
//
ruleset: function () {
var selectors, s, rules, debugInfo;
var selectors, rules, debugInfo;

parserInput.save();

if (context.dumpLineNumbers) {
debugInfo = getDebugInfo(parserInput.i);
}

while (true) {
s = this.selector();
if (!s) {
break;
}
if (selectors) {
selectors.push(s);
} else {
selectors = [ s ];
}
parserInput.commentStore.length = 0;
if (s.condition && selectors.length > 1) {
error("Guards are only currently allowed on a single selector.");
}
if (!parserInput.$char(',')) { break; }
if (s.condition) {
error("Guards are only currently allowed on a single selector.");
}
parserInput.commentStore.length = 0;
}
selectors = this.selectors();

if (selectors && (rules = this.block())) {
parserInput.forget();
Expand Down
5 changes: 4 additions & 1 deletion lib/less/tree/element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ var Node = require("./node"),
Paren = require("./paren"),
Combinator = require("./combinator");

var Element = function (combinator, value, index, currentFileInfo, visibilityInfo) {
var Element = function (combinator, value, isVariable, index, currentFileInfo, visibilityInfo) {
this.combinator = combinator instanceof Combinator ?
combinator : new Combinator(combinator);

Expand All @@ -13,6 +13,7 @@ var Element = function (combinator, value, index, currentFileInfo, visibilityInf
} else {
this.value = "";
}
this.isVariable = isVariable;
this._index = index;
this._fileInfo = currentFileInfo;
this.copyVisibilityInfo(visibilityInfo);
Expand All @@ -30,12 +31,14 @@ Element.prototype.accept = function (visitor) {
Element.prototype.eval = function (context) {
return new Element(this.combinator,
this.value.eval ? this.value.eval(context) : this.value,
this.isVariable,
this.getIndex(),
this.fileInfo(), this.visibilityInfo());
};
Element.prototype.clone = function () {
return new Element(this.combinator,
this.value,
this.isVariable,
this.getIndex(),
this.fileInfo(), this.visibilityInfo());
};
Expand Down
2 changes: 1 addition & 1 deletion lib/less/tree/mixin-definition.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ var Selector = require("./selector"),

var Definition = function (name, params, rules, condition, variadic, frames, visibilityInfo) {
this.name = name;
this.selectors = [new Selector([new Element(null, name, this._index, this._fileInfo)])];
this.selectors = [new Selector([new Element(null, name, false, this._index, this._fileInfo)])];
this.params = params;
this.condition = condition;
this.variadic = variadic;
Expand Down
88 changes: 76 additions & 12 deletions lib/less/tree/ruleset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,20 +41,72 @@ Ruleset.prototype.accept = function (visitor) {
}
};
Ruleset.prototype.eval = function (context) {
var thisSelectors = this.selectors, selectors,
selCnt, selector, i, hasOnePassingSelector = false;
var that = this, selectors, selCnt, selector, i, hasOnePassingSelector = false;

if (thisSelectors && (selCnt = thisSelectors.length)) {
if (this.selectors && (selCnt = this.selectors.length)) {
selectors = new Array(selCnt);
defaultFunc.error({
type: "Syntax",
message: "it is currently only allowed in parametric mixin guards,"
});
for (i = 0; i < selCnt; i++) {
selector = thisSelectors[i].eval(context);
selectors[i] = selector;
if (selector.evaldCondition) {
hasOnePassingSelector = true;
selector = this.selectors[i].eval(context);
var removeSelector = false;
for (var j = 0; j < selector.elements.length; j++) {
var el = selector.elements[j];
// If selector elements were variables, re-parse to see if they are actually selectors
if (el.isVariable) {
var selectorsToInsert;
if (Array.isArray(el.value.value)) {
// Convert var evaluated to list as list of selectors
selectorsToInsert = new Array(el.value.value.length);
el.value.value.forEach(function(val, k) {
selectorsToInsert[k] = new Selector(
[new Element(
null,
val.value,
false,
el.getIndex(),
el.fileInfo()
)]
);
})
} else if (typeof el.value.value === 'string') {
this.parse.parseNode(
el.value.value,
["selectors"],
el.getIndex(),
el.fileInfo(),
function(err, result) {
el.isVariable = false;
if (result) {
result = utils.flattenArray(result);
// If the parsed element matches itself, it's still an element
if (result.length !== 1 || el.value.value !== result[0].elements[0].value) {
selectorsToInsert = result;
}
}
});
}
if (selectorsToInsert) {
this.selectors = this.selectors.slice(0, i + 1)
.concat(selectorsToInsert, this.selectors.slice(i + 1));
selCnt += selectorsToInsert.length;
removeSelector = true;
}
}
}
if (removeSelector) {
selCnt -= 1;
this.selectors.splice(i, 1);
i -= 1;
continue;
}
else {
selectors[i] = selector;
if (selector.evaldCondition) {
hasOnePassingSelector = true;
}
}
}
defaultFunc.reset();
Expand Down Expand Up @@ -162,7 +214,7 @@ Ruleset.prototype.eval = function (context) {
// for rulesets, check if it is a css guard and can be removed
if (rule instanceof Ruleset && rule.selectors && rule.selectors.length === 1) {
// check if it can be folded in (e.g. & where)
if (rule.selectors[0].isJustParentSelector()) {
if (rule.selectors[0] && rule.selectors[0].isJustParentSelector()) {
rsRules.splice(i--, 1);

for (var j = 0; (subRule = rule.rules[j]); j++) {
Expand Down Expand Up @@ -507,7 +559,13 @@ Ruleset.prototype.joinSelector = function (paths, context, selector) {
} else {
var insideParent = new Array(elementsToPak.length);
for (j = 0; j < elementsToPak.length; j++) {
insideParent[j] = new Element(null, elementsToPak[j], originalElement._index, originalElement._fileInfo);
insideParent[j] = new Element(
null,
elementsToPak[j],
originalElement.isVariable,
originalElement._index,
originalElement._fileInfo
);
}
replacementParen = new Paren(new Selector(insideParent));
}
Expand All @@ -516,7 +574,7 @@ Ruleset.prototype.joinSelector = function (paths, context, selector) {

function createSelector(containedElement, originalElement) {
var element, selector;
element = new Element(null, containedElement, originalElement._index, originalElement._fileInfo);
element = new Element(null, containedElement, originalElement.isVariable, originalElement._index, originalElement._fileInfo);
selector = new Selector([element]);
return selector;
}
Expand Down Expand Up @@ -550,7 +608,13 @@ Ruleset.prototype.joinSelector = function (paths, context, selector) {
combinator = parentEl.combinator;
}
// join the elements so far with the first part of the parent
newJoinedSelector.elements.push(new Element(combinator, parentEl.value, replacedElement._index, replacedElement._fileInfo));
newJoinedSelector.elements.push(new Element(
combinator,
parentEl.value,
replacedElement.isVariable,
replacedElement._index,
replacedElement._fileInfo
));
newJoinedSelector.elements = newJoinedSelector.elements.concat(addPath[0].elements.slice(1));
}

Expand Down Expand Up @@ -684,7 +748,7 @@ Ruleset.prototype.joinSelector = function (paths, context, selector) {
// the combinator used on el should now be applied to the next element instead so that
// it is not lost
if (sel.length > 0) {
sel[0].elements.push(new Element(el.combinator, '', el._index, el._fileInfo));
sel[0].elements.push(new Element(el.combinator, '', el.isVariable, el._index, el._fileInfo));
}
selectorsMultiplied.push(sel);
}
Expand Down
2 changes: 1 addition & 1 deletion lib/less/tree/selector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ Selector.prototype.getElements = function(els) {
return els;
};
Selector.prototype.createEmptySelectors = function() {
var el = new Element('', '&', this._index, this._fileInfo),
var el = new Element('', '&', false, this._index, this._fileInfo),
sels = [new Selector([el], null, null, this._index, this._fileInfo)];
sels[0].mediaEmpty = true;
return sels;
Expand Down
18 changes: 17 additions & 1 deletion lib/less/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* jshint proto: true */
module.exports = {
var utils = {
getLocation: function(index, inputStream) {
var n = index + 1,
line = null,
Expand Down Expand Up @@ -65,5 +65,21 @@ module.exports = {
}
}
return obj1;
},
flattenArray: function(arr, result) {
result = result || [];
for (var i = 0, length = arr.length; i < length; i++) {
var value = arr[i];
if (Array.isArray(value)) {
utils.flattenArray(value, result);
} else {
if (value !== undefined) {
result.push(value);
}
}
}
return result;
}
};

module.exports = utils;
1 change: 1 addition & 0 deletions lib/less/visitors/extend-visitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ ProcessExtendsVisitor.prototype = {
firstElement = new tree.Element(
match.initialCombinator,
replacementSelector.elements[0].value,
replacementSelector.elements[0].isVariable,
replacementSelector.elements[0].getIndex(),
replacementSelector.elements[0].fileInfo()
);
Expand Down
21 changes: 21 additions & 0 deletions test/css/parse-interpolation.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
input[type=text]:focus,
input[type=email]:focus,
input[type=password]:focus,
textarea:focus {
foo: bar;
}
.a + .z,
.b + .z,
.c + .z {
color: blue;
}
.master-page-1 .selector-1,
.master-page-1 .selector-2 {
background-color: red;
}
.fruit-apple,
.fruit-satsuma,
.fruit-banana,
.fruit-pear {
content: "Just a test.";
}
2 changes: 1 addition & 1 deletion test/less-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ module.exports = function() {
var oneTestOnly = process.argv[2],
isFinished = false;

var isVerbose = process.env.npm_config_loglevel === 'verbose';
var isVerbose = process.env.npm_config_loglevel !== 'concise';

var normalFolder = 'test/less';
var bomFolder = 'test/less-bom';
Expand Down
29 changes: 29 additions & 0 deletions test/less/parse-interpolation.less
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
@inputs: input[type=text], input[type=email], input[type=password], textarea;

@{inputs} {
&:focus {
foo: bar;
}
}

@classes: ~".a, .b, .c";

@{classes} {
Copy link
Member

@seven-phases-max seven-phases-max Jun 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

What about more complex tests like:

.bar {
    foo-@{classes}&:hover, baz {...}
}

etc?

Copy link
Member Author

@matthew-dean matthew-dean Jun 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Shit, that borks it entirely. I reverted this PR. Any other test cases you can think of? Basically, I put in the test cases that people had filed as their use cases, but yeah it should be more robust.

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically, the result of that should be an error, but it doesn't even get close. I THINK that's because I had it jump out if it's an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had this working perfectly with:

.bar {
    foo-@{classes}&:hover, baz {...}
}

and more complex cases, and then I tried....

@c: ~'.a, .b';
@d: ~' > .c';
@e: ~' + .d';

@{c}@{d}@{e} {
  foo: bar;
}

...aaaaaand wasted the rest of my weekend lol.

The problem was I had a kind of "selector merging" behavior with something like:

@textClasses: ~'[class="text"], .text';

input@{textClasses} {
  background: red;
}

//outputting
input[class="text"],
input.text {
  background: red;
}

That is, if we assume that interpolated text should be merged into the selector list, then it stands to reason that it should merged with any adjacent elements and combinators.

All of that I got working, but naïvely. As soon as there were multiple vars containing multiple selectors, elements, and combinators, it borked. Not only would it be inconsistent behavior, it's not clear what the expected behavior SHOULD be.

Maybe a more rational approach is to try to stringify everything in the selector, including the eval'd variable, THEN re-parse the entire selector list, rather than try to parse the variable and merge. That would not allow that input[class="text"], input.text output, but that seems to have add-on misery with multiple vars and combinators.

I dunno. My brain has died now.

+ .z {
color: blue;
}
}

@my-selector: ~'.selector-1, .selector-2';
.master-page-1 {
@{my-selector} {
background-color: red;
}
}

@list: apple, satsuma, banana, pear;
@{list} {
.fruit-& {
content: "Just a test.";
}
}