From 8b3f0320f6a61a8d6aa2bf6541ee8e6eb1c48dc3 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 16 Mar 2017 16:02:50 -0400 Subject: [PATCH 1/4] test for #312 --- .../binding-input-checkbox-group/_config.js | 70 +++++++++++++++++++ .../binding-input-checkbox-group/main.html | 7 ++ test/helpers.js | 6 +- 3 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 test/generator/samples/binding-input-checkbox-group/_config.js create mode 100644 test/generator/samples/binding-input-checkbox-group/main.html diff --git a/test/generator/samples/binding-input-checkbox-group/_config.js b/test/generator/samples/binding-input-checkbox-group/_config.js new file mode 100644 index 000000000000..0a0e02f7208f --- /dev/null +++ b/test/generator/samples/binding-input-checkbox-group/_config.js @@ -0,0 +1,70 @@ +export default { + data: { + values: [ 'Alpha', 'Beta', 'Gamma' ], + selected: [ 'Beta' ] + }, + + html: ` + + + + + + +

Beta

`, + + test ( assert, component, target, window ) { + const inputs = target.querySelectorAll( 'input' ); + assert.equal( inputs[0].checked, false ); + assert.equal( inputs[1].checked, true ); + assert.equal( inputs[2].checked, false ); + + const event = new window.Event( 'change' ); + + inputs[0].checked = true; + inputs[0].dispatchEvent( event ); + + assert.equal( target.innerHTML, ` + + + + + + +

Alpha, Beta

+ ` ); + + component.set({ selected: [ 'Beta', 'Gamma' ] }); + assert.equal( inputs[0].checked, false ); + assert.equal( inputs[1].checked, true ); + assert.equal( inputs[2].checked, true ); + + assert.equal( target.innerHTML, ` + + + + + + +

Beta, Gamma

+ ` ); + } +}; diff --git a/test/generator/samples/binding-input-checkbox-group/main.html b/test/generator/samples/binding-input-checkbox-group/main.html new file mode 100644 index 000000000000..2ca1c29b5dca --- /dev/null +++ b/test/generator/samples/binding-input-checkbox-group/main.html @@ -0,0 +1,7 @@ +{{#each values as value}} + +{{/each}} + +

{{selected}}

\ No newline at end of file diff --git a/test/helpers.js b/test/helpers.js index cd5073081116..8d653fb19394 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -71,8 +71,12 @@ function cleanChildren ( node ) { child.data = child.data.replace( /\s{2,}/, '\n' ); + if ( child.data === '\n' ) { + node.removeChild( child ); + } + // text - if ( previous && previous.nodeType === 3 ) { + else if ( previous && previous.nodeType === 3 ) { previous.data += child.data; previous.data = previous.data.replace( /\s{2,}/, '\n' ); From 2c4c7079e5644bb9c836beefc3d355149e07d13e Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 16 Mar 2017 16:10:00 -0400 Subject: [PATCH 2/4] tweak htmlEqual helper so tests are easier to write --- test/helpers.js | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/test/helpers.js b/test/helpers.js index 8d653fb19394..905233280c5d 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -71,12 +71,8 @@ function cleanChildren ( node ) { child.data = child.data.replace( /\s{2,}/, '\n' ); - if ( child.data === '\n' ) { - node.removeChild( child ); - } - // text - else if ( previous && previous.nodeType === 3 ) { + if ( previous && previous.nodeType === 3 ) { previous.data += child.data; previous.data = previous.data.replace( /\s{2,}/, '\n' ); @@ -106,11 +102,11 @@ function cleanChildren ( node ) { export function setupHtmlEqual () { return env().then( window => { assert.htmlEqual = ( actual, expected, message ) => { - window.document.body.innerHTML = actual.trim(); + window.document.body.innerHTML = actual.replace( />[\s\r\n]+<' ).trim(); cleanChildren( window.document.body, '' ); actual = window.document.body.innerHTML; - window.document.body.innerHTML = expected.trim(); + window.document.body.innerHTML = expected.replace( />[\s\r\n]+<' ).trim(); cleanChildren( window.document.body, '' ); expected = window.document.body.innerHTML; From 5360bbf09f1035dbe4c8d01ee8afab351f0fa3ba Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 16 Mar 2017 16:46:41 -0400 Subject: [PATCH 3/4] make test harder to pass --- .../samples/binding-input-checkbox-group/_config.js | 12 ++++++++++-- .../samples/binding-input-checkbox-group/main.html | 4 ++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/test/generator/samples/binding-input-checkbox-group/_config.js b/test/generator/samples/binding-input-checkbox-group/_config.js index 0a0e02f7208f..154c4490cbfc 100644 --- a/test/generator/samples/binding-input-checkbox-group/_config.js +++ b/test/generator/samples/binding-input-checkbox-group/_config.js @@ -1,7 +1,15 @@ +const values = [ + { name: 'Alpha' }, + { name: 'Beta' }, + { name: 'Gamma' } +]; + export default { + // solo: true, + data: { - values: [ 'Alpha', 'Beta', 'Gamma' ], - selected: [ 'Beta' ] + values, + selected: [ values[1] ] }, html: ` diff --git a/test/generator/samples/binding-input-checkbox-group/main.html b/test/generator/samples/binding-input-checkbox-group/main.html index 2ca1c29b5dca..211c371e5316 100644 --- a/test/generator/samples/binding-input-checkbox-group/main.html +++ b/test/generator/samples/binding-input-checkbox-group/main.html @@ -1,7 +1,7 @@ {{#each values as value}} {{/each}} -

{{selected}}

\ No newline at end of file +

{{selected.map( function ( value ) { return value.name; }) }}

\ No newline at end of file From 7b057e4fc29f9d1fc24dfe3cbebf422dd7d8be6a Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Fri, 17 Mar 2017 17:45:25 -0400 Subject: [PATCH 4/4] implement bind:group for checkbox inputs (#312) --- src/generators/Generator.js | 2 + src/generators/dom/index.js | 4 ++ src/generators/dom/visitors/Element.js | 4 +- .../attributes/addElementAttributes.js | 18 +++-- .../visitors/attributes/addElementBinding.js | 71 ++++++++++++++++--- .../binding/getStaticAttributeValue.js | 11 +++ .../attributes/getStaticAttributeValue.js | 0 src/shared/dom.js | 8 +++ .../binding-input-checkbox-group/_config.js | 10 +-- .../binding-input-checkbox-group/main.html | 2 +- 10 files changed, 105 insertions(+), 25 deletions(-) create mode 100644 src/generators/dom/visitors/attributes/binding/getStaticAttributeValue.js create mode 100644 src/generators/dom/visitors/attributes/getStaticAttributeValue.js diff --git a/src/generators/Generator.js b/src/generators/Generator.js index f2757dbe37a9..f0571d96c163 100644 --- a/src/generators/Generator.js +++ b/src/generators/Generator.js @@ -22,6 +22,8 @@ export default class Generator { this.components = {}; this.events = {}; + this.bindingGroups = []; + // track which properties are needed, so we can provide useful info // in dev mode this.expectedProperties = {}; diff --git a/src/generators/dom/index.js b/src/generators/dom/index.js index 3c337a2f2a4a..406e676e95df 100644 --- a/src/generators/dom/index.js +++ b/src/generators/dom/index.js @@ -343,6 +343,10 @@ export default function dom ( parsed, source, options, names ) { ); } + if ( generator.bindingGroups.length ) { + constructorBlock.addLine( `this._bindingGroups = [ ${Array( generator.bindingGroups.length ).fill( '[]' ).join( ', ' )} ];` ); + } + constructorBlock.addBlock( deindent` this._observers = { pre: Object.create( null ), diff --git a/src/generators/dom/visitors/Element.js b/src/generators/dom/visitors/Element.js index 275f98429e44..0e76e0d708ed 100644 --- a/src/generators/dom/visitors/Element.js +++ b/src/generators/dom/visitors/Element.js @@ -20,7 +20,8 @@ export default { allUsedContexts: [], init: new CodeBuilder(), - update: new CodeBuilder() + update: new CodeBuilder(), + teardown: new CodeBuilder() }; const isToplevel = generator.current.localElementDepth === 0; @@ -85,6 +86,7 @@ export default { generator.current.builders.init.addBlock( local.init ); if ( !local.update.isEmpty() ) generator.current.builders.update.addBlock( local.update ); + if ( !local.teardown.isEmpty() ) generator.current.builders.teardown.addBlock( local.teardown ); generator.createMountStatement( name ); diff --git a/src/generators/dom/visitors/attributes/addElementAttributes.js b/src/generators/dom/visitors/attributes/addElementAttributes.js index a6e577de403c..f355354ab24f 100644 --- a/src/generators/dom/visitors/attributes/addElementAttributes.js +++ b/src/generators/dom/visitors/attributes/addElementAttributes.js @@ -2,6 +2,7 @@ import attributeLookup from './lookup.js'; import addElementBinding from './addElementBinding'; import deindent from '../../../../utils/deindent.js'; import flattenReference from '../../../../utils/flattenReference.js'; +import getStaticAttributeValue from './binding/getStaticAttributeValue.js'; export default function addElementAttributes ( generator, node, local ) { node.attributes.forEach( attribute => { @@ -13,8 +14,12 @@ export default function addElementAttributes ( generator, node, local ) { let dynamic = false; - const isBoundOptionValue = node.name === 'option' && name === 'value'; // TODO check it's actually bound - const propertyName = isBoundOptionValue ? '__value' : metadata && metadata.propertyName; + const isIndirectlyBoundValue = name === 'value' && ( + node.name === 'option' || // TODO check it's actually bound + node.name === 'input' && /^(checkbox|radio)$/.test( getStaticAttributeValue( node, 'type' ) ) + ); + + const propertyName = isIndirectlyBoundValue ? '__value' : metadata && metadata.propertyName; const isXlink = name.slice( 0, 6 ) === 'xlink:'; @@ -134,12 +139,11 @@ export default function addElementAttributes ( generator, node, local ) { local.update.addLine( updater ); } - if ( isBoundOptionValue ) { - local.init.addLine( `${local.name}.value = ${local.name}.__value` ); + if ( isIndirectlyBoundValue ) { + const updateValue = `${local.name}.value = ${local.name}.__value;`; - if (dynamic) { - local.update.addLine( `${local.name}.value = ${local.name}.__value` ); - } + local.init.addLine( updateValue ); + if ( dynamic ) local.update.addLine( updateValue ); } } diff --git a/src/generators/dom/visitors/attributes/addElementBinding.js b/src/generators/dom/visitors/attributes/addElementBinding.js index c96ed057d7b7..7958fc0480f6 100644 --- a/src/generators/dom/visitors/attributes/addElementBinding.js +++ b/src/generators/dom/visitors/attributes/addElementBinding.js @@ -1,9 +1,10 @@ import deindent from '../../../../utils/deindent.js'; import flattenReference from '../../../../utils/flattenReference.js'; import getSetter from './binding/getSetter.js'; +import getStaticAttributeValue from './binding/getStaticAttributeValue.js'; export default function createBinding ( generator, node, attribute, current, local ) { - const { name } = flattenReference( attribute.value ); + const { name, keypath } = flattenReference( attribute.value ); const { snippet, contexts, dependencies } = generator.contextualise( attribute.value ); if ( dependencies.length > 1 ) throw new Error( 'An unexpected situation arose. Please raise an issue at https://github.com/sveltejs/svelte/issues — thanks!' ); @@ -14,20 +15,20 @@ export default function createBinding ( generator, node, attribute, current, loc const handler = current.getUniqueName( `${local.name}ChangeHandler` ); - const isMultipleSelect = node.name === 'select' && node.attributes.find( attr => attr.name.toLowerCase() === 'multiple' ); // TODO ensure that this is a static attribute - const value = getBindingValue( local, node, attribute, isMultipleSelect ); + const isMultipleSelect = node.name === 'select' && node.attributes.find( attr => attr.name.toLowerCase() === 'multiple' ); // TODO use getStaticAttributeValue + const bindingGroup = attribute.name === 'group' ? getBindingGroup( generator, current, attribute, keypath ) : null; + const value = getBindingValue( generator, local, node, attribute, isMultipleSelect, bindingGroup ); const eventName = getBindingEventName( node ); let setter = getSetter({ current, name, context: '__svelte', attribute, dependencies, snippet, value }); - - // special case - if ( node.name === 'select' && !isMultipleSelect ) { - setter = `var selectedOption = ${local.name}.selectedOptions[0] || ${local.name}.options[0];\n` + setter; - } - let updateElement; + // special case + else if ( attribute.name === 'group' ) { + const type = getStaticAttributeValue( node, 'type' ); + + if ( type === 'checkbox' ) { + local.init.addLine( + `component._bindingGroups[${bindingGroup}].push( ${local.name} );` + ); + + local.teardown.addBlock( + `component._bindingGroups[${bindingGroup}].splice( component._bindingGroups[${bindingGroup}].indexOf( ${local.name} ), 1 );` + ); + + updateElement = `${local.name}.checked = ~${snippet}.indexOf( ${local.name}.__value );`; + } + + else if ( type === 'radio' ) { + throw new Error( 'TODO' ); + } + + else { + throw new Error( `Unexpected bind:group` ); // TODO catch this in validation with a better error + } + } + + // everything else + else { updateElement = `${local.name}.${attribute.name} = ${snippet};`; } @@ -93,14 +122,34 @@ function getBindingEventName ( node ) { return 'change'; } -function getBindingValue ( local, node, attribute, isMultipleSelect ) { +function getBindingValue ( generator, local, node, attribute, isMultipleSelect, bindingGroup ) { + // + if ( attribute.name === 'group' ) { + return `${generator.helper( 'getBindingGroupValue' )}( component._bindingGroups[${bindingGroup}] )`; + } + + // everything else return `${local.name}.${attribute.name}`; +} + +function getBindingGroup ( generator, current, attribute, keypath ) { + // TODO handle contextual bindings — `keypath` should include unique ID of + // each block that provides context + let index = generator.bindingGroups.indexOf( keypath ); + if ( index === -1 ) { + index = generator.bindingGroups.length; + generator.bindingGroups.push( keypath ); + } + + return index; } \ No newline at end of file diff --git a/src/generators/dom/visitors/attributes/binding/getStaticAttributeValue.js b/src/generators/dom/visitors/attributes/binding/getStaticAttributeValue.js new file mode 100644 index 000000000000..20e5183956c1 --- /dev/null +++ b/src/generators/dom/visitors/attributes/binding/getStaticAttributeValue.js @@ -0,0 +1,11 @@ +export default function getStaticAttributeValue ( node, name ) { + const attribute = node.attributes.find( attr => attr.name.toLowerCase() === name ); + if ( !attribute ) return null; + + if ( attribute.value.length !== 1 || attribute.value[0].type !== 'Text' ) { + // TODO catch this in validation phase, give a more useful error (with location etc) + throw new Error( `'${name} must be a static attribute` ); + } + + return attribute.value[0].data; +} \ No newline at end of file diff --git a/src/generators/dom/visitors/attributes/getStaticAttributeValue.js b/src/generators/dom/visitors/attributes/getStaticAttributeValue.js new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/src/shared/dom.js b/src/shared/dom.js index 574b672ce567..5657a1133365 100644 --- a/src/shared/dom.js +++ b/src/shared/dom.js @@ -53,3 +53,11 @@ export function setAttribute ( node, attribute, value ) { export function setXlinkAttribute ( node, attribute, value ) { node.setAttributeNS( 'http://www.w3.org/1999/xlink', attribute, value ); } + +export function getBindingGroupValue ( group ) { + var value = []; + for ( var i = 0; i < group.length; i += 1 ) { + if ( group[i].checked ) value.push( group[i].__value ); + } + return value; +} \ No newline at end of file diff --git a/test/generator/samples/binding-input-checkbox-group/_config.js b/test/generator/samples/binding-input-checkbox-group/_config.js index 154c4490cbfc..fac1795eca26 100644 --- a/test/generator/samples/binding-input-checkbox-group/_config.js +++ b/test/generator/samples/binding-input-checkbox-group/_config.js @@ -5,13 +5,13 @@ const values = [ ]; export default { - // solo: true, - data: { values, selected: [ values[1] ] }, + 'skip-ssr': true, // values are rendered as [object Object] + html: ` {{/each}} -

{{selected.map( function ( value ) { return value.name; }) }}

\ No newline at end of file +

{{selected.map( function ( value ) { return value.name; }).join( ', ' ) }}

\ No newline at end of file