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

petermuessig · 2021-06-23T17:54:52Z

No description provided.

petermuessig · 2021-06-23T17:55:58Z

This change ensures that the library variables of sap.tnt in sap_fiori_3 theme defined in the global.less will also be considered for the css_variables.source.less.

RandomByte · 2021-06-23T18:02:49Z

Not directly related to this: But have you ever tested how url() parameters behave with the current CSS variable generation?

We noticed a difference, probably because Less resolves those URLs coming from imports while the CSS variables somehow "take over" the value without this resolution taking place. We think the first finding (sapDefault_URI) in your Snippix from yesterday's meeting shows such a case.

Maybe we could add a CSS var test case for imported parameters containing something like url('111')?

petermuessig · 2021-06-23T18:04:33Z

@RandomByte - yes the URLs are resolved - I don't know who it does yet -> it could also be related to the access via the browser API or the less generation does it. Will check it.

petermuessig · 2021-06-23T18:12:52Z

@RandomByte : AFAICS the rewrite of the URL seems to be missing for the CSS variables. I need to discuss this with @matz3 tomorrow. For the theme-parameters it is done...

RandomByte · 2021-06-23T20:39:46Z

We had a brief look yesterday and found that it might be related to the css-variables-collector using isPreEvalVisitor: true while the variable-collector uses isPreVisitor: true. When using the latter, the URLs are resolved as expected.

lib/plugin/css-variables-collector.js

petermuessig · 2021-06-25T12:01:07Z

URL values in variables are now resolved properly!

petermuessig · 2021-06-25T14:28:42Z

lib/plugin/css-variables-collector.js

+		const vars = {};
+		Object.keys(this.vars).forEach((key) => {
+			const override = this.vars[key].updateAfterEval && varsOverride[key] !== undefined;
+			if (override && process && process.env && process.env.LESS_OPENUI5_CSSVARS_REPORT_OVERRIDE) {


console log only with ENV variable set (to be on the safe side for eventual usage in browser checking process.env to be available first)

For that I think you would need to to a typeof process check, as just using process will result in a ReferenceError.

petermuessig · 2021-06-28T05:57:03Z

The last change of the tests was necessary to ensure that the last definition of the variable in the less files wins. With the old bahavior of checking the rules in the ruleset it could be that the variable defined in a nested less file overrule the top-level variable which is not the expected behavior. In this case, in sap.ui.table, the wrong variable definition for the delete icon won. This now ensures a proper precedence order.

matz3 · 2021-06-28T06:23:17Z

lib/plugin/css-variables-collector.js

+		// in all other cases with no filename (indicates calculated variables)
+		// or in case of variables in standalone less files we just include them!
+		const include = !filename ||
+			(this.config.libPath ? filename.startsWith(`/resources/${this.config.libPath}/themes/`) : true);


We can't rely on the fact that the path starts with /resources/. In the grunt/connect use-case they just start with /sap/....
The current solution in #168 is to lookup the library namespace within the path. Not a bullet-proof solution in case an absolute real FS path is used, but it will at least support the both main use cases (UI5 Tooling + grunt/connect).

Ok, so more my initial regex approach again!

matz3 · 2021-06-28T06:26:45Z

lib/plugin/css-variables-collector.js

+		const vars = {};
+		Object.keys(this.vars).forEach((key) => {
+			const override = this.vars[key].updateAfterEval && varsOverride[key] !== undefined;
+			if (override && process && process.env && process.env.LESS_OPENUI5_CSSVARS_REPORT_OVERRIDE) {


For that I think you would need to to a typeof process check, as just using process will result in a ReferenceError.

matz3 · 2021-06-28T06:29:32Z

lib/plugin/css-variables-collector.js

-			}
-		});
+	visitUrl(node, visitArgs) {
+		// we mark the less variables which should be updated after eval


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.

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!

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

It is described in my comments

matz3 · 2021-06-28T06:31:20Z

lib/plugin/css-variables-urlresolver.js

+
+const less = require("../thirdparty/less");
+
+const CSSVariablesUrlResolverPlugin = module.exports = function(config) {


Why do we need another plugin? Can't we just re-use what the variable-collector plugin provides?

Because of running it at a later point of time! isPreEvalVisitor vs. isPreVisitor - otherwise we have to write ugly if conditions in the coding

Can you give me some more details here? VariableCollector also uses isPreVisitor: true, so I would expect it to work almost the same as this plugin.

The logic for the detection of the URLs is pretty simple and it need to run in the PreVisitor phase. The CSS Var Collector is running in the PreEvalVisitor phase to not resolve the vars. I don't want to run the full enchilada of the code of the CSS Var Collector again in the PreVisitor phase as this is unnecessary and IMO the code of the URL Collector is pretty trivial.

ahh - got your point - I am checking the url-collector right now

There's no need to run anything again.
The variables are already collected and available:

less-openui5/lib/index.js

Line 237 in a6fea7c

result.variables = oVariableCollector.getVariables(Object.keys(mFileMappings[filename] || {}));

It's a different tree.toCSS run, but both are based on the exact same input, so I wouldn't expect any differences in the values. It's the list we already use for the inline theming parameters, so their values are exactly what we expect. That's why I don't expect the need for another plugin.

url-collector is not what I meant, and I think it doesn't really fit for this purpose.

variable-collector seems to be sufficient!

petermuessig · 2021-06-28T07:59:36Z

For that I think you would need to to a typeof process check, as just using process will result in a ReferenceError.

typeof process is not different than process, right? it should be window.process then - let me think...

lib/index.js

lib/plugin/css-variables-collector.js

petermuessig requested a review from matz3 June 23, 2021 17:54

[FIX] Variables: improved detection of library variables

c8f3595

petermuessig force-pushed the fix-libverdetection branch from ed381d5 to c8f3595 Compare June 23, 2021 18:01

matz3 reviewed Jun 24, 2021

View reviewed changes

lib/plugin/css-variables-collector.js Outdated Show resolved Hide resolved

[FIX] Variables: resolve url values in variables

77ca02c

[INTERNAL] Variables: report override only with env var

6217f5a

petermuessig commented Jun 25, 2021

View reviewed changes

petermuessig added 2 commits June 25, 2021 17:16

[FIX] Variables: handle variables in the proper order

1fbb1a6

[FIX] Variables: adopted the test to new behavior (last var wins)

b2ec78d

matz3 reviewed Jun 28, 2021

View reviewed changes

[INTERNAL] Variables: use regex for library-local var detection

64c00e9

petermuessig force-pushed the fix-libverdetection branch from 840957d to 64c00e9 Compare June 28, 2021 08:11

petermuessig added 3 commits June 28, 2021 11:29

[FIX] Variables: reuse of variables-collector

c99433f

[INTERNAL] Variables: using the variable value for override

1f41cc1

[INTERNAL] Variables: improved tests and coverage to be 100%

9e52bc9

petermuessig force-pushed the fix-libverdetection branch from 5363729 to 9e52bc9 Compare June 28, 2021 11:07

petermuessig and others added 4 commits June 28, 2021 13:16

[INTERNAL] Variables: fixed the indentation

6169b39

[INTERNAL] Enhance tests

76edeed

[INTERNAL] Fix RegExp for relative paths used in tests

15c6fb8

[INTERNAL] Cleanup

a24aa11

matz3 reviewed Jun 28, 2021

View reviewed changes

lib/index.js Outdated Show resolved Hide resolved

lib/index.js Outdated Show resolved Hide resolved

lib/plugin/css-variables-collector.js Outdated Show resolved Hide resolved

matz3 changed the title ~~[FIX] Variables: improved detection of library variables~~ [FIX] CSS Variables: Fix variable handling / relative urls Jun 28, 2021

matz3 approved these changes Jun 28, 2021

View reviewed changes

matz3 merged commit 6ace17f into master Jun 28, 2021

matz3 deleted the fix-libverdetection branch June 28, 2021 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

petermuessig commented Jun 23, 2021

petermuessig commented Jun 23, 2021

RandomByte commented Jun 23, 2021 •

edited

petermuessig commented Jun 23, 2021

petermuessig commented Jun 23, 2021 •

edited

RandomByte commented Jun 23, 2021

petermuessig commented Jun 25, 2021

petermuessig Jun 25, 2021

matz3 Jun 28, 2021

petermuessig commented Jun 28, 2021

matz3 Jun 28, 2021

petermuessig Jun 28, 2021

matz3 Jun 28, 2021

matz3 Jun 28, 2021

petermuessig Jun 28, 2021

matz3 Jun 28, 2021

petermuessig Jun 28, 2021

matz3 Jun 28, 2021

petermuessig Jun 28, 2021

matz3 Jun 28, 2021

petermuessig Jun 28, 2021

petermuessig Jun 28, 2021

matz3 Jun 28, 2021

matz3 Jun 28, 2021

petermuessig Jun 28, 2021

petermuessig commented Jun 28, 2021


		const less = require("../thirdparty/less");

		const CSSVariablesUrlResolverPlugin = module.exports = function(config) {

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

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

Conversation

petermuessig commented Jun 23, 2021

petermuessig commented Jun 23, 2021

RandomByte commented Jun 23, 2021 • edited

petermuessig commented Jun 23, 2021

petermuessig commented Jun 23, 2021 • edited

RandomByte commented Jun 23, 2021

petermuessig commented Jun 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petermuessig commented Jun 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petermuessig commented Jun 28, 2021

RandomByte commented Jun 23, 2021 •

edited

petermuessig commented Jun 23, 2021 •

edited