Skip to content
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

fix(css-vars): nullish v-bind in style should not lead to unexpected inheritance #12461

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -884,9 +884,9 @@ export default {

return (_ctx, _push, _parent, _attrs) => {
const _cssVars = { style: {
"--xxxxxxxx-count": (count.value),
"--xxxxxxxx-style\\\\.color": (style.color),
"--xxxxxxxx-height\\\\ \\\\+\\\\ \\\\\\"px\\\\\\"": (height.value + "px")
":--xxxxxxxx-count": (count.value),
":--xxxxxxxx-style\\\\.color": (style.color),
":--xxxxxxxx-height\\\\ \\\\+\\\\ \\\\\\"px\\\\\\"": (height.value + "px")
}}
_push(\`<!--[--><div\${
_ssrRenderAttrs(_cssVars)
Expand Down
6 changes: 3 additions & 3 deletions packages/compiler-sfc/__tests__/compileScript.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -646,10 +646,10 @@ describe('SFC compile <script setup>', () => {
expect(content).toMatch(`return (_ctx, _push`)
expect(content).toMatch(`ssrInterpolate`)
expect(content).not.toMatch(`useCssVars`)
expect(content).toMatch(`"--${mockId}-count": (count.value)`)
expect(content).toMatch(`"--${mockId}-style\\\\.color": (style.color)`)
expect(content).toMatch(`":--${mockId}-count": (count.value)`)
expect(content).toMatch(`":--${mockId}-style\\\\.color": (style.color)`)
expect(content).toMatch(
`"--${mockId}-height\\\\ \\\\+\\\\ \\\\\\"px\\\\\\"": (height.value + "px")`,
`":--${mockId}-height\\\\ \\\\+\\\\ \\\\\\"px\\\\\\"": (height.value + "px")`,
)
assertCode(content)
})
Expand Down
7 changes: 6 additions & 1 deletion packages/compiler-sfc/src/style/cssVars.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,12 @@ export function genCssVarsFromList(
return `{\n ${vars
.map(
key =>
`"${isSSR ? `--` : ``}${genVarName(id, key, isProd, isSSR)}": (${key})`,
// The `:` prefix here is used in `ssrRenderStyle` to distinguish whether
// a custom property comes from `ssrCssVars`. If it does, we need to reset
// its value to `initial` on the component instance to avoid unintentionally
// inheriting the same property value from a different instance of the same
// component in the outer scope.
`"${isSSR ? `:--` : ``}${genVarName(id, key, isProd, isSSR)}": (${key})`,
)
.join(',\n ')}\n}`
}
Expand Down
4 changes: 2 additions & 2 deletions packages/runtime-core/src/component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -582,13 +582,13 @@ export interface ComponentInternalInstance {
* For updating css vars on contained teleports
* @internal
*/
ut?: (vars?: Record<string, string>) => void
ut?: (vars?: Record<string, unknown>) => void

/**
* dev only. For style v-bind hydration mismatch checks
* @internal
*/
getCssVars?: () => Record<string, string>
getCssVars?: () => Record<string, unknown>

/**
* v2 compat only, for caching mutated $options
Expand Down
7 changes: 3 additions & 4 deletions packages/runtime-core/src/hydration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
isReservedProp,
isString,
normalizeClass,
normalizeCssVarValue,
normalizeStyle,
stringifyStyle,
} from '@vue/shared'
Expand Down Expand Up @@ -938,10 +939,8 @@ function resolveCssVars(
) {
const cssVars = instance.getCssVars()
for (const key in cssVars) {
expectedMap.set(
`--${getEscapedCssVarName(key, false)}`,
String(cssVars[key]),
)
const value = normalizeCssVarValue(cssVars[key])
expectedMap.set(`--${getEscapedCssVarName(key, false)}`, value)
}
}
if (vnode === root && instance.parent) {
Expand Down
23 changes: 23 additions & 0 deletions packages/runtime-dom/__tests__/helpers/useCssVars.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -465,4 +465,27 @@ describe('useCssVars', () => {
render(h(App), root)
expect(colorInOnMount).toBe(`red`)
})

test('should set vars as `initial` for nullish values', async () => {
// `getPropertyValue` cannot reflect the real value for white spaces and JSDOM also
// doesn't 100% reflect the real behavior of browsers, so we only keep the test for
// `initial` value here.
// The value normalization is tested in packages/shared/__tests__/cssVars.spec.ts.
const state = reactive<Record<string, unknown>>({
foo: undefined,
bar: null,
})
const root = document.createElement('div')
const App = {
setup() {
useCssVars(() => state)
return () => h('div')
},
}
render(h(App), root)
await nextTick()
const style = (root.children[0] as HTMLElement).style
expect(style.getPropertyValue('--foo')).toBe('initial')
expect(style.getPropertyValue('--bar')).toBe('initial')
})
})
15 changes: 9 additions & 6 deletions packages/runtime-dom/src/helpers/useCssVars.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,16 @@ import {
warn,
watch,
} from '@vue/runtime-core'
import { NOOP, ShapeFlags } from '@vue/shared'
import { NOOP, ShapeFlags, normalizeCssVarValue } from '@vue/shared'

export const CSS_VAR_TEXT: unique symbol = Symbol(__DEV__ ? 'CSS_VAR_TEXT' : '')
/**
* Runtime helper for SFC's CSS variable injection feature.
* @private
*/
export function useCssVars(getter: (ctx: any) => Record<string, string>): void {
export function useCssVars(
getter: (ctx: any) => Record<string, unknown>,
): void {
if (!__BROWSER__ && !__TEST__) return

const instance = getCurrentInstance()
Expand Down Expand Up @@ -64,7 +66,7 @@ export function useCssVars(getter: (ctx: any) => Record<string, string>): void {
})
}

function setVarsOnVNode(vnode: VNode, vars: Record<string, string>) {
function setVarsOnVNode(vnode: VNode, vars: Record<string, unknown>) {
if (__FEATURE_SUSPENSE__ && vnode.shapeFlag & ShapeFlags.SUSPENSE) {
const suspense = vnode.suspense!
vnode = suspense.activeBranch!
Expand Down Expand Up @@ -94,13 +96,14 @@ function setVarsOnVNode(vnode: VNode, vars: Record<string, string>) {
}
}

function setVarsOnNode(el: Node, vars: Record<string, string>) {
function setVarsOnNode(el: Node, vars: Record<string, unknown>) {
if (el.nodeType === 1) {
const style = (el as HTMLElement).style
let cssText = ''
for (const key in vars) {
style.setProperty(`--${key}`, vars[key])
cssText += `--${key}: ${vars[key]};`
const value = normalizeCssVarValue(vars[key])
style.setProperty(`--${key}`, value)
cssText += `--${key}: ${value};`
}
;(style as any)[CSS_VAR_TEXT] = cssText
}
Expand Down
15 changes: 15 additions & 0 deletions packages/server-renderer/__tests__/ssrRenderAttrs.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,4 +203,19 @@ describe('ssr: renderStyle', () => {
}),
).toBe(`color:&quot;&gt;&lt;script;`)
})

test('useCssVars handling', () => {
expect(
ssrRenderStyle({
fontSize: null,
':--v1': undefined,
':--v2': null,
':--v3': '',
':--v4': ' ',
':--v5': 'foo',
':--v6': 0,
'--foo': 1,
}),
).toBe(`--v1:initial;--v2:initial;--v3: ;--v4: ;--v5:foo;--v6:0;--foo:1;`)
})
})
21 changes: 20 additions & 1 deletion packages/server-renderer/src/helpers/ssrRenderAttrs.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import {
escapeHtml,
isArray,
isObject,
isRenderableAttrValue,
isSVGTag,
stringifyStyle,
Expand All @@ -12,6 +14,7 @@ import {
isString,
makeMap,
normalizeClass,
normalizeCssVarValue,
normalizeStyle,
propsToAttrMap,
} from '@vue/shared'
Expand Down Expand Up @@ -93,6 +96,22 @@ export function ssrRenderStyle(raw: unknown): string {
if (isString(raw)) {
return escapeHtml(raw)
}
const styles = normalizeStyle(raw)
const styles = normalizeStyle(ssrResetCssVars(raw))
return escapeHtml(stringifyStyle(styles))
}

function ssrResetCssVars(raw: unknown) {
if (!isArray(raw) && isObject(raw)) {
const res: Record<string, unknown> = {}
for (const key in raw) {
// `:` prefixed keys are coming from `ssrCssVars`
if (key.startsWith(':--')) {
res[key.slice(1)] = normalizeCssVarValue(raw[key])
} else {
res[key] = raw[key]
}
}
return res
}
return raw
}
27 changes: 27 additions & 0 deletions packages/shared/__tests__/cssVars.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { normalizeCssVarValue } from '../src'

describe('utils/cssVars', () => {
test('should normalize css binding values correctly', () => {
expect(normalizeCssVarValue(null)).toBe('initial')
expect(normalizeCssVarValue(undefined)).toBe('initial')
expect(normalizeCssVarValue('')).toBe(' ')
expect(normalizeCssVarValue(' ')).toBe(' ')
expect(normalizeCssVarValue('foo')).toBe('foo')
expect(normalizeCssVarValue(0)).toBe('0')
})

test('should warn on invalid css binding values', () => {
const warning =
'[Vue warn] Invalid value used for CSS binding. Expected a string or a finite number but received:'
expect(normalizeCssVarValue(NaN)).toBe('NaN')
expect(warning).toHaveBeenWarnedTimes(1)
expect(normalizeCssVarValue(Infinity)).toBe('Infinity')
expect(warning).toHaveBeenWarnedTimes(2)
expect(normalizeCssVarValue(-Infinity)).toBe('-Infinity')
expect(warning).toHaveBeenWarnedTimes(3)
expect(normalizeCssVarValue({})).toBe('[object Object]')
expect(warning).toHaveBeenWarnedTimes(4)
expect(normalizeCssVarValue([])).toBe('')
expect(warning).toHaveBeenWarnedTimes(5)
})
})
24 changes: 24 additions & 0 deletions packages/shared/src/cssVars.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* Normalize CSS var value created by `v-bind` in `<style>` block
* See https://github.com/vuejs/core/pull/12461#issuecomment-2495804664
*/
export function normalizeCssVarValue(value: unknown): string {
if (value == null) {
return 'initial'
}

if (typeof value === 'string') {
return value === '' ? ' ' : value
}

if (typeof value !== 'number' || !Number.isFinite(value)) {
Copy link
Member Author

@Justineo Justineo Nov 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewers: Apart from the proposed solution at #12461 (comment), I believe it is reasonable to allow finite numbers to be bound to CSS properties. For other data types, I couldn’t determine any clear expected behavior so a development-time warning is added.

if (__DEV__) {
console.warn(
'[Vue warn] Invalid value used for CSS binding. Expected a string or a finite number but received:',
value,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: The value is not converted to a string in the warning message because it may include empty arrays, and excessive effort to stringify the value is unnecessary in this context.

)
}
}

return String(value)
}
1 change: 1 addition & 0 deletions packages/shared/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@ export * from './escapeHtml'
export * from './looseEqual'
export * from './toDisplayString'
export * from './typeUtils'
export * from './cssVars'