Skip to content

Commit

Permalink
Sort shorthand across multi-properties (#1732)
Browse files Browse the repository at this point in the history 
* Fix sortShorthand when mixed with multi-property classes

Example: this `{-webkit-text-decoration-color:initial;text-decoration-color:initial}` blocks all sorting above/below it, effectively creating a new "sorting bucket" because it always returns `0` as `a` or `b`.
```css
._y44v1tgl{animation:kbayob8 5s ease infinite}
._syaz1qtk{color:var(--ds-link)}
._1jmq18uv{-webkit-text-decoration-color:initial;text-decoration-color:initial}
._zulph461{gap:var(--ds-space-050)}
```

Added a test to replicate and fix the issue.

* Extend tests

* Fixup tests and remove debugging export
  • Loading branch information
@kylorhall-atlassian
kylorhall-atlassian authored Oct 24, 2024
1 parent 7637444 commit 124243c
Show file tree
Hide file tree
Showing 4 changed files with 268 additions and 70 deletions.
5 changes: 5 additions & 0 deletions .changeset/heavy-actors-appear.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@compiled/css': patch
---

Fix sortShorthand when mixed with multi-property classes such as `._1jmq18uv{-webkit-text-decoration-color:initial;text-decoration-color:initial}` (previously, these broke sorting as they exited early).
128 changes: 121 additions & 7 deletions packages/css/src/plugins/__tests__/sort-shorthand-declarations.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,125 @@ describe('sort shorthand vs. longhand declarations', () => {
process.env.BROWSERSLIST = 'last 1 version';
});

it('sorts against multi-property classes: simple', () => {
// We need to make sure this `{-webkit-text-decoration-color:initial;text-decoration-color:initial}` doesn't "block" sorting
const actual = transform(outdent`
._zulph461{gap:3px}
._bfhk1ayf{background-color:red}
._1jmq18uv{-webkit-text-decoration-color:initial;text-decoration-color:initial}
._bfhk1ayf{background-color:red}
._y44v1tgl{background:red}
._kkk21kw7{all:inherit}
`);

expect(actual).toMatchInlineSnapshot(`
"
._kkk21kw7{all:inherit}._zulph461{gap:3px}
._y44v1tgl{background:red}
._bfhk1ayf{background-color:red}
._1jmq18uv{-webkit-text-decoration-color:initial;text-decoration-color:initial}
._bfhk1ayf{background-color:red}"
`);
});

it('sorts against multi-property classes fully', () => {
const actual = transform(outdent`
._zulph461{gap:var(--ds-space-050)}
._y44v1tgl{animation:kbayob8 5s ease infinite}
._j6xt1fef:hover{background:var(--_hcgrh3)}
._bfhk29zg, ._irr329zg:hover{background-color:var(--ds-background-selected,#deebff)}
._bfhk1ayf{background-color:var(--_rgmfhj)}
._irr312tn:hover{background-color:var(--_1hwiu8a)}
._syaz1qtk{color:var(--ds-link)}
._1jmq18uv:hover .linkText{-webkit-text-decoration-color:initial;text-decoration-color:initial}
._1owrotz2:active, ._jzfzotz2:focus, ._1mq6otz2:hover{padding:0 var(--ds-space-050,4px)}
._apju8stv:hover .linkText{-webkit-text-decoration-line:underline;text-decoration-line:underline}
@media (min-width:1024px){
._j6xt1fef:active{background:var(--_hcgrh3)}
._syaz1qtk{color:var(--ds-link)}
._1jmq18uv:hover .linkText{-webkit-text-decoration-color:initial;text-decoration-color:initial}
._1owrotz2:active, ._jzfzotz2:focus, ._1mq6otz2:hover{padding:0 var(--ds-space-050,4px)}
._apju8stv:hover .linkText{-webkit-text-decoration-line:underline;text-decoration-line:underline}
._kkk21kw7{all:inherit}
._ku3unqa1:hover .linkText{-webkit-text-decoration-style:solid;text-decoration-style:solid}
._18s8paju{margin:var(--ds-space-150,9pt) auto 28px auto}
._y44v1syn{animation:kbpspdk 5s ease infinite}
}
._ku3unqa1:hover .linkText{-webkit-text-decoration-style:solid;text-decoration-style:solid}
._18s8paju{margin:var(--ds-space-150,9pt) auto 28px auto}
._y44v1syn{animation:kbpspdk 5s ease infinite}
@keyframes kbpspdk{0%{stroke-dashoffset:10}}
._y44vbxa2{animation:k1qo9wnt 5s ease infinite}
@media (min-width:30rem){
._1letfkly{margin:0 var(--ds-space-400,2pc)}
._1wbfxncg{padding:var(--ds-space-800,4pc)}
._1tn2iyik{font:var(--ds-font-heading-xlarge,normal 600 29px/2pc ui-sans-serif,-apple-system,BlinkMacSystemFont,"Segoe UI",Ubuntu,system-ui,"Helvetica Neue",sans-serif)}
._l7hko0gd [data-ds--text-field--input]{font:var(--ds-font-heading-medium,normal 500 20px/24px ui-sans-serif,-apple-system,BlinkMacSystemFont,"Segoe UI",Ubuntu,system-ui,"Helvetica Neue",sans-serif)}._1odf1h6o{justify-content:center}
._j6xt1fef:active{background:var(--_hcgrh3)}
._syaz1qtk{color:var(--ds-link)}
._1jmq18uv:hover .linkText{-webkit-text-decoration-color:initial;text-decoration-color:initial}
._1owrotz2:active, ._jzfzotz2:focus, ._1mq6otz2:hover{padding:0 var(--ds-space-050,4px)}
._apju8stv:hover .linkText{-webkit-text-decoration-line:underline;text-decoration-line:underline}
._kkk21kw7{all:inherit}
._ku3unqa1:hover .linkText{-webkit-text-decoration-style:solid;text-decoration-style:solid}
._18s8paju{margin:var(--ds-space-150,9pt) auto 28px auto}
._y44v1syn{animation:kbpspdk 5s ease infinite}
._pwyo12x7{padding-inline-end:var(--ds-space-075,6px)}
._1b7p1epz{padding-left:var(--ds-space-1000,5pc)}
._jvmr1epz{padding-right:var(--ds-space-1000,5pc)}
}
._kkk21kw7{all:inherit}
`);

expect(actual).toMatchInlineSnapshot(`
"
._kkk21kw7{all:inherit}._zulph461{gap:var(--ds-space-050)}
._y44v1tgl{animation:kbayob8 5s ease infinite}
._18s8paju{margin:var(--ds-space-150,9pt) auto 28px auto}
._y44v1syn{animation:kbpspdk 5s ease infinite}
._y44vbxa2{animation:k1qo9wnt 5s ease infinite}
._bfhk29zg, ._irr329zg:hover{background-color:var(--ds-background-selected,#deebff)}
._bfhk1ayf{background-color:var(--_rgmfhj)}
._syaz1qtk{color:var(--ds-link)}
._1jmq18uv:hover .linkText{-webkit-text-decoration-color:initial;text-decoration-color:initial}
._apju8stv:hover .linkText{-webkit-text-decoration-line:underline;text-decoration-line:underline}
._ku3unqa1:hover .linkText{-webkit-text-decoration-style:solid;text-decoration-style:solid}
._j6xt1fef:hover{background:var(--_hcgrh3)}
._irr312tn:hover{background-color:var(--_1hwiu8a)}
._1owrotz2:active, ._jzfzotz2:focus, ._1mq6otz2:hover{padding:0 var(--ds-space-050,4px)}
@keyframes kbpspdk{0%{stroke-dashoffset:10}}
@media (min-width:30rem){
._kkk21kw7{all:inherit}
._1letfkly{margin:0 var(--ds-space-400,2pc)}
._1wbfxncg{padding:var(--ds-space-800,4pc)}
._1tn2iyik{font:var(--ds-font-heading-xlarge,normal 600 29px/2pc ui-sans-serif,-apple-system,BlinkMacSystemFont,"Segoe UI",Ubuntu,system-ui,"Helvetica Neue",sans-serif)}
._l7hko0gd [data-ds--text-field--input]{font:var(--ds-font-heading-medium,normal 500 20px/24px ui-sans-serif,-apple-system,BlinkMacSystemFont,"Segoe UI",Ubuntu,system-ui,"Helvetica Neue",sans-serif)}
._18s8paju{margin:var(--ds-space-150,9pt) auto 28px auto}
._y44v1syn{animation:kbpspdk 5s ease infinite}._1odf1h6o{justify-content:center}
._syaz1qtk{color:var(--ds-link)}
._1jmq18uv:hover .linkText{-webkit-text-decoration-color:initial;text-decoration-color:initial}
._apju8stv:hover .linkText{-webkit-text-decoration-line:underline;text-decoration-line:underline}
._ku3unqa1:hover .linkText{-webkit-text-decoration-style:solid;text-decoration-style:solid}
._pwyo12x7{padding-inline-end:var(--ds-space-075,6px)}
._1b7p1epz{padding-left:var(--ds-space-1000,5pc)}
._jvmr1epz{padding-right:var(--ds-space-1000,5pc)}
._j6xt1fef:active{background:var(--_hcgrh3)}
._1owrotz2:active, ._jzfzotz2:focus, ._1mq6otz2:hover{padding:0 var(--ds-space-050,4px)}
}
@media (min-width:1024px){
._kkk21kw7{all:inherit}
._18s8paju{margin:var(--ds-space-150,9pt) auto 28px auto}
._y44v1syn{animation:kbpspdk 5s ease infinite}
._syaz1qtk{color:var(--ds-link)}
._1jmq18uv:hover .linkText{-webkit-text-decoration-color:initial;text-decoration-color:initial}
._apju8stv:hover .linkText{-webkit-text-decoration-line:underline;text-decoration-line:underline}
._ku3unqa1:hover .linkText{-webkit-text-decoration-style:solid;text-decoration-style:solid}
._j6xt1fef:active{background:var(--_hcgrh3)}
._1owrotz2:active, ._jzfzotz2:focus, ._1mq6otz2:hover{padding:0 var(--ds-space-050,4px)}
}"
`);
});

it('leaves untouched when no crossover is present', () => {
const actual = transform(outdent`
.a {
Expand Down Expand Up @@ -374,13 +493,8 @@ describe('sort shorthand vs. longhand declarations', () => {
.b { all: unset; }
`);

// WARNING: This is technically wrong as `.a { … }` is not sorted as we expect;
// it _should_ be 'abcdef' not 'eabcdf'.
//
// We are ok with this because we expect atomicifyRules to run before this plugin,
// so each class will never have more than one property.
expect(actual).toMatchInlineSnapshot(`
".e { border-top: none; }
"
.a {
all: reset;
border: 2px dashed;
Expand All @@ -390,7 +504,7 @@ describe('sort shorthand vs. longhand declarations', () => {
}
.c { all: unset; }
.b { all: unset; }
.d { border: none; }
.d { border: none; }.e { border-top: none; }
.f { border-block-start-color: transparent; }"
`);
});
Expand Down
8 changes: 5 additions & 3 deletions packages/css/src/plugins/sort-shorthand-declarations.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,17 @@ const findDeclaration = (node: ChildNode): Declaration | undefined => {
}

if ('nodes' in node) {
if (node.nodes.length === 1 && nodeIsDeclaration(node.nodes[0])) {
return node.nodes[0];
}
// Return the first node that is a declaration, if we find one
return node.nodes.find(nodeIsDeclaration);
}

return undefined;
};

const sortNodes = (a: ChildNode, b: ChildNode): number => {
// NOTE: These return the first declaration when the class has multiple properties
// eg. `-webkit-text-decoration:initial;text-decoration:initial` would sort
// against `-webkit-text-decoration`, which may not be perfect in all cases
const aDecl = findDeclaration(a);
const bDecl = findDeclaration(b);

Expand Down
Loading

0 comments on commit 124243c

Please sign in to comment.