From 599ac1663275f153341405230d84dd033eaf89b0 Mon Sep 17 00:00:00 2001 From: David Greenspan Date: Mon, 27 Jan 2014 14:39:35 -0800 Subject: [PATCH] Treat null/undefined/false attributes as absent So, in HTML, the following are equivalent, and both mean that a checkbox is checked, because the `checked` attribute is present: - `` - `` We can't mess with that. On the other hand, in Spacebars before this commit, the following would *also* result in the checkbox being checked, regardless of whether `foo` evaluates to null, undefined, false, or the empty string: - `` - `` With this commit, the checkbox will NOT be checked if `foo` evaluates to null, undefined, or false. To achieve this: - In HTMLjs, an attribute is considered absent if its value is "nully" after being fully evaluated (i.e. after expanding functions and components via HTML.evaluateDynamicAttributes / HTML.evaluate). A nully value is one consisting of null, undefined, an empty array, or an array of those things. `false` is not nully and renders as "false". An empty string is not nully, and will "prop open" an attribute that would otherwise collapse into absence. - Spacebars.mustache converts null, undefined, and false to null. So if you use {{foo}} anywhere in a template and foo evaluates to "false", that gets to converted to a null in HTMLjs (which is ignored). (true is rendered as "true".) - When parsing HTML, an attribute that consists of *no tokens* becomes an empty string in the HTMLjs, which props open the attribute (unlike null or an empty array). (Since comment tokens are stripped during tokenization, if there are only comments in an attribute value that counts as no tokens.) --- packages/html-tools/parse.js | 26 +++--- packages/htmljs/html.js | 4 +- packages/htmljs/htmljs_test.js | 4 + .../spacebars-compiler/spacebars_tests.js | 9 ++ packages/spacebars-compiler/tojs.js | 3 +- packages/spacebars-tests/template_tests.html | 24 +++++ packages/spacebars-tests/template_tests.js | 91 ++++++++++++++++++- packages/spacebars/spacebars-runtime.js | 11 ++- packages/templating/templating_tests.js | 4 +- packages/ui/render_tests.js | 18 +++- 10 files changed, 168 insertions(+), 26 deletions(-) diff --git a/packages/html-tools/parse.js b/packages/html-tools/parse.js index 6841fa9b54..df7c9d765b 100644 --- a/packages/html-tools/parse.js +++ b/packages/html-tools/parse.js @@ -294,12 +294,17 @@ var convertCharRef = function (token) { }; // Input is always a dictionary (even if zero attributes) and each -// value in the dictionary is an array of `Chars` and `CharRef` -// tokens. An empty array means the attribute has a value of "". +// value in the dictionary is an array of `Chars`, `CharRef`, +// and maybe `Special` tokens. // // Output is null if there are zero attributes, and otherwise a -// dictionary. Each value in the dictionary is a string (possibly -// empty) or an array of non-empty strings and CharRef tags. +// dictionary. Each value in the dictionary is HTMLjs (e.g. a +// string or an array of `Chars`, `CharRef`, and `Special` +// nodes). +// +// An attribute value with no input tokens is represented as "", +// not an empty array, in order to prop open empty attributes +// with no template tags. var parseAttrs = function (attrs) { var result = null; @@ -316,13 +321,7 @@ var parseAttrs = function (attrs) { } else if (token.t === 'Special') { outParts.push(HTML.Special(token.v)); } else if (token.t === 'Chars') { - var str = token.v; - var N = outParts.length; - - if (N && (typeof outParts[N - 1] === 'string')) - outParts[N - 1] += str; - else - outParts.push(str); + pushOrAppendString(outParts, token.v); } } @@ -331,9 +330,8 @@ var parseAttrs = function (attrs) { // array, even if there is only one Special. result[k] = outParts; } else { - var outValue = (outParts.length === 0 ? '' : - (outParts.length === 1 ? outParts[0] : - outParts)); + var outValue = (inValue.length === 0 ? '' : + (outParts.length === 1 ? outParts[0] : outParts)); var properKey = HTML.properCaseAttributeName(k); result[properKey] = outValue; } diff --git a/packages/htmljs/html.js b/packages/htmljs/html.js index 3d6f84d33a..e6aa67ac27 100644 --- a/packages/htmljs/html.js +++ b/packages/htmljs/html.js @@ -15,7 +15,7 @@ HTML.evaluate = function (node, parentComponent) { if (node == null) { return node; } else if (typeof node === 'function') { - return node(); + return HTML.evaluate(node(), parentComponent); } else if (node instanceof Array) { var result = []; for (var i = 0; i < node.length; i++) @@ -24,7 +24,7 @@ HTML.evaluate = function (node, parentComponent) { } else if (typeof node.instantiate === 'function') { // component var instance = node.instantiate(parentComponent || null); - var content = instance.render(); + var content = instance.render('STATIC'); return HTML.evaluate(content, instance); } else if (node instanceof HTML.Tag) { var newChildren = []; diff --git a/packages/htmljs/htmljs_test.js b/packages/htmljs/htmljs_test.js index 9e0c35efc1..6c5d940259 100644 --- a/packages/htmljs/htmljs_test.js +++ b/packages/htmljs/htmljs_test.js @@ -105,4 +105,8 @@ Tinytest.add("htmljs - attributes", function (test) { test.equal(HTML.evaluateDynamicAttributes({x: function () { return 'abc'; }, $dynamic: [{ x: function () { return 'def'; }}]}), { x: 'def' }); +}); + +Tinytest.add("htmljs - details", function (test) { + test.equal(HTML.toHTML(false), "false"); }); \ No newline at end of file diff --git a/packages/spacebars-compiler/spacebars_tests.js b/packages/spacebars-compiler/spacebars_tests.js index e7ea10e478..176ba50f10 100644 --- a/packages/spacebars-compiler/spacebars_tests.js +++ b/packages/spacebars-compiler/spacebars_tests.js @@ -224,4 +224,13 @@ Tinytest.add("spacebars - parse", function (test) { test.equal(HTML.toJS(Spacebars.parse('')), 'HTML.A({b: "c"})'); + + // currently, if there are only comments, the attribute is truthy. This is + // because comments are stripped during tokenization. If we include + // comments in the token stream, these cases will become falsy for selected. + test.equal(HTML.toJS(Spacebars.parse('')), + 'HTML.INPUT({selected: ""})'); + test.equal(HTML.toJS(Spacebars.parse('')), + 'HTML.INPUT({selected: ""})'); + }); diff --git a/packages/spacebars-compiler/tojs.js b/packages/spacebars-compiler/tojs.js index ad85985d06..a7534a2ae7 100644 --- a/packages/spacebars-compiler/tojs.js +++ b/packages/spacebars-compiler/tojs.js @@ -38,7 +38,8 @@ HTML.Tag.prototype.toJS = function (options) { kvStrs.push(toObjectLiteralKey(k) + ': ' + HTML.toJS(this.attrs[k], options)); } - argStrs.push('{' + kvStrs.join(', ') + '}'); + if (kvStrs.length) + argStrs.push('{' + kvStrs.join(', ') + '}'); } for (var i = 0; i < this.children.length; i++) { diff --git a/packages/spacebars-tests/template_tests.html b/packages/spacebars-tests/template_tests.html index 3034956f46..a5c4da1552 100644 --- a/packages/spacebars-tests/template_tests.html +++ b/packages/spacebars-tests/template_tests.html @@ -52,6 +52,10 @@ {{{html}}} + + @@ -424,3 +428,23 @@ Hi there! + + + + + + + + + + diff --git a/packages/spacebars-tests/template_tests.js b/packages/spacebars-tests/template_tests.js index efb82ade7c..d74ec312a2 100644 --- a/packages/spacebars-tests/template_tests.js +++ b/packages/spacebars-tests/template_tests.js @@ -140,6 +140,13 @@ Tinytest.add("spacebars - templates - triple", function (test) { span = elems[0]; test.equal(span.className, 'hi'); test.equal(stripComments(span.innerHTML), 'blah'); + + var tmpl = Template.spacebars_template_test_triple2; + tmpl.html = function () {}; + tmpl.html2 = function () { return null; }; + // no tmpl.html3 + div = renderToDiv(tmpl); + test.equal(stripComments(div.innerHTML), 'xy'); }); Tinytest.add("spacebars - templates - inclusion args", function (test) { @@ -784,7 +791,6 @@ testAsyncMulti('spacebars - template - rendered template is DOM in rendered call function (test, expect) { var tmpl = Template.spacebars_template_test_aaa; tmpl.rendered = expect(function () { - console.log('in rendered'); test.equal(trim(stripComments(div.innerHTML)), "aaa"); }); var div = renderToDiv(tmpl); @@ -1251,3 +1257,86 @@ Tinytest.add('spacebars - templates - inclusion helpers are isolated', function Deps.flush({_throwErrors: true}); }, /Expected null or template/); }); + +Tinytest.add('spacebars - templates - nully attributes', function (test) { + var tmpls = { + 0: Template.spacebars_template_test_nully_attributes0, + 1: Template.spacebars_template_test_nully_attributes1, + 2: Template.spacebars_template_test_nully_attributes2, + 3: Template.spacebars_template_test_nully_attributes3, + 4: Template.spacebars_template_test_nully_attributes4, + 5: Template.spacebars_template_test_nully_attributes5, + 6: Template.spacebars_template_test_nully_attributes6 + }; + + var run = function (whichTemplate, data, expectTrue) { + //var withData = UI.With(function () { return data; }, + //tmpls[whichTemplate]); + var templateWithData = tmpls[whichTemplate].withData(function () { + return data; }); + var div = renderToDiv(templateWithData); + var input = div.querySelector('input'); + var descr = JSON.stringify([whichTemplate, data, expectTrue]); + if (expectTrue) { + test.isTrue(input.checked, descr); + test.equal(typeof input.getAttribute('stuff'), 'string', descr); + } else { + test.isFalse(input.checked); + test.equal(JSON.stringify(input.getAttribute('stuff')), 'null', descr); + } + + var html = HTML.toHTML(templateWithData); + test.equal(/ checked="[^"]*"/.test(html), !! expectTrue); + test.equal(/ stuff="[^"]*"/.test(html), !! expectTrue); + }; + + run(0, {}, true); + + var truthies = [true, '']; + var falsies = [false, null, undefined]; + + _.each(truthies, function (x) { + run(1, {foo: x}, true); + }); + _.each(falsies, function (x) { + run(1, {foo: x}, false); + }); + + _.each(truthies, function (x) { + _.each(truthies, function (y) { + run(2, {foo: x, bar: y}, true); + }); + _.each(falsies, function (y) { + run(2, {foo: x, bar: y}, true); + }); + }); + _.each(falsies, function (x) { + _.each(truthies, function (y) { + run(2, {foo: x, bar: y}, true); + }); + _.each(falsies, function (y) { + run(2, {foo: x, bar: y}, false); + }); + }); + + run(3, {foo: true}, false); + run(3, {foo: false}, false); +}); + +Tinytest.add("spacebars - templates - double", function (test) { + var tmpl = Template.spacebars_template_test_double; + + var run = function (foo, expectedResult) { + tmpl.foo = foo; + var div = renderToDiv(tmpl); + test.equal(stripComments(div.innerHTML), expectedResult); + }; + + run('asdf', 'asdf'); + run(1.23, '1.23'); + run(0, '0'); + run(true, 'true'); + run(false, ''); + run(null, ''); + run(undefined, ''); +}); diff --git a/packages/spacebars/spacebars-runtime.js b/packages/spacebars/spacebars-runtime.js index 53026443d9..86159eb29b 100644 --- a/packages/spacebars/spacebars-runtime.js +++ b/packages/spacebars/spacebars-runtime.js @@ -125,9 +125,10 @@ Spacebars.mustache = function (value/*, args*/) { if (result instanceof Handlebars.SafeString) return HTML.Raw(result.toString()); else - // map `null` and `undefined` to "", stringify anything else - // (e.g. strings, booleans, numbers including 0). - return String(result == null ? '' : result); + // map `null`, `undefined`, and `false` to null, which is important + // so that attributes with nully values are considered absent. + // stringify anything else (e.g. strings, booleans, numbers including 0). + return (result == null || result === false) ? null : String(result); }; Spacebars.attrMustache = function (value/*, args*/) { @@ -151,7 +152,9 @@ Spacebars.attrMustache = function (value/*, args*/) { // Called on the return value from `Spacebars.mustache` in case the // template uses triple-stache (`{{{foo bar baz}}}`). Spacebars.makeRaw = function (value) { - if (value instanceof HTML.Raw) + if (value == null) // null or undefined + return null; + else if (value instanceof HTML.Raw) return value; else return HTML.Raw(value); diff --git a/packages/templating/templating_tests.js b/packages/templating/templating_tests.js index 20ef216d55..c19deff2aa 100644 --- a/packages/templating/templating_tests.js +++ b/packages/templating/templating_tests.js @@ -583,10 +583,10 @@ Tinytest.add('templating - helper typecast Issue #617', function (test) { }); Tinytest.add('templating - each falsy Issue #801', function (test) { - //Minor test for issue #801 + //Minor test for issue #801 (#each over array containing nulls) Template.test_template_issue801.values = function() { return [0,1,2,null,undefined,false]; }; var div = renderToDiv(Template.test_template_issue801); - test.equal(canonicalizeHtml(div.innerHTML), "012false"); + test.equal(canonicalizeHtml(div.innerHTML), "012"); }); Tinytest.add('templating - duplicate template error', function (test) { diff --git a/packages/ui/render_tests.js b/packages/ui/render_tests.js index 9663cfc97f..f251bb9d63 100644 --- a/packages/ui/render_tests.js +++ b/packages/ui/render_tests.js @@ -21,7 +21,8 @@ Tinytest.add("ui - render - basic", function (test) { materialize(input, div); test.equal(canonicalizeHtml(div.innerHTML), expectedInnerHTML); test.equal(toHTML(input), expectedHTML); - test.equal(toCode(input), expectedCode); + if (typeof expectedCode !== 'undefined') + test.equal(toCode(input), expectedCode); }; run(P('Hello'), @@ -73,6 +74,20 @@ Tinytest.add("ui - render - basic", function (test) { '
', 'HTML.DIV({"class": "foo"}, HTML.UL(HTML.LI(HTML.P(HTML.A({href: "#one"}, "One"))), HTML.LI(HTML.P("Two", HTML.BR(), "Three"))))'); + + // Test nully attributes + run(BR({x: null, + y: [[], []], + a: [['']]}), + '
', + '
', + 'HTML.BR({a: [[""]]})'); + + run(BR({ + x: function () { return function () { return []; }; }, + a: function () { return function () { return ''; }; }}), + '
', + '
'); }); // test that we correctly update the 'value' property on input fields @@ -640,4 +655,3 @@ Tinytest.add("ui - render - SVG", function (test) { test.equal(circle.namespaceURI, "http://www.w3.org/2000/svg"); test.equal(circle.parentNode.namespaceURI, "http://www.w3.org/2000/svg"); }); -