-
-
Notifications
You must be signed in to change notification settings - Fork 80
Preparation for resolving shortcut property issues #261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
05d1ddb to
38299f6
Compare
|
Can you add a more detailed description of this PR? What changes, why are those changes good, does this fix any bugs currently or is it only for use in a follow-up, etc.? Why do we need a new generated properties list and how does it differ from the other generated lists? What specs does this implement? That sort of thing. |
domenic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My biggest worry here is the module-level mutable state.
Otherwise, there are likely improvements that could be made to reduce complexity and code duplication, etc., but they are not blockers, and I am happy to continue with you being the expert on these parts of the code.
lib/normalize.js
Outdated
| for (const position of borderPositions) { | ||
| const longhandProperty = `${namePart}-${position}-${linePart}`; | ||
| const longhandItem = createPropertyItem(longhandProperty, lineValue, priority); | ||
| if (longhandProperties.has(longhandProperty)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern (lines 1346-1354) appears multiple times. Can we extract it into a helper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are still other instances of the pattern I identified:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we have these three duplicated:
Lines 1306 to 1314 in a35b915
if (properties.has(longhandProperty)) { const { priority: existingPriority } = properties.get(longhandProperty); if (!existingPriority) { properties.delete(longhandProperty); properties.set(longhandProperty, longhandItem); } } else { properties.set(longhandProperty, longhandItem); } Lines 1330 to 1338 in a35b915
if (safe && properties.has(property)) { const { priority: existingPriority } = properties.get(property); if (!existingPriority) { properties.delete(property); properties.set(property, item); } } else { properties.set(property, item); } Lines 1401 to 1409 in a35b915
} else if (longhandProperties.has(property)) { const { priority } = longhandProperties.get(property); if (!priority) { longhandProperties.delete(property); longhandProperties.set(property, item); } } else { longhandProperties.set(property, item); }
I think maybe you missed what I was referring to in the original comment, which was specifically the block from 1346-1354? Both of your attempted refactorings so far do not focus on that specific code pattern, but instead pull out larger chunks of code. And because they pull out larger chunks of code, they miss and duplicate the smaller pattern I am pointing to.
Updated generateBorderLineShorthand, generateBorderPositionShorthand, and generateBorderNameShorthand to use a default parameter for priority instead of reassigning within the function. This simplifies the function signatures and improves code clarity.
Replaces direct calls to createPropertyItem with getPropertyItem for border-related properties in prepareBorderProperties, streamlining property item retrieval and improving code consistency.
Replaces the borderCollections object with borderCollectionConfig and dynamically constructs borderCollections with items and priorityItems maps. Renames priorItems to priorityItems throughout for clarity and consistency, and updates related logic in prepareBorderShorthands.
Renamed 'namePriorItems' to 'namePriorityItems' in prepareBorderShorthands for consistency and clarity.
Extracted repeated logic for updating border longhand properties into a new helper function, updatePositionLonghands. This improves code readability and maintainability in the border property normalization process.
Introduces the updateLonghandProperties helper to consolidate logic for updating longhand properties in multiple places. This reduces code duplication and improves maintainability in normalize.js.
Replaces all instances of the variable 'nameItem' with 'shorthandItem'. No functional changes were made; this is a variable renaming for better meaning and code readability.
Renamed generateBorderNameShorthand to generateBorderShorthand and updated related variable names for clarity. This improves code readability and better reflects the function's purpose.
Summary
This PR serves as a preparation step to resolve long-standing issues with shorthand properties.
It introduces a mechanism to auto-generate CSS property definitions and refactors the codebase to utilize them.
Key Changes and Reasons
1. Introducing
propertyDefinitions.jslib/generated/propertyDefinitions.jsbased on property metadata.lib/generated/propertyDefinitions.jsis designed to be the single source of truth for CSS properties. In the future, this is intended to replacepropertiesWithResolvedValueImplementedin jsdom'slib/jsdom/living/helpers/style-rules.js, allowing jsdom to delegate property metadata management entirely tocssstyle.2. Splitting Functions and Clarifying Roles
3. Refactoring with Constants
Effect
While the direct effects of this PR are limited regarding the overall shorthand behavior, it explicitly resolves the following test case:
web-platform-tests/to-upstream/css/cssom/style-border-shorthand-var.htmlOnce merged, this PR establishes the necessary data structures and infrastructure, allowing us to implement the complex logic for expanding shorthands into longhands in a follow-up PR with high reliability and low maintenance cost.