Issue 1587 - Escape curly brackets for elemhide filters

qunit/tests/cssEscaping.js

-module(
-  "CSS escaping",
+(function()
+{
+  var filterClasses = require("filterClasses");
+  var Filter = filterClasses.Filter;
+  var ElemHideFilter = filterClasses.ElemHideFilter;
+
+  module(
+    "CSS escaping",
+    {
+      setup: function()
+      {
+        var frame = document.createElement("iframe");
+        frame.srcdoc = '<script src="../include.postload.js"></script>';
+
+        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()
   {
-    setup: function()
+    var escapeCSS = this.escapeCSS;
+    var quote = this.quote;
+
+    function testSelector(opts)
     {
-      var frame = document.createElement("iframe");
-      frame.srcdoc = '<script src="../include.postload.js"></script>';
+      var mustMatch = opts.mustMatch !== false;
+      var doc = document.implementation.createHTMLDocument();
 
-      stop();
-      frame.addEventListener("load", function()
+      var style = doc.createElement("style");
+      doc.documentElement.appendChild(style);
+      style.sheet.insertRule(opts.selector + " {}", 0);
+
+      var element;
+      try
       {
-        start();
+        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;
+      }
 
-        this.escapeCSS = frame.contentWindow.escapeCSS;
-        this.quote = frame.contentWindow.quote;
+      if (element)
+      {
+        for (var attr in opts.attributes)
+          element.setAttribute(attr, opts.attributes[attr]);
 
-        document.body.removeChild(frame);
-      }.bind(this));
+        doc.documentElement.appendChild(element);
+      }
 
-      document.body.appendChild(frame);
+      var foundElement = doc.querySelector(opts.selector);
+      var filter = Filter.fromText("##" + opts.selector);
 
-      this.filterClasses = require("filterClasses");

[Wladimir Palant, 2014/11/20 14:33:05]

As with the other review, no point having this in the test setup - you can
require the module at the top of the file.

[Sebastian Noack, 2014/11/20 15:47:27]

Done, using an IFEE.

-    }
-  }
-);
-
-test("CSS escaping", function()
-{
-  var escapeCSS = this.escapeCSS;
-  var quote = this.quote;
-
-  var Filter = this.filterClasses.Filter;
-  var ElemHideFilter = this.filterClasses.ElemHideFilter;
-
-  function testSelector(opts)
-  {
-    var mustMatch = opts.mustMatch !== false;
-    var doc = document.implementation.createHTMLDocument();
-
-    var style = doc.createElement("style");
-    doc.documentElement.appendChild(style);
-    style.sheet.insertRule(opts.selector + " {}", 0);
-
-    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 (!(filter instanceof ElemHideFilter))
+      {
+        ok(false, opts.selector + " (not allowed in elemhide filters)");
+      }
+      else
+      {
+        if (mustMatch)
+          equal(foundElement, element, opts.selector);
+        else
+          ok(true, opts.selector);
+      }
     }
 
-    if (element)
+    function testEscape(s)
     {
-      for (var attr in opts.attributes)
-        element.setAttribute(attr, opts.attributes[attr]);
+      testSelector({
+        selector: escapeCSS(s),
+        tagName: s
+      });
 
-      doc.documentElement.appendChild(element);
+      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}
+      });
     }
 
-    var foundElement = doc.querySelector(opts.selector);
-    var filter = Filter.fromText("##" + opts.selector);
+    for (var i = 0; i < 0x80; i++)
+    {
+      var chr = String.fromCharCode(i);
 
-    if (!(filter instanceof ElemHideFilter))
-    {
-      ok(false, opts.selector + " (not allowed in elemhide filters)");

[Wladimir Palant, 2014/11/20 14:33:05]

There are very few cases where code like this is justified:

  if (foo)
    ok(false);

Normally, you just check foo with qUnit:

  ok(filter instanceof ElemHideFilter, opts.selector + " allows in elemhide
filters");

[Sebastian Noack, 2014/11/20 15:47:27]

So far testSelector() only generates one assert per call. And since it is called
1540 times I prefer to keep it that way. All you need to know is whether the
selector is valid CSS, and whether it is compatible with elemhide filters. And
the message we log here provides those information.

[Wladimir Palant, 2014/11/20 17:01:20]

Not sure I agree but not really that important...

+      // 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);
     }
-    else
-    {
-      if (mustMatch)
-        equal(foundElement, element, opts.selector);
-      else
-        ok(true, opts.selector);
-    }
-  }
 
-  function testEscape(s)
-  {
-    testSelector({
-      selector: escapeCSS(s),
-      tagName: s
-    });
-
-    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}
-    });
-  }
-
-  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");
-});
+    // Test some non-ASCII characters. However, those shouldn't require escaping.
+    testEscape("\uD83D\uDE3B\u2665\u00E4");
+  });
+})();