From f68b3a3b8c7bd3e023a3576f6f6a7ac84580267f Mon Sep 17 00:00:00 2001 From: Conduitry Date: Wed, 23 Oct 2019 12:43:20 -0400 Subject: [PATCH] Fix boolean attributes in presence of spread attributes (#3775) * add failing tests * fix boolean attributes along with spreads (DOM mode) * fix boolean attributes along with spreads (SSR mode) * update changelog (#3764) * fix removing attributes in spreads --- CHANGELOG.md | 1 + src/compiler/compile/nodes/Attribute.ts | 2 +- .../render_dom/wrappers/Element/Attribute.ts | 11 +++++-- .../render_dom/wrappers/Element/index.ts | 20 ++++++------ .../compile/render_ssr/handlers/Element.ts | 32 +++++++++---------- src/runtime/internal/dom.ts | 4 ++- src/runtime/internal/ssr.ts | 2 +- .../_config.js | 3 ++ .../main.svelte | 1 + .../attribute-boolean-with-spread/_config.js | 3 ++ .../attribute-boolean-with-spread/main.svelte | 1 + .../samples/spread-element-removal/_config.js | 3 ++ .../spread-element-removal/main.svelte | 1 + 13 files changed, 52 insertions(+), 32 deletions(-) create mode 100644 test/runtime/samples/attribute-boolean-case-insensitive/_config.js create mode 100644 test/runtime/samples/attribute-boolean-case-insensitive/main.svelte create mode 100644 test/runtime/samples/attribute-boolean-with-spread/_config.js create mode 100644 test/runtime/samples/attribute-boolean-with-spread/main.svelte create mode 100644 test/runtime/samples/spread-element-removal/_config.js create mode 100644 test/runtime/samples/spread-element-removal/main.svelte diff --git a/CHANGELOG.md b/CHANGELOG.md index e39b8b4972..31fc1eddd1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ * Throw exception immediately when calling `createEventDispatcher()` after component instantiation ([#3667](https://github.com/sveltejs/svelte/pull/3667)) * Fix globals shadowing contextual template scope ([#3674](https://github.com/sveltejs/svelte/issues/3674)) * Fix error resulting from trying to set a read-only property when spreading element attributes ([#3681](https://github.com/sveltejs/svelte/issues/3681)) +* Fix handling of boolean attributes in presence of other spread attributes ([#3764](https://github.com/sveltejs/svelte/issues/3764)) ## 3.12.1 diff --git a/src/compiler/compile/nodes/Attribute.ts b/src/compiler/compile/nodes/Attribute.ts index c09f9c3074..97d2fd7b2e 100644 --- a/src/compiler/compile/nodes/Attribute.ts +++ b/src/compiler/compile/nodes/Attribute.ts @@ -9,7 +9,7 @@ import TemplateScope from './shared/TemplateScope'; import { x } from 'code-red'; export default class Attribute extends Node { - type: 'Attribute'; + type: 'Attribute' | 'Spread'; start: number; end: number; scope: TemplateScope; diff --git a/src/compiler/compile/render_dom/wrappers/Element/Attribute.ts b/src/compiler/compile/render_dom/wrappers/Element/Attribute.ts index d2def7e5cf..2cd284108c 100644 --- a/src/compiler/compile/render_dom/wrappers/Element/Attribute.ts +++ b/src/compiler/compile/render_dom/wrappers/Element/Attribute.ts @@ -44,9 +44,7 @@ export default class AttributeWrapper { const element = this.parent; const name = fix_attribute_casing(this.node.name); - let metadata = element.node.namespace ? null : attribute_lookup[name]; - if (metadata && metadata.applies_to && !~metadata.applies_to.indexOf(element.node.name)) - metadata = null; + const metadata = this.get_metadata(); const is_indirectly_bound_value = name === 'value' && @@ -193,6 +191,13 @@ export default class AttributeWrapper { } } + get_metadata() { + if (this.parent.node.namespace) return null; + const metadata = attribute_lookup[fix_attribute_casing(this.node.name)]; + if (metadata && metadata.applies_to && !metadata.applies_to.includes(this.parent.node.name)) return null; + return metadata; + } + get_class_name_text() { const scoped_css = this.node.chunks.some((chunk: Text) => chunk.synthetic); const rendered = this.render_chunks(); diff --git a/src/compiler/compile/render_dom/wrappers/Element/index.ts b/src/compiler/compile/render_dom/wrappers/Element/index.ts index d7c1c7686b..bb4c2d310a 100644 --- a/src/compiler/compile/render_dom/wrappers/Element/index.ts +++ b/src/compiler/compile/render_dom/wrappers/Element/index.ts @@ -573,8 +573,7 @@ export default class ElementWrapper extends Wrapper { } }); - // @ts-ignore todo: - if (this.node.attributes.find(attr => attr.type === 'Spread')) { + if (this.node.attributes.some(attr => attr.is_spread)) { this.add_spread_attributes(block); return; } @@ -591,21 +590,24 @@ export default class ElementWrapper extends Wrapper { const initial_props = []; const updates = []; - this.node.attributes - .filter(attr => attr.type === 'Attribute' || attr.type === 'Spread') + this.attributes .forEach(attr => { - const condition = attr.dependencies.size > 0 - ? changed(Array.from(attr.dependencies)) + const condition = attr.node.dependencies.size > 0 + ? changed(Array.from(attr.node.dependencies)) : null; - if (attr.is_spread) { - const snippet = attr.expression.manipulate(block); + if (attr.node.is_spread) { + const snippet = attr.node.expression.manipulate(block); initial_props.push(snippet); updates.push(condition ? x`${condition} && ${snippet}` : snippet); } else { - const snippet = x`{ ${attr.name}: ${attr.get_value(block)} }`; + const metadata = attr.get_metadata(); + const snippet = x`{ ${ + (metadata && metadata.property_name) || + fix_attribute_casing(attr.node.name) + }: ${attr.node.get_value(block)} }`; initial_props.push(snippet); updates.push(condition ? x`${condition} && ${snippet}` : snippet); diff --git a/src/compiler/compile/render_ssr/handlers/Element.ts b/src/compiler/compile/render_ssr/handlers/Element.ts index 762152f4d5..1f7c0b7b9f 100644 --- a/src/compiler/compile/render_ssr/handlers/Element.ts +++ b/src/compiler/compile/render_ssr/handlers/Element.ts @@ -1,5 +1,4 @@ import { is_void } from '../../../utils/names'; -import Attribute from '../../nodes/Attribute'; import Class from '../../nodes/Class'; import { get_attribute_value, get_class_attribute_value } from './shared/get_attribute_value'; import { get_slot_scope } from './shared/get_slot_scope'; @@ -80,62 +79,61 @@ export default function(node: Element, renderer: Renderer, options: RenderOption let add_class_attribute = class_expression ? true : false; - if (node.attributes.find(attr => attr.is_spread)) { + if (node.attributes.some(attr => attr.is_spread)) { // TODO dry this out const args = []; node.attributes.forEach(attribute => { if (attribute.is_spread) { args.push(attribute.expression.node); } else { - if (attribute.name === 'value' && node.name === 'textarea') { + const name = attribute.name.toLowerCase(); + if (name === 'value' && node.name.toLowerCase() === 'textarea') { node_contents = get_attribute_value(attribute); } else if (attribute.is_true) { args.push(x`{ ${attribute.name}: true }`); } else if ( - boolean_attributes.has(attribute.name) && + boolean_attributes.has(name) && attribute.chunks.length === 1 && attribute.chunks[0].type !== 'Text' ) { // a boolean attribute with one non-Text chunk - args.push(x`{ ${attribute.name}: ${(attribute.chunks[0] as Expression).node} }`); - } else if (attribute.name === 'class' && class_expression) { + args.push(x`{ ${attribute.name}: ${(attribute.chunks[0] as Expression).node} || null }`); + } else if (name === 'class' && class_expression) { // Add class expression args.push(x`{ ${attribute.name}: [${get_class_attribute_value(attribute)}, ${class_expression}].join(' ').trim() }`); } else { - args.push(x`{ ${attribute.name}: ${attribute.name === 'class' ? get_class_attribute_value(attribute) : get_attribute_value(attribute)} }`); + args.push(x`{ ${attribute.name}: ${(name === 'class' ? get_class_attribute_value : get_attribute_value)(attribute)} }`); } } }); renderer.add_expression(x`@spread([${args}])`); } else { - node.attributes.forEach((attribute: Attribute) => { - if (attribute.type !== 'Attribute') return; - - if (attribute.name === 'value' && node.name === 'textarea') { + node.attributes.forEach(attribute => { + const name = attribute.name.toLowerCase(); + if (name === 'value' && node.name.toLowerCase() === 'textarea') { node_contents = get_attribute_value(attribute); } else if (attribute.is_true) { renderer.add_string(` ${attribute.name}`); } else if ( - boolean_attributes.has(attribute.name) && + boolean_attributes.has(name) && attribute.chunks.length === 1 && attribute.chunks[0].type !== 'Text' ) { // a boolean attribute with one non-Text chunk renderer.add_string(` `); renderer.add_expression(x`${(attribute.chunks[0] as Expression).node} ? "${attribute.name}" : ""`); - } else if (attribute.name === 'class' && class_expression) { + } else if (name === 'class' && class_expression) { add_class_attribute = false; - renderer.add_string(` class="`); + renderer.add_string(` ${attribute.name}="`); renderer.add_expression(x`[${get_class_attribute_value(attribute)}, ${class_expression}].join(' ').trim()`); renderer.add_string(`"`); } else if (attribute.chunks.length === 1 && attribute.chunks[0].type !== 'Text') { - const { name } = attribute; const snippet = (attribute.chunks[0] as Expression).node; - renderer.add_expression(x`@add_attribute("${name}", ${snippet}, ${boolean_attributes.has(name) ? 1 : 0})`); + renderer.add_expression(x`@add_attribute("${attribute.name}", ${snippet}, ${boolean_attributes.has(name) ? 1 : 0})`); } else { renderer.add_string(` ${attribute.name}="`); - renderer.add_expression(attribute.name === 'class' ? get_class_attribute_value(attribute) : get_attribute_value(attribute)); + renderer.add_expression((name === 'class' ? get_class_attribute_value : get_attribute_value)(attribute)); renderer.add_string(`"`); } }); diff --git a/src/runtime/internal/dom.ts b/src/runtime/internal/dom.ts index 481fb7d74a..c60f437863 100644 --- a/src/runtime/internal/dom.ts +++ b/src/runtime/internal/dom.ts @@ -93,7 +93,9 @@ export function set_attributes(node: Element & ElementCSSInlineStyle, attributes // @ts-ignore const descriptors = Object.getOwnPropertyDescriptors(node.__proto__); for (const key in attributes) { - if (key === 'style') { + if (attributes[key] == null) { + node.removeAttribute(key); + } else if (key === 'style') { node.style.cssText = attributes[key]; } else if (descriptors[key] && descriptors[key].set) { node[key] = attributes[key]; diff --git a/src/runtime/internal/ssr.ts b/src/runtime/internal/ssr.ts index d8fbf15f0a..208a4637c7 100644 --- a/src/runtime/internal/ssr.ts +++ b/src/runtime/internal/ssr.ts @@ -13,7 +13,7 @@ export function spread(args) { if (invalid_attribute_name_character.test(name)) return; const value = attributes[name]; - if (value === undefined) return; + if (value == null) return; if (value === true) str += " " + name; const escaped = String(value) diff --git a/test/runtime/samples/attribute-boolean-case-insensitive/_config.js b/test/runtime/samples/attribute-boolean-case-insensitive/_config.js new file mode 100644 index 0000000000..16e5ade6d2 --- /dev/null +++ b/test/runtime/samples/attribute-boolean-case-insensitive/_config.js @@ -0,0 +1,3 @@ +export default { + html: `` +}; diff --git a/test/runtime/samples/attribute-boolean-case-insensitive/main.svelte b/test/runtime/samples/attribute-boolean-case-insensitive/main.svelte new file mode 100644 index 0000000000..9020894086 --- /dev/null +++ b/test/runtime/samples/attribute-boolean-case-insensitive/main.svelte @@ -0,0 +1 @@ + diff --git a/test/runtime/samples/attribute-boolean-with-spread/_config.js b/test/runtime/samples/attribute-boolean-with-spread/_config.js new file mode 100644 index 0000000000..270804170d --- /dev/null +++ b/test/runtime/samples/attribute-boolean-with-spread/_config.js @@ -0,0 +1,3 @@ +export default { + html: `` +}; diff --git a/test/runtime/samples/attribute-boolean-with-spread/main.svelte b/test/runtime/samples/attribute-boolean-with-spread/main.svelte new file mode 100644 index 0000000000..d0616e3024 --- /dev/null +++ b/test/runtime/samples/attribute-boolean-with-spread/main.svelte @@ -0,0 +1 @@ + diff --git a/test/runtime/samples/spread-element-removal/_config.js b/test/runtime/samples/spread-element-removal/_config.js new file mode 100644 index 0000000000..270804170d --- /dev/null +++ b/test/runtime/samples/spread-element-removal/_config.js @@ -0,0 +1,3 @@ +export default { + html: `` +}; diff --git a/test/runtime/samples/spread-element-removal/main.svelte b/test/runtime/samples/spread-element-removal/main.svelte new file mode 100644 index 0000000000..f6adc82c80 --- /dev/null +++ b/test/runtime/samples/spread-element-removal/main.svelte @@ -0,0 +1 @@ +