Skip to content

[FIX] CSS Variables: Fix variable handling / relative urls #172

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

Merged
merged 13 commits into from
Jun 28, 2021
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
10 changes: 8 additions & 2 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,10 @@ Builder.prototype.build = function(options) {
// inject the library name as prefix (e.g. "sap.m" as "_sap_m")
if (options.library && typeof options.library.name === "string") {
const libName = config.libName = options.library.name;
config.libPath = libName.replace(/\./g, "/");
config.prefix = "_" + libName.replace(/\./g, "_") + "_";
} else {
config.prefix = ""; // no library, no prefix
}

// Keep filename for later usage (see ImportCollectorPlugin) as less modifies the parserOptions.filename
Expand Down Expand Up @@ -287,10 +290,13 @@ Builder.prototype.build = function(options) {
// generate the skeleton-css and the less-variables
const CSSVariablesCollectorPlugin = require("./plugin/css-variables-collector");
const oCSSVariablesCollector = new CSSVariablesCollectorPlugin(config);
const VariableCollector = require("./plugin/variable-collector");
const oVariableCollector = new VariableCollector(config);
result.cssSkeleton = tree.toCSS(Object.assign({}, options.compiler, {
plugins: [oCSSVariablesCollector]
plugins: [oCSSVariablesCollector, oVariableCollector]
}));
result.cssVariablesSource = oCSSVariablesCollector.toLessVariables();
const varsOverride = oVariableCollector.getAllVariables();
result.cssVariablesSource = oCSSVariablesCollector.toLessVariables(varsOverride);
if (oRTL) {
const oCSSVariablesCollectorRTL = new CSSVariablesCollectorPlugin(config);
result.cssSkeletonRtl = tree.toCSS(Object.assign({}, options.compiler, {
Expand Down
91 changes: 56 additions & 35 deletions lib/plugin/css-variables-collector.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@ const CSSVariablesCollectorPlugin = module.exports = function(config) {
this.ruleStack = [];
this.mixinStack = [];
this.parenStack = [];
this.importStack = [];
};

CSSVariablesCollectorPlugin.prototype = {

// needed to keep the less variable references intact to use this info for the CSS variables references
isPreEvalVisitor: true,

isReplacing: true,

_isInMixinOrParen() {
Expand All @@ -27,27 +28,44 @@ CSSVariablesCollectorPlugin.prototype = {
return this.ruleStack.length > 0 && !this.ruleStack[this.ruleStack.length - 1].variable;
},

_isInGlobalOrBaseImport() {
return this.config.libName !== "sap.ui.core" && this.importStack.filter((importNode) => {
return /\/(?:global|base)\.less$/.test(importNode.importedFilename);
}).length > 0;
_isVarInLibrary({varName, filename}) {
// for libraries we check that the file is within the libraries theme path
// in all other cases with no filename (indicates calculated variables)
// or in case of variables in standalone less files we just include them!
const regex = new RegExp(`(^|/)${this.config.libPath}/themes/`);
const include = !filename ||
(this.config.libPath ? regex.test(filename) : true);
return include;
},

_isRelevant() {
return !this._isInMixinOrParen() && this._isVarInRule();
},

toLessVariables() {
toLessVariables(varsOverride) {
const vars = {};
Object.keys(this.vars).forEach((key) => {
const override = this.vars[key].updateAfterEval && varsOverride[key] !== undefined;
/*
if (override) {
console.log(`Override variable "${key}" from "${this.vars[key].css}" to "${varsOverride[key]}"`);
}
*/
vars[key] = {
css: override ? varsOverride[key] : this.vars[key].css,
export: this.vars[key].export
};
});
let lessVariables = "";
Object.keys(this.vars).forEach((value, index) => {
lessVariables += "@" + value + ": " + this.vars[value].css + ";\n";
Object.keys(vars).forEach((value, index) => {
lessVariables += "@" + value + ": " + vars[value].css + ";\n";
});
Object.keys(this.calcVars).forEach((value, index) => {
lessVariables += "@" + value + ": " + this.calcVars[value].css + ";\n";
});
lessVariables += "\n:root {\n";
Object.keys(this.vars).forEach((value, index) => {
if (this.vars[value].export) {
Object.keys(vars).forEach((value, index) => {
if (vars[value].export) {
lessVariables += "--" + value + ": @" + value + ";\n";
}
});
Expand Down Expand Up @@ -89,15 +107,17 @@ CSSVariablesCollectorPlugin.prototype = {

visitOperation(node, visitArgs) {
if (this._isRelevant()) {
// console.log("visitOperation", this.ruleStack[this.ruleStack.length - 1], this._getCSS(node));
return new less.tree.Call("calc", [new less.tree.Expression([node.operands[0], new less.tree.Anonymous(node.op), node.operands[1]])]);
}
return node;
},

visitCall(node, visitArgs) {
// if variables are used inside rules, generate a new dynamic variable for it!
// if variables are used inside rules, generate a new calculated variable for it!
const isRelevantFunction = typeof less.tree.functions[node.name] === "function" && ["rgba"].indexOf(node.name) === -1;
if (this._isRelevant() && isRelevantFunction) {
// console.log("visitCall", this.ruleStack[this.ruleStack.length - 1], this._getCSS(node));
const css = this._getCSS(node);
let newName = this.config.prefix + "function_" + node.name + Object.keys(this.vars).length;
// check for duplicate value in vars already
Expand All @@ -109,7 +129,9 @@ CSSVariablesCollectorPlugin.prototype = {
}
this.calcVars[newName] = {
css: css,
export: !this._isInGlobalOrBaseImport()
export: this._isVarInLibrary({
varName: newName
})
};
return new less.tree.Call("var", [new less.tree.Anonymous("--" + newName, node.index, node.currentFileInfo, node.mapLines)]);
}
Expand All @@ -119,6 +141,7 @@ CSSVariablesCollectorPlugin.prototype = {
visitNegative(node, visitArgs) {
// convert negative into calc function
if (this._isRelevant()) {
// console.log("visitNegative", this.ruleStack[this.ruleStack.length - 1], this._getCSS(node));
return new less.tree.Call("calc", [new less.tree.Expression([new less.tree.Anonymous("-1"), new less.tree.Anonymous("*"), node.value])]);
}
return node;
Expand All @@ -133,6 +156,20 @@ CSSVariablesCollectorPlugin.prototype = {
},

visitRule(node, visitArgs) {
// check rule for being a variable declaration
const isVarDeclaration = typeof node.name === "string" && node.name.startsWith("@");
if (!this._isInMixinOrParen() && isVarDeclaration) {
// add the variable declaration to the list of vars
const varName = node.name.substr(1);
const isVarInLib = this._isVarInLibrary({
varName,
filename: node.currentFileInfo.filename
});
this.vars[varName] = {
css: this._getCSS(node.value),
export: isVarInLib
};
}
// store the rule context for the call variable extraction
this.ruleStack.push(node);
return node;
Expand Down Expand Up @@ -168,29 +205,13 @@ CSSVariablesCollectorPlugin.prototype = {
return node;
},

visitImport(node, visitArgs) {
// store the import context
this.importStack.push(node);
return node;
},

visitImportOut(node) {
// remove import context
this.importStack.pop();
return node;
},

visitRuleset(node, visitArgs) {
node.rules.forEach((value) => {
const isVarDeclaration = value instanceof less.tree.Rule && typeof value.name === "string" && value.name.startsWith("@");
if (!this._isInMixinOrParen() && isVarDeclaration) {
// add the variable declaration to the list of vars
this.vars[value.name.substr(1)] = {
css: this._getCSS(value.value),
export: !this._isInGlobalOrBaseImport()
};
}
});
visitUrl(node, visitArgs) {
// we mark the less variables which should be updated after eval
Copy link
Member

Choose a reason for hiding this comment

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

My expectation was actually the other way round.
To use the "override" values (from the new preVisitor) in general, and only use the current ones for the variable references (which was the main reason for enabling the preEvalVisitor mode).
Otherwise I'd expect that also some other values might differ unexpectedly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are issues within the isPreEval phase where less variables with the value none also have the URL content - so doing it this way is the best to determine properly the variables and apply only the needed later! All other options require much more code!

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Would be great to have those specialities documented somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is described in my comments

// => strangewise less variables with "none" values are also urls
// after the less variables have been evaluated
if (this.ruleStack.length > 0 && this.ruleStack[0].variable) {
this.vars[this.ruleStack[0].name.substr(1)].updateAfterEval = true;
}
return node;
}

Expand Down
28 changes: 28 additions & 0 deletions test/expected/complex/test-cssvars-skeleton.css
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,34 @@ a:hover {
color: #fff;
background: var(--link-color);
}
.background {
background-image: var(--backgroundImage);
}
.lazy-eval {
width: var(--var);
}
.nested .calc-vars {
height: var(--c);
width: var(--d);
color: var(--function_lighten14);
top: calc(-1 * var(--top));
line-height: calc(var(--lineHeight) / 2);
background: #428bca;
border-color: #92bce0;
background: #5697d0;
}
.nested .calc-vars-duplicate {
color: var(--function_lighten14);
padding-left: calc(20px + 0.5rem);
border-color: #ffffff;
background: #ffffff;
}
.nested .calc-vars-duplicate somePrefix.somePadding {
padding: 1rem;
box-sizing: border-box;
}
@media (max-width: 599px) {
.nested .calc-vars-duplicate somePrefix.somePadding {
padding: 0;
}
}
11 changes: 10 additions & 1 deletion test/expected/complex/test-cssvars-variables.css
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,15 @@
--link-color-hover: #3071a9;
--other-var: 0 0 0.25rem 0 rgba(0, 0, 0, 0.15), inset 0 -0.0625rem 0 0 #428bca;
--link-color-active: var(--link-color);
--backgroundVariant: 1;
--backgroundImage: url(img.png);
--a: 100%;
--var: var(--a);
--a: 9%;
--b: #245682;
--padLeft: 20px;
--top: 5rem;
--lineHeight: 2rem;
--c: calc(20%);
--d: -100%;
--function_lighten14: #3071a9;
}
33 changes: 33 additions & 0 deletions test/expected/complex/test-cssvars-variables.source.less
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
@link-color: #428bca;
@link-color-hover: darken(@link-color, 10%);
@other-var: 0 0 0.25rem 0 rgba(0, 0, 0, 0.15), inset 0 -0.0625rem 0 0 @link-color;
@link-color-active: @link-color;
@backgroundVariant: 1;
@backgroundImage: url(img.png);
@a: 100%;
@var: @a;
@b: darken(@link-color-active, 20%);
@padLeft: 20px;
@top: 5rem;
@lineHeight: 2rem;
@c: calc(100% - 80px);
@d: -@a;
@function_lighten14: lighten(@b, 10%);

:root {
--link-color: @link-color;
--link-color-hover: @link-color-hover;
--other-var: @other-var;
--link-color-active: @link-color-active;
--backgroundVariant: @backgroundVariant;
--backgroundImage: @backgroundImage;
--a: @a;
--var: @var;
--b: @b;
--padLeft: @padLeft;
--top: @top;
--lineHeight: @lineHeight;
--c: @c;
--d: @d;
--function_lighten14: @function_lighten14;
}
28 changes: 28 additions & 0 deletions test/expected/complex/test.css
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,34 @@ a:hover {
color: #fff;
background: #428bca;
}
.background {
background-image: url(img.png);
}
.lazy-eval {
width: 9%;
}
.nested .calc-vars {
height: calc(20%);
width: -100%;
color: #3071a9;
top: -5rem;
line-height: 1rem;
background: #428bca;
border-color: #92bce0;
background: #5697d0;
}
.nested .calc-vars-duplicate {
color: #3071a9;
padding-left: calc(20px + 0.5rem);
border-color: #ffffff;
background: #ffffff;
}
.nested .calc-vars-duplicate somePrefix.somePadding {
padding: 1rem;
box-sizing: border-box;
}
@media (max-width: 599px) {
.nested .calc-vars-duplicate somePrefix.somePadding {
padding: 0;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@
}

/* Inline theming parameters */
#sap-ui-theme-my\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22color1%22%3A%22%23fefefe%22%7D')}
#sap-ui-theme-my\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22color1%22%3A%22%23fefefe%22%2C%22url1%22%3A%22url(%27111%27)%22%7D')}
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@
}

/* Inline theming parameters */
#sap-ui-theme-my\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22color1%22%3A%22%23fefefe%22%7D')}
#sap-ui-theme-my\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22color1%22%3A%22%23fefefe%22%2C%22url1%22%3A%22url(%27111%27)%22%7D')}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
:root {
--color1: #ffffff;
--url1: url('../base/111');
}
.fooContrast {
--color1: #000000;
}

/* Inline theming parameters */
#sap-ui-theme-my\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22color1%22%3A%22%23ffffff%22%2C%22url1%22%3A%22url(%27..%2Fbase%2F111%27)%22%7D%2C%22scopes%22%3A%7B%22fooContrast%22%3A%7B%22color1%22%3A%22%23000000%22%7D%7D%7D')}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
@color1: #ffffff;
@url1: url('../base/111');

:root {
--color1: @color1;
--url1: @url1;
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@
}

/* Inline theming parameters */
#sap-ui-theme-my\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22color1%22%3A%22%23ffffff%22%7D%2C%22scopes%22%3A%7B%22fooContrast%22%3A%7B%22color1%22%3A%22%23000000%22%7D%7D%7D')}
#sap-ui-theme-my\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22color1%22%3A%22%23ffffff%22%2C%22url1%22%3A%22url(%27..%2Fbase%2F111%27)%22%7D%2C%22scopes%22%3A%7B%22fooContrast%22%3A%7B%22color1%22%3A%22%23000000%22%7D%7D%7D')}
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@
}

/* Inline theming parameters */
#sap-ui-theme-my\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22color1%22%3A%22%23ffffff%22%7D%2C%22scopes%22%3A%7B%22fooContrast%22%3A%7B%22color1%22%3A%22%23000000%22%7D%7D%7D')}
#sap-ui-theme-my\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22color1%22%3A%22%23ffffff%22%2C%22url1%22%3A%22url(%27..%2Fbase%2F111%27)%22%7D%2C%22scopes%22%3A%7B%22fooContrast%22%3A%7B%22color1%22%3A%22%23000000%22%7D%7D%7D')}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
.myUiLibRule1 {
color: var(--color1);
}
.myUiLibRule2 {
padding: 1px 4px 3px 2px;
}
.fooContrast.myUiLibRule1,
.fooContrast .myUiLibRule1 {
color: var(--color1);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
.myUiLibRule1 {
color: var(--color1);
}
.myUiLibRule2 {
padding: 1px 2px 3px 4px;
}
.fooContrast.myUiLibRule1,
.fooContrast .myUiLibRule1 {
color: var(--color1);
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@
}

/* Inline theming parameters */
#sap-ui-theme-my\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22color1%22%3A%22%23ffffff%22%7D%2C%22scopes%22%3A%7B%22barContrast%22%3A%7B%22color1%22%3A%22%23000000%22%7D%7D%7D')}
#sap-ui-theme-my\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22color1%22%3A%22%23ffffff%22%2C%22url1%22%3A%22url(%27..%2Fbase%2F111%27)%22%7D%2C%22scopes%22%3A%7B%22barContrast%22%3A%7B%22color1%22%3A%22%23000000%22%7D%7D%7D')}
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@
}

/* Inline theming parameters */
#sap-ui-theme-my\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22color1%22%3A%22%23ffffff%22%7D%2C%22scopes%22%3A%7B%22barContrast%22%3A%7B%22color1%22%3A%22%23000000%22%7D%7D%7D')}
#sap-ui-theme-my\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22color1%22%3A%22%23ffffff%22%2C%22url1%22%3A%22url(%27..%2Fbase%2F111%27)%22%7D%2C%22scopes%22%3A%7B%22barContrast%22%3A%7B%22color1%22%3A%22%23000000%22%7D%7D%7D')}
5 changes: 5 additions & 0 deletions test/expected/simple/test-cssvars-variables.source.less
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
@myColor: #ff0000;

:root {
--myColor: @myColor;
}
Loading