From 531ba81dbade67f9c355ff319281f6383a72182b Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Sat, 25 Jul 2026 16:26:54 +1000 Subject: [PATCH] Make shouldInheritFont inherit font size and line height (#268) The Capsize trim compound variants set fontSize and lineHeight, and vanilla-extract applies compound variants after simple ones, so they won over the shouldInheritFont variant's `inherit`. Family, weight and colour inherited; size and line height did not. The broken combination was the default one, because trim is only disabled automatically when lineClamp is set, and every existing test for the prop passed shouldDisableTrim, so the broken path had no coverage. Condition the trim compounds on shouldInheritFont: false. The fix sits in the recipe rather than the Text component because the recipe is exported publicly from @luke-ui/react/recipes, so a component-level fix would leave direct recipe consumers broken. Also validate two theme inputs the emitter trusted. The scrim was the only authored colour written into the stylesheet unchecked, because it is deliberately excluded from OKLCH parsing (its alpha channel does not fit the pattern) and nothing was put in its place, so a scrim carrying a semicolon or brace emitted malformed CSS into all four theme scopes. It now shares one isUnsafeCssValue helper with the depth and control finish checks. That helper tests the type before trimming, so a rung explicitly set to undefined produces the validator's message naming the field instead of a TypeError, and defineTheme filters undefined values before merging so an explicit undefined falls back to the curated default rather than overwriting it. Emitted theme CSS is unchanged: the v2 goldens and visual captures are byte-identical. --- .../react/src/recipes/text.browser.test.ts | 11 ++++++ .../@luke-ui/react/src/recipes/text.css.ts | 6 +++- .../react/src/theme/build-theme.test.ts | 34 +++++++++++++++++++ .../@luke-ui/react/src/theme/build-theme.ts | 20 +++++++++-- .../react/src/theme/define-theme.test.ts | 27 +++++++++++++++ .../@luke-ui/react/src/theme/define-theme.ts | 19 +++++++++-- 6 files changed, 112 insertions(+), 5 deletions(-) diff --git a/packages/@luke-ui/react/src/recipes/text.browser.test.ts b/packages/@luke-ui/react/src/recipes/text.browser.test.ts index 1eeff547..517354c2 100644 --- a/packages/@luke-ui/react/src/recipes/text.browser.test.ts +++ b/packages/@luke-ui/react/src/recipes/text.browser.test.ts @@ -91,6 +91,17 @@ test('font inheritance also preserves surrounding currentColor', () => { expect(style.fontWeight).toBe('500'); }); +test('shouldInheritFont alone inherits the ancestor font size and line height', () => { + const root = mountRoot(); + root.style.font = '18px / 22px serif'; + const element = root.appendChild(document.createElement('span')); + element.className = text({ shouldInheritFont: true }); + const style = getComputedStyle(element); + + expect(style.fontSize).toBe('18px'); + expect(style.lineHeight).toBe('22px'); +}); + test('an explicit semantic colour overrides inherited currentColor', () => { const root = mountRoot(); root.style.color = 'rgb(1, 2, 3)'; diff --git a/packages/@luke-ui/react/src/recipes/text.css.ts b/packages/@luke-ui/react/src/recipes/text.css.ts index d8eb883c..435254ef 100644 --- a/packages/@luke-ui/react/src/recipes/text.css.ts +++ b/packages/@luke-ui/react/src/recipes/text.css.ts @@ -115,7 +115,11 @@ const sizeStepCompoundVariants = fontSizeSteps.map((size) => { const { baselineTrim, capHeightTrim, fontSize, lineHeight } = vars.font[size]; return { style: createLayeredTextStyle({ baselineTrim, capHeightTrim, fontSize, lineHeight }), - variants: { shouldDisableTrim: false, size } as const, + // `shouldInheritFont: true` asks the browser to resolve font size and line height from + // the surrounding context, not this step's Capsize metrics. Without this condition, the + // compound's own `fontSize`/`lineHeight` always wins over the plain `shouldInheritFont` + // variant, because vanilla-extract applies compound variants after simple ones. + variants: { shouldDisableTrim: false, shouldInheritFont: false, size } as const, }; }); diff --git a/packages/@luke-ui/react/src/theme/build-theme.test.ts b/packages/@luke-ui/react/src/theme/build-theme.test.ts index 7c657d57..5b53fc8f 100644 --- a/packages/@luke-ui/react/src/theme/build-theme.test.ts +++ b/packages/@luke-ui/react/src/theme/build-theme.test.ts @@ -340,6 +340,40 @@ describe('buildTheme foundation validation', () => { 'dark.depth.overlay: must be a non-empty CSS box-shadow value', ); }); + + it('rejects an unsafe scrim value with a message naming the field', () => { + const unsafeScrim: ThemeFoundation = { + ...tactileFoundation, + light: { + ...tactileFoundation.light, + color: { ...tactileFoundation.light.color, scrim: 'oklch(0 0 0 / 0.2); } .evil {' }, + }, + name: 'unsafe-scrim', + }; + + expect(() => buildTheme(unsafeScrim)).toThrow( + 'light.color.scrim: must be a non-empty CSS colour value', + ); + }); + + it("produces the validator's message rather than a TypeError for a shadow rung set to undefined", () => { + // `defineTheme` now filters `undefined` rungs before merging (define-theme.test.ts covers that + // fallback), but `buildTheme` is also called directly with a raw foundation (tests, tooling, + // or any future composition that does not go through `defineTheme`). The validator's guard must + // stay robust to that shape regardless of caller, rather than crash inside `.trim()`. + const undefinedDepthRung = { + ...tactileFoundation, + dark: { + ...tactileFoundation.dark, + depth: { ...tactileFoundation.dark.depth, resting: undefined }, + }, + name: 'undefined-depth-rung', + } as unknown as ThemeFoundation; + + expect(() => buildTheme(undefinedDepthRung)).toThrow( + 'dark.depth.resting: must be a non-empty CSS box-shadow value', + ); + }); }); describe('buildTheme independent modes', () => { diff --git a/packages/@luke-ui/react/src/theme/build-theme.ts b/packages/@luke-ui/react/src/theme/build-theme.ts index 45aba635..c084d8d8 100644 --- a/packages/@luke-ui/react/src/theme/build-theme.ts +++ b/packages/@luke-ui/react/src/theme/build-theme.ts @@ -468,6 +468,19 @@ function validateContrast(mode: ColorMode, colorValues: SemanticColorValues): Va return { checks, failures }; } +/** + * Whether a value is unsafe to emit verbatim into the generated stylesheet: anything other than a + * non-empty string, or a string containing a statement-breaking character (`;`, `{`, `}`). Shared by + * every authored-but-unparsed CSS value — the depth box-shadow rungs, the action-control-finish + * background-images, and the scrim colour (deliberately excluded from OKLCH colour parsing because + * its alpha channel does not fit that pattern) — so the rule has one home. Checking `typeof value` + * rather than assuming a string keeps this guard correct even when a caller other than `defineTheme` + * hands `buildTheme` a foundation with a rung explicitly set to `undefined`. + */ +function isUnsafeCssValue(value: unknown): boolean { + return typeof value !== 'string' || value.trim() === '' || /[;{}]/.test(value); +} + function validateFoundation(foundation: ThemeFoundation): void { const issues: Array = []; try { @@ -486,13 +499,16 @@ function validateFoundation(foundation: ThemeFoundation): void { issues.push(`${mode}.color.${field}: ${errorMessage(error)}`); } } + if (isUnsafeCssValue(modeFoundation.color.scrim)) { + issues.push(`${mode}.color.scrim: must be a non-empty CSS colour value`); + } for (const [name, value] of Object.entries(modeFoundation.depth)) { - if (value.trim() === '' || /[;{}]/.test(value)) { + if (isUnsafeCssValue(value)) { issues.push(`${mode}.depth.${name}: must be a non-empty CSS box-shadow value`); } } for (const [name, value] of Object.entries(modeFoundation.actionControlFinish)) { - if (value.trim() === '' || /[;{}]/.test(value)) { + if (isUnsafeCssValue(value)) { issues.push( `${mode}.actionControlFinish.${name}: must be a non-empty CSS background-image value`, ); diff --git a/packages/@luke-ui/react/src/theme/define-theme.test.ts b/packages/@luke-ui/react/src/theme/define-theme.test.ts index b98ea81d..517d829d 100644 --- a/packages/@luke-ui/react/src/theme/define-theme.test.ts +++ b/packages/@luke-ui/react/src/theme/define-theme.test.ts @@ -121,6 +121,20 @@ describe('defineTheme partial per-mode merges', () => { expect(extractValue(blocks.mediaDark, '--luke-depth-resting')).toBe(defaultDepth.dark.resting); }); + it('falls back to the curated default for a rung explicitly authored as undefined', () => { + // Composed authoring naturally produces `{ resting: condition ? value : undefined }`. An + // explicit `undefined` must behave exactly like an omitted rung, not overwrite the default with + // `undefined` (which previously crashed the validator's `.trim()` guard). + const blocks = splitBlocks( + defineTheme({ + color: { accent: '#3b82f6' }, + depth: { light: { resting: undefined } }, + name: 'undefined-depth-rung', + }), + ); + expect(extractValue(blocks.baseLight, '--luke-depth-resting')).toBe(defaultDepth.light.resting); + }); + it('defaults the omitted dark side of a partial colour without bleeding the light override', () => { const infoVarNames = [ '--luke-color-intent-info-text', @@ -148,6 +162,19 @@ describe('defineTheme partial per-mode merges', () => { }); }); +describe('defineTheme scrim validation', () => { + it('rejects an unsafe authored scrim value with a message naming the field', () => { + // The scrim is deliberately excluded from OKLCH colour parsing (its alpha channel does not fit + // that pattern) and emitted verbatim, so it needs its own shape check rather than none at all. + expect(() => + defineTheme({ + color: { accent: '#3b82f6', scrim: 'oklch(0 0 0 / 0.2); } .evil {' }, + name: 'unsafe-scrim', + }), + ).toThrow('color.scrim: must be a non-empty CSS colour value'); + }); +}); + describe('normalizeTheme resolves the source-tier `background` split from `neutral`', () => { it('omitted background resolves to the resolved neutral canvas anchor in both modes', () => { const foundation = normalizeTheme({ diff --git a/packages/@luke-ui/react/src/theme/define-theme.ts b/packages/@luke-ui/react/src/theme/define-theme.ts index 42e54cd0..e4823cb3 100644 --- a/packages/@luke-ui/react/src/theme/define-theme.ts +++ b/packages/@luke-ui/react/src/theme/define-theme.ts @@ -228,12 +228,27 @@ export function normalizeTheme(input: ThemeInput): ThemeFoundation { /** Resolves one mode's source colours and materials. */ function buildModeFoundation(input: ThemeInput, mode: ColorMode): ThemeModeFoundation { return { - actionControlFinish: { ...defaultControlFinish, ...(input.actionControlFinish?.[mode] ?? {}) }, + actionControlFinish: { + ...defaultControlFinish, + ...omitUndefined(input.actionControlFinish?.[mode] ?? {}), + }, color: resolveColors(input, mode), - depth: { ...defaultDepth[mode], ...(input.depth?.[mode] ?? {}) }, + depth: { ...defaultDepth[mode], ...omitUndefined(input.depth?.[mode] ?? {}) }, }; } +/** + * Drops keys whose value is explicitly `undefined`. Composed authoring naturally produces objects + * like `{ resting: someCondition ? value : undefined }`, and an object spread keeps an + * explicitly-`undefined` key, which would otherwise overwrite (rather than fall back to) the curated + * default it is merged over. + */ +function omitUndefined>(record: T): Partial { + return Object.fromEntries( + Object.entries(record).filter(([, value]) => value !== undefined), + ) as Partial; +} + /** Resolves every source-colour role for one mode into the strings `buildTheme` accepts. */ function resolveColors(input: ThemeInput, mode: ColorMode): ThemeSourceColors { const { color } = input; -- 2.51.2