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

chrome/content/elemHideEmulation.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
  * 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/>.
  */
 
 /* globals filterToRegExp */
 
 "use strict";
 
 const abpSelectorRegexp = /:-abp-([\w-]+)\(/i;
 
+let reportError = () => {};
+
 function splitSelector(selector)
 {
   if (selector.indexOf(",") == -1)
     return [selector];
 
   let selectors = [];
   let start = 0;
   let level = 0;
   let sep = "";
 
@@ skipping 37 matching lines @@
       return i + 1;
   return 0;
 }
 
 function makeSelector(node, selector)
 {
   if (!node.parentElement)
   {
     let newSelector = ":root";
     if (selector)
-      newSelector += " > ";
-    return newSelector + selector;
+      newSelector += " > " + selector;
+    return newSelector;
   }
   let idx = positionInParent(node);
   if (idx > 0)
   {
     let newSelector = `${node.tagName}:nth-child(${idx})`;
     if (selector)
-      newSelector += " > ";
-    return makeSelector(node.parentElement, newSelector + selector);
+      newSelector += " > " + selector;
+    return makeSelector(node.parentElement, newSelector);
   }
 
   return selector;
 }
 
-function parseSelectorContent(content)
+function parseSelectorContent(content, startIndex)
 {
   let parens = 1;
   let quote = null;
-  let i;
-  for (i = 0; i < content.length; i++)
+  let i = startIndex;
+  for (; i < content.length; i++)
   {
     let c = content[i];
     if (c == "\\")
     {
       // Ignore escaped characters
       i++;
     }
     else if (quote)
     {
       if (c == quote)
         quote = null;
     }
     else if (c == "'" || c == '"')
       quote = c;
     else if (c == "(")
       parens++;
     else if (c == ")")
     {
       parens--;
       if (parens == 0)
         break;
     }
   }
 
   if (parens > 0)
     return null;
-  return {text: content.substr(0, i), end: i};
+  return {text: content.substring(startIndex, i), end: i};
 }
 
 /** Parse the selector
  * @param {string} selector the selector to parse
- * @param {Number} level the depth level. 0 is top
  * @return {Object} selectors is an array of objects,
  * or null in case of errors. hide is true if we'll hide
  * elements instead of styles..
  */
-function parseSelector(selector, level = 0)

[Wladimir Palant, 2017/06/07 08:32:58]

Level parameter is no longer used.

[hub, 2017/06/07 14:15:08]

I meant to remove it. it is gone now.

+function parseSelector(selector)
 {
   if (selector.length == 0)
-    return {selectors: [], hide: false};
+    return [];
 
   let match = abpSelectorRegexp.exec(selector);
   if (!match)
-    return {selectors: [new PlainSelector(selector)], hide: false};
-
-  let hide = false;
+    return [new PlainSelector(selector)];
+
   let selectors = [];
-  let suffixStart = match.index;
-  if (suffixStart > 0)
+  if (match.index > 0)
     selectors.push(new PlainSelector(selector.substr(0, match.index)));

[Wladimir Palant, 2017/06/07 08:32:58]

I meant - suffixStart variable shouldn't be declared/assigned here at all.

[hub, 2017/06/07 14:15:08]

done

 
   let startIndex = match.index + match[0].length;
-  let content = parseSelectorContent(selector.substr(startIndex));
-  if (content == null)
-  {
-    console.error(new SyntaxError("Failed parsing AdBlock Plus " +
-                                  `selector ${selector}, didn't ` +
-                                  "find closing parenthesis."));
-    return {selectors: null, hide: false};
+  let content = parseSelectorContent(selector, startIndex);
+  if (!content)
+  {
+    reportError(new SyntaxError("Failed to parse Adblock Plus " +
+                                `selector ${selector}, ` +
+                                "due to unmatched parentheses."));
+    return null;
   }
   if (match[1] == "properties")
     selectors.push(new PropsSelector(content.text));
   else if (match[1] == "has")
   {
     let hasSelector = new HasSelector(content.text);
     if (!hasSelector.valid())
-      return {selectors: null, hide: false};
+      return null;
     selectors.push(hasSelector);
-    hide = true;
   }
   else
   {
     // this is an error, can't parse selector.
-    console.error(new SyntaxError("Failed parsing AdBlock Plus " +
-                                  `selector ${selector}, invalid ` +
-                                  `pseudo-class -abp-${match[1]}().`));
-    return {selectors: null, hide: false};
-  }
-
-  suffixStart = startIndex + content.end + 1;
-
-  let suffix = parseSelector(selector.substr(suffixStart), level);
-  if (suffix.selectors == null)
-    return {selectors: null, hide: false};
-
-  selectors.push(...suffix.selectors);
-  hide |= suffix.hide;

[Wladimir Palant, 2017/06/07 08:32:58]

Using numerical operators on boolean values isn't a good idea in JavaScript,
this should really be:

  hide = hide || suffix.hide;

Either way, dragging this state all the way through introduces significant
complexity which IMHO isn't worth it. How about you return selectors as you did
before and when you need to decide whether a particular chain should hide you do
the following:

  let hide = selectors.some(s => s instanceof HasSelector);

If I see it correctly, this check needs to be performed only once per chain, so
not worth optimizing.

[hub, 2017/06/07 14:15:09]

The problem with moving this out is that the decision of hiding is performed
somewhere else and not in the parse selector logic. 
When I implemented :-abp-contains (it is another patch), I kept the changes in
here. This is a different tradeoff I guess, but I prefer it that way.

I have changed this line though, to do as you suggested.

[Wladimir Palant, 2017/06/07 14:53:41]

Assuming that I understood this sentence correctly, you can still generalize
this by adding a requiresHiding property to the prototype of HasSelector and
such - it would be true for HasSelector and false for everything else. You could
still replace this whole lot of complexity by four lines of code where three are
trivial. And you replace `if (!pattern.hide)` (the only place where this
property is actually used) by:

  if (!selectors.some(s => s.requiresHiding))

[hub, 2017/06/08 00:17:19]

done

-
-  return {selectors, hide};
-}
-
-function stringifyStyle(style)
+    reportError(new SyntaxError("Failed to parse Adblock Plus " +
+                                `selector ${selector}, invalid ` +
+                                `pseudo-class :-abp-${match[1]}().`));
+    return null;
+  }
+
+  let suffix = parseSelector(selector.substr(content.end + 1));
+  if (suffix == null)
+    return null;
+
+  selectors.push(...suffix);
+
+  return selectors;
+}
+
+/** Stringified style objects
+ * @typedef {Object} StringifiedStyle
+ * @property {string} style CSS style represented by a string.
+ * @property {string[]} subSelectors selectors the CSS properties apply to.
+ */
+
+/**
+ * Produce a string representation of the stylesheet entry.
+ * @param {CSSStyleRule} rule the CSS style rule.
+ * @return {StringifiedStyle} the stringified style.
+ */
+function stringifyStyle(rule)
 {
   let styles = [];
-  for (let i = 0; i < style.length; i++)
-  {
-    let property = style.item(i);
-    let value = style.getPropertyValue(property);
-    let priority = style.getPropertyPriority(property);
-    styles.push(property + ": " + value + (priority ? " !" + priority : "") +
-                ";");
+  for (let i = 0; i < rule.style.length; i++)
+  {
+    let property = rule.style.item(i);
+    let value = rule.style.getPropertyValue(property);
+    let priority = rule.style.getPropertyPriority(property);
+    styles.push(`${property}: ${value}${priority ? " !" + priority : ""};`);
   }
   styles.sort();
-  return styles.join(" ");
+  return {
+    style: styles.join(" "),
+    subSelectors: splitSelector(rule.selectorText)
+  };
 }
 
 function* evaluate(chain, index, prefix, subtree, styles)
 {
   if (index >= chain.length)
   {
     yield prefix;
     return;
   }
   for (let [selector, element] of
        chain[index].getSelectors(prefix, subtree, styles))
     yield* evaluate(chain, index + 1, selector, element, styles);
 }
 
 function PlainSelector(selector)
 {
   this._selector = selector;
 }
 
 PlainSelector.prototype = {
   /**
    * Generator function returning a pair of selector
    * string and subtree.
    * @param {string} prefix the prefix for the selector.
    * @param {Node} subtree the subtree we work on.
-   * @param {Array} styles the stringified stylesheet objects.
+   * @param {StringifiedStyle[]} styles the stringified style objects.
    */
   *getSelectors(prefix, subtree, styles)
   {
     yield [prefix + this._selector, subtree];
   }
 };
 
 const incompletePrefixRegexp = /[\s>+~]$/;
 
-function HasSelector(selector, level = 0)
-{
-  let inner = parseSelector(selector, level + 1);
-  this._innerSelectors = inner ? inner.selectors : null;
+function HasSelector(selector)
+{
+  this._innerSelectors = parseSelector(selector);
 }
 
 HasSelector.prototype = {
+  requiresHiding: true,
+
   valid()
   {
     return this._innerSelectors != null;
   },
 
   *getSelectors(prefix, subtree, styles)
   {
     for (let element of this.getElements(prefix, subtree, styles))
       yield [makeSelector(element, ""), element];
   },
 
   /**
    * Generator function returning selected elements.
    * @param {string} prefix the prefix for the selector.
    * @param {Node} subtree the subtree we work on.
-   * @param {Array} styles the stringified stylesheet objects.
+   * @param {StringifiedStyle[]} styles the stringified style objects.
    */
   *getElements(prefix, subtree, styles)
   {
     let actualPrefix = (!prefix || incompletePrefixRegexp.test(prefix)) ?
         prefix + "*" : prefix;
     let elements = subtree.querySelectorAll(actualPrefix);
     for (let element of elements)
     {
       let newPrefix = makeSelector(element, "");
       let iter = evaluate(this._innerSelectors, 0, newPrefix + " ",
@@ skipping 57 matching lines @@
     catch (e)
     {
       // Invalid URL, assume that it is first-party.
       return true;
     }
   },
 
   addSelectors(stylesheets)
   {
     let selectors = [];
-    let filters = [];
-
-    let hideElements = [];
-    let filterElements = [];

[Wladimir Palant, 2017/06/07 08:32:59]

This array contains filters, not elements. How about naming these four variables
selectors, selectorFilters, elements and elementFilters? This seems consistent.

[hub, 2017/06/07 14:15:08]

make sense. done.

+    let selectorFilters = [];
+
+    let elements = [];
+    let elementFilters = [];
 
     let cssStyles = [];
 
     for (let stylesheet of stylesheets)
     {
       // Explicitly ignore third-party stylesheets to ensure consistent behavior
       // between Firefox and Chrome.
       if (!this.isSameOrigin(stylesheet))
         continue;
 
       let rules = stylesheet.cssRules;
       if (!rules)
         continue;
 
       for (let rule of rules)
       {
         if (rule.type != rule.STYLE_RULE)
           continue;
 
-        let style = stringifyStyle(rule.style);
-        let subSelectors = splitSelector(rule.selectorText);
-        cssStyles.push({style, subSelectors});
+        cssStyles.push(stringifyStyle(rule));
       }
     }
 
+    let {document} = this.window;
     for (let pattern of this.patterns)
+    {
       for (let selector of evaluate(pattern.selectors,
                                     0, "", document, cssStyles))
-        if (!pattern.hide)
+      {
+        if (!pattern.selectors.some(s => s.requiresHiding))
         {
           selectors.push(selector);
-          filters.push(pattern.text);
+          selectorFilters.push(pattern.text);
         }
         else
+        {
           for (let element of document.querySelectorAll(selector))

[Wladimir Palant, 2017/06/07 08:32:59]

Nit: we require brackets around multi-line blocks, even if these only contain a
single statement.

[hub, 2017/06/07 14:15:09]

Done.

           {
-            hideElements.push(element);
-            filterElements.push(pattern.text);
+            elements.push(element);
+            elementFilters.push(pattern.text);
           }
-
-    this.addSelectorsFunc(selectors, filters);
-    this.hideElemsFunc(hideElements, filterElements);
+        }
+      }
+    }
+
+    this.addSelectorsFunc(selectors, selectorFilters);
+    this.hideElemsFunc(elements, elementFilters);
   },
 
   onLoad(event)
   {
     let stylesheet = event.target.sheet;
     if (stylesheet)
       this.addSelectors([stylesheet]);
   },
 
   apply()
   {
     this.getFiltersFunc(patterns =>
     {
+      let oldReportError = reportError;
+      reportError = error => this.window.console.error(error);
+
       this.patterns = [];
       for (let pattern of patterns)
       {
-        let {selectors, hide} = parseSelector(pattern.selector);
+        let selectors = parseSelector(pattern.selector);
         if (selectors != null && selectors.length > 0)
-          this.patterns.push({selectors, hide, text: pattern.text});
+          this.patterns.push({selectors, text: pattern.text});
       }
 
       if (this.patterns.length > 0)
       {
         let {document} = this.window;
         this.addSelectors(document.styleSheets);
         document.addEventListener("load", this.onLoad.bind(this), true);
       }
+      reportError = oldReportError;
     });
   }
 };