Issue 3143 - Filter elements with :-abp-has()

lib/filterClasses.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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
@@ skipping 908 matching lines @@
    * @type {string}
    */
   selectorDomain: null,
   /**
    * CSS selector for the HTML elements that should be hidden
    * @type {string}
    */
   selector: null
 });
 
+const abpSelectorRegExp = /\[-abp-selector=(["'])(.+)\1\]/;

[Wladimir Palant, 2017/05/16 13:50:39]

We never released this syntax, no point for having backwards compatibility for
it.

+const abpPropertySelectorRegExp = /\[-abp-properties=(["'])([^"']+)\1\]/;

[Wladimir Palant, 2017/05/16 13:50:40]

I definitely think that there should be something about "legacy" in the name.
Maybe legacyPropertyRuleRegExp?

+
 /**
  * Creates an element hiding filter from a pre-parsed text representation
  *
  * @param {string} text         same as in Filter()
  * @param {string} domain
  *   domain part of the text representation (can be empty)
  * @param {boolean} isException exception rule indicator
  * @param {string} tagName      tag name part (can be empty)
  * @param {string} attrRules    attribute matching rules (can be empty)
  * @param {string} selector     raw CSS selector (can be empty)
@@ skipping 39 matching lines @@
     else
       return new InvalidFilter(text, "filter_elemhide_nocriteria");
   }
 
   // We don't allow ElemHide filters which have any empty domains.
   // Note: The ElemHide.prototype.domainSeparator is duplicated here, if that
   // changes this must be changed too.
   if (domain && /(^|,)~?(,|$)/.test(domain))
     return new InvalidFilter(text, "filter_invalid_domain");
 
+  let isHideEmulation = false;
+  if ((selector.indexOf("[-abp-properties=") != -1) ||
+      (selector.indexOf("[-abp-selector=") != -1))
+  {

[Wladimir Palant, 2017/05/16 13:50:40]

This *definitely* needs to go after the isException check - an exception is
never an emulation rule.

+    isHideEmulation = true;
+
+    // Element hiding emulation filters are inefficient so we need to make sure
+    // that they're only applied if they specify active domains
+    if (!/,[^~][^,.]*\.[^,]/.test("," + domain))
+      return new InvalidFilter(text, "filter_elemhideemulation_nodomain");

[Wladimir Palant, 2017/05/16 13:50:39]

No need to duplicate code, we have that check done below.

+
+    let match = abpSelectorRegExp.exec(selector);
+    if (match)
+    {
+      let prefix = selector.substr(0, match.index);
+      let suffix = selector.substr(match.index + match[0].length);
+      selector = prefix + match[2] + suffix;
+    }
+    match = abpPropertySelectorRegExp.exec(selector);
+    if (match)
+    {
+      let prefix = selector.substr(0, match.index);
+      let suffix = selector.substr(match.index + match[0].length);
+      selector = prefix + ":-abp-properties(" +
+        match[1] + match[2] + match[1] + ")" + suffix;
+    }
+  }
+
   if (isException)
     return new ElemHideException(text, domain, selector);
 
-  if (selector.indexOf("[-abp-properties=") != -1)
+  if (isHideEmulation)

[Wladimir Palant, 2017/05/16 13:50:40]

How does that support :-abp-properties() and :-abp-has() syntax? This is
supposed to be the proper syntax, with [-abp-properties="..."] only supported
for backwards compatibility.

IMHO the logic should be something like:

  let legacyMatch = legacyPropertyRuleRegExp.exec(selector);
  if (legacyMatch)
  {
    // Convert legacy CSS Property Rules syntax
    selector = selector.substr(0, legacyMatch.index) +
               ":-abp-properties(" +
               legacyMatch[2] +
               ")" +
               selector.substr(legacyMatch.index + legacyMatch[0].length);
    text = domain + "##" + selector;
  }

  if (selector.includes(":-abp-properties(") || selector.includes(":-abp-has("))
  {
    ...
  }

Note that quotation marks shouldn't be kept when converting
[-abp-properties="..."], pseudo-classes don't need those.

   {
     // Element hiding emulation filters are inefficient so we need to make sure
     // that they're only applied if they specify active domains
     if (!/,[^~][^,.]*\.[^,]/.test("," + domain))
       return new InvalidFilter(text, "filter_elemhideemulation_nodomain");
 
     return new ElemHideEmulationFilter(text, domain, selector);
   }
 
   return new ElemHideFilter(text, domain, selector);
@@ skipping 45 matching lines @@
  */
 function ElemHideEmulationFilter(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideEmulationFilter = ElemHideEmulationFilter;
 
 ElemHideEmulationFilter.prototype = extend(ElemHideBase, {
   type: "elemhideemulation"
 });