From 7cc2d89073e41adaf9de475a996cfb356cab404c Mon Sep 17 00:00:00 2001 From: Jordan Pittman Date: Thu, 31 Oct 2024 10:39:56 -0400 Subject: [PATCH] Fix non-deterministic variant sorting (#14835) Right now our variant sorting is sensitive to the authoring order of classes or the order in which we scan files because we weren't comparing variant values when sorting. This PR addresses this by: - "default" values in variants appear first - Then any named values - Then any arbitrary values - Finally, compare the values themselves when all else is equal --------- Co-authored-by: Robin Malfait Co-authored-by: Adam Wathan <4323180+adamwathan@users.noreply.github.com> Co-authored-by: Adam Wathan --- CHANGELOG.md | 1 + .../tailwindcss/src/compat/config.test.ts | 12 +- packages/tailwindcss/src/compat/plugin-api.ts | 26 +- packages/tailwindcss/src/variants.test.ts | 295 +++++++++++++----- packages/tailwindcss/src/variants.ts | 53 +++- 5 files changed, 276 insertions(+), 111 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b2e53387f..87ee79815 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Detect classes in new files when using `@tailwindcss/postcss` ([#14829](https://github.com/tailwindlabs/tailwindcss/pull/14829)) - Fix crash when using `@source` containing `..` ([#14831](https://github.com/tailwindlabs/tailwindcss/pull/14831)) +- Ensure instances of the same variant with different values are always sorted deterministically (e.g. `data-focus:flex` and `data-active:flex`) ([#14835](https://github.com/tailwindlabs/tailwindcss/pull/14835)) - _Upgrade (experimental)_: Install `@tailwindcss/postcss` next to `tailwindcss` ([#14830](https://github.com/tailwindlabs/tailwindcss/pull/14830)) - _Upgrade (experimental)_: Remove whitespace around `,` separator when print arbitrary values ([#14838](https://github.com/tailwindlabs/tailwindcss/pull/14838)) diff --git a/packages/tailwindcss/src/compat/config.test.ts b/packages/tailwindcss/src/compat/config.test.ts index 2bc147383..6f1e5d156 100644 --- a/packages/tailwindcss/src/compat/config.test.ts +++ b/packages/tailwindcss/src/compat/config.test.ts @@ -1109,16 +1109,16 @@ test('creates variants for `data`, `supports`, and `aria` theme options at the s 'print:flex', ]), ).toMatchInlineSnapshot(` - ".aria-polite\\:underline { - &[aria-live="polite"] { - text-decoration-line: underline; - } - } - .aria-hidden\\:flex { + ".aria-hidden\\:flex { &[aria-hidden="true"] { display: flex; } } + .aria-polite\\:underline { + &[aria-live="polite"] { + text-decoration-line: underline; + } + } .data-checked\\:underline { &[data-ui~="checked"] { text-decoration-line: underline; diff --git a/packages/tailwindcss/src/compat/plugin-api.ts b/packages/tailwindcss/src/compat/plugin-api.ts index fd3782241..04fb8d43a 100644 --- a/packages/tailwindcss/src/compat/plugin-api.ts +++ b/packages/tailwindcss/src/compat/plugin-api.ts @@ -156,12 +156,12 @@ export function buildPluginApi( if (a.kind !== 'functional' || z.kind !== 'functional') { return 0 } - if (!a.value || !z.value) { - return 0 - } - let aValue = options?.values?.[a.value.value] ?? a.value.value - let zValue = options?.values?.[z.value.value] ?? z.value.value + let aValueKey = a.value ? a.value.value : 'DEFAULT' + let zValueKey = z.value ? z.value.value : 'DEFAULT' + + let aValue = options?.values?.[aValueKey] ?? aValueKey + let zValue = options?.values?.[zValueKey] ?? zValueKey if (options && typeof options.sort === 'function') { return options.sort( @@ -170,11 +170,19 @@ export function buildPluginApi( ) } - let aOrder = defaultOptionKeys.indexOf(a.value.value) - let zOrder = defaultOptionKeys.indexOf(z.value.value) + let aOrder = defaultOptionKeys.indexOf(aValueKey) + let zOrder = defaultOptionKeys.indexOf(zValueKey) - if (aOrder - zOrder === 0) return aValue < zValue ? -1 : 1 - return aOrder - zOrder + // Sort arbitrary values after configured values + aOrder = aOrder === -1 ? defaultOptionKeys.length : aOrder + zOrder = zOrder === -1 ? defaultOptionKeys.length : zOrder + + if (aOrder !== zOrder) return aOrder - zOrder + + // SAFETY: The values don't need to be checked for equality as they + // are guaranteed to be unique since we sort a list of de-duped + // variants and different (valid) variants cannot produce the same AST. + return aValue < zValue ? -1 : 1 }, ) }, diff --git a/packages/tailwindcss/src/variants.test.ts b/packages/tailwindcss/src/variants.test.ts index e154255d8..d34966506 100644 --- a/packages/tailwindcss/src/variants.test.ts +++ b/packages/tailwindcss/src/variants.test.ts @@ -1,4 +1,6 @@ +import dedent from 'dedent' import { expect, test } from 'vitest' +import createPlugin from './plugin' import { compileCss, run } from './test-utils/run' import { Compounds, compoundsForSelectors } from './variants' @@ -1623,14 +1625,20 @@ test('supports', async () => { } } - @supports (display: grid) { - .supports-\\[display\\:grid\\]\\:flex { + @supports (display: grid) and font-format(opentype) { + .supports-\\[\\(display\\:grid\\)_and_font-format\\(opentype\\)\\]\\:grid { + display: grid; + } + } + + @supports (--test: var(--tw)) { + .supports-\\[--test\\]\\:flex { display: flex; } } - @supports selector(A > B) { - .supports-\\[selector\\(A_\\>_B\\)\\]\\:flex { + @supports (display: grid) { + .supports-\\[display\\:grid\\]\\:flex { display: flex; } } @@ -1641,26 +1649,20 @@ test('supports', async () => { } } - @supports (display: grid) and font-format(opentype) { - .supports-\\[\\(display\\:grid\\)_and_font-format\\(opentype\\)\\]\\:grid { - display: grid; - } - } - @supports font-tech(color-COLRv1) { .supports-\\[font-tech\\(color-COLRv1\\)\\]\\:flex { display: flex; } } - @supports var(--test) { - .supports-\\[var\\(--test\\)\\]\\:flex { + @supports selector(A > B) { + .supports-\\[selector\\(A_\\>_B\\)\\]\\:flex { display: flex; } } - @supports (--test: var(--tw)) { - .supports-\\[--test\\]\\:flex { + @supports var(--test) { + .supports-\\[var\\(--test\\)\\]\\:flex { display: flex; } }" @@ -2399,24 +2401,12 @@ test('aria', async () => { 'peer-aria-[modal]:flex', 'peer-aria-checked:flex', 'peer-aria-[valuenow=1]:flex', - 'peer-aria-[modal]/parent-name:flex', - 'peer-aria-checked/parent-name:flex', - 'peer-aria-[valuenow=1]/parent-name:flex', + 'peer-aria-[modal]/sibling-name:flex', + 'peer-aria-checked/sibling-name:flex', + 'peer-aria-[valuenow=1]/sibling-name:flex', ]), ).toMatchInlineSnapshot(` - ".group-aria-\\[modal\\]\\:flex:is(:where(.group)[aria-modal] *) { - display: flex; - } - - .group-aria-checked\\:flex:is(:where(.group)[aria-checked="true"] *) { - display: flex; - } - - .group-aria-\\[valuenow\\=1\\]\\:flex:is(:where(.group)[aria-valuenow="1"] *) { - display: flex; - } - - .group-aria-\\[modal\\]\\/parent-name\\:flex:is(:where(.group\\/parent-name)[aria-modal] *) { + ".group-aria-checked\\:flex:is(:where(.group)[aria-checked="true"] *) { display: flex; } @@ -2424,11 +2414,19 @@ test('aria', async () => { display: flex; } - .group-aria-\\[valuenow\\=1\\]\\/parent-name\\:flex:is(:where(.group\\/parent-name)[aria-valuenow="1"] *) { + .group-aria-\\[modal\\]\\:flex:is(:where(.group)[aria-modal] *) { display: flex; } - .peer-aria-\\[modal\\]\\:flex:is(:where(.peer)[aria-modal] ~ *) { + .group-aria-\\[modal\\]\\/parent-name\\:flex:is(:where(.group\\/parent-name)[aria-modal] *) { + display: flex; + } + + .group-aria-\\[valuenow\\=1\\]\\:flex:is(:where(.group)[aria-valuenow="1"] *) { + display: flex; + } + + .group-aria-\\[valuenow\\=1\\]\\/parent-name\\:flex:is(:where(.group\\/parent-name)[aria-valuenow="1"] *) { display: flex; } @@ -2436,19 +2434,23 @@ test('aria', async () => { display: flex; } + .peer-aria-checked\\/sibling-name\\:flex:is(:where(.peer\\/sibling-name)[aria-checked="true"] ~ *) { + display: flex; + } + + .peer-aria-\\[modal\\]\\:flex:is(:where(.peer)[aria-modal] ~ *) { + display: flex; + } + + .peer-aria-\\[modal\\]\\/sibling-name\\:flex:is(:where(.peer\\/sibling-name)[aria-modal] ~ *) { + display: flex; + } + .peer-aria-\\[valuenow\\=1\\]\\:flex:is(:where(.peer)[aria-valuenow="1"] ~ *) { display: flex; } - .peer-aria-\\[modal\\]\\/parent-name\\:flex:is(:where(.peer\\/parent-name)[aria-modal] ~ *) { - display: flex; - } - - .peer-aria-checked\\/parent-name\\:flex:is(:where(.peer\\/parent-name)[aria-checked="true"] ~ *) { - display: flex; - } - - .peer-aria-\\[valuenow\\=1\\]\\/parent-name\\:flex:is(:where(.peer\\/parent-name)[aria-valuenow="1"] ~ *) { + .peer-aria-\\[valuenow\\=1\\]\\/sibling-name\\:flex:is(:where(.peer\\/sibling-name)[aria-valuenow="1"] ~ *) { display: flex; } @@ -2460,11 +2462,11 @@ test('aria', async () => { display: flex; } - .aria-\\[valuenow\\=1\\]\\:flex[aria-valuenow="1"] { + .aria-\\[valuenow_\\=_\\"1\\"\\]\\:flex[aria-valuenow="1"] { display: flex; } - .aria-\\[valuenow_\\=_\\"1\\"\\]\\:flex[aria-valuenow="1"] { + .aria-\\[valuenow\\=1\\]\\:flex[aria-valuenow="1"] { display: flex; }" `) @@ -2493,12 +2495,12 @@ test('data', async () => { 'group-data-[foo$=bar_baz_i]/parent-name:flex', 'peer-data-[disabled]:flex', - 'peer-data-[disabled]/parent-name:flex', + 'peer-data-[disabled]/sibling-name:flex', 'peer-data-[foo=1]:flex', - 'peer-data-[foo=1]/parent-name:flex', - 'peer-data-[foo=bar baz]/parent-name:flex', - "peer-data-[foo$='bar'_i]/parent-name:flex", - 'peer-data-[foo$=bar_baz_i]/parent-name:flex', + 'peer-data-[foo=1]/sibling-name:flex', + 'peer-data-[foo=bar baz]/sibling-name:flex', + "peer-data-[foo$='bar'_i]/sibling-name:flex", + 'peer-data-[foo$=bar_baz_i]/sibling-name:flex', ]), ).toMatchInlineSnapshot(` ".group-data-\\[disabled\\]\\:flex:is(:where(.group)[data-disabled] *) { @@ -2509,6 +2511,14 @@ test('data', async () => { display: flex; } + .group-data-\\[foo\\$\\=\\'bar\\'_i\\]\\/parent-name\\:flex:is(:where(.group\\/parent-name)[data-foo$="bar" i] *) { + display: flex; + } + + .group-data-\\[foo\\$\\=bar_baz_i\\]\\/parent-name\\:flex:is(:where(.group\\/parent-name)[data-foo$="bar baz" i] *) { + display: flex; + } + .group-data-\\[foo\\=1\\]\\:flex:is(:where(.group)[data-foo="1"] *) { display: flex; } @@ -2521,19 +2531,19 @@ test('data', async () => { display: flex; } - .group-data-\\[foo\\$\\=\\'bar\\'_i\\]\\/parent-name\\:flex:is(:where(.group\\/parent-name)[data-foo$="bar" i] *) { - display: flex; - } - - .group-data-\\[foo\\$\\=bar_baz_i\\]\\/parent-name\\:flex:is(:where(.group\\/parent-name)[data-foo$="bar baz" i] *) { - display: flex; - } - .peer-data-\\[disabled\\]\\:flex:is(:where(.peer)[data-disabled] ~ *) { display: flex; } - .peer-data-\\[disabled\\]\\/parent-name\\:flex:is(:where(.peer\\/parent-name)[data-disabled] ~ *) { + .peer-data-\\[disabled\\]\\/sibling-name\\:flex:is(:where(.peer\\/sibling-name)[data-disabled] ~ *) { + display: flex; + } + + .peer-data-\\[foo\\$\\=\\'bar\\'_i\\]\\/sibling-name\\:flex:is(:where(.peer\\/sibling-name)[data-foo$="bar" i] ~ *) { + display: flex; + } + + .peer-data-\\[foo\\$\\=bar_baz_i\\]\\/sibling-name\\:flex:is(:where(.peer\\/sibling-name)[data-foo$="bar baz" i] ~ *) { display: flex; } @@ -2541,19 +2551,11 @@ test('data', async () => { display: flex; } - .peer-data-\\[foo\\=1\\]\\/parent-name\\:flex:is(:where(.peer\\/parent-name)[data-foo="1"] ~ *) { + .peer-data-\\[foo\\=1\\]\\/sibling-name\\:flex:is(:where(.peer\\/sibling-name)[data-foo="1"] ~ *) { display: flex; } - .peer-data-\\[foo\\=bar\\ baz\\]\\/parent-name\\:flex:is(:where(.peer\\/parent-name)[data-foo="bar baz"] ~ *) { - display: flex; - } - - .peer-data-\\[foo\\$\\=\\'bar\\'_i\\]\\/parent-name\\:flex:is(:where(.peer\\/parent-name)[data-foo$="bar" i] ~ *) { - display: flex; - } - - .peer-data-\\[foo\\$\\=bar_baz_i\\]\\/parent-name\\:flex:is(:where(.peer\\/parent-name)[data-foo$="bar baz" i] ~ *) { + .peer-data-\\[foo\\=bar\\ baz\\]\\/sibling-name\\:flex:is(:where(.peer\\/sibling-name)[data-foo="bar baz"] ~ *) { display: flex; } @@ -2561,7 +2563,19 @@ test('data', async () => { display: flex; } - .data-\\[potato\\=salad\\]\\:flex[data-potato="salad"] { + .data-\\[foo\\$\\=\\'bar\\'_i\\]\\:flex[data-foo$="bar" i] { + display: flex; + } + + .data-\\[foo\\$\\=bar_baz_i\\]\\:flex[data-foo$="bar baz" i] { + display: flex; + } + + .data-\\[foo\\=1\\]\\:flex[data-foo="1"] { + display: flex; + } + + .data-\\[foo\\=bar_baz\\]\\:flex[data-foo="bar baz"] { display: flex; } @@ -2577,19 +2591,7 @@ test('data', async () => { display: flex; } - .data-\\[foo\\=1\\]\\:flex[data-foo="1"] { - display: flex; - } - - .data-\\[foo\\=bar_baz\\]\\:flex[data-foo="bar baz"] { - display: flex; - } - - .data-\\[foo\\$\\=\\'bar\\'_i\\]\\:flex[data-foo$="bar" i] { - display: flex; - } - - .data-\\[foo\\$\\=bar_baz_i\\]\\:flex[data-foo$="bar baz" i] { + .data-\\[potato\\=salad\\]\\:flex[data-potato="salad"] { display: flex; }" `) @@ -2881,6 +2883,7 @@ test('variant order', async () => { 'contrast-less:flex', 'contrast-more:flex', 'dark:flex', + 'data-custom:flex', 'data-[custom=true]:flex', 'default:flex', 'disabled:flex', @@ -3123,10 +3126,6 @@ test('variant order', async () => { display: flex; } - .aria-\\[custom\\=true\\]\\:flex[aria-custom="true"] { - display: flex; - } - .aria-busy\\:flex[aria-busy="true"] { display: flex; } @@ -3163,6 +3162,14 @@ test('variant order', async () => { display: flex; } + .aria-\\[custom\\=true\\]\\:flex[aria-custom="true"] { + display: flex; + } + + .data-custom\\:flex[data-custom] { + display: flex; + } + .data-\\[custom\\=true\\]\\:flex[data-custom="true"] { display: flex; } @@ -3285,6 +3292,124 @@ test('variant order', async () => { `) }) +test('variants with the same root are sorted deterministically', async () => { + function permute(arr: string[]): string[][] { + if (arr.length <= 1) return [arr] + + return arr.flatMap((item, i) => + permute([...arr.slice(0, i), ...arr.slice(i + 1)]).map((permutation) => [ + item, + ...permutation, + ]), + ) + } + + let classLists = permute([ + 'data-hover:flex', + 'data-focus:flex', + 'data-active:flex', + 'data-[foo]:flex', + 'data-[bar]:flex', + 'data-[baz]:flex', + ]) + + for (let classList of classLists) { + let output = await compileCss('@tailwind utilities;', classList) + + expect(output.trim()).toEqual( + dedent(css` + .data-active\:flex[data-active] { + display: flex; + } + + .data-focus\:flex[data-focus] { + display: flex; + } + + .data-hover\:flex[data-hover] { + display: flex; + } + + .data-\[bar\]\:flex[data-bar] { + display: flex; + } + + .data-\[baz\]\:flex[data-baz] { + display: flex; + } + + .data-\[foo\]\:flex[data-foo] { + display: flex; + } + `), + ) + } +}) + +test('matchVariant sorts deterministically', async () => { + function permute(arr: string[]): string[][] { + if (arr.length <= 1) return [arr] + + return arr.flatMap((item, i) => + permute([...arr.slice(0, i), ...arr.slice(i + 1)]).map((permutation) => [ + item, + ...permutation, + ]), + ) + } + + let classLists = permute([ + 'is-data:flex', + 'is-data-foo:flex', + 'is-data-bar:flex', + 'is-data-[potato]:flex', + 'is-data-[sandwich]:flex', + ]) + + for (let classList of classLists) { + let output = await compileCss('@tailwind utilities; @plugin "./plugin.js";', classList, { + loadModule(id: string) { + return { + base: '/', + module: createPlugin(({ matchVariant }) => { + matchVariant('is-data', (value) => `&:is([data-${value}])`, { + values: { + DEFAULT: 'default', + foo: 'foo', + bar: 'bar', + }, + }) + }), + } + }, + }) + + expect(output.trim()).toEqual( + dedent(css` + .is-data\:flex[data-default] { + display: flex; + } + + .is-data-foo\:flex[data-foo] { + display: flex; + } + + .is-data-bar\:flex[data-bar] { + display: flex; + } + + .is-data-\[potato\]\:flex[data-potato] { + display: flex; + } + + .is-data-\[sandwich\]\:flex[data-sandwich] { + display: flex; + } + `), + ) + } +}) + test.each([ // These are style rules [['.foo'], Compounds.StyleRules], diff --git a/packages/tailwindcss/src/variants.ts b/packages/tailwindcss/src/variants.ts index cdebac3c1..d51569aae 100644 --- a/packages/tailwindcss/src/variants.ts +++ b/packages/tailwindcss/src/variants.ts @@ -198,6 +198,8 @@ export class Variants { if (z === null) return 1 if (a.kind === 'arbitrary' && z.kind === 'arbitrary') { + // SAFETY: The selectors don't need to be checked for equality as they + // are guaranteed to be unique since we sort a list of de-duped variants return a.selector < z.selector ? -1 : 1 } else if (a.kind === 'arbitrary') { return 1 @@ -213,21 +215,50 @@ export class Variants { if (a.kind === 'compound' && z.kind === 'compound') { let order = this.compare(a.variant, z.variant) - if (order === 0) { - if (a.modifier && z.modifier) { - return a.modifier.value < z.modifier.value ? -1 : 1 - } else if (a.modifier) { - return 1 - } else if (z.modifier) { - return -1 - } + if (order !== 0) return order + + if (a.modifier && z.modifier) { + // SAFETY: The modifiers don't need to be checked for equality as they + // are guaranteed to be unique since we sort a list of de-duped variants + return a.modifier.value < z.modifier.value ? -1 : 1 + } else if (a.modifier) { + return 1 + } else if (z.modifier) { + return -1 + } else { + return 0 } - return order } let compareFn = this.compareFns.get(aOrder) - if (compareFn === undefined) return a.root < z.root ? -1 : 1 - return compareFn(a, z) + if (compareFn !== undefined) return compareFn(a, z) + + if (a.root !== z.root) return a.root < z.root ? -1 : 1 + + // SAFETY: Variants `a` and `z` are both functional at this point. Static + // variants are de-duped by the `DefaultMap` and checked earlier. + let aValue = (a as Extract).value + let zValue = (z as Extract).value + + // While no functional variant in core supports a "default" value the parser + // will see something like `data:flex` and still parse and store it as a + // functional variant even though it actually produces no CSS. This means + // that we need to handle the case where the value is `null` here. Even + // though _for valid utilities_ this never happens. + if (aValue === null) return -1 + if (zValue === null) return 1 + + // Variants with arbitrary values should appear after any with named values + if (aValue.kind === 'arbitrary' && zValue.kind !== 'arbitrary') return 1 + if (aValue.kind !== 'arbitrary' && zValue.kind === 'arbitrary') return -1 + + // SAFETY: The values don't need to be checked for equality as they are + // guaranteed to be unique since we sort a list of de-duped variants. The + // only way this could matter would be when two different variants parse to + // the same AST. That is only possible with arbitrary values when spaces are + // involved. e.g. `data-[a_b]:flex` and `data-[a ]:flex` but this is not a + // concern for us because spaces are not allowed in variant names. + return aValue.value < zValue.value ? -1 : 1 } keys() {