From 1dea49dbb804f8c76dddd46ebef29316867db72b Mon Sep 17 00:00:00 2001
From: Avital Oliver <avital@thewe.net>
Date: Wed, 6 Nov 2013 16:57:35 -0800
Subject: [PATCH] Reactive SELECT tags work in all browsers.

---
 packages/animation/animated_each.js        |   4 +-
 packages/spacebars-tests/template_tests.js | 142 +++++++++++----------
 packages/test-helpers/canonicalize_html.js |  40 +++---
 packages/ui/domrange.js                    |   2 +-
 packages/ui/render.js                      |   6 +-
 5 files changed, 101 insertions(+), 93 deletions(-)

diff --git a/packages/animation/animated_each.js b/packages/animation/animated_each.js
index 6192ab074e..e5e8740656 100644
--- a/packages/animation/animated_each.js
+++ b/packages/animation/animated_each.js
@@ -234,8 +234,6 @@ var apply = function (el, events) {
 
   events = events || ['add', 'remove', 'move'];
 
-  // xcxc make these events accept functions, so that we can not
-  // animate initial data but still animate subsequent inserts
   if (_.contains(events, 'add')) {
     $(el)[0].$uihooks.insertElement = function (n, parent, next) {
       animateInsert(n, parent, next);
@@ -265,7 +263,7 @@ AnimatedList = Package.ui.Component.extend({
     var self = this;
     var childEls = _.filter(self.$('*'), function (n) {
       return n.parentNode === self.firstNode().parentNode;
-    }); // xcxc we'd like something like jquery's `.children()`
+    }); // XXX we'd like something like jquery's `.children()`
     if (childEls.length !== 1)
       throw new Error("#AnimatedList must have precisely one top-level child element");
     apply(childEls, self.events && self.events.split(' '));
diff --git a/packages/spacebars-tests/template_tests.js b/packages/spacebars-tests/template_tests.js
index e8a4c2291b..cf7a045da0 100644
--- a/packages/spacebars-tests/template_tests.js
+++ b/packages/spacebars-tests/template_tests.js
@@ -4,6 +4,11 @@ var renderToDiv = function (comp) {
   return div;
 };
 
+// strip empty comments created by DomRange on IE
+var stripComments = function (str) {
+  return str.replace(/\<\!--IE--\>/g, '');
+};
+
 var trim = function (str) {
   return str.replace(/^\s+|\s+$/g, '');
 };
@@ -22,7 +27,7 @@ Tinytest.add("spacebars - templates - simple helper", function (test) {
   };
   var div = renderToDiv(tmpl);
 
-  test.equal(div.innerHTML, "124");
+  test.equal(stripComments(div.innerHTML), "124");
 });
 
 Tinytest.add("spacebars - templates - dynamic template", function (test) {
@@ -34,12 +39,12 @@ Tinytest.add("spacebars - templates - dynamic template", function (test) {
     return R.get() === 'aaa' ? aaa : bbb;
   };
   var div = renderToDiv(tmpl);
-  test.equal(div.innerHTML, "aaa");
+  test.equal(stripComments(div.innerHTML), "aaa");
 
   R.set('bbb');
   Deps.flush();
 
-  test.equal(div.innerHTML, "bbb");
+  test.equal(stripComments(div.innerHTML), "bbb");
 });
 
 Tinytest.add("spacebars - templates - interpolate attribute", function (test) {
@@ -77,7 +82,7 @@ Tinytest.add("spacebars - templates - dynamic attrs", function (test) {
   R2.set('');
   R3.set('');
   Deps.flush();
-  test.equal(span.innerHTML, 'hi');
+  test.equal(stripComments(span.innerHTML), 'hi');
   test.isFalse(span.hasAttribute('n'));
   test.isFalse(span.hasAttribute('selected'));
   test.equal(span.getAttribute('zanzibar'), 'where the heart is');
@@ -101,7 +106,7 @@ Tinytest.add("spacebars - templates - triple", function (test) {
   Deps.flush();
   elems = $(div).find("> *");
   test.equal(elems.length, 0);
-  test.equal(div.innerHTML, 'asdf');
+  test.equal(stripComments(div.innerHTML), 'asdf');
 
   R.set('<span class="hi">blah</span>');
   Deps.flush();
@@ -110,7 +115,7 @@ Tinytest.add("spacebars - templates - triple", function (test) {
   test.equal(elems[0].nodeName, 'SPAN');
   span = elems[0];
   test.equal(span.className, 'hi');
-  test.equal(span.innerHTML, 'blah');
+  test.equal(stripComments(span.innerHTML), 'blah');
 });
 
 Tinytest.add("spacebars - templates - inclusion args", function (test) {
@@ -122,31 +127,31 @@ Tinytest.add("spacebars - templates - inclusion args", function (test) {
   var div = renderToDiv(tmpl);
   // `{{> foo bar}}`, with `foo` resolving to Template.aaa,
   // which consists of "aaa"
-  test.equal(div.innerHTML, 'aaa');
+  test.equal(stripComments(div.innerHTML), 'aaa');
   R.set(Template.spacebars_template_test_bbb);
   Deps.flush();
-  test.equal(div.innerHTML, 'bbb');
+  test.equal(stripComments(div.innerHTML), 'bbb');
 
   ////// Ok, now `foo` *is* Template.aaa
   tmpl.foo = Template.spacebars_template_test_aaa;
   div = renderToDiv(tmpl);
-  test.equal(div.innerHTML, 'aaa');
+  test.equal(stripComments(div.innerHTML), 'aaa');
 
   ////// Ok, now `foo` is a template that takes an argument; bar is a string.
   tmpl.foo = Template.spacebars_template_test_bracketed_this;
   tmpl.bar = 'david';
   div = renderToDiv(tmpl);
-  test.equal(div.innerHTML, '[david]');
+  test.equal(stripComments(div.innerHTML), '[david]');
 
   ////// Now `foo` is a template that takes an arg; bar is a function.
   tmpl.foo = Template.spacebars_template_test_bracketed_this;
   R = ReactiveVar('david');
   tmpl.bar = function () { return R.get(); };
   div = renderToDiv(tmpl);
-  test.equal(div.innerHTML, '[david]');
+  test.equal(stripComments(div.innerHTML), '[david]');
   R.set('avi');
   Deps.flush();
-  test.equal(div.innerHTML, '[avi]');
+  test.equal(stripComments(div.innerHTML), '[avi]');
 });
 
 Tinytest.add("spacebars - templates - inclusion args 2", function (test) {
@@ -161,13 +166,13 @@ Tinytest.add("spacebars - templates - inclusion args 2", function (test) {
   tmpl.bar = 4;
   tmpl.baz = function () { return R.get(); };
   var div = renderToDiv(tmpl);
-  test.equal(div.innerHTML, '[7]');
+  test.equal(stripComments(div.innerHTML), '[7]');
   R.set(11);
   Deps.flush();
-  test.equal(div.innerHTML, '[15]');
+  test.equal(stripComments(div.innerHTML), '[15]');
   R.set(999);
   Deps.flush();
-  test.equal(div.innerHTML, 'aaa');
+  test.equal(stripComments(div.innerHTML), 'aaa');
 });
 
 Tinytest.add("spacebars - templates - inclusion args 3", function (test) {
@@ -182,11 +187,11 @@ Tinytest.add("spacebars - templates - inclusion args 3", function (test) {
   tmpl.bar = function () { return R1.get(); };
   tmpl.baz = function () { return R2.get(); };
   var div = renderToDiv(tmpl);
-  test.equal(div.innerHTML, '7');
+  test.equal(stripComments(div.innerHTML), '7');
   R1.set(11);
   R2.set(13);
   Deps.flush();
-  test.equal(div.innerHTML, '24');
+  test.equal(stripComments(div.innerHTML), '24');
 
   tmpl.foo = UI.Component.extend({
     render: function (buf) {
@@ -200,11 +205,11 @@ Tinytest.add("spacebars - templates - inclusion args 3", function (test) {
   R1 = ReactiveVar(20);
   R2 = ReactiveVar(23);
   div = renderToDiv(tmpl);
-  test.equal(div.innerHTML, '43');
+  test.equal(stripComments(div.innerHTML), '43');
   R1.set(10);
   R2.set(17);
   Deps.flush();
-  test.equal(div.innerHTML, '27');
+  test.equal(stripComments(div.innerHTML), '27');
 });
 
 Tinytest.add("spacebars - templates - inclusion dotted args", function (test) {
@@ -223,11 +228,11 @@ Tinytest.add("spacebars - templates - inclusion dotted args", function (test) {
 
   var div = renderToDiv(tmpl.withData({symbol:'%'}));
   test.equal(initCount, 1);
-  test.equal(div.innerHTML, '[%david]');
+  test.equal(stripComments(div.innerHTML), '[%david]');
 
   R.set('avi');
   Deps.flush();
-  test.equal(div.innerHTML, '[%avi]');
+  test.equal(stripComments(div.innerHTML), '[%avi]');
   // check that invalidating the argument to `foo` doesn't require
   // creating a new `foo`.
   test.equal(initCount, 1);
@@ -249,7 +254,7 @@ Tinytest.add("spacebars - templates - inclusion slashed args", function (test) {
 
   var div = renderToDiv(tmpl.withData({symbol:'%'}));
   test.equal(initCount, 1);
-  test.equal(div.innerHTML, '[%david]');
+  test.equal(stripComments(div.innerHTML), '[%david]');
 });
 
 Tinytest.add("spacebars - templates - block helper", function (test) {
@@ -259,11 +264,11 @@ Tinytest.add("spacebars - templates - block helper", function (test) {
     return R.get();
   };
   var div = renderToDiv(tmpl);
-  test.equal(trim(div.innerHTML), "bar");
+  test.equal(trim(stripComments(div.innerHTML)), "bar");
 
   R.set(Template.spacebars_template_test_elsecontent);
   Deps.flush();
-  test.equal(trim(div.innerHTML), "baz");
+  test.equal(trim(stripComments(div.innerHTML)), "baz");
 });
 
 Tinytest.add("spacebars - templates - block helper function with one string arg", function (test) {
@@ -275,7 +280,7 @@ Tinytest.add("spacebars - templates - block helper function with one string arg"
       return null;
   };
   var div = renderToDiv(tmpl);
-  test.equal(trim(div.innerHTML), "content");
+  test.equal(trim(stripComments(div.innerHTML)), "content");
 });
 
 Tinytest.add("spacebars - templates - block helper function with one helper arg", function (test) {
@@ -289,11 +294,11 @@ Tinytest.add("spacebars - templates - block helper function with one helper arg"
       return null;
   };
   var div = renderToDiv(tmpl);
-  test.equal(trim(div.innerHTML), "content");
+  test.equal(trim(stripComments(div.innerHTML)), "content");
 
   R.set("baz");
   Deps.flush();
-  test.equal(trim(div.innerHTML), "");
+  test.equal(trim(stripComments(div.innerHTML)), "");
 });
 
 Tinytest.add("spacebars - templates - block helper component with one helper arg", function (test) {
@@ -301,11 +306,11 @@ Tinytest.add("spacebars - templates - block helper component with one helper arg
   var R = ReactiveVar(true);
   tmpl.bar = function () { return R.get(); };
   var div = renderToDiv(tmpl);
-  test.equal(trim(div.innerHTML), "content");
+  test.equal(trim(stripComments(div.innerHTML)), "content");
 
   R.set(false);
   Deps.flush();
-  test.equal(trim(div.innerHTML), "");
+  test.equal(trim(stripComments(div.innerHTML)), "");
 });
 
 Tinytest.add("spacebars - templates - block helper component with three helper args", function (test) {
@@ -318,11 +323,11 @@ Tinytest.add("spacebars - templates - block helper component with three helper a
     return x === y;
   };
   var div = renderToDiv(tmpl);
-  test.equal(trim(div.innerHTML), "content");
+  test.equal(trim(stripComments(div.innerHTML)), "content");
 
   R.set("baz");
   Deps.flush();
-  test.equal(trim(div.innerHTML), "");
+  test.equal(trim(stripComments(div.innerHTML)), "");
 });
 
 Tinytest.add("spacebars - templates - block helper with dotted arg", function (test) {
@@ -346,37 +351,37 @@ Tinytest.add("spacebars - templates - block helper with dotted arg", function (t
   tmpl.qux = function () { return R3.get(); };
 
   var div = renderToDiv(tmpl);
-  test.equal(div.innerHTML, "[111]");
+  test.equal(stripComments(stripComments(div.innerHTML)), "[111]");
   test.equal(initCount, 1);
 
   R1.set(2);
   Deps.flush();
-  test.equal(div.innerHTML, "[112]");
+  test.equal(stripComments(div.innerHTML), "[112]");
   test.equal(initCount, 1);
 
   R2.set(20);
   Deps.flush();
-  test.equal(div.innerHTML, "[122]");
+  test.equal(stripComments(div.innerHTML), "[122]");
   test.equal(initCount, 1);
 
   R3.set(200);
   Deps.flush();
-  test.equal(div.innerHTML, "[222]");
+  test.equal(stripComments(div.innerHTML), "[222]");
   test.equal(initCount, 1);
 
   R2.set(30);
   Deps.flush();
-  test.equal(div.innerHTML, "[232]");
+  test.equal(stripComments(div.innerHTML), "[232]");
   test.equal(initCount, 1);
 
   R1.set(3);
   Deps.flush();
-  test.equal(div.innerHTML, "[233]");
+  test.equal(stripComments(div.innerHTML), "[233]");
   test.equal(initCount, 1);
 
   R3.set(300);
   Deps.flush();
-  test.equal(div.innerHTML, "[333]");
+  test.equal(stripComments(div.innerHTML), "[333]");
   test.equal(initCount, 1);
 });
 
@@ -407,13 +412,13 @@ Tinytest.add("spacebars - templates - nested content", function (test) {
     return R.get();
   };
   var div = renderToDiv(tmpl);
-  test.equal(trim(div.innerHTML), 'hello');
+  test.equal(trim(stripComments(div.innerHTML)), 'hello');
   R.set(false);
   Deps.flush();
-  test.equal(trim(div.innerHTML), 'world');
+  test.equal(trim(stripComments(div.innerHTML)), 'world');
   R.set(true);
   Deps.flush();
-  test.equal(trim(div.innerHTML), 'hello');
+  test.equal(trim(stripComments(div.innerHTML)), 'hello');
 
   // Also test that `{{> content}}` in a custom block helper works.
   tmpl = Template.spacebars_template_test_nested_content2;
@@ -422,13 +427,13 @@ Tinytest.add("spacebars - templates - nested content", function (test) {
     return R.get();
   };
   div = renderToDiv(tmpl);
-  test.equal(trim(div.innerHTML), 'hello');
+  test.equal(trim(stripComments(div.innerHTML)), 'hello');
   R.set(false);
   Deps.flush();
-  test.equal(trim(div.innerHTML), 'world');
+  test.equal(trim(stripComments(div.innerHTML)), 'world');
   R.set(true);
   Deps.flush();
-  test.equal(trim(div.innerHTML), 'hello');
+  test.equal(trim(stripComments(div.innerHTML)), 'hello');
 });
 
 Tinytest.add("spacebars - templates - ..", function (test) {
@@ -442,13 +447,16 @@ Tinytest.add("spacebars - templates - ..", function (test) {
   tmpl.foo.bar.items = [{title: "item"}];
   var div = renderToDiv(tmpl);
 
-  test.equal(
-    trim(div.innerHTML).replace(/ /g, '').split('\n'),
-    [
-      // {{> spacebars_template_test_dots_subtemplate}}
-      "item", "item", "bar", "foo", "item", "bar", "foo",
-      // {{> spacebars_template_test_dots_subtemplate ..}}
-      "bar", "bar", "item", "bar", "bar", "item", "bar"]);
+  // XXX this is disgusting, i know.
+  var htmlWithWhitespace = div.innerHTML.replace(/\<\!--IE--\>/g, ' ');
+  var lines = _.filter(trim(htmlWithWhitespace).split(/\s/), function (line) {
+    return line !== "";
+  });
+  test.equal(lines, [
+    // {{> spacebars_template_test_dots_subtemplate}}
+    "item", "item", "bar", "foo", "item", "bar", "foo",
+    // {{> spacebars_template_test_dots_subtemplate ..}}
+    "bar", "bar", "item", "bar", "bar", "item", "bar"]);
 });
 
 Tinytest.add("spacebars - templates - select tags", function (test) {
@@ -465,25 +473,20 @@ Tinytest.add("spacebars - templates - select tags", function (test) {
   tmpl.selectedAttr = function () { return this.selected ? "selected" : ""; };
 
   var div = renderToDiv(tmpl);
+  // Find <select> element. Not `div.firstChild` since that's the
+  // DomRange marker (a text or comment node)
+  var selectEl = div.firstChild.nextSibling;
 
-  // returns contents of `div` in the form eg ["<select>", "</select>"]
+  // returns canonicalized contents of `div` in the form eg
+  // ["<select>", "</select>"].
   var divContent = function () {
-    var lines = trim(div.innerHTML)
-      .replace(/\>\</g, '>\n<')
+    var lines = trim(canonicalizeHtml(div.innerHTML))
+      .replace(/selected="selected"/g, 'selected=""')
+      .replace(/\>\s*\</g, '>\n<')
       .split('\n');
-    var trimmedLines = _.filter(
+    return trimmedLines = _.filter(
       _.map(lines, trim),
       function (x) { return x !== ""; });
-
-    // ensure that `selected=""` appears at the end of the attribute list
-    // if present
-    return _.map(trimmedLines, function (line) {
-      if (/^\<option selected="" /.test(line)) {
-        return line.replace('selected="" ', '').replace('>', ' selected="">');
-      } else {
-        return line;
-      }
-    });
   };
   test.equal(divContent(), ["<select>", "</select>"]);
 
@@ -504,12 +507,13 @@ Tinytest.add("spacebars - templates - select tags", function (test) {
     '<select>',
     '<optgroup label="one">',
     '<option value="value1">label1</option>',
-    '<option value="value2" selected="">label2</option>',
+    '<option selected="" value="value2">label2</option>',
     '</optgroup>',
     '<optgroup label="two">',
     '</optgroup>',
     '</select>'
   ]);
+  test.equal(selectEl.value, "value2");
 
   // swap selection
   options.update({value: "value1"}, {$set: {selected: true}});
@@ -519,13 +523,14 @@ Tinytest.add("spacebars - templates - select tags", function (test) {
   test.equal(divContent(), [
     '<select>',
     '<optgroup label="one">',
-    '<option value="value1" selected="">label1</option>',
+    '<option selected="" value="value1">label1</option>',
     '<option value="value2">label2</option>',
     '</optgroup>',
     '<optgroup label="two">',
     '</optgroup>',
     '</select>'
   ]);
+  test.equal(selectEl.value, "value1");
 
   // change value and label
   options.update({value: "value1"}, {$set: {value: "value1.0"}});
@@ -535,13 +540,14 @@ Tinytest.add("spacebars - templates - select tags", function (test) {
   test.equal(divContent(), [
     '<select>',
     '<optgroup label="one">',
-    '<option value="value1.0" selected="">label1</option>',
+    '<option selected="" value="value1.0">label1</option>',
     '<option value="value2">label2.0</option>',
     '</optgroup>',
     '<optgroup label="two">',
     '</optgroup>',
     '</select>'
   ]);
+  test.equal(selectEl.value, "value1.0");
 
 });
 
diff --git a/packages/test-helpers/canonicalize_html.js b/packages/test-helpers/canonicalize_html.js
index 633c97cce8..c1798cd2d4 100644
--- a/packages/test-helpers/canonicalize_html.js
+++ b/packages/test-helpers/canonicalize_html.js
@@ -1,6 +1,6 @@
 canonicalizeHtml = function(html) {
   var h = html;
-  // kill IE-specific comments inserted by Spark
+  // kill IE-specific comments inserted by DomRange
   h = h.replace(/<!--IE-->/g, '');
   // ignore exact text of comments
   h = h.replace(/<!--.*?-->/g, '<!---->');
@@ -14,29 +14,33 @@ canonicalizeHtml = function(html) {
     // Drop expando property used by Sizzle (part of jQuery) which leaks into
     // attributes in IE8. Note that its value always contains spaces.
     attrs = attrs.replace(/sizcache[0-9]+="[^"]*"/g, ' ');
+    // Similarly for expando properties used by jQuery to track data.
+    attrs = attrs.replace(/jQuery[0-9]+="[0-9]+"/g, ' ');
     attrs = attrs.replace(/\s*=\s*/g, '=');
     attrs = attrs.replace(/^\s+/g, '');
     attrs = attrs.replace(/\s+$/g, '');
     attrs = attrs.replace(/\s+/g, ' ');
-    var attrList = attrs.split(' ');
-    // put attributes in alphabetical order
-    attrList.sort();
     var tagContents = [tagName];
-    for(var i=0; i<attrList.length; i++) {
-      var a = attrList[i].split('=');
-      if (a.length < 2)
-        a.push(a[0]); // things like checked=checked, in theory
-      var key = a[0];
-      // Drop another expando property used by Sizzle.
-      if (key === 'sizset')
-        continue;
-      var value = a[1];
-      value = value.replace(/["'`]/g, '"');
-      if (value.charAt(0) !== '"')
-        value = '"'+value+'"';
-      tagContents.push(key+'='+value);
-    }
 
+    if (attrs !== '') {
+      var attrList = attrs.split(' ');
+      // put attributes in alphabetical order
+      attrList.sort();
+      for(var i=0; i<attrList.length; i++) {
+        var a = attrList[i].split('=');
+        if (a.length < 2)
+          a.push(a[0]); // things like checked=checked, in theory
+        var key = a[0];
+        // Drop another expando property used by Sizzle.
+        if (key === 'sizset')
+          continue;
+        var value = a[1];
+        value = value.replace(/["'`]/g, '"');
+        if (value.charAt(0) !== '"')
+          value = '"'+value+'"';
+        tagContents.push(key+'='+value);
+      }
+    }
     return '<'+tagContents.join(' ')+'>';
   });
   return h;
diff --git a/packages/ui/domrange.js b/packages/ui/domrange.js
index 476e63b4aa..9374aa786c 100644
--- a/packages/ui/domrange.js
+++ b/packages/ui/domrange.js
@@ -86,7 +86,7 @@ var textExpandosSupported = (function () {
 var createMarkerNode = (
   textExpandosSupported ?
     function () { return document.createTextNode(""); } :
-  function () { return document.createComment(""); });
+  function () { return document.createComment("IE"); });
 
 DomBackend.onRemoveElement = function (element) {
   nodeRemoved(element, true);
diff --git a/packages/ui/render.js b/packages/ui/render.js
index 3e2fc99243..02a5aab230 100644
--- a/packages/ui/render.js
+++ b/packages/ui/render.js
@@ -263,7 +263,7 @@ makeRenderBuffer = function (options) {
       // XXX maybe check validity of the object now if `arg.kind`
       randomString = randomString || Random.id();
       var commentString = randomString + '_' + (commentUid++);
-      push('<!--', commentString, '-->');
+      push('<script type="', commentString, '"></script>');
       componentsToRender = componentsToRender || {};
       componentsToRender[commentString] = arg;
     } else if (arg.attrs) {
@@ -331,9 +331,9 @@ makeRenderBuffer = function (options) {
     var wire = function (n, atTopLevel) {
       // returns what ended up in the place of `n`:
       // component, node, or null
-      if (n.nodeType === 8) { // COMMENT
+      if (n.tagName === 'SCRIPT') {
         if (componentsToRender) {
-          var spec = componentsToRender[n.nodeValue];
+          var spec = componentsToRender[n.getAttribute('type')];
           if (! spec)
             return n;
           var kind, props;