fix(css-vars): nullish v-bind in style should not lead to unexpected inheritance #12461
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
edison1105 added ready to merge 
| I think this would also close #7474 and the associated PR #7475. There's an underlying question here about whether we consider Then there's a follow up decision about where to draw the line. What about an empty string (Playground)? Or a white-space value like I don't want to expand the scope of this PR unnecessarily, but I do think it's important to be clear why we're drawing the line at nullish values. |
| @skirtles-code Thanks for the feedback! Great questions! Here’s what I thought:
The situation is more complicated than I expected. I'm investigating the current behavior of Vue and CSS custom properties. Current behavior:
For Vue, For CSS custom properties:
Since we are binding values to CSS, we should align with CSS behavior, where an empty string is treated as equivalent to white spaces. To fix the SSR/CSR mismatch and ensure consistent resetting, I propose handling values as follows:
For other string values, we should output them as-is. For unsupported types of values, we should warn about the binding and retain the current behavior (rendering everything as a string, likely using WDYT? /cc @adamdehaven @skirtles-code @edison1105 |
| @Justineo I thought we determined that a whitespace character would be ideal? |
| After rereading the spec, I believe that only the |
The white space character seems to work in practice if you can try? |
White space sets a valid empty value, which will prevent fallback value to take effect. |
I'm not sure injecting In my testing, See the example taken from here
|
It is the standard way to reset a custom property.
https://codepen.io/Justineo/pen/VYZZPOz As shown in this pen,
That’s why we should use
We certainly don’t want to invalidate the fallback value |
| The reproduction looks correct; thanks for putting that together |
There was a problem hiding this comment.
Copilot reviewed 5 out of 9 changed files in this pull request and generated no suggestions.
Files not reviewed (4)
- packages/compiler-sfc/tests/snapshots/compileScript.spec.ts.snap: Language not supported
- packages/runtime-core/src/component.ts: Evaluated as low risk
- packages/compiler-sfc/tests/compileScript.spec.ts: Evaluated as low risk
- packages/runtime-dom/src/helpers/useCssVars.ts: Evaluated as low risk
| TLDR: I think the latest proposal looks good. I wasn't initially clear about the difference between
Using I also like the idea of treating an empty string as a space. That prevents the variable being inherited from the parent component and also seems consistent with vanilla CSS. It wasn't immediately clear to me why a space character would be better than In that example, we would want |
| @skirtles-code Thank you for the demos! I’ll update the code to align with our latest conclusions. |
| return value === '' ? ' ' : value | ||
| } | ||
| | ||
| if (typeof value !== 'number' || !Number.isFinite(value)) { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
Doesn't seem to be related to this PR. |
| @edison1105 is there anything else myself or @Justineo can provide here to help this PR along? I have a workaround in my project in a draft PR; however, it's definitely "messy" requiring me to manually inject |
| @adamdehaven |
edison1105 added the 🔨 p3-minor-bug 
| @yyx990803 Please review this PR. |
WalkthroughThis change introduces a new utility, Changes
Sequence Diagram(s)sequenceDiagram participant Component participant useCssVars participant normalizeCssVarValue participant DOM Component->>useCssVars: Provide state for CSS vars useCssVars->>normalizeCssVarValue: Normalize each var value normalizeCssVarValue-->>useCssVars: Return normalized value useCssVars->>DOM: Set style property with normalized value sequenceDiagram participant SSRCompiler participant ssrRenderStyle participant normalizeCssVarValue participant OutputHTML SSRCompiler->>ssrRenderStyle: Pass style object with :-- keys ssrRenderStyle->>normalizeCssVarValue: Normalize :-- var values normalizeCssVarValue-->>ssrRenderStyle: Return normalized value ssrRenderStyle->>OutputHTML: Output style attribute with normalized vars Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
| /ecosystem-ci run |
| 📝 Ran ecosystem CI: Open
|
v-bind()should not add custom properties to the component's style attribute when their value isundefined. #7474Currently, we generate CSS custom properties on component roots for each
v-bindin SFC<style>blocks. When one component instance is nested inside another, the nested instance can inherit values from the outer instance, even if its own binding value isundefinedornull. Ideally, style bindings should be scoped per component instance since they depend on the instance's state. However, with the current approach using CSS custom properties, we can't generate unique hashed names for each instance because CSS can only reference a single property.To address this, the improvement here is to "reset" these custom properties when the binding value is nullish.
Current Behavior:
--<hash>-<name>:undefinedforundefinedbut ignorenull, which causes inconsistencies and is related to the hydration warning in SSR hydration errors when combining inline styles with v-bind() in CSS #12439.This PR:
--<hash>-<name>:initialfor nullish values. This properly resets the custom property, as explained in the CSS spec. It behaves as if no custom property is defined in ancestor elements.SSR Note:
:prefix to distinguish style entries coming fromv-bindin styles. This allows us to process them separately from user-defined custom properties inssrRenderStyle. Alternatively, we can also create a new runtime helper and modify the codegen here:Not sure if my current approach is preferred. Let me know if you have other opinions.
Summary by CodeRabbit
New Features
null,undefined, or invalid values.Bug Fixes
nullorundefinedvalues now default to'initial'to prevent unintended inheritance.Refactor
Documentation
Chores