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

class:/style: directives can be overridden by class/style attributes #13270

Open
dummdidumm opened this issue Sep 16, 2024 · 4 comments · Fixed by #13353
Open

class:/style: directives can be overridden by class/style attributes #13270

dummdidumm opened this issue Sep 16, 2024 · 4 comments · Fixed by #13353
Labels
Milestone

Comments

@dummdidumm
Copy link
Member

Describe the bug

I'm not sure if this is related to #13171 or not, but class:/style: directives can be overridden by class/style attributes, which they shouldn't - the directives always take precedent. It happens if the class:/style: directive doesn't update/rerun while the corresponding class/style attribute does update.

The solution is probably to ensure that the directives are always in the same template effect as the corresponding attributes so that they rerun. Alternatively there's some hidden property on the DOM elements that tells them what other classes should be on there at that point.

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAACl2P4WrDMAyEX0WIjaQ0kLGfmeOy51jGcB1lNXGdYMspw_jdhxfGRn_qdLr7lHAylgJ2bwmduhJ2-Lqu2CB_rWUIG1kmbDAs0euiiKC9WVkObmBLDJuykaCHh8CKqX46vAyu7KboNJvFwaj8XB9S0Qb2xNE7YB-p-DjfmT-JP7RVIdxf7DWP8AwnqG4Xw1RBB9XZKj1Xv1Gi_YNzYjQb_GR1BaFPO0jetT79q8pStKPZ5ODEOTIvDhanrdFzn-pDL_fy4zHLcDOsL6LdXRIbvC6jmQyN2JWf8nv-BkMMS5xSAQAA

Logs

No response

System Info

irrelevant

Severity

blocking an upgrade

@dummdidumm dummdidumm added the bug label Sep 16, 2024
@dummdidumm dummdidumm added this to the 5.0 milestone Sep 16, 2024
@paoloricciuti
Copy link
Member

Oh yeah it's my fault...I thought reverting my initial fix and using deriveds was enough but dummily didn't test for this.

But you are right, the two should be in the same template effect.

However I was confident it was not a problem because I remember a test failing for this when I made my initial fix and it was not failing with the derived.

@paoloricciuti
Copy link
Member

Uh actually: the problem was there before because the template effects were still separate (testing this on a version before the PR still has the problem)...but I should've fixed it in my pr probably 😁

@Rich-Harris
Copy link
Member

Working on this locally. Noticed another issue, possibly as a result of #13171 — we create an unnecessary derived when there's only a single call expression in the set of class/style directives/attributes — this...

const class_directive = $.derived(lol);

$.template_effect(() => $.toggle_class(button, "banana", $.get(class_directive)));

...should be this:

$.template_effect(() => $.toggle_class(button, "banana", lol()));

@paoloricciuti
Copy link
Member

paoloricciuti commented Sep 19, 2024

But when you create the class directive you can't know if while visiting the nodes we will add something else to the template_effect (for example a future set_text). But maybe we can delay the creation of the deriveds after the analysis is complete where we can check how many nodes are in the update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants