Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Spreading attributes (containing value) on <option>, value attribute get's replaced by option's inner text #9107

Closed
yusaf opened this issue Aug 15, 2023 · 4 comments · Fixed by #9125

Comments

@yusaf
Copy link

yusaf commented Aug 15, 2023

Describe the bug

Spreading attributes with a value attribute on <option> elements get's replaced with the option's inner text

so the following

<select>
	<option {...{ value: 'sms', class: 'option' }}>SMS</option>
	<option {...{ value: 'email', class: 'option' }}>E-mail</option>
	<option {...{ value: 'letter', class: 'option' }}>Letter</option>
</select>

renders as

<select>
	<option value="SMS" class="option">SMS</option>
	<option value="E-mail" class="option">E-mail</option>
	<option value="Letter" class="option">Letter</option>
</select>

When you would expect

<select>
	<option value="sms" class="option">SMS</option>
	<option value="email" class="option">E-mail</option>
	<option value="letter" class="option">Letter</option>
</select>

I tried looking to see if this is expected behaviour as it seem's svelte intentionally replaces the value with the inner text, but I couldn't find anything or any bugs related to this issue.

Reproduction

https://svelte.dev/repl/4d0c421a2c77451e8ef7e4c4db7a9ca6?version=4.2.0

Logs

No response

System Info

System:
    OS: macOS 13.4
    CPU: (10) arm64 Apple M1 Max
    Memory: 3.50 GB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 19.8.1 - /opt/homebrew/bin/node
    npm: 9.5.1 - /opt/homebrew/bin/npm
    pnpm: 8.6.3 - ~/Library/pnpm/pnpm
  Browsers:
    Chrome: 115.0.5790.170
    Chrome Canary: 118.0.5949.0
    Safari: 16.5

Severity

annoyance

@Artxe2
Copy link
Contributor

Artxe2 commented Aug 16, 2023

Here's the problem.

} else if (key === '__value') {

} else if (key === '__value') {
	/** @type {any} */ (node).value = node[key] = attributes[key];
}

I think that need to change it as follows.

} else if (key === '__value') {
	node[key] = attributes[key];
	// @ts-ignore
	if (!('value' in attributes)) node.value = node[key];
}

@yusaf
Copy link
Author

yusaf commented Aug 16, 2023

Here's the problem.

} else if (key === '__value') {

} else if (key === '__value') {
	/** @type {any} */ (node).value = node[key] = attributes[key];
}

I think that need to change it as follows.

} else if (key === '__value') {
	node[key] = attributes[key];
	// @ts-ignore
	if (!('value' in attributes)) node.value = node[key];
}

I'm not at all familiar with the internals of svelte to be honest, however, I'm not sure if this maybe should be a fix for the compiler instead of runtime as I also noticed in the repl JS output it was down to __value taking precedence.

However, looking at your fix

In your example node['__value'] = attributes['__value']; will be set regardless of whether there is a value attribute. wouldn't we want to skip the __value key altogether if we have a value attribute?

Unless there is a further purpose to having the key __value always set, might it be better to change to?

} else if (key === '__value' && !('value' in attributes)) {
	/** @type {any} */ (node).value = node[key] = attributes[key];
}

and the value attribute would get treated like any other attribute being added to the node.

@Artxe2
Copy link
Contributor

Artxe2 commented Aug 16, 2023

I'm not at all familiar with the internals of svelte to be honest

The same goes for me.

I saw your question and tried the following code

<select value="email">
	<option value='sms' class='option'>SMS</option>
	<option value='email' class='option'>E-mail</option>
	<option value='letter' class='option'>Letter</option>
</select>

It seems like a bug that doesn't recognize that __value is not necessary when pass on the value with ....

@yusaf
Copy link
Author

yusaf commented Aug 16, 2023

It seems like a bug that doesn't recognize that __value is not necessary when pass on the value with ....

Yep you hit the nail on the head, using attributes normally works fine, it's just when spread it's an issue.

dummdidumm pushed a commit that referenced this issue Sep 20, 2023
… option's inner text (#9125)

fixes #9107
Apart from the problem with the option the same happens with the textarea.
kelvinsjk pushed a commit to kelvinsjk/svelte that referenced this issue Oct 19, 2023
… option's inner text (sveltejs#9125)

fixes sveltejs#9107
Apart from the problem with the option the same happens with the textarea.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
2 participants