Refactor CSSStyleDeclaration by asamuzaK · Pull Request #284 · jsdom/cssstyle

asamuzaK · 2026-01-06T14:09:59Z

Motivation

This PR refactors CSSStyleDeclaration to better match the internal architecture requirements for jsdom integration.
Key changes include overhauling the constructor, removing the setter for length, and converting internal methods to class methods.

Part of #255 (comment)

Changes

1. Constructor & Initialization Updates

Direct Property Assignment: Replaced Object.defineProperties with direct property assignments for initializing internal state, improving readability and performance.
Internal Property Cleanup:
- Renamed _setInProgress to _updating for clarity.
- Grouped internal properties with comments (e.g., "Internals for jsdom", "Internals for CSS declaration block").

2. Length Property

Remove set length: Removed the setter for length. The property is now read-only, consistent with the CSSOM interface.
Replace splice: Array.prototype.splice calls were removed because they attempt to set the read-only length property.
- Implemented _clearIndexedProperties() to handle full clears.
- Implemented _removeIndexedProperty(index) to handle single item removal and index shifting.
- Although it is not directly related to length setter, Array.prototype.splice.call(this, property) are also removed and implemented as _getIndexOf(property).

3. Internal Methods Refactoring

Class Methods: Moved internal methods (like _setProperty, _borderSetter, _flexBoxSetter, etc.) from Object.defineProperties(CSSStyleDeclaration.prototype, ...) directly into the class definition.
Argument Renaming: Renamed the prior argument to priority in all setters for consistency.
- Also renamed prop to property and val to value.

4. Documentation

JSDoc Overhaul: Updated JSDoc comments throughout the file to provide clearer API documentation.

asamuzaK · 2026-01-06T21:09:21Z

cssstyle/lib/CSSStyleDeclaration.js

Line 131 in 87e5577

const valueObj = parseCSS(

This line runs the parser every time set cssText() is called, so caching it here should result in a slight performance improvement.

However, this contradicts what was previously stated in #271 (comment). What do you think?

domenic

This all looks very nice, just one question and a few potential simplifications. I appreciate this small incremental commit.

domenic · 2026-01-07T06:09:49Z

-        enumerable: false,
-        writable: true
-      },
+    const { context } = opt;


Shorten these lines by doing constructor(onChangeCallback, { context }) {

For now, not making it optional will cause regressions.
I'm planning to change the arguments in a follow-up PR before a major bump, so let us leave it as is.

Sorry, I meant to type constructor(onChangeCallback, { context } = {})

domenic · 2026-01-07T06:10:50Z

-      },
+    // Internals for jsdom
+    this._global = globalThis;
+    this._onChange = typeof onChangeCallback === "function" ? onChangeCallback : null;


Let's simplify

Suggested change

this._onChange = typeof onChangeCallback === "function" ? onChangeCallback : null;

this._onChange = onChangeCallback;

If the caller wants to pass null (or undefined) they can do so directly, instead of allowing any non-function value to trigger this.

domenic · 2026-01-07T06:14:07Z

      throw new this._global.DOMException(msg, name);
    }
-    Array.prototype.splice.call(this, 0, this._length);
+    this._clearIndexedProperties();


I really like these improvements to indexed property handling. Centralizing that all makes it much more clear.

bd39426

Please take a look.

Hmm, personally I think it was clearer with two functions, but the new version is fine too.

domenic · 2026-01-07T06:14:16Z

-      this[prop] = "";
-      this.removeProperty(prop);
+      if (Object.hasOwn(propertyDescriptors, property)) {
+        propertyDescriptors[property].set.call(this, value);


Oh, this is very cool!

Let's add something like

// TODO: refactor handlers to not require `.call()`

for the future.

domenic · 2026-01-07T06:15:32Z

+    }
+    if (this._values.has(property)) {
+      const index = this._getIndexOf(property);
+      // The property already exists but is not indexed into `this` so add it.


I am confused how this could happen... Does it ever happen during our tests?

This block has been around for a long time so I've never verified it, but it appears to be dead code.
Removed.

domenic · 2026-01-07T06:16:04Z

+      this._priorities.delete(property);
+    }
+    this._values.set(property, value);
+    if (typeof this._onChange === "function" && !this._updating && this.cssText !== originalText) {


Per the above simplification let's just do if (this._onChange && ...)

Done, also elsewhere.

domenic · 2026-01-07T06:26:18Z

This line runs the parser every time set cssText() is called, so caching it here should result in a slight performance improvement.

However, this contradicts what was previously stated in #271 (comment). What do you think?

First, I think doing any extra caching should be done in a separate PR from this one.

Anyway, my position is similar to that previous comment. We should try to balance the number of caches vs. the performance improvement.

I think the best way to balance this would be to benchmark 3 different setups:

3 caches: the current set of caches (isValidPropertyValue, parsePropertyValue, and resolveCalc)
4 caches: the current set of caches plus a parseCSS cache
2 caches: parseCSS and resolveCalc caches only. (isValidPropertyValue and parsePropertyValue rely on the parseCSS cache, but add a bit of their own work which is not cached.)

When we have those numbers we can make a judgement call. Like, if (3) is 90% as good as (1) or (2), then probably (3) is best. But if (2) gives a 100% performance improvement, then we should just use four caches.

Our benchmarks won't be perfect; in particular, as mentioned in #271 (comment), real-world apps might have worse cache access patterns. But we can react to any reports that people send and add them to the benchmarks and reconsider at that time. Until we get such reports, we can treat the benchmarks as our guide, with a small fudge factor like: 10% worse performance is acceptable if it means fewer caches.

What do you think of this approach?

domenic

To avoid extra delay I will integrate the parameter change myself and merge

Refactor CSSStyleDeclaration

87e5577

asamuzaK force-pushed the style branch from 40345e9 to 87e5577 Compare January 6, 2026 14:11

domenic reviewed Jan 7, 2026

View reviewed changes

asamuzaK force-pushed the style branch from e6b6d7f to 32bd54d Compare January 7, 2026 14:19

Update CSSStyleDeclaration.js

5fe1be8

asamuzaK force-pushed the style branch from 32bd54d to 5fe1be8 Compare January 7, 2026 14:23

asamuzaK requested a review from domenic January 7, 2026 14:34

asamuzaK mentioned this pull request Jan 7, 2026

Compare cache patterns #287

Closed

Integrate to _removeIndexedProperties()

bd39426

asamuzaK force-pushed the style branch 2 times, most recently from 918cb27 to ce4e10f Compare January 7, 2026 16:08

Update CSSStyleDeclaration.js

1d17234

asamuzaK force-pushed the style branch from ce4e10f to 1d17234 Compare January 7, 2026 16:11

Remove default value

4564322

domenic approved these changes Jan 8, 2026

View reviewed changes

Parameter declaration cleanup

0078e60

domenic merged commit f712f54 into jsdom:main Jan 8, 2026
3 checks passed

asamuzaK deleted the style branch January 8, 2026 03:55

	this._onChange = typeof onChangeCallback === "function" ? onChangeCallback : null;
	this._onChange = onChangeCallback;

Uh oh!

Uh oh!

Conversation

asamuzaK commented Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

1. Constructor & Initialization Updates

2. Length Property

3. Internal Methods Refactoring

4. Documentation

Uh oh!

asamuzaK commented Jan 6, 2026

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domenic commented Jan 7, 2026

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

asamuzaK commented Jan 6, 2026 •

edited

Loading