Issue 1527 - Properly escape generated CSS selectors

include.postload.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 // Click-to-hide stuff
 var clickHide_activated = false;
 var clickHide_filters = null;
 var currentElement = null;
 var clickHideFilters = null;
 var highlightedElementsSelector = null;
 var clickHideFiltersDialog = null;
 var lastRightClickEvent = null;
 
-var ctrlChar = /[\x00-\x1F\x7F]/;
-
 function escapeChar(chr)
 {
-  if (ctrlChar.test(chr) || /\d/.test(chr))
-    return "\\" + chr.charCodeAt(0).toString(16) + " ";
+  var code = chr.charCodeAt(0);
+
+  if (code <= 0x1F || code == 0x7F || /\d/.test(chr))
+    return "\\" + code.toString(16) + " ";
 
   return "\\" + chr;
 }
 
 function quote(value)
 {
-  return '"' + value.replace(new RegExp('["\\\\]|' + ctrlChar.source, "g"), escapeChar) + '"';

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

Generating regular expressions dynamically for no good reason whatsoever is a
clear step back as far as readability and maintainability goes :-(

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

The idea was not to hard-code the char codes for control characters twice. But
the way I previously reused the regex lead to buggy behavior, since RegExp
objects using the global flag, persist the postion of the last match and resume
at that position when calling .test() the next time. But frankly, I also dislike
this construct, and removed it now.

+  return '"' + value.replace(/["\\\x00-\x1F\x7F]/g, escapeChar) + '"';
 }
 
 function escapeCSS(s)
 {
-  return s.replace(/^\d|^-(?![^\d\-])|[^\w\-\u0080-\uFFFF]/g, escapeChar);

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

Thinking more about this, I'm definitely opposed to a special case for dashes
not followed by a digit or dash here.

1) This special case is irrelevant on real websites.
2) It makes the rules more complicated and much harder to verify (as opposed to:
"always escape dashes if they are the first character").
3) Mozilla implements this logic differently which shows that this rule isn't
canonical.
4) This makes the otherwise trivial regular expression much harder to
understand.

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

It's probably not worth to argue here. But I still think that the rule is easier
to read when the leading dash isn't escaped, and therefore should only be
escaped when necessary, and that a look-ahead doesn't make the regex much harder
to understand.

Instead escaping the leading dash, you can also escape the digit or dash in the
second position. That is what Gecko's CSS.escape() function does, when the
string starts with a dash followed by a digit. However, Gecko doesn't escape
sequences of leading dashes, since those need no escaping there.

+  return s.replace(/^[\d\-]|[^\w\-\u0080-\uFFFF]/g, escapeChar);
 }
 
 function supportsShadowRoot(element)
 {
   if (!("createShadowRoot" in element))
     return false;
 
   // There are some elements (e.g. <textarea>), which don't
   // support author created shadow roots and throw an exception.
   var clone = element.cloneNode(false);
@@ skipping 615 matching lines @@
         break;
       default:
         sendResponse({});
         break;
     }
   });
 
   if (window == window.top)
     ext.backgroundPage.sendMessage({type: "report-html-page"});
 }