Spread properties on inputs have unexpected readonly attributes

[larryosborn]

Describe the bug
Spread properties on <input> tags can have an unexpected behavior on readonly attributes. If a tag has readonly={false} along with a spread, then the readonly attribute is included in the tag as readonly="false". This results in a readonly input.

To Reproduce

https://svelte.dev/repl/956c1eeb49b24d8c91942878eaedb4e1?version=3.12.1

<script>
	let props = {
		type: 'text'
	}
	let value ='foo'
	let disabled = false
	let readonly = false
</script>
<div>
	Example A: Attribute is readonly="false"<br>
	<input {...props} bind:value {disabled} {readonly}>
</div>

<div>
	Example B: The readonly attribute is omitted<br>
	<input type="text" bind:value {disabled} {readonly}>
</div>

Expected behavior
In the example provided, the readonly attribute should be omitted if false.

Information about your Svelte project:

• Your browser and the version: Version 77.0.3865.120 (Official Build) (64-bit)

• Your operating system: OS X 10

• Svelte version 3.12.1

Severity
minor

[Conduitry]

If any of the attributes on an element are spread attributes, we're currently bailing on all of the logic we have for which attributes are boolean and should be set via properties. We need to continue using properties where appropriate.

I'm guessing the AttributeWrapper should expose a method that indicates whether this is an attribute that needs to be set via a property. My initial thought was, when there are spread attributes, to pull out the (non-spread) boolean attributes from the array of updates we pass to get_spread_update() and to instead include the regular property updates for them afterwards. I'm not sure, though, how this would interact with spread attributes which contain some of the same boolean attributes, or whether there's any reasonable way for us to handle that.

[Conduitry]

Took an initial shot at this in this branch. This doesn't break any tests, and seems to generate more correct code in situations where we have property-only attributes (i.e., those with attribute_lookup metadata) along with spread attributes. However, it does move all attributes with metadata before all other attributes (which are kept in the same relative order so that the spreads can cascade), which isn't going to be quite the correct behavior.

[Conduitry]

I suppose the most correct thing we could do (without shipping attribute lookup metadata in the compiled components) would be to split up the list of attributes and spreads into contiguous chunks of spreads-and-regular-attributes vs chunks of attributes-with-property-lookup-metada. We'd then produce one set_attributes(get_spread_update(...)) for each contiguous chunk of type 1 interwoven with property assignments for each contiguous chunk of type 2. Everything then should hopefully just work, with later things overriding earlier things in the proper way.

I suppose even better would be to render regular individual attr calls for each attribute in a chunk of type 1 that happened to contain no spreads. But then this is all making me wonder what really the value is of combining things that aren't spreads with get_spread_update(). Is it performance? Code size? It's certainly more complicated and slightly more likely to be incorrect, as set_attributes() has some logic that does some introspection on the DOM node prototype in question to see whether there are settable properties with those identical names, and uses those if present instead of setting the attributes. It seems safer to use set_attributes() only for things that are actually spreads, where we truly have no way of looking up what the attributes might be at compile time.

[mrkishi]

At first glance that looks sensible, but get_spread_update would have to be updated in a not-immediately-obvious-to-me way for this to work. If you have an attribute defined explicitly and there's a later spread, how would it know to revert to the previous explicit value if the attribute is then dropped from the spread? I believe that's the main reason they were all bundled together as multiple levels of attributes.

As an alternative, could we reconsider adding the tag properties mapping metadata to runtime? Since we're using attr for most things, it's not that big these days — and we could tree-shake any metadata from unused tags.

Say:

interface PropertyMap {
	[attr: string]: string;
}

export function set_attributes(node: HTMLElement, attributes: { [x: string]: any }, properties_map: PropertyMap) {
	for (const key in attributes) {
		const value = attributes[key];

		// this could be expanded to cover other special-cased 
		// attributes in AttributeWrapper, like selects
		if (value == null) {
			node.removeAttribute(key);
		} else if (key === 'style') {
			node.style.cssText = value;
		} else if (key === 'hidden') {
			node.hidden = value;
		} else if (properties_map[key]) {
			node[properties_map[key]] = value;
		} else {
			node.setAttribute(key, value);
		}
	}
}

Where properties_map is resolved at compilation to one of these:

export const attrs_iframe = { allowfullscreen: 'allowFullscreen', allowpaymentrequest: 'allowPaymentRequest' };
export const attrs_script = { async: 'async', defer: 'defer', nomodule: 'noModule' };
export const attrs_button = { autofocus: 'autofocus', disabled: 'disabled', formnovalidate: 'formNoValidate', value: 'value' };
export const attrs_input = { autofocus: 'autofocus', checked: 'checked', disabled: 'disabled', formnovalidate: 'formNoValidate', indeterminate: 'indeterminate', multiple: 'multiple', readonly: 'readOnly', required: 'required', value: 'value' };
export const attrs_keygen = { autofocus: 'autofocus', disabled: 'disabled' };
export const attrs_select = { autofocus: 'autofocus', disabled: 'disabled', multiple: 'multiple', required: 'required', value: 'value' };
export const attrs_textarea = { autofocus: 'autofocus', disabled: 'disabled', readonly: 'readOnly', required: 'required', value: 'value' };
export const attrs_audio = { autoplay: 'autoplay', controls: 'controls', loop: 'loop', muted: 'muted' };
export const attrs_video = { autoplay: 'autoplay', controls: 'controls', loop: 'loop', muted: 'muted', playsinline: 'playsInline' };
export const attrs_track = { default: 'default' };
export const attrs_fieldset = { disabled: 'disabled' };
export const attrs_optgroup = { disabled: 'disabled' };
export const attrs_option = { disabled: 'disabled', selected: 'selected', value: 'value' };
export const attrs_img = { ismap: 'isMap' };
export const attrs_bgsound = { loop: 'loop' };
export const attrs_form = { novalidate: 'noValidate' };
export const attrs_details = { open: 'open' };
export const attrs_dialog = { open: 'open' };
export const attrs_ol = { reversed: 'reversed' };
export const attrs_li = { value: 'value' };
export const attrs_meter = { value: 'value' };
export const attrs_progress = { value: 'value' };
export const attrs_param = { value: 'value' };

export const attrs_any = { hidden: 'hidden' };

Now, if #3750 goes somewhere, the mapping might become a big dispatch table to setters instead... Then it might be too big (or not, thanks to minification? idk).

[Conduitry]

If you have something like <div {...attrs} title='something static'> then we need the title='something static' to override anything set by {...attrs}. This means either you need to combine the updates with get_spread_update([attrs, { title: 'something static' }]) or you need to put an extra attr(node, 'title', 'something static') in the update method, which wouldn't ordinarily be there. This is why we can't just use the regular update code for attributes if they appear after spread props. Argh.

This also means that something like <input {...attrs} required={is_required}> would break. We need to re-set input.required = is_required after every time attrs is updated as well. This is why the code was originally written to have all the updates be combined into one object which is passed to set_attributes(). Which isn't the correct behavior in some cases, as this issue shows.

So: New angle of attack. Don't fight the set_attributes(get_spread_update(...)). Instead, make sure we have all of the single-key objects in it (those from regular non-spread attributes) have their keys adjusted (if necessary) according to the attribute metadata lookup table. In this case, that would mean having one of the things in the array be { readOnly: whatever } with a capital O.

[Conduitry]

Pushed to my branch the changes I mentioned in the last comment. All existing tests still pass, and now the generated update code for the element in question in the original repro is:

			set_attributes(input0, get_spread_update(input0_levels, [
				changed.props && ctx.props,
				changed.disabled && ({ disabled }),
				changed.readonly && ({ readOnly: readonly })
			]));

i.e., we're now setting readOnly, which will find a prop, instead of readonly, which will fall back to an attribute and causes the incorrect behavior.

In light of #3750, runtime lookups might become impractical. I'd still like to try to avoid them if we can. What I have here seems to be a 'lightest touch' type of solution.

[Conduitry]

If we go ahead with this type of solution, this actually won't fix #3680. We'll still need some way of setting __value on the elements. Simply rewriting the value attribute as { __value: whatever } won't help, because set_attributes() checks whether the property exists on the node prototype and is writable. Maybe there can be a special case for this in set_attributes()? I'm still hoping for a nicer solution.