Skip to content

Conversation

@asamuzaK
Copy link
Contributor

@asamuzaK asamuzaK commented Dec 29, 2025

Description:

This PR improves the internal architecture of cssstyle by refining how property modules export their metadata and how values are resolved via AST types. It also ensures better compliance with Web Platform Tests and streamlines the value processing logic.

Key Changes:

  • Property Name Exports: Each property module now explicitly exports its property name as module.exports.property. This replaces previously hardcoded strings (e.g., "background") with a more maintainable, reference-based approach for property names.
    • NOTE: Planning to apply this to normalize.js in a separate follow-up PR.
  • New AST Resolvers in parsers.js: Added a set of helper functions (resolvers) that dispatch values to the appropriate parser based on their AST (Abstract Syntax Tree) type. This logic has been applied across the property files in lib/properties/*.js to streamline value processing.

Additional Improvements:

  • Refactored Priority Calculation: As a related cleanup, the logic for determining the priority argument in property setters has been simplified and unified. This reduces code duplication and ensures consistent behavior across different properties.

Impact:

Testing:

  • Verified that npm test passes.
  • Confirmed the fix for wpt:tuwpt css/cssom/serialize-whitespace.html.

@domenic
Copy link
Member

domenic commented Dec 31, 2025

All of this looks good except I don't understand the last commit. Can you expand on what it fixes and give it a better commit title/message?

@asamuzaK
Copy link
Contributor Author

getPropertyDescriptor() is a function that returns generic getters and setters for properties not included in properties/*.js.
In the original implementation, the setter only performed a simple process of parsing the CSS input and setting it as-is. In this PR, I have updated it to handle properties more appropriately (at least to a minimum extent) by utilizing AST_TYPES.

I had already submitted the changes to getPropertyDescriptor() as a separate PR #272, but since both properties/*.js and utils/propertyDescriptor.js now share the same logic of importing and using AST_TYPES, I decided to put them into this single PR.

Each property module now exports its property name as `module.exports.property`. This change enables more consistent and DRY usage of property names throughout the codebase, especially for shorthand and initial value maps, reducing hardcoded strings and potential typos.
Simplifies and unifies the logic for determining the priority argument in property setters across background, border, flex, font, margin, and padding property modules. The new approach uses a single conditional to check shorthand and property priorities, improving code clarity and maintainability.
getPropertyDescriptor() is used for the generic getters and setters that do not have implementations in properties/*.js. Change from just parsing CSS and setting it as-is, to handle properties more correctly by using the AST types.
@domenic
Copy link
Member

domenic commented Dec 31, 2025

I've updated the commit description to include that and will merge after CI finishes.

As always, let me know when is a good time for a new release!

@domenic domenic merged commit dcfbbaf into jsdom:main Dec 31, 2025
3 checks passed
@asamuzaK asamuzaK deleted the props branch December 31, 2025 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants