Add longhands field to CSS shorthand properties#1821
Add longhands field to CSS shorthand properties#1821lifeiscontent wants to merge 3 commits intow3c:mainfrom
longhands field to CSS shorthand properties#1821Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to add a shorthandFor field to CSS shorthand properties across 39 CSS specification files, listing constituent longhands in CSS specification order. The feature addresses issue #1766, which requests machine-readable data for shorthand property expansions.
Changes:
- Adds
tools/add-shorthand-longhands.jsscript to detect and add shorthandFor data - Updates 39 CSS spec files with shorthandFor data for shorthand properties
- Uses three detection strategies: syntax parsing, positional orderings, and manual mappings
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 37 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/add-shorthand-longhands.js | Script to detect shorthand properties and add shorthandFor arrays |
| ed/css/scroll-animations.json | Added shorthandFor for scroll-timeline, view-timeline, and animation-range |
| ed/css/pointer-animations.json | Added shorthandFor for pointer-timeline |
| ed/css/motion.json | Added shorthandFor for offset property |
| ed/css/css-ui.json | Added shorthandFor for outline, caret, and interest-delay (has bugs) |
| ed/css/css-transitions.json | Added shorthandFor for transition |
| ed/css/css-text.json | Added shorthandFor for white-space |
| ed/css/css-text-decor.json | Added shorthandFor for text-decoration and text-emphasis |
| ed/css/css-text-decor-4.json | Added shorthandFor for text-decoration and text-emphasis |
| ed/css/css-text-4.json | Added shorthandFor for white-space, text-wrap, and text-spacing |
| ed/css/css-speech.json | Added shorthandFor for pause, rest, and cue |
| ed/css/css-sizing-4.json | Added shorthandFor for contain-intrinsic-size |
| ed/css/css-scroll-snap.json | Added shorthandFor for scroll-padding, scroll-margin, and related logical properties |
| ed/css/css-rhythm.json | Added shorthandFor for block-step |
| ed/css/css-position.json | Added shorthandFor for inset-block, inset-inline, and inset |
| ed/css/css-overscroll.json | Added shorthandFor for overscroll-behavior |
| ed/css/css-overflow.json | Added shorthandFor for overflow and overflow-clip-margin |
| ed/css/css-overflow-4.json | Added shorthandFor for overflow-clip-margin variants and line-clamp |
| ed/css/css-multicol.json | Added shorthandFor for columns |
| ed/css/css-masking.json | Added shorthandFor for mask and mask-border |
| ed/css/css-logical.json | Added shorthandFor for logical margin/padding/inset/border properties |
| ed/css/css-lists.json | Added shorthandFor for list-style |
| ed/css/css-inline.json | Added shorthandFor for vertical-align |
| ed/css/css-grid.json | Added shorthandFor for grid-template, grid, grid-row, grid-column, and grid-area |
| ed/css/css-grid-3.json | Added shorthandFor for item-flow |
| ed/css/css-gaps.json | Added shorthandFor for rule-* properties (has multiple bugs) |
| ed/css/css-fonts.json | Added shorthandFor for font, font-synthesis, and font-variant |
| ed/css/css-flexbox.json | Added shorthandFor for flex-flow and flex |
| ed/css/css-conditional-5.json | Added shorthandFor for container |
| ed/css/css-color-adjust.json | Added shorthandFor for color-adjust |
| ed/css/css-cascade.json | Added shorthandFor for all (special "all-properties" value) |
| ed/css/css-box.json | Added shorthandFor for margin and padding |
| ed/css/css-borders.json | Added shorthandFor for border properties, corner properties, border-image, and box-shadow (has many bugs) |
| ed/css/css-backgrounds.json | Added shorthandFor for background-position, background, border properties, border-radius, border-image, and box-shadow |
| ed/css/css-backgrounds-4.json | Added shorthandFor for background-position and background |
| ed/css/css-animations.json | Added shorthandFor for animation |
| ed/css/css-animations-2.json | Added shorthandFor for timeline-trigger-range, timeline-trigger-exit-range, and timeline-trigger |
| ed/css/css-anchor-position.json | Added shorthandFor for position-try |
| ed/css/css-align.json | Added shorthandFor for place-content, place-self, place-items, and gap |
| ed/css/CSS.json | Added shorthandFor for margin, padding, border properties, overflow, list-style, background properties, font-variant, font, text-decoration, white-space, and outline |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fe3a98e to
10f54dc
Compare
tidoust
left a comment
There was a problem hiding this comment.
Thanks @lifeiscontent!
First, this provides an excellent basis for reasoning and iterating over. That's great!
Automatic detection of shorthands through "see individual properties" values seems to be working well enough. Only 11 properties out of 162 are detected through an hardcoded name. Making the match case-insensitive would get us down to 9... and it will also add two shorthands that currently fall through the cracks: -webkit-text-stroke and text-decoration-skip. There may be a few additional shorthand properties that are not currently captured but the list seems solid as-is.
Automatic computation of longhands is more problematic. Out of the 162 shorthand properties, only 17 seem to have their list of longhands computed automatically from their syntax and, looking at them, I see 4 properties in that list that I'm not sure are correct:
line-clamp
What aboutmax-linesandcontinue?rule-width
Shorthand forcolumn-rule-widthonly? Shouldn't it be forrow-rule-widthas well?rule
Shorthand forcolumn-ruleonly? Shouldn't it be forrow-ruleas well?vertical-align
What aboutbaseline-source?
In other words, it seems hard to rely on the syntax to compute longhands... and yet I'd love it if the list of hardcoded mappings between shorthands and longhands would be more an exception to the rule than the rule, to ease data maintenance.
I need to dig into this more. One approach I'm wondering about is whether the property name of the shorthand could be used to find the longhands (typically by looking at all properties that have all name tokens that appear in the base property name, and consider all properties that have the same tokens + one). That approach does not always work (e.g. mask-border is not a longhand of mask), but it should roughly work for properties such as margin, padding, background-position, font-variant, border-*, etc.
Ordering of the longhand properties could also be done with a more generic set of hardcoded rules:
xbeforeymodebeforelinebeforewidthbeforestylebeforecoloralignbeforejustify- (with some care)
topbeforerightbeforebottombeforeleft
There may still be value in improving the crawler to extract more contextual information about shorthands. That's something else I'll look into. But it could be done in a second time.
Good news is that this should be easier now that you've created an initial benchmark against which other approaches may be compared!
About the pull request itself:
- I would rename
add-shorthand-longhands.jstoadd-css-longhands.jsto be explicit that the script touches CSS extracts. - Come to think about it,
longhandsmay be a better key name thanshorthandForto stick to a single word. - CSS extracts under
ed/cssare raw results from the crawl. They shouldn't be touched and shouldn't be included in the PR. The script will run as part of data curation and update CSS extracts that will end up in thecuratedbranch. - If the script detects a shorthand without longhands at the end, it should throw instead of silently reporting the problems to the console. This will prevent data curation and force us to look into the problem. That's a good thing.
- I do not like the fact that
shorthandForis an array of strings except forallwhere it's just the valueall-properties. That forces consumers to always test the value. I'm wondering to what extent we shouldn't leavealluntouched. Consumers that need to handleallwill likely need dedicated logic for it in any case. - Note for self: the JSON schema for the CSS extracts defined in Reffy allows adding a property, but it would be even better to extend it with the new property to validate that it is an array of strings.
- Regardless of how longhands are produced, we should add a check in
test/css/all.jsto guarantee that all longhands exist in the extracts.
10f54dc to
e1ef9e7
Compare
|
Thanks for the thorough review! I've addressed all the feedback: Naming changes:
Behavior changes:
Bug fixes:
New mappings (from case-insensitive detection):
Testing:
Note: The upstream fix for Results: 214 shorthands detected, 213 with longhands, 1 skipped ( |
longhands field to CSS shorthand properties
tidoust
left a comment
There was a problem hiding this comment.
Thanks! Last thing that's missing is integration in tools/prepare-curated.js, typically after the call to amendCssSyntaxes. Would you be able to do that?
|
@tidoust I'll try to give it a shot later today :) |
Address review feedback: - Rename script to add-css-longhands.js - Rename field from shorthandFor to longhands - Remove ed/css changes (raw crawl results) - Throw errors when longhands can't be resolved - Skip `all` property (consumers need dedicated logic) - Add validation test for longhands in test/css/all.js - Make shorthand detection case-insensitive - Fix bug mappings: line-clamp, rule-width, rule, vertical-align - Add mappings for -webkit-text-stroke and text-decoration-skip 214 shorthands detected, 213 with longhands, 1 skipped (all).
… scroll-initial-target workaround
e1ef9e7 to
f6616d0
Compare
tidoust
left a comment
There was a problem hiding this comment.
A few more fixes, comparing the longhands here with longhands extracted from the mdn/data package.
Also, this comparison reveals that -webkit-mask and grid-gap are missing. Instead of adding them to the hardcoded list, I would rather leverage the fact that they have a legacyAliasOf property that targets the shorthand property that they are legacy alias of, and copy over the longhands of that target property. This would also remove the need to list the longhands of -webkit-line-clamp (it will just take those of line-clamp).
tools/add-css-longhands.js
Outdated
| 'grid-column': ['grid-column-start', 'grid-column-end'], | ||
|
|
||
| // Multi-column | ||
| 'columns': ['column-width', 'column-count'], |
There was a problem hiding this comment.
| 'columns': ['column-width', 'column-count'], | |
| 'columns': ['column-width', 'column-count', 'column-height'], |
There was a problem hiding this comment.
Fixed in 1886bf0. columns now expands to column-width, column-count, and column-height.
tools/add-css-longhands.js
Outdated
| 'outline': ['outline-width', 'outline-style', 'outline-color'], | ||
|
|
||
| // Text decoration | ||
| 'text-decoration': ['text-decoration-line', 'text-decoration-style', 'text-decoration-color'], |
There was a problem hiding this comment.
| 'text-decoration': ['text-decoration-line', 'text-decoration-style', 'text-decoration-color'], | |
| 'text-decoration': ['text-decoration-line', 'text-decoration-thickness', 'text-decoration-style', 'text-decoration-color'], |
There was a problem hiding this comment.
Fixed in 1886bf0. Added text-decoration-thickness to the text-decoration longhands list.
tools/add-css-longhands.js
Outdated
| 'transition-property', | ||
| 'transition-duration', | ||
| 'transition-timing-function', | ||
| 'transition-delay' |
There was a problem hiding this comment.
Level 2 adds transition-behavior.
| 'transition-delay' | |
| 'transition-delay', | |
| 'transition-behavior' |
There was a problem hiding this comment.
Fixed in 1886bf0. Added transition-behavior to the transition longhands list.
|
Addressed in 1886bf0. Additional updates from the latest review:
This also covers missing aliases such as I also re-ran the script on a temp copy of |
Summary
Adds a
shorthandForfield to CSS shorthand properties, listing their constituent longhands in CSS specification order.Addresses #1766
Changes
tools/add-shorthand-longhands.jsscript to detect and add shorthandForDetection strategies
The script uses three strategies:
<'property-name'>referencesExample output
{ "margin": ["margin-top", "margin-right", "margin-bottom", "margin-left"], "flex": ["flex-grow", "flex-shrink", "flex-basis"], "border-radius": ["border-top-left-radius", "border-top-right-radius", "border-bottom-right-radius", "border-bottom-left-radius"], "all": "all-properties" }Coverage
209 shorthands with 100% coverage
Includes:
allproperty uses"all-properties"string valueNotes
scroll-initial-targetis excluded as a false positive (has incorrectcomputedValue: "see individual properties"in the spec). Filed upstream: [css-scroll-snap-2] scroll-initial-target has incorrect 'see individual properties' in computed value csswg-drafts#13425background-tbdis excluded as it's a placeholder property