Issue 1527 - Properly escape generated CSS selectors

qunit/tests/cssEscaping.js

-module("CSS escaping");
-asyncTest("CSS escaping", function()
+module(
+  "CSS escaping",
+  {
+    setup: function()
+    {
+      var frame = document.createElement("iframe");
+      frame.srcdoc='<script src="../include.postload.js"></script>';

[Wladimir Palant, 2014/11/10 12:34:15]

Nit: Missing spaces around =

+
+      stop();
+      frame.addEventListener("load", function()
+      {
+        start();
+
+        this.escapeCSS = frame.contentWindow.escapeCSS;
+        this.quote = frame.contentWindow.quote;
+
+        document.body.removeChild(frame);
+      }.bind(this));
+
+      document.body.appendChild(frame);
+    }
+  }
+);
+
+test("CSS escaping", function()
 {
-  var frame = document.createElement("iframe");
-  frame.srcdoc='<script src="../include.postload.js"></script>';
+  var escapeCSS = this.escapeCSS;
+  var quote = this.quote;
 
-  frame.addEventListener("load", function()

[Wladimir Palant, 2014/11/06 21:26:06]

Nit: This should be part of test setup (defined in module() call), with teardown
that will remove the frame from the document again. Setup can also call stop()
and start() to perform asynchronous actions.

Note the setup function can also initialize escapeCSS and quote properties:

   this.escapeCSS = frame.contentWindow.escapeCSS;

[Sebastian Noack, 2014/11/10 12:13:18]

I already wondered whether qunit has setup/teardown hooks.

+  function testSelector(opts)
   {
-    start();
+    var mustMatch = opts.mustMatch !== false;
+    var doc = document.implementation.createHTMLDocument();
 
-    var escapeCSS = frame.contentWindow.escapeCSS;
-    var quote = frame.contentWindow.quote;
+    var style = doc.createElement("style");
+    doc.documentElement.appendChild(style);
+    style.sheet.insertRule(opts.selector + " {}");
 
-    document.body.removeChild(frame);

[Wladimir Palant, 2014/11/06 21:26:06]

Won't removing the frame immediately prevent escapeCSS from calling other
functions from that frame?

[Sebastian Noack, 2014/11/10 12:13:18]

No, the test passes without any errors. I think it's because the cached
functions, keep references to either their window object, or directly to the
functions they call and variables they access.

-
-    function testSelector(opts) {

[Wladimir Palant, 2014/11/06 21:26:06]

Nit: opening bracket on next line please.

[Sebastian Noack, 2014/11/10 12:13:18]

Done.

-      var mustMatch = opts.mustMatch !== false;
-      var doc = document.implementation.createHTMLDocument();
-
-      var style = doc.createElement("style");
-      doc.documentElement.appendChild(style);
-      style.sheet.insertRule(opts.selector + " {}");
-
-      var element;
-      try
-      {
-        element = doc.createElement(opts.tagName || "div");
-      }
-      catch (e)
-      {
-        // Some characters we are going to test can not occur in tag names,
-        // but we still have to make sure that no exception is thrown when
-        // calling .querySelector() and .insertRule()
-        element = null;
-        mustMatch = false;
-      }
-
-      if (element)
-      {
-        for (var attr in opts.attributes)
-          element.setAttribute(attr, opts.attributes[attr]);
-
-        doc.documentElement.appendChild(element);
-      }
-
-      var foundElement = doc.querySelector(opts.selector);
-      if (mustMatch)
-        equal(foundElement, element, opts.selector);
-      else
-        ok(true, opts.selector);
+    var element;
+    try
+    {
+      element = doc.createElement(opts.tagName || "div");
+    }
+    catch (e)
+    {
+      // Some characters we are going to test can not occur in tag names,
+      // but we still have to make sure that no exception is thrown when
+      // calling .querySelector() and .insertRule()
+      element = null;
+      mustMatch = false;
     }
 
-    function testEscape(s)
+    if (element)
     {
-      testSelector({
-        selector: escapeCSS(s),
-        tagName: s
-      });
+      for (var attr in opts.attributes)
+        element.setAttribute(attr, opts.attributes[attr]);
 
-      testSelector({
-        selector: "#" + escapeCSS(s),
-        attributes: {id: s}
-      });
-
-      testSelector({
-        selector: "." + escapeCSS(s),
-        attributes: {class: s},
-
-        // Whitespace characters split the class name, hence the selector
-        // won't match. But we still have to make sure that no exception
-        // is thrown when calling .querySelector() and .insertRule()
-        mustMatch: !/\s/.test(s)
-      });
-
-      testSelector({
-        selector: "[foo=" + quote(s) + "]",
-        attributes: {foo: s}
-      });
+      doc.documentElement.appendChild(element);
     }
 
-    for (var i = 0; i < 0x80; i++)
-    {
-      var chr = String.fromCharCode(i);
+    var foundElement = doc.querySelector(opts.selector);
+    if (mustMatch)
+      equal(foundElement, element, opts.selector);
+    else
+      ok(true, opts.selector);
+  }
 
-      // Make sure that all ASCII characters are correctly escaped.
-      testEscape(chr);
+  function testEscape(s)
+  {
+    testSelector({
+      selector: escapeCSS(s),
+      tagName: s
+    });
 
-      // Some characters are only escaped when in the first positon,
-      // so we still have to make sure that everything is correctly escaped
-      // in subsequent positions. At the same time we make sure that
-      // leading dashes are escaped, when followed by certain characters.
-      testEscape("-" + chr);

[Wladimir Palant, 2014/11/06 21:26:06]

How about testing a letter as leading character as well?

[Sebastian Noack, 2014/11/10 12:13:18]

I considered that case redundant. But fair enough.

-    }
+    testSelector({
+      selector: "#" + escapeCSS(s),
+      attributes: {id: s}
+    });
 
-    // Test some non-ASCII characters. However, those shouldn't require escaping.
-    testEscape("😻♥ä");

[Wladimir Palant, 2014/11/06 21:26:06]

Better not have Unicode characters in a JavaScript file - browsers don't always
recognize UTF-8 correctly. "\u1234\u4321\u009F" will be less error-prone.

[Sebastian Noack, 2014/11/10 12:13:18]

I actually enjoyed seeing a cat with heart-shaped eyes, when reading this file.
;) But never mind.

-  });
+    testSelector({
+      selector: "." + escapeCSS(s),
+      attributes: {class: s},
 
-  document.body.appendChild(frame);
+      // Whitespace characters split the class name, hence the selector
+      // won't match. But we still have to make sure that no exception
+      // is thrown when calling .querySelector() and .insertRule()
+      mustMatch: !/\s/.test(s)
+    });
+
+    testSelector({
+      selector: "[foo=" + quote(s) + "]",
+      attributes: {foo: s}
+    });
+  }
+
+  for (var i = 0; i < 0x80; i++)
+  {
+    var chr = String.fromCharCode(i);
+
+    // Make sure that all ASCII characters are correctly escaped.
+    testEscape(chr);
+
+    // Some characters are only escaped when in the first positon,
+    // so we still have to make sure that everything is correctly escaped
+    // in subsequent positions.
+    testEscape("x" + chr);
+   
+    // Leading dashes must be escaped, when followed by certain characters.
+    testEscape("-" + chr);
+  }
+
+  // Test some non-ASCII characters. However, those shouldn't require escaping.
+  testEscape("\uD83D\uDE3B\u2665\u00E4");
 });