chore: rewrite set_style() to handle directives (#15418)

* add style attribute when needed

* set_style()

* to_style()

* remove `style=""`

* use cssTest for perfs

* base test

* test

* changeset

* revert dom.style.cssText

* format name

* use style.cssText + adapt test

* Apply suggestions from code review

suggestions from dummdidumm

Co-authored-by: Simon H <[email protected]>

* fix priority

* lint

* yawn

* update test

* we can simplify some stuff now

* simplify

* more

* simplify some more

* more

* more

* more

* more

* more

* remove continue

* tweak

* tweak

* tweak

* skip hash argument where possible

* tweak

* tweak

* tweak

* tweak

---------

Co-authored-by: Simon H <[email protected]>
Co-authored-by: Rich Harris <[email protected]>

Author: 3 people

.changeset/strange-planes-shout.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: make `style:` directive and CSS handling more robust

packages/svelte/src/compiler/phases/2-analyze/index.js

@@ -769,17 +769,24 @@ export function analyze_component(root, source, options) {
 		}
 
 		let has_class = false;
+		let has_style = false;
 		let has_spread = false;
 		let has_class_directive = false;
+		let has_style_directive = false;
 
 		for (const attribute of node.attributes) {
 			// The spread method appends the hash to the end of the class attribute on its own
 			if (attribute.type === 'SpreadAttribute') {
 				has_spread = true;
 				break;
+			} else if (attribute.type === 'Attribute') {
+				has_class ||= attribute.name.toLowerCase() === 'class';
+				has_style ||= attribute.name.toLowerCase() === 'style';
+			} else if (attribute.type === 'ClassDirective') {
+				has_class_directive = true;
+			} else if (attribute.type === 'StyleDirective') {
+				has_style_directive = true;
 			}
-			has_class_directive ||= attribute.type === 'ClassDirective';
-			has_class ||= attribute.type === 'Attribute' && attribute.name.toLowerCase() === 'class';
 		}
 
 		// We need an empty class to generate the set_class() or class="" correctly
@@ -796,6 +803,21 @@ export function analyze_component(root, source, options) {
 				])
 			);
 		}
+
+		// We need an empty style to generate the set_style() correctly
+		if (!has_spread && !has_style && has_style_directive) {
+			node.attributes.push(
+				create_attribute('style', -1, -1, [
+					{
+						type: 'Text',
+						data: '',
+						raw: '',
+						start: -1,
+						end: -1
+					}
+				])
+			);
+		}
 	}
 
 	// TODO

packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js

@@ -1,4 +1,4 @@
-/** @import { Expression, ExpressionStatement, Identifier, MemberExpression, ObjectExpression, Statement } from 'estree' */
+/** @import { ArrayExpression, Expression, ExpressionStatement, Identifier, MemberExpression, ObjectExpression, Statement } from 'estree' */
 /** @import { AST } from '#compiler' */
 /** @import { SourceLocation } from '#shared' */
 /** @import { ComponentClientTransformState, ComponentContext } from '../types' */
@@ -20,9 +20,9 @@ import { build_getter } from '../utils.js';
 import {
 	get_attribute_name,
 	build_attribute_value,
-	build_style_directives,
 	build_set_attributes,
-	build_set_class
+	build_set_class,
+	build_set_style
 } from './shared/element.js';
 import { process_children } from './shared/fragment.js';
 import {
@@ -215,13 +215,18 @@ export function RegularElement(node, context) {
 
 	const node_id = context.state.node;
 
-	// Then do attributes
-	let is_attributes_reactive = has_spread;
-
 	if (has_spread) {
 		const attributes_id = b.id(context.state.scope.generate('attributes'));
 
-		build_set_attributes(attributes, class_directives, context, node, node_id, attributes_id);
+		build_set_attributes(
+			attributes,
+			class_directives,
+			style_directives,
+			context,
+			node,
+			node_id,
+			attributes_id
+		);
 
 		// If value binding exists, that one takes care of calling $.init_select
 		if (node.name === 'select' && !bindings.has('value')) {
@@ -262,11 +267,13 @@ export function RegularElement(node, context) {
 			}
 
 			const name = get_attribute_name(node, attribute);
+
 			if (
 				!is_custom_element &&
 				!cannot_be_set_statically(attribute.name) &&
 				(attribute.value === true || is_text_attribute(attribute)) &&
-				(name !== 'class' || class_directives.length === 0)
+				(name !== 'class' || class_directives.length === 0) &&
+				(name !== 'style' || style_directives.length === 0)
 			) {
 				let value = is_text_attribute(attribute) ? attribute.value[0].data : true;
 
@@ -287,27 +294,30 @@ export function RegularElement(node, context) {
 						}`
 					);
 				}
-				continue;
-			}
+			} else if (name === 'autofocus') {
+				let { value } = build_attribute_value(attribute.value, context);
+				context.state.init.push(b.stmt(b.call('$.autofocus', node_id, value)));
+			} else if (name === 'class') {
+				const is_html = context.state.metadata.namespace === 'html' && node.name !== 'svg';
+				build_set_class(node, node_id, attribute, class_directives, context, is_html);
+			} else if (name === 'style') {
+				build_set_style(node_id, attribute, style_directives, context);
+			} else if (is_custom_element) {
+				build_custom_element_attribute_update_assignment(node_id, attribute, context);
+			} else {
+				const { value, has_state } = build_attribute_value(
+					attribute.value,
+					context,
+					(value, metadata) => (metadata.has_call ? get_expression_id(context.state, value) : value)
+				);
+
+				const update = build_element_attribute_update(node, node_id, name, value, attributes);
 
-			const is =
-				is_custom_element && name !== 'class'
-					? build_custom_element_attribute_update_assignment(node_id, attribute, context)
-					: build_element_attribute_update_assignment(
-							node,
-							node_id,
-							attribute,
-							attributes,
-							class_directives,
-							context
-						);
-			if (is) is_attributes_reactive = true;
+				(has_state ? context.state.update : context.state.init).push(b.stmt(update));
+			}
 		}
 	}
 
-	// style directives must be applied last since they could override class/style attributes
-	build_style_directives(style_directives, node_id, context, is_attributes_reactive);
-
 	if (
 		is_load_error_element(node.name) &&
 		(has_spread || has_use || lookup.has('onload') || lookup.has('onerror'))
@@ -519,6 +529,36 @@ export function build_class_directives_object(class_directives, context) {
 	return b.object(properties);
 }
 
+/**
+ * @param {AST.StyleDirective[]} style_directives
+ * @param {ComponentContext} context
+ * @return {ObjectExpression | ArrayExpression}}
+ */
+export function build_style_directives_object(style_directives, context) {
+	let normal_properties = [];
+	let important_properties = [];
+
+	for (const directive of style_directives) {
+		const expression =
+			directive.value === true
+				? build_getter({ name: directive.name, type: 'Identifier' }, context.state)
+				: build_attribute_value(directive.value, context, (value, metadata) =>
+						metadata.has_call ? get_expression_id(context.state, value) : value
+					).value;
+		const property = b.init(directive.name, expression);
+
+		if (directive.modifiers.includes('important')) {
+			important_properties.push(property);
+		} else {
+			normal_properties.push(property);
+		}
+	}
+
+	return important_properties.length
+		? b.array([b.object(normal_properties), b.object(important_properties)])
+		: b.object(normal_properties);
+}
+
 /**
  * Serializes an assignment to an element property by adding relevant statements to either only
  * the init or the the init and update arrays, depending on whether or not the value is dynamic.
@@ -543,73 +583,29 @@ export function build_class_directives_object(class_directives, context) {
  * Returns true if attribute is deemed reactive, false otherwise.
  * @param {AST.RegularElement} element
  * @param {Identifier} node_id
- * @param {AST.Attribute} attribute
+ * @param {string} name
+ * @param {Expression} value
  * @param {Array<AST.Attribute | AST.SpreadAttribute>} attributes
- * @param {AST.ClassDirective[]} class_directives
- * @param {ComponentContext} context
- * @returns {boolean}
  */
-function build_element_attribute_update_assignment(
-	element,
-	node_id,
-	attribute,
-	attributes,
-	class_directives,
-	context
-) {
-	const state = context.state;
-	const name = get_attribute_name(element, attribute);
-	const is_svg = context.state.metadata.namespace === 'svg' || element.name === 'svg';
-	const is_mathml = context.state.metadata.namespace === 'mathml';
-
-	const is_autofocus = name === 'autofocus';
-
-	let { value, has_state } = build_attribute_value(attribute.value, context, (value, metadata) =>
-		metadata.has_call
-			? // if it's autofocus we will not add this to a template effect so we don't want to get the expression id
-				// but separately memoize the expression
-				is_autofocus
-				? memoize_expression(state, value)
-				: get_expression_id(state, value)
-			: value
-	);
+function build_element_attribute_update(element, node_id, name, value, attributes) {
+	if (name === 'muted') {
+		// Special case for Firefox who needs it set as a property in order to work
+		return b.assignment('=', b.member(node_id, b.id('muted')), value);
+	}
 
-	if (is_autofocus) {
-		state.init.push(b.stmt(b.call('$.autofocus', node_id, value)));
-		return false;
+	if (name === 'value') {
+		return b.call('$.set_value', node_id, value);
 	}
 
-	// Special case for Firefox who needs it set as a property in order to work
-	if (name === 'muted') {
-		if (!has_state) {
-			state.init.push(b.stmt(b.assignment('=', b.member(node_id, b.id('muted')), value)));
-			return false;
-		}
-		state.update.push(b.stmt(b.assignment('=', b.member(node_id, b.id('muted')), value)));
-		return false;
+	if (name === 'checked') {
+		return b.call('$.set_checked', node_id, value);
 	}
 
-	/** @type {Statement} */
-	let update;
+	if (name === 'selected') {
+		return b.call('$.set_selected', node_id, value);
+	}
 
-	if (name === 'class') {
-		return build_set_class(
-			element,
-			node_id,
-			attribute,
-			value,
-			has_state,
-			class_directives,
-			context,
-			!is_svg && !is_mathml
-		);
-	} else if (name === 'value') {
-		update = b.stmt(b.call('$.set_value', node_id, value));
-	} else if (name === 'checked') {
-		update = b.stmt(b.call('$.set_checked', node_id, value));
-	} else if (name === 'selected') {
-		update = b.stmt(b.call('$.set_selected', node_id, value));
-	} else if (
+	if (
 		// If we would just set the defaultValue property, it would override the value property,
 		// because it is set in the template which implicitly means it's also setting the default value,
 		// and if one updates the default value while the input is pristine it will also update the
@@ -620,62 +616,49 @@ function build_element_attribute_update_assignment(
 		) ||
 			(element.name === 'textarea' && element.fragment.nodes.length > 0))
 	) {
-		update = b.stmt(b.call('$.set_default_value', node_id, value));
-	} else if (
+		return b.call('$.set_default_value', node_id, value);
+	}
+
+	if (
 		// See defaultValue comment
 		name === 'defaultChecked' &&
 		attributes.some(
 			(attr) => attr.type === 'Attribute' && attr.name === 'checked' && attr.value === true
 		)
 	) {
-		update = b.stmt(b.call('$.set_default_checked', node_id, value));
-	} else if (is_dom_property(name)) {
-		update = b.stmt(b.assignment('=', b.member(node_id, name), value));
-	} else {
-		const callee = name.startsWith('xlink') ? '$.set_xlink_attribute' : '$.set_attribute';
-		update = b.stmt(
-			b.call(
-				callee,
-				node_id,
-				b.literal(name),
-				value,
-				is_ignored(element, 'hydration_attribute_changed') && b.true
-			)
-		);
+		return b.call('$.set_default_checked', node_id, value);
 	}
 
-	if (has_state) {
-		state.update.push(update);
-		return true;
-	} else {
-		state.init.push(update);
-		return false;
+	if (is_dom_property(name)) {
+		return b.assignment('=', b.member(node_id, name), value);
 	}
+
+	return b.call(
+		name.startsWith('xlink') ? '$.set_xlink_attribute' : '$.set_attribute',
+		node_id,
+		b.literal(name),
+		value,
+		is_ignored(element, 'hydration_attribute_changed') && b.true
+	);
 }
 
 /**
- * Like `build_element_attribute_update_assignment` but without any special attribute treatment.
+ * Like `build_element_attribute_update` but without any special attribute treatment.
  * @param {Identifier}	node_id
  * @param {AST.Attribute} attribute
  * @param {ComponentContext} context
- * @returns {boolean}
  */
 function build_custom_element_attribute_update_assignment(node_id, attribute, context) {
-	const state = context.state;
-	const name = attribute.name; // don't lowercase, as we set the element's property, which might be case sensitive
-	let { value, has_state } = build_attribute_value(attribute.value, context);
+	const { value, has_state } = build_attribute_value(attribute.value, context);
 
-	const update = b.stmt(b.call('$.set_custom_element_data', node_id, b.literal(name), value));
+	// don't lowercase name, as we set the element's property, which might be case sensitive
+	const call = b.call('$.set_custom_element_data', node_id, b.literal(attribute.name), value);
 
-	if (has_state) {
-		// this is different from other updates — it doesn't get grouped,
-		// because set_custom_element_data may not be idempotent
-		state.init.push(b.stmt(b.call('$.template_effect', b.thunk(update.expression))));
-		return true;
-	} else {
-		state.init.push(update);
-		return false;
-	}
+	// this is different from other updates — it doesn't get grouped,
+	// because set_custom_element_data may not be idempotent
+	const update = has_state ? b.call('$.template_effect', b.thunk(call)) : call;
+
+	context.state.init.push(b.stmt(update));
 }
 
 /**
@@ -686,7 +669,6 @@ function build_custom_element_attribute_update_assignment(node_id, attribute, co
  * @param {Identifier} node_id
  * @param {AST.Attribute} attribute
  * @param {ComponentContext} context
- * @returns {boolean}
  */
 function build_element_special_value_attribute(element, node_id, attribute, context) {
 	const state = context.state;
@@ -699,7 +681,7 @@ function build_element_special_value_attribute(element, node_id, attribute, cont
 		metadata.has_call
 			? // if is a select with value we will also invoke `init_select` which need a reference before the template effect so we memoize separately
 				is_select_with_value
-				? memoize_expression(context.state, value)
+				? memoize_expression(state, value)
 				: get_expression_id(state, value)
 			: value
 	);
@@ -743,9 +725,7 @@ function build_element_special_value_attribute(element, node_id, attribute, cont
 			value,
 			update
 		);
-		return true;
 	} else {
 		state.init.push(update);
-		return false;
 	}
 }