fix: use setAttribute for spellcheck and translate

[paoloricciuti]

Svelte 5 rewrite

Closes #12664

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

🦋 Changeset detected

Latest commit: c5ff21c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

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

[Rich-Harris]

I had to wonder whether there are other similar attributes, namely enumerated attributes corresponding to boolean properties, and so I updated the comment to reflect what I learned.

It turns out, though, that the list of global enumerated attributes with "true" and "false" as options (at least according to that repo) includes draggable and spellcheck but not translate, which is spectacularly awkward:

div = document.createElement('div'); div.translate; // true div.setAttribute('translate', 'false'); div.translate; // true div.setAttribute('translate', 'no'); div.translate; // false div.translate = true; div.setAttribute('translate'); // 'yes'

So in other words it seems we don't want to include translate in this list. If anything we probably want all these to work:

<script>  let a = { translate: false };  let b = { translate: 'no' }; </script> <div translate={false}>...</div> <div translate="no">...</div> <div {...a}>...</div> <div {...b}>...</div>

We're currently wrong (in the translate={false} case) and inconsistent. With this PR we are wrong, but consistently.

I also think that we should probably tell users (at compile time, where attribute values are known statically, and at runtime otherwise) if they set an enumerated attribute to something other than one of the possible values.

Also: contenteditable is an enumerated attribute whose possible values are "true" and "false" — should it be in the list alongside draggable and spellcheck?

[Rich-Harris]

countermanding the approval until we can figure out the correct behaviour

[paoloricciuti]

Uhm...i think the reason why this is not working for the last two bits is because we are only using always_set_through_set_attribute in get_setters which in turn is only used set_attributes and not in set_attribute. I think if we include the list in set_attribute we should have all greens. Let me check.

[paoloricciuti]

Uhm...i think the reason why this is not working for the last two bits is because we are only using always_set_through_set_attribute in get_setters which in turn is only used set_attributes and not in set_attribute. I think if we include the list in set_attribute we should have all greens. Let me check.

Ok no, different reason: we always call element.setAttribute in set_attribute but we actually just pass the stringified value in. So in the case of {false} we pass "false" to setAttribute resulting in the wrong value. The solution could be:

1. We split the array always_set_through_set_attribute removing the enumerable properties and we keep an array of the enumerable properties. We use this second array to check in set_attributes if we should use setAttribute and in set_attribute (singular) to check if we should use the setter instead (so that if we pass a boolean we let the setter assign the correct value).
2. We special case translate in set_attribute changing "false" to "yes"

I'm also wondering if there's a way to reuse more logic between set_attribute and set_attributes

[paoloricciuti]

For the moment i've implemented the first option because i think if the user passes an actual boolean it likely means the respective boolean parameter ("yes" in case of translate). Is this what we want? I think it can work.

Btw if you look at the linked issue there are a bunch of those properties including contenteditable but by testing it doesn't seems the browser is following contenteditable="false" and it still goes in content editable mode if the attribute is present.

[dummdidumm]

We have a list of attributes somewhere which we set through the corresponding property instead. Could we use that instead?
Also, for these kinds of attributes I think it would be better to instead decide which variant we officially support, so that people are expected to only provide yes/no or true/false

[paoloricciuti]

Also, for these kinds of attributes I think it would be better to instead decide which variant we officially support, so that people are expected to only provide yes/no or true/false

I think when it's in string form we should stick to what the dom does...but I think people also expect to be able to use booleans just like they do with element.draggable=true.

We have a list of attributes somewhere which we set through the corresponding property instead. Could we use that instead?

Where can I find such list?

[webJose]

Hello. I have a question: Will this correct the other attributes as well? For example, disabled is showing as disabled=false. I ask because of the PR's title and because it closes an issue that mentions an incomplete list of other attributes affected.

[paoloricciuti]

Hello. I have a question: Will this correct the other attributes as well? For example, disabled is showing as disabled=false. I ask because of the PR's title and because it closes an issue that mentions an incomplete list of other attributes affected.

If you look at comments in the issue most of the list there is actually already fine.

[webJose]

Thanks for dropping by. It is a good thing that most are already fine. The list, however, is an incomplete list as the author states, and I have an instance at work where it adds disabled=false instead of eliminating the attribute. I don't remember which version of Svelte v5 I'm using there right now, so I cannot answer to this potential question at this time.

[Rich-Harris]

Opened #13327 with the approach outlined above. I don't see any reason we shouldn't Do The Right Thing when encountering both <div translate="no"> and <div translate={false}>.

Out of curiosity I checked to see what React does. They do the correct thing for <div translate="no"> but print an error for {false}...

Warning: Received false for a non-boolean attribute translate.

...so much for 'we make you use className and htmlFor because JSX deals with DOM properties, not attributes'

[paoloricciuti]

I don't see any reason we shouldn't Do The Right Thing when encountering both <div translate="no"> and <div translate={false}>.

Wait was this doing the wrong thing? I was under the impression that this was already doing the right thing that's why i didn't touch it

[paoloricciuti]

But regardless...if you found a solution that is better let's go with it...closing.