fix: strip trailing semicolons in style directive reactive updates#18200
fix: strip trailing semicolons in style directive reactive updates#18200OfirHaf wants to merge 4 commits into
Conversation
…Property
When a reactive style directive value contains a trailing semicolon (e.g.
`style:background={`conic-gradient(...);'`}`), the initial render works
because it goes through cssText which tolerates semicolons, but subsequent
reactive updates call el.style.setProperty() which silently rejects values
containing semicolons, leaving the style unchanged.
Strip trailing semicolons from the value in update_styles before calling
setProperty, making reactive updates consistent with the initial render.
Fixes sveltejs#18182
🦋 Changeset detectedLatest commit: e86e2c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -15,7 +15,10 @@ function update_styles(dom, prev = {}, next, priority) { | |||
| if (next[key] == null) { | |||
There was a problem hiding this comment.
Small readability nit: since value is already assigned from next[key], this can use the local variable consistently.
| if (next[key] == null) { | |
| if (value == null) { |
There was a problem hiding this comment.
Also, this PR requires a changeset.
There was a problem hiding this comment.
Done, applied the suggestion — changed to if (value == null).
There was a problem hiding this comment.
Added the changeset in .changeset/cuddly-cougars-grow.md with a patch bump.
Applies reviewer suggestion for consistency since value is already assigned from next[key] on the line above. Also adds required changeset for patch version bump.
|
Would you mind adding a small regression test? The location is probably under |
|
Added a regression test under |
|
I kind of dislike doing the replacement for setting, strictly speaking, invalid value. So maybe a better way would be just logging a dev-only warning about |
|
That's a fair point — silently stripping the value isn't ideal. Would it work to keep the strip but add a |
|
Good point @7nik — just pushed an update that adds a DEV-only |
What's happening
When a
style:directive value contains a trailing semicolon, initial render and reactive updates behave differently:dom.style.cssText, which is lenient and accepts trailing semicolons (the browser strips them).dom.style.setProperty(key, value), which silently rejects values containing semicolons. The style never updates.Reproduction from #18182:
Click the button — background never changes because
setPropertydrops the value.Fix
In
update_styles, strip trailing semicolons from the value before passing it tosetProperty, so both paths handle the input consistently.This is intentionally narrow — only trailing semicolons are removed, which is what the browser would strip anyway. Semicolons inside the value (e.g. in
url("data:...")) are not affected.Note: #18173 handles the related SSR/security case for structural semicolons in the
to_stylepath. This PR covers the client-side reactive update path inupdate_styles.Closes #18182