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

undefined and false in css variables in inline styles are not removed #5322

Closed
hit12369 opened this issue Jan 25, 2022 · 6 comments · Fixed by #5348
Closed

undefined and false in css variables in inline styles are not removed #5322

hit12369 opened this issue Jan 25, 2022 · 6 comments · Fixed by #5348
Assignees
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. 🐞 bug Something isn't working

Comments

@hit12369
Copy link

hit12369 commented Jan 25, 2022

Version

3.2.29

Reproduction link

sfc.vuejs.org/

Steps to reproduce

Use :style="{'--var': undefined}" or :style="{'--var': false}" in a template.

It's rendered as style="--var: undefined" or style="--var: false" which seems incorrect, --var should be removed instead. Using null works correctly and --var is removed.

What is expected?

Inline css variable should be removed.

What is actually happening?

It's actually being rendered as undefined.

@NaincyKumariKnoldus
Copy link

Hey @hit12369 Can I do this please?

@posva posva added 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. has workaround A workaround has been found to avoid the problem and removed has workaround A workaround has been found to avoid the problem 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. labels Jan 27, 2022
@posva
Copy link
Member

posva commented Jan 27, 2022

This seems to be just how CSS variables work:

$0.style.setProperty('--color', undefined) // sets the string undefined

@posva posva closed this as completed Jan 27, 2022
@hit12369
Copy link
Author

hit12369 commented Jan 27, 2022

This seems to be just how CSS variables work:

$0.style.setProperty('--color', undefined) // sets the string undefined

While it is true, setting it as undefined removes the property in vue 2.

vue2 example: https://jsfiddle.net/zb1x79m0/

Also it would ensure consistency with non variable css properties.

$0.style.color = 'red'
$0.style.color = undefined // color does not change, but if we use :style="{color: undefined}" in vue, color gets removed

Example playground link: sfc.vuejs.org

@LinusBorg
Copy link
Member

Seems we indeed have an inconsistency here. the code responsible for remoiving style properties relies on null being set for undefined values, which does happen when the style definition is wrapped in normalizeStyle.

_createElementVNode("h1", {
      style: _normalizeStyle({color: x.value })
    }, " color ", 4 /* STYLE */),

but normalizeStyle isn't being used when we have a plain object not using variables:

_createElementVNode("h1", {
      style: {color: undefined }
    }, " color "),

(Compiled code snippets taken from @hit12369 's sfc playground in their latest comment).

@LinusBorg LinusBorg reopened this Jan 27, 2022
@LinusBorg LinusBorg added 🐞 bug Something isn't working 🔩 p2-edge-case labels Jan 27, 2022
@hit12369
Copy link
Author

There's a problem even with normalizeStyle where initial render sets the property as undefined but any rerender correctly removes the property.

Playground link: sfc.vuejs.org

@LinusBorg LinusBorg added 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. and removed 🔩 p2-edge-case labels Jan 27, 2022
@lidlanca
Copy link
Contributor

sound like undefined or false should not be used.
but handling this edge case, will add checks for an edge case that can be easily avoided.

use empty string '' or null to remove
and this will be on par with how the native style.setProperty works.

I would have said, just remove the style from the object, but that will actually not work for reactive object, without cloning/spreading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. 🐞 bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants