Better parsing for {{! }} comments

We now no longer check whether we're at a closing
HTML tag by peeking for "</" since that would fail
if there were a {{! }} comment directly before it.

Instead, we tokenize and then rewind. Presumably
this does have an effect on compilation time but it's
probably fine. An alternative would have been to
explicitly parse {{! }} comments rather than implicitly
skipping them before generally parsing tokens.

packages/html-tools/parse.js

@@ -50,16 +50,27 @@ HTMLTools.parseFragment = function (input, options) {
     result = getContent(scanner, shouldStop);
   }
   if (! scanner.isEOF()) {
+    var posBefore = scanner.pos;
+
+    try {
+      var endTag = getHTMLToken(scanner);
+    } catch (e) {
+      // ignore errors from getSpecialTag
+    }
+
     // XXX we make some assumptions about shouldStop here, like that it
     // won't tell us to stop at an HTML end tag.  Should refactor
     // `shouldStop` into something more suitable.
-    if (scanner.rest().slice(0, 2) === '</') {
-      var closeTag = scanner.rest().slice(2).match(/^[a-z]*/i)[0];
+    if (endTag && endTag.t === 'Tag' && endTag.isEnd) {
+      var closeTag = endTag.n;
       var isVoidElement = HTML.isVoidElement(closeTag);
       scanner.fatal("Unexpected HTML close tag" +
                     (isVoidElement ?
-                     '.  <' + closeTag + '> should have no close tag.' : ''));
+                     '.  <' + endTag.n + '> should have no close tag.' : ''));
     }
+
+    scanner.pos = posBefore; // rewind, we'll continue parsing as usual
+
     if (! shouldStop)
       scanner.fatal("Expected EOF");
   }
@@ -99,14 +110,10 @@ getContent = HTMLTools.Parse.getContent = function (scanner, shouldStopFunc) {
   var items = [];
 
   while (! scanner.isEOF()) {
-    // Stop at any top-level end tag.  We could use the tokenizer
-    // but these two characters are a giveaway.
-    if (scanner.rest().slice(0, 2) === '</')
-      break;
-
     if (shouldStopFunc && shouldStopFunc(scanner))
       break;
 
+    var posBefore = scanner.pos;
     var token = getHTMLToken(scanner);
     if (! token)
       // tokenizer reached EOF on its own, e.g. while scanning
@@ -125,10 +132,11 @@ getContent = HTMLTools.Parse.getContent = function (scanner, shouldStopFunc) {
       // token.v is an object `{ ... }`
       items.push(HTMLTools.Special(token.v));
     } else if (token.t === 'Tag') {
-      if (token.isEnd)
-        // we've already screened for `</` so this shouldn't be
-        // possible.
-        scanner.fatal("Assertion failed: didn't expect end tag");
+      if (token.isEnd) {
+        // rewind; we'll parse the end token later
+        scanner.pos = posBefore;
+        break;
+      }
 
       var tagName = token.n;
       // is this an element with no close tag (a BR, HR, IMG, etc.) based
@@ -146,7 +154,7 @@ getContent = HTMLTools.Parse.getContent = function (scanner, shouldStopFunc) {
       if (isVoid || token.isSelfClosing) {
         items.push(attrs ? tagFunc(attrs) : tagFunc());
       } else {
-        // parse HTMl tag contents.
+        // parse HTML tag contents.
 
         // HTML treats a final `/` in a tag as part of an attribute, as in `<a href=/foo/>`, but the template author who writes `<circle r={{r}}/>`, say, may not be thinking about that, so generate a good error message in the "looks like self-close" case.
         var looksLikeSelfClose = (scanner.input.substr(scanner.pos - 2, 2) === '/>');
@@ -160,21 +168,12 @@ getContent = HTMLTools.Parse.getContent = function (scanner, shouldStopFunc) {
           content = getContent(scanner, shouldStopFunc);
         }
 
-        if (scanner.rest().slice(0, 2) !== '</') {
-          scanner.fatal('Expected "' + tagName + '" end tag' + (looksLikeSelfClose ? ' -- if the "<' + token.n + ' />" tag was supposed to self-close, try adding a space before the "/"' : ''));
-        }
-
-        var endTag = getTagToken(scanner);
+        var endTag = getHTMLToken(scanner);
 
-        if (! (endTag.t === 'Tag' && endTag.isEnd))
-          // we've already seen `</` so this shouldn't be possible
-          // without erroring.
-          scanner.fatal("Assertion failed: expected end tag");
+        if (! (endTag && endTag.t === 'Tag' && endTag.isEnd && endTag.n === tagName))
+          scanner.fatal('Expected "' + tagName + '" end tag' + (looksLikeSelfClose ? ' -- if the "<' + token.n + ' />" tag was supposed to self-close, try adding a space before the "/"' : ''));
 
         // XXX support implied end tags in cases allowed by the spec
-        if (endTag.n !== tagName) {
-          scanner.fatal('Expected "' + tagName + '" end tag, found "' + endTag.n + '"' + (looksLikeSelfClose ? ' -- if the "<' + token.n + ' />" tag was supposed to self-close, try adding a space before the "/"' : ''));
-        }
 
         // make `content` into an array suitable for applying tag constructor
         // as in `FOO.apply(null, content)`.
@@ -338,4 +337,4 @@ var parseAttrs = function (attrs) {
   }
 
   return result;
-};
+};

packages/html-tools/parse_tests.js

@@ -41,7 +41,6 @@ Tinytest.add("html-tools - parser getContent", function (test) {
 
 
   succeed('', null);
-  succeed('^^^</', null);
   succeed('abc', 'abc');
   succeed('abc^^^</x>', 'abc');
   succeed('a&lt;b', ['a', CharRef({html: '&lt;', str: '<'}), 'b']);
@@ -152,9 +151,12 @@ Tinytest.add("html-tools - parseFragment", function (test) {
   test.equal(HTML.toJS(HTMLTools.parseFragment("<div><p id=foo>Hello</p></div>")),
              HTML.toJS(DIV(P({id:'foo'}, 'Hello'))));
 
-  test.throws(function() {
-    HTMLTools.parseFragment('asdf</a>');
-  });
+  _.each(['asdf</br>', '{{!foo}}</br>', '{{!foo}} </br>',
+          'asdf</a>', '{{!foo}}</a>', '{{!foo}} </a>'], function (badFrag) {
+            test.throws(function() {
+              HTMLTools.parseFragment(badFrag);
+            }, /Unexpected HTML close tag/);
+          });
 
   (function () {
     var p = HTMLTools.parseFragment('<p></p>');
@@ -218,8 +220,8 @@ Tinytest.add("html-tools - parseFragment", function (test) {
 Tinytest.add("html-tools - getSpecialTag", function (test) {
 
   // match a simple tag consisting of `{{`, an optional `!`, one
-  // or more ASCII letters or spaces, and a closing `}}`.
-  var mustache = /^\{\{(!?[a-zA-Z 0-9]+)\}\}/;
+  // or more ASCII letters, spaces or html tags, and a closing `}}`.
+  var mustache = /^\{\{(!?[a-zA-Z 0-9</>]+)\}\}/;
 
   // This implementation of `getSpecialTag` looks for "{{" and if it
   // finds it, it will match the regex above or fail fatally trying.
@@ -346,6 +348,12 @@ Tinytest.add("html-tools - getSpecialTag", function (test) {
   succeed('x{{!foo}}{{!bar}}y', 'xy');
   succeed('x{{!foo}}{{bar}}y', ['x', Special({stuff: 'bar'}), 'y']);
   succeed('x{{foo}}{{!bar}}y', ['x', Special({stuff: 'foo'}), 'y']);
+  succeed('<div>{{!foo}}{{!bar}}</div>', DIV());
+  succeed('<div>{{!foo}}<br />{{!bar}}</div>', DIV(BR()));
+  succeed('<div> {{!foo}} {{!bar}} </div>', DIV("   "));
+  succeed('<div> {{!foo}} <br /> {{!bar}}</div>', DIV("  ", BR(), " "));
+  succeed('{{! <div></div> }}', null);
+  succeed('{{!<div></div>}}', null);
 
   succeed('', null);
   succeed('{{!foo}}', null);

packages/spacebars-compiler/compile_tests.js

@@ -292,4 +292,9 @@ Tinytest.add("spacebars - compiler errors", function (test) {
           "First argument must be a function");
   isError("{{#foo 0 x=0}}{{/foo}}",
           "First argument must be a function");
+
+  _.each(['asdf</br>', '{{!foo}}</br>', '{{!foo}} </br>',
+          'asdf</a>', '{{!foo}}</a>', '{{!foo}} </a>'], function (badFrag) {
+            isError(badFrag, "Unexpected HTML close tag");
+          });
 });