jsdom / cssstyle

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

Enable vars and nested calc&var #127

Open snowystinger opened 3 years ago

snowystinger commented 3 years ago

This library already just passes calc's right through. I propose doing the same with var and nested calcs and nested vars. We don't need to resolve them, but I do need them not to be swallowed silently in my tests.

codecov-io commented 3 years ago

Codecov Report

Merging #127 (6b0100e) into master (b527ed7) will increase coverage by 0.26%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #127      +/-   ##
==========================================
+ Coverage   37.39%   37.65%   +0.26%     
==========================================
  Files          87       87              
  Lines        1182     1187       +5     
  Branches      227      229       +2     
==========================================
+ Hits          442      447       +5     
  Misses        633      633              
  Partials      107      107              
Impacted Files Coverage Δ
lib/parsers.js 80.88% <100.00%> (+0.23%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b527ed7...6b0100e. Read the comment docs.

CharlyTorregrosa commented 2 years ago

Hi! AFAIK this closes #125 Could be consider @jsakas ?

hughes-ch commented 2 years ago

Just stumbled across this pull request after trying to figure out why var() wasn't working.

Instead of defining a new VAR parser type, should CSSStyleDeclaration.setProperty be updated instead? There's already a check for custom properties which can be leveraged. I'd suggest doing this (the only change is to setting isCustomProperty):

CSSStyleDeclaration.prototype = {
...
  setProperty: function(name, value, priority) {
...
    var isCustomProperty = name.indexOf('--') === 0 ||
        (typeof value === 'string' && /^var\(--[-\w]+,?.*\)$/.test(value));
    if (isCustomProperty) {
      this._setProperty(name, value, priority);
      return;
    }
...
  },

  // Included for context (already implemented on master)
  _setProperty: function(name, value, priority) {
    if (value === undefined) {
      return;
    }
    if (value === null || value === '') {
      this.removeProperty(name);
      return;
    }
    if (this._values[name]) {
      // Property already exist. Overwrite it.
      var index = Array.prototype.indexOf.call(this, name);
      if (index < 0) {
        this[this._length] = name;
        this._length++;
      }
    } else {
      // New property.
      this[this._length] = name;
      this._length++;
    }
    this._values[name] = value;
    this._importants[name] = priority;
    this._onChange(this.cssText);
  },

As implemented in this pull request, only the color property will be able to use var(). Other properties (such as font-size) will still run into issues parsing it.

snowystinger commented 2 years ago

Thanks for taking a look, I'll see if I can find some time to update and check if that works, authors of the repo should feel free to push to my fork as well if they get to it before I do

effervescentia commented 1 year ago

@domenic @jsakas I'd like to follow up here (and on this PR) since it appears that there are a number of missing features from cssstyle and jsdom that prevent CSS variable inheritance to be testable using JSDOM

here is a minimum-viable failure case using the vanilla-extract styling library which relies heavily on CSS variables to enable performant theming and dynamic styling

https://stackblitz.com/edit/node-x7idfl?file=Button.test.tsx

filipkis commented 3 months ago

I want to also add my voice to the issue as we're using JSDOM for server side rendering and the fact var's are not supported definitely causes big issues for us.

snowystinger commented 2 months ago

@hughes-ch thanks for the suggestion, I think that will work. I've updated the PR. Apologies for the long wait