Issue 1587 - Escape curly brackets for elemhide filters

qunit/tests/cssEscaping.js

 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);
+
+      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;
@@ skipping 12 matching lines @@
 
     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);
+    var filter = Filter.fromText("##" + opts.selector);
+
+    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...

+    }
     else
-      ok(true, opts.selector);
+    {
+      if (mustMatch)
+        equal(foundElement, element, opts.selector);
+      else
+        ok(true, opts.selector);
+    }
   }
 
   function testEscape(s)
   {
     testSelector({
       selector: escapeCSS(s),
       tagName: s
     });
 
     testSelector({
@@ skipping 29 matching lines @@
     // 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");
 });