Split into CSSStyleDeclaration and CSSStyleProperties

[asamuzaK]

Part of #235
Rebased #243

Add fixes for #242, fixes for #247, fixes for jsdom/jsdom#3939, fixes for jsdom/jsdom#3940, fixes for jsdom/jsdom#3944

Spec: https://drafts.csswg.org/cssom/#the-cssstyledeclaration-interface

• Breaking change: the functionality provided by CSSStyleDeclaration is now provided by CSSStyleProperties, i.e. use new CSSStyleProperties() to create an instance.
• Breaking change: the arguments to new CSSStyleProperties() are different from the arguments to new CSSStyleDeclaration().
  • The first argument is a Window.
  • The second argument is an options object.
    • The on change callback previously received as the first argument should be included in the options object using onChange key.

  const node = document.createElement("div");
  const callback = cssText => {
    console.log(cssText);
  };
  const style = new CSSStyleProperties(window, {
    onChange: callback
  });

• Switched main entry point from ./lib/CSSStyleDeclaration.js to ./lib/index.js.
  • Exporting 3 files from index.js.
    • CSSStyleDeclaration.js
    • CSSStypeProperties.js
    • propertyList.js
      • List of supported properties and their definitions.
• Updated dependencies and devDependencies.

[asamuzaK]

Back to draft, will enable after jsdom@27 issues are addressed.

[asamuzaK]

Ready for review.

[domenic]

Have you tested this with a local fork of jsdom yet? It's probably best to merge and release only once we know it works.

[domenic]

Can we add a comment for what this will be used for? (In general, changing options after creation seems unusual, so documenting it is good.)

[asamuzaK]

Done.
Please check.

[domenic]

I think I am still confused. I tried reading the constructor and cannot find any documentation on what options / styleOpts / styleOptions is going to contain. I guess it is passed through to the parser code?

Do we really need that level of configurability from the outside? If I understand correctly, there are two possible types of CSSStyleDeclaration: the ones returned from the style getter, and the ones returned from getComputedStyle(). Could we just have those two modes be options passed from the outside in the constructor, and then the code in this file determines how to set up the style options passed to the parser code?

In other words, why do we need to change this over the lifetime of the CSSStyleDeclaration object, instead of setting it once at creation?

[domenic]

Why this file is separate from CSSStyleProperties.test.js? Maybe one or both of them should get a comment, if the separation is intentional?

[asamuzaK]

properties.test.js is unit testing for each file under properties/*, while CSSStyleDeclaration.test.js and CSSStyleProperties.test.js are for instance testing, but I don't think a clear policy has been decided yet.
We will need it to be decided in the future, though.
@see #211

[domenic]

Yeah, it does not seem very well-separated. E.g. CSSStyleProperties.test.js has tests for top, bottom, left, right, clear, clip, lots of border properties...

I guess it does not block this PR, but it would be nice to clean up at some point. Maybe just merging the two files.

[asamuzaK]

Have you tested this with a local fork of jsdom yet? It's probably best to merge and release only once we know it works.

Yes.
I am testing it locally.

[domenic]

Would it be possible to replace all the CacheItem/NullItem/etc. complexity with the following?

// The lru-cache module requires non-null values, so substitute in this sentinel when we are given one.
const nullSentinel = Symbol("null");

function setCache(key, value) {
  lruCache.set(key, value === null ? nullSentinel : value);
}

function getCache(key) {
  const value = lruCache.get(key);
  return value === nullSentinel ? null : value;
}

(If necessary you could add back the stuff like no-opping on falsy keys in setCache, but that seems a bit strange too.)

[asamuzaK]

Simplified cache handling.

[domenic]

Yeah, it does not seem very well-separated. E.g. CSSStyleProperties.test.js has tests for top, bottom, left, right, clear, clip, lots of border properties...

I guess it does not block this PR, but it would be nice to clean up at some point. Maybe just merging the two files.

[domenic]

I think I am still confused. I tried reading the constructor and cannot find any documentation on what options / styleOpts / styleOptions is going to contain. I guess it is passed through to the parser code?

Do we really need that level of configurability from the outside? If I understand correctly, there are two possible types of CSSStyleDeclaration: the ones returned from the style getter, and the ones returned from getComputedStyle(). Could we just have those two modes be options passed from the outside in the constructor, and then the code in this file determines how to set up the style options passed to the parser code?

In other words, why do we need to change this over the lifetime of the CSSStyleDeclaration object, instead of setting it once at creation?

[domenic]

I have read the constructor a bit more carefully and have some suggestions.

• I think it is coded a bit too defensively. We should assume it will always be used in jsdom. So we don't need a default for globalObject or the options. We don't need to have a guard to check if context is truthy. We can assume the caller always passes a correct value for format.

• From what I can tell there are basically four types of data stored on the class:

  • Stuff that directly corresponds to the spec, i.e. https://drafts.csswg.org/cssom/#css-declaration-block : _ownerNode, _parentRule, _readonly, _computed, _updating
  • Stuff that corresponds to the spec's "declarations": _values, _priorities, _length.
  • Stuff that is about interfacing with jsdom: _global, _onChange
  • Stuff that is used internally: _options
    It would be nice to make this separation a bit clearer, just using whitespace and comments.

• It is strange that some things that seem like they should be booleans (_readonly, _computed, _updating) are left as undefined instead of false.

• There is a mismatch between the documentation and the ...styleOpts. As mentioned elsewhere, the styleOpts being open-ended is a bit hard to understand.

Here is a rough suggestion for your consideration:

  /**
   * @param {object} globalObject - Window
   * @param {object} opt - Options
   * @param {object} opt.context - Element or CSSStyleRule
   * @param {string} opt.format - "specifiedValue" or "computedValue"
   * @param {Function} opt.onChange - Callback when cssText is changed or the property is removed
   * @param {boolean} opt.readOnly - The read-only flag
   * @param {???} opt.styleOpts - Document this, or encapsulate it better per discussion below.
   */
  constructor(globalObject, { context, format, onChange, readOnly, styleOpts }) {
    // These help interface with jsdom.
    this._global = globalObject;
    this._onChange = onChange;

    // These correspond to https://drafts.csswg.org/cssom/#css-declaration-block.
    this._computed = format === "computedValue";
    this._readonly = readOnly ?? false;
    this._parentRule = null;
    this._ownerNode = null;
    if (context) {
      if (context.nodeType === 1) {
        this._ownerNode = context;
      } else if (Object.hasOwn(context, "parentRule")) {
        this._parentRule = context;
      }
    }
    this._updating = false;

    // These correspond to the specification's "declarations".
    this._values = new Map();
    this._priorities = new Map();
    this._length = 0;

    // These are used internally for TODO describe the purpose.
    this._options = {
      ...styleOpts,
      format
    };
  }

[asamuzaK]

Here's the rough processing steps for getComputedStyle():

• First, cssstyle saves the specified values.
• Next, jsdom provides several options for converting to calculated values.
  For example, the currentColor value, the em to px conversion value, etc.
  The method that receives these values is setOptions().
• Finally, cssstyle uses the saved specified values and options to derive a calculated value at runtime and returns that value. (not included in this PR)

HTML:

<div style="color: green; font-size: 12px;">
  <span style="border-color: currentColor; border-width: calc(1em / 3);"></span>
</div>

jsdom:

const declarationForElement = elt => {
  const elementImpl = Element.convert(elt);
  const declaration = CSSStyleProperties.createImpl(elementImpl, [], {});
  // store specified values; in the above case for span will be:
  declaration.cssText = "border-color: currentColor; border-width: calc(1em / 3);";
  // provide options and make declaration readonly
  declaration.setOptions({
    currentColor: "green",
    dimension: {
      em: 12
    },
    format: "computedValue",
    readOnly: true
  });
  return declaration;
};

[asamuzaK]

I've uploaded a draft PR to the jsdom side so you can see it.
jsdom/jsdom#3957