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.

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

CSS Custom Properties not supported #89

Closed
jcousins-ynap opened this issue May 23, 2019 · 8 comments
Closed

CSS Custom Properties not supported #89

jcousins-ynap opened this issue May 23, 2019 · 8 comments

Comments

@jcousins-ynap
Copy link
Contributor

CSS Custom properties are ignored as are the rules which use them.

const
  cssstyle = require('cssstyle'),
  customProp = new cssstyle.CSSStyleDeclaration(),
  normalProp = new cssstyle.CSSStyleDeclaration();

customProp.cssText = `--hello-world: blue; background: var(--hello-world);`
console.log(customProp.background) // "" -- ❌ expected "blue"

normalProp.cssText = `background: blue;`
console.log(normalProp.background) // "blue" -- ✅

Unlike the CSS color names PR, I have no idea how to resolve this issue.
I don't mind contributing to solve this issue, but I will likely need some help.

@ckedwards
Copy link

If the point is to match CSSStyleDeclaration then this kind of resolution is not necessary.

For example, when I tried this in chrome:

var styleObj = document.styleSheets[0].cssRules[0].style;
styleObj.cssText = "--hello-world: blue; background: var(--hello-world);"
console.log(styleObj.background)
// Output: var(--hello-world)

It seems like custom properties just need to be set on the CSSStyleDeclaration even though they aren't in allProperties or allExtraProperties.

A custom property is defined as "any valid identifier that starts with two dashes, except -- itself"
https://drafts.csswg.org/css-variables-1/#custom-property

@sheijne
Copy link

sheijne commented Jan 16, 2020

Honestly, why has this issue been open for so long? There are also multiple issues describing this problem in the jsdom repo, which go back multiple years..

I've been looking through the cssstyle source and it appears that this issue should be easy to solve.

Correct me if I'm wrong, but the problem seems to be with the way properties are filtered:

https://github.com/jsdom/cssstyle/blob/master/lib/CSSStyleDeclaration.js#L60

if (!allProperties.has(lowercaseName) && !allExtraProperties.has(lowercaseName)) {

According to the spec:

A custom property is any property whose name starts with two dashes (U+002D HYPHEN-MINUS), like --foo. The production corresponds to this: it’s defined as any valid identifier that starts with two dashes. Custom properties are solely for use by authors and users; CSS will never give them a meaning beyond what is presented here.

So a solution could be:

if (!lowercaseName.startsWith('--') && !allProperties.has(lowercaseName) && !allExtraProperties.has(lowercaseName)) {

Willing to provide a hand, not sure if I'm the right person for the job, since I'm completely unfamiliar with the source.

@jsakas
Copy link
Member

jsakas commented Jan 20, 2020

@sheijne I agree we should get this one resolved. I think your suggestion is a good start, however the spec also states css custom properties are case sensitive so we need to account for that. I will try to put up a PR for this soon. In the meantime if you want to take a stab at it (including test cases) that would be awesome.

@jsakas
Copy link
Member

jsakas commented Jan 23, 2020

We have added partial support for css custom properties in 2.2.0. Properties beginning with -- can now be added to the style object.

@jsakas jsakas closed this as completed Jan 23, 2020
@IgorNovozhilov
Copy link

@jsakas, original example still doesn't work, is there any work being done to add this feature?

@raveclassic
Copy link

Folks, any updates on this?

@adamayres
Copy link

I believe there are two issues here:

1.) Unable to set a value into a CSS variable via the style attribute.
2.) Unable to reference a CSS variable via the style attribute.

The first issue was fixed here by this code change:

var isCustomProperty = name.indexOf('--') === 0;
if (isCustomProperty) {
this._setProperty(name, value, priority);
return;
}

This fix allowed a new CSS property to be declared, example:

const customProp = new cssstyle.CSSStyleDeclaration();
customProp.cssText = '--hello-world: blue;';
console.log(customProp.cssText);

// => --hello-world: blue; 

However, this does not fix the second issue. If you attempt to reference a CSS variable, it is removed. Example:

const customProp = new cssstyle.CSSStyleDeclaration();
customProp.cssText = '--hello-world: blue; background: var(--hello-world)';
console.log(customProp.cssText);

// => --hello-world: blue; 

I believe the code that removes it is as follows...

When calling the setter on customProp.cssText, it goes into the setProperty in CSSStyleDeclaration for each key/value declared in the cssText:

setProperty: function(name, value, priority) {

This code then attempts to call a setter for the CSS property onto this here:

this[lowercaseName] = value;

For each type of CSS property there are setters defined. For example, the background goes through the shorthandSetter:

set: shorthandSetter('background', shorthand_for),

The shorthandSetter ultimately calls into the shorthandParser which then iterates over the possible shorthand_for types.

Iterates here:

cssstyle/lib/parsers.js

Lines 543 to 552 in b527ed7

parts.forEach(function(part, i) {
var part_valid = false;
Object.keys(shorthand_for).forEach(function(property) {
if (shorthand_for[property].isValid(part, i)) {
part_valid = true;
obj[property] = part;
}
});
valid = valid && part_valid;
});

The possible shorthand_for types for background are declared here:

var shorthand_for = {
'background-color': require('./backgroundColor'),
'background-image': require('./backgroundImage'),
'background-repeat': require('./backgroundRepeat'),
'background-attachment': require('./backgroundAttachment'),
'background-position': require('./backgroundPosition'),
};

It then attempts to check if the value is valid, via an isValid method, for each of the short hand types. In our case, we would expect that the check for the backgroundColor to be valid for a value of var(--hello-world), however the code does not recognize CSS properties.

The isValid for the backgroundColor here:

https://github.com/jsdom/cssstyle/blob/master/lib/properties/backgroundColor.js#L5-L21

Ultimately calls into parseColor here:

exports.parseColor = function parseColor(val) {

The parseColor does not support this notation.

Thoughts on a potential fix

In parsers, such as parseColor, a check could be done to see if the value is a CSS variable and simply return the value. Example:

exports.parseColor = function parseColor(val) {
  if (val.startsWith('var(--') {
    return val;
  }
  ...

This would need to be done for each parser that could potentially run on a value where a CSS variable is allowed.

Also, in order to support CSS variables that are used as parts of things like a hsla color, for example:

const customProp = new cssstyle.CSSStyleDeclaration();
customProp.cssText = 'background: hsl(var(--test4), 0%, 100%)';

The RegEx's used in the parsers, such as the one in parseColor, would need to be updated to match these as valid patterns which they currently do not support.

cssstyle/lib/parsers.js

Lines 31 to 35 in b527ed7

var colorRegEx1 = /^#([0-9a-fA-F]{3,4}){1,2}$/;
var colorRegEx2 = /^rgb\(([^)]*)\)$/;
var colorRegEx3 = /^rgba\(([^)]*)\)$/;
var calcRegEx = /^calc\(([^)]*)\)$/;
var colorRegEx4 = /^hsla?\(\s*(-?\d+|-?\d*.\d+)\s*,\s*(-?\d+|-?\d*.\d+)%\s*,\s*(-?\d+|-?\d*.\d+)%\s*(,\s*(-?\d+|-?\d*.\d+)\s*)?\)/;

@aayla-secura
Copy link

Any updates or plan to fix this? There doesn't seem to be a way currently to test style that reference a custom property as it's just silently dropped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants