fix: set strings as attributes, non-strings as properties if property exists (#13327)

* fix: set strings as attributes, non-strings as properties if property exists

* simplify

* remove draggable from list, no longer needed

* in fact we dont need the lookup at all

* lint

* beef up test

* correctly SSR translate attribute

Author: Rich-Harris

.changeset/perfect-ants-allow.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: set strings as attributes, non-strings as properties if property exists

packages/svelte/src/internal/client/dom/elements/attributes.js

@@ -81,8 +81,6 @@ export function set_checked(element, checked) {
  * @param {boolean} [skip_warning]
  */
 export function set_attribute(element, attribute, value, skip_warning) {
-	value = value == null ? null : value + '';
-
 	// @ts-expect-error
 	var attributes = (element.__attributes ??= {});
 
@@ -95,7 +93,7 @@ export function set_attribute(element, attribute, value, skip_warning) {
 			(attribute === 'href' && element.nodeName === 'LINK')
 		) {
 			if (!skip_warning) {
-				check_src_in_dev_hydration(element, attribute, value);
+				check_src_in_dev_hydration(element, attribute, value ?? '');
 			}
 
 			// If we reset these attributes, they would result in another network request, which we want to avoid.
@@ -113,8 +111,11 @@ export function set_attribute(element, attribute, value, skip_warning) {
 		element[LOADING_ATTR_SYMBOL] = value;
 	}
 
-	if (value === null) {
+	if (value == null) {
 		element.removeAttribute(attribute);
+	} else if (attribute in element && typeof value !== 'string') {
+		// @ts-ignore
+		element[attribute] = value;
 	} else {
 		element.setAttribute(attribute, value);
 	}
@@ -287,15 +288,15 @@ export function set_attributes(
 				name = normalize_attribute(name);
 			}
 
-			if (setters.includes(name)) {
+			if (setters.includes(name) && typeof value !== 'string') {
+				// @ts-ignore
+				element[name] = value;
+			} else if (typeof value !== 'function') {
 				if (hydrating && (name === 'src' || name === 'href' || name === 'srcset')) {
-					if (!skip_warning) check_src_in_dev_hydration(element, name, value);
+					if (!skip_warning) check_src_in_dev_hydration(element, name, value ?? '');
 				} else {
-					// @ts-ignore
-					element[name] = value;
+					set_attribute(element, name, value);
 				}
-			} else if (typeof value !== 'function') {
-				set_attribute(element, name, value);
 			}
 		}
 	}
@@ -350,16 +351,6 @@ export function set_dynamic_element_attributes(node, prev, next, css_hash) {
 	);
 }
 
-/**
- * List of attributes that should always be set through the attr method,
- * because updating them through the property setter doesn't work reliably.
- * In the example of `width`/`height`, the problem is that the setter only
- * accepts numeric values, but the attribute can also be set to a string like `50%`.
- * In case of draggable trying to set `element.draggable='false'` will actually set
- * draggable to `true`. If this list becomes too big, rethink this approach.
- */
-var always_set_through_set_attribute = ['width', 'height', 'draggable'];
-
 /** @type {Map<string, string[]>} */
 var setters_cache = new Map();
 
@@ -375,7 +366,7 @@ function get_setters(element) {
 		descriptors = get_descriptors(proto);
 
 		for (var key in descriptors) {
-			if (descriptors[key].set && !always_set_through_set_attribute.includes(key)) {
+			if (descriptors[key].set) {
 				setters.push(key);
 			}
 		}
@@ -389,12 +380,12 @@ function get_setters(element) {
 /**
  * @param {any} element
  * @param {string} attribute
- * @param {string | null} value
+ * @param {string} value
  */
 function check_src_in_dev_hydration(element, attribute, value) {
 	if (!DEV) return;
 	if (attribute === 'srcset' && srcset_url_equal(element, value)) return;
-	if (src_url_equal(element.getAttribute(attribute) ?? '', value ?? '')) return;
+	if (src_url_equal(element.getAttribute(attribute) ?? '', value)) return;
 
 	w.hydration_attribute_changed(
 		attribute,
@@ -420,12 +411,12 @@ function split_srcset(srcset) {
 
 /**
  * @param {HTMLSourceElement | HTMLImageElement} element
- * @param {string | undefined | null} srcset
+ * @param {string} srcset
  * @returns {boolean}
  */
 function srcset_url_equal(element, srcset) {
 	var element_urls = split_srcset(element.srcset);
-	var urls = split_srcset(srcset ?? '');
+	var urls = split_srcset(srcset);
 
 	return (
 		urls.length === element_urls.length &&

packages/svelte/src/internal/server/index.js

@@ -147,6 +147,19 @@ export function head(payload, fn) {
 	head_payload.out += BLOCK_CLOSE;
 }
 
+/**
+ * `<div translate={false}>` should be rendered as `<div translate="no">` and _not_
+ * `<div translate="false">`, which is equivalent to `<div translate="yes">`. There
+ * may be other odd cases that need to be added to this list in future
+ * @type {Record<string, Map<any, string>>}
+ */
+const replacements = {
+	translate: new Map([
+		[true, 'yes'],
+		[false, 'no']
+	])
+};
+
 /**
  * @template V
  * @param {string} name
@@ -156,7 +169,8 @@ export function head(payload, fn) {
  */
 export function attr(name, value, is_boolean = false) {
 	if (value == null || (!value && is_boolean) || (value === '' && name === 'class')) return '';
-	const assignment = is_boolean ? '' : `="${escape_html(value, true)}"`;
+	const normalized = (name in replacements && replacements[name].get(value)) || value;
+	const assignment = is_boolean ? '' : `="${escape_html(normalized, true)}"`;
 	return ` ${name}${assignment}`;
 }
 

packages/svelte/tests/runtime-runes/samples/attribute-if-string/_config.js

@@ -0,0 +1,24 @@
+import { test } from '../../test';
+
+export default test({
+	test({ assert, target, variant, hydrate }) {
+		function check(/** @type {boolean} */ condition) {
+			const divs = /** @type {NodeListOf<HTMLDivElement>} */ (
+				target.querySelectorAll(`.translate-${condition} div`)
+			);
+
+			divs.forEach((div, i) => {
+				assert.equal(div.translate, condition, `${i + 1} of ${divs.length}: ${div.outerHTML}`);
+			});
+		}
+
+		check(false);
+		check(true);
+
+		if (variant === 'hydrate') {
+			hydrate();
+			check(false);
+			check(true);
+		}
+	}
+});

packages/svelte/tests/runtime-runes/samples/attribute-if-string/main.svelte

@@ -0,0 +1,19 @@
+<div class="translate-false">
+	<div translate={false}></div>
+	<div translate="no"></div>
+	<div {...{ translate: false }}></div>
+	<div {...{ translate: 'no' }}></div>
+</div>
+
+<div class="translate-true">
+	<div></div>
+	<div translate={true}></div>
+	<div translate="yes"></div>
+	<div {...{ translate: true }}></div>
+	<div {...{ translate: 'yes' }}></div>
+
+	<div translate="false"></div>
+	<div translate="banana"></div>
+	<div {...{ translate: 'false' }}></div>
+	<div {...{ translate: 'banana' }}></div>
+</div>