jsdom / cssstyle

A Node.js implementation of the CSS Object Model CSSStyleDeclaration interface
MIT License
109 stars 70 forks source link

CSS Custom Properties not supported #89

Closed jcousins-ynap closed 4 years ago

jcousins-ynap commented 5 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

IgorNovozhilov commented 3 years ago

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

raveclassic commented 2 years ago

Folks, any updates on this?

adamayres commented 2 years ago

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:

https://github.com/jsdom/cssstyle/blob/b527ed722364dc6d156487c652df100572075dee/lib/CSSStyleDeclaration.js#L59-L63

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:

https://github.com/jsdom/cssstyle/blob/b527ed722364dc6d156487c652df100572075dee/lib/CSSStyleDeclaration.js#L51

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

https://github.com/jsdom/cssstyle/blob/b527ed722364dc6d156487c652df100572075dee/lib/CSSStyleDeclaration.js#L69

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

https://github.com/jsdom/cssstyle/blob/b527ed722364dc6d156487c652df100572075dee/lib/properties/background.js#L15

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

Iterates here: https://github.com/jsdom/cssstyle/blob/b527ed722364dc6d156487c652df100572075dee/lib/parsers.js#L543-L552

The possible shorthand_for types for background are declared here:

https://github.com/jsdom/cssstyle/blob/b527ed722364dc6d156487c652df100572075dee/lib/properties/background.js#L6-L12

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:

https://github.com/jsdom/cssstyle/blob/b527ed722364dc6d156487c652df100572075dee/lib/parsers.js#L288

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.

https://github.com/jsdom/cssstyle/blob/b527ed722364dc6d156487c652df100572075dee/lib/parsers.js#L31-L35

aayla-secura commented 2 months ago

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.