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 19 matching lines @@
         start = i + 1;
       }
     }
   }
 
   selectors.push(selector.substring(start));
   return selectors;
 }
 
 /** Return position of node from parent.
- * @param {Node} node - the node to find the position of.

[Wladimir Palant, 2017/06/01 10:16:36]

Nit (here and elsewhere): the minus sign is unnecessary, the format doesn't
expect any separators between parameter name and description. I assume that
JSDoc will make it part of the description which is ugly.

[hub, 2017/06/01 18:22:55]

Acknowledged.

- * @return {number} 1 base index like for :nth-child(), or 0 on error.

[Wladimir Palant, 2017/06/01 10:16:38]

Nit: I think it's "One-based"

[hub, 2017/06/01 18:22:58]

Acknowledged.

+ * @param {Node} node the node to find the position of.
+ * @return {number} One-based index like for :nth-child(), or 0 on error.
  */
 function positionInParent(node)
 {
-  if (!node)
-    return 0;

[Wladimir Palant, 2017/06/01 10:16:37]

Nit: With the current code we can no longer get null for node here.

[hub, 2017/06/01 18:22:58]

Acknowledged.

   let {children} = node.parentNode;
   for (let i = 0; i < children.length; i++)
     if (children[i] == node)
       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;
 }
 
-const abpSelectorRegexp = /:-abp-(properties|has|[A-Za-z\d-]*)\(/i;

[Wladimir Palant, 2017/06/01 10:16:36]

Ok, let's say that [A-Za-z\d-]* clause is forward compatibility - new pseudos
introduced in future should cause the filter to be ignored. But I would prefer
[\w-]+ here which is simpler and makes sure that there is at least one
character.

[hub, 2017/06/01 18:22:56]

this regexp is now gone. I use the one from issue 5287.

-
-function parseSelectorContent(content, quoted = false)

[Wladimir Palant, 2017/06/01 10:16:35]

Why this quoted parameter? As I mentioned before, -abp-properties(...) contents
aren't supposed to be quoted - they only were before because it was an
"attribute" but now we have a pseudo-class.

[hub, 2017/06/01 18:22:56]

this is gone too. I may have missed the comment about properties not being
quoted.

+function parseSelectorContent(content, startIndex)
 {
   let parens = 1;
-  let i = 0;
   let quote = null;
-  let originalLength = content.length;
-  if (quoted)
-    content = content.trim();

[Wladimir Palant, 2017/06/01 10:16:35]

This call will remove whitespace on both sides - you will get bogus results if
whitespace is removed on the left side, because suffixStart calculation below
will be incorrect. Either way, if we want to remove the whitespace we should do
it in PropsSelector constructor - not sure whether we do, IMHO the original code
didn't.

[hub, 2017/06/01 18:22:56]

it is gone. see above.

-  while (i < content.length)

[Wladimir Palant, 2017/06/01 10:16:35]

This should really be a for loop:

  for (let i = 0; i < content.length; i++)

[hub, 2017/06/01 18:22:58]

current version is your parser from issue 5287.

+  let i = startIndex;
+  for (; i < content.length; i++)
   {
     let c = content[i];
-    if (quoted && i == 0)
-    {
-      if (c != "'" && c != '"')
-        return null;
-    }
     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;
     }
-    i++;
-  }
+  }
+
   if (parens > 0)
     return null;
-  if (quoted)
-  {
-    let end = content.substr(0, i).lastIndexOf(content[0]);
-    return {text: content.substr(1, end - 1),
-      end: i + (originalLength - content.length)};
-  }
-  return {text: content.substr(0, i), end: i};
-}
-
-function parseSelector(selector, level = 0)
+  return {text: content.substring(startIndex, i), end: i};
+}
+
+/** Parse the selector
+ * @param {string} selector the selector to parse
+ * @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)
 {
   if (selector.length == 0)
     return [];
 
   let match = abpSelectorRegexp.exec(selector);
   if (!match)
     return [new PlainSelector(selector)];
 
   let selectors = [];
-  let suffixStart = match.index;
-  if (suffixStart > 0)
-    selectors.push(new PlainSelector(selector.substr(0, suffixStart)));

[Wladimir Palant, 2017/06/01 10:16:34]

I don't think that reusing suffixStart variable for two very different values
helps make this code readable. Not that it is really necessary:

  if (match.index > 0)
    selectors.push(new PlainSelector(selector.substr(0, match.index)));

[hub, 2017/06/01 18:22:58]

Acknowledged.

+  if (match.index > 0)
+    selectors.push(new PlainSelector(selector.substr(0, match.index)));
 
   let startIndex = match.index + match[0].length;
-  let content = null;
+  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")
-  {
-    content = parseSelectorContent(selector.substr(startIndex), true);
-    if (content == null)
-    {
-      console.error(new SyntaxError("Failed to parse AdBlock Plus " +
-                                    `selector ${selector}, invalid ` +
-                                    "properties string."));
-      return null;
-    }

[Wladimir Palant, 2017/06/01 10:16:35]

Setting and checking content variable should be done outside the if block - this
logic is common to any pseudo, no need to duplicate code.

[hub, 2017/06/01 18:22:57]

there was the exception for "quoted", but now that it is gone, will fix that.

-
     selectors.push(new PropsSelector(content.text));
-  }
   else if (match[1] == "has")
   {
-    if (level > 0)
-    {
-      console.error(new SyntaxError("Failed to parse AdBlock Plus " +
-                                    `selector ${selector}, invalid ` +
-                                    "nested :-abp-has()."));

[Wladimir Palant, 2017/06/01 10:16:36]

While nested :-abp-has() might not be very efficient, does the current code
really have issues with it?

[hub, 2017/06/01 18:22:57]

There was an issue, but it is fixed now. Removing this limitation as well.

-      return null;
-    }
-
-    content = parseSelectorContent(selector.substr(startIndex));
-    if (content == null)
-    {
-      console.error(new SyntaxError("Failed parsing AdBlock Plus " +
-                                    `selector ${selector}, didn't ` +
-                                    "find closing parenthesis."));
-      return null;
-    }
-
     let hasSelector = new HasSelector(content.text);
     if (!hasSelector.valid())
       return null;
     selectors.push(hasSelector);
   }
   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]}.`));
+    reportError(new SyntaxError("Failed to parse Adblock Plus " +
+                                `selector ${selector}, invalid ` +
+                                `pseudo-class :-abp-${match[1]}().`));
     return null;
   }
 
-  suffixStart = startIndex + content.end + 1;

[Wladimir Palant, 2017/06/01 10:16:36]

Really, content.end should just have the proper value. So parseSelectorContent()
should get the full selector an not a substring, and a startIndex parameter
which is the initial value for i. And content.end should be set to the index of
the first character after the content.

[hub, 2017/06/01 18:22:55]

I preferred to just pass a string to parseSelectorContent. but ok. I'll do it
that way.

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

Well, you didn't. Whatever, not that important I guess...

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

I did really intend to do it. Done now.

-
-  let suffix = parseSelector(selector.substr(suffixStart), level);
+  let suffix = parseSelector(selector.substr(content.end + 1));
   if (suffix == null)
     return null;
 
   selectors.push(...suffix);
 
   return selectors;
 }
 
-function *findPropsSelectors(styles, prefix, regexp)

[Wladimir Palant, 2017/06/01 10:16:37]

This is functionality that is only used by PropsSelector - it should be a member
of that class.

[hub, 2017/06/01 18:23:00]

Acknowledged.

-{
-  for (let style of styles)
-    if (regexp.test(style.style))
-      for (let subSelector of style.subSelectors)
-        yield prefix + subSelector;
-}
-
-function stringifyStyle(style)
+/** 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 stylesheets.

[Wladimir Palant, 2017/06/01 10:16:38]

Nit: The type here should be more specific: {string[]}

[hub, 2017/06/01 18:23:00]

They are not strings. I'll rephrase to say "the stringified stylesheet objects"

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

Right, they aren't, my original assumptions were wrong. How about you use
@typedef then: http://usejsdoc.org/tags-typedef.html. This way you should be
able to define a StringifiedStyle type properly and use it here.

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

Done.

+   * @param {string} prefix the prefix for the selector.
+   * @param {Node} subtree the subtree we work on.
+   * @param {StringifiedStyle[]} styles the stringified style objects.
    */
   *getSelectors(prefix, subtree, styles)
   {
     yield [prefix + this._selector, subtree];
   }
 };
 
-const prefixEndRegexp = /[\s>+~]$/;

[Wladimir Palant, 2017/06/01 10:16:34]

This variable name doesn't really tell much about the purpose,
incompletePrefixRegexp maybe?

[hub, 2017/06/01 18:22:57]

Acknowledged.

-
-function HasSelector(selector, level = 0)
-{
-  this._innerSelectors = parseSelector(selector, level + 1);
+const incompletePrefixRegexp = /[\s>+~]$/;
+
+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 stylesheets.

[Wladimir Palant, 2017/06/01 10:16:37]

Nit: Like above, the type is string[]

[hub, 2017/06/01 18:22:55]

a above. not a string array.

+   * @param {string} prefix the prefix for the selector.
+   * @param {Node} subtree the subtree we work on.
+   * @param {StringifiedStyle[]} styles the stringified style objects.
    */
   *getElements(prefix, subtree, styles)
   {
-    let actualPrefix = (!prefix || prefixEndRegexp.test(prefix)) ?
+    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, "", element, styles);
+      let iter = evaluate(this._innerSelectors, 0, newPrefix + " ",
+                          element, styles);
       for (let selector of iter)
         // we insert a space between the two. It becomes a no-op if selector
         // doesn't have a combinator
-        if (subtree.querySelector(newPrefix + " " + selector))
+        if (subtree.querySelector(selector))
           yield element;
     }
   }
 };
 
 function PropsSelector(propertyExpression)
 {
   let regexpString;
   if (propertyExpression.length >= 2 && propertyExpression[0] == "/" &&
       propertyExpression[propertyExpression.length - 1] == "/")
   {
     regexpString = propertyExpression.slice(1, -1)
       .replace("\\x7B ", "{").replace("\\x7D ", "}");
   }
   else
     regexpString = filterToRegExp(propertyExpression);
 
   this._regexp = new RegExp(regexpString, "i");
 }
 
 PropsSelector.prototype = {
+  *findPropsSelectors(styles, prefix, regexp)
+  {
+    for (let style of styles)
+      if (regexp.test(style.style))
+        for (let subSelector of style.subSelectors)
+          yield prefix + subSelector;
+  },
+
   *getSelectors(prefix, subtree, styles)
   {
-    for (let selector of findPropsSelectors(styles, prefix, this._regexp))
+    for (let selector of this.findPropsSelectors(styles, prefix, this._regexp))
       yield [selector, subtree];
   }
 };
 
-function ElemHideEmulation(window, getFiltersFunc, addSelectorsFunc)
+function ElemHideEmulation(window, getFiltersFunc, addSelectorsFunc,
+                           hideElemsFunc)
 {
   this.window = window;
   this.getFiltersFunc = getFiltersFunc;
   this.addSelectorsFunc = addSelectorsFunc;
+  this.hideElemsFunc = hideElemsFunc;
 }
 
 ElemHideEmulation.prototype = {
   isSameOrigin(stylesheet)
   {
     try
     {
       return new URL(stylesheet.href).origin == this.window.location.origin;
     }
     catch (e)
     {
       // Invalid URL, assume that it is first-party.
       return true;
     }
   },
 
   addSelectors(stylesheets)
   {
     let selectors = [];
-    let filters = [];
+    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));
       }
     }
 
-    for (let patterns of this.selPatterns)
-      for (let selector of evaluate(patterns.selectors,
+    let {document} = this.window;
+    for (let pattern of this.patterns)
+    {
+      for (let selector of evaluate(pattern.selectors,
                                     0, "", document, cssStyles))
       {
-        selectors.push(selector);
-        filters.push(patterns.text);
+        if (!pattern.selectors.some(s => s.requiresHiding))
+        {
+          selectors.push(selector);
+          selectorFilters.push(pattern.text);
+        }
+        else
+        {
+          for (let element of document.querySelectorAll(selector))
+          {
+            elements.push(element);
+            elementFilters.push(pattern.text);
+          }
+        }
       }
-
-    this.addSelectorsFunc(selectors, filters);
+    }
+
+    this.addSelectorsFunc(selectors, selectorFilters);
+    this.hideElemsFunc(elements, elementFilters);
   },
 
   onLoad(event)
   {
     let stylesheet = event.target.sheet;
     if (stylesheet)
       this.addSelectors([stylesheet]);
   },
 
   apply()
   {
     this.getFiltersFunc(patterns =>
     {
-      this.selPatterns = [];
-
+      let oldReportError = reportError;
+      reportError = error => this.window.console.error(error);
+
+      this.patterns = [];
       for (let pattern of patterns)
       {
         let selectors = parseSelector(pattern.selector);
         if (selectors != null && selectors.length > 0)
-          this.selPatterns.push({selectors, text: pattern.text});
+          this.patterns.push({selectors, text: pattern.text});
       }
 
-      if (this.selPatterns.length > 0)
+      if (this.patterns.length > 0)
       {
         let {document} = this.window;
         this.addSelectors(document.styleSheets);
         document.addEventListener("load", this.onLoad.bind(this), true);
       }
+      reportError = oldReportError;
     });
   }
 };