Skip to content

Conversation

asamuzaK
Copy link
Contributor

@asamuzaK asamuzaK commented Jul 30, 2025

@asamuzaK asamuzaK marked this pull request as draft July 30, 2025 20:56
@asamuzaK asamuzaK force-pushed the valid branch 4 times, most recently from 96f666d to e75e388 Compare August 2, 2025 07:05
@asamuzaK asamuzaK changed the title Replace isValid() with isValidPropertyValue() Use cssTree features whenever possible Aug 2, 2025
@asamuzaK asamuzaK marked this pull request as ready for review August 30, 2025 04:14
@asamuzaK asamuzaK marked this pull request as draft August 30, 2025 04:36
@asamuzaK asamuzaK marked this pull request as ready for review August 30, 2025 11:57
@asamuzaK
Copy link
Contributor Author

PR ready.
It's a near complete rewrite, but should be non-breaking.

@asamuzaK
Copy link
Contributor Author

To run test against jsdom locally:

cssstyle:

npm link

cd to jsdom and run:

npm link cssstyle && npm run test

Don't forget to run below against jsdom afterwards.

npm i

@asamuzaK asamuzaK force-pushed the valid branch 5 times, most recently from b7e65ec to 98a06e3 Compare August 30, 2025 20:12
@domenic
Copy link
Member

domenic commented Aug 31, 2025

I'll merge this, but what should we write in the changelog? It seems like a lot of properties were improved, and some new ones were added? Can we get a list of all of them? Or should we just give up and say "lots of parsing and serialization improvements to all properties"?

@asamuzaK
Copy link
Contributor Author

asamuzaK commented Aug 31, 2025

No new properties are added.
Regardless of whether they are supported or not, the parser/lexer is now more powerful and accurate than before, so it is able to catch invalid values.
That's why some of the foo-invalid.html tests pass.

Summary of this PR:

  • Improved internal processing by switching from regular expression matching to AST-based parsing.
  • Improved getter and setter of cssText.
  • Improved serialization of border, flex, margin, and padding shorthands.
  • Added static propertyList() method to the CSSStyleDeclaration class that lists the supported properties and their data.
    It is intended to eventually replace jsdom's https://github.com/jsdom/jsdom/blob/main/lib/jsdom/living/helpers/style-rules.js#L22, but is still experimental.

@domenic
Copy link
Member

domenic commented Aug 31, 2025

  • Added static propertyList() method to the CSSStyleDeclaration class that lists the supported properties and their data.

Oh, this seems bad, since it's not in the spec. Can you export it from the module somewhere else instead of putting it on the CSSStyleDeclaration class?

(If we ever created wrappers using webidl2js per #235, then it wouldn't matter, but right now jsdom directly exposes cssstyle's CSSStyleDeclaration, so it needs to not have extra methods outside of the spec.)

@asamuzaK
Copy link
Contributor Author

asamuzaK commented Aug 31, 2025

It's fine with me to expose propertyList() separately from the CSSStyleDeclaration class, but if that's the case, wouldn't it be better to create a wrapper as soon as possible?
Currently, CSSStyleDeclaration accepts arguments that are not in the spec, i.e. constructor(onChangeCallback, opt).

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