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

chrome/content/elemHideEmulation.js

Index: chrome/content/elemHideEmulation.js
===================================================================
--- a/chrome/content/elemHideEmulation.js
+++ b/chrome/content/elemHideEmulation.js
@@ -14,18 +14,16 @@
  * 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";
 
-let propertySelectorRegExp = /\[-abp-properties=(["'])([^"']+)\1\]/;
-
 function splitSelector(selector)
 {
   if (selector.indexOf(",") == -1)
     return [selector];
 
   let selectors = [];
   let start = 0;
   let level = 0;
@@ -54,131 +52,377 @@
       }
     }
   }
 
   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.

+ */
+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;
+  }
+  let idx = positionInParent(node);
+  if (idx > 0)
+  {
+    let newSelector = `${node.tagName}:nth-child(${idx})`;
+    if (selector)
+      newSelector += " > ";
+    return makeSelector(node.parentElement, newSelector + selector);
+  }
+
+  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.

+{
+  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 c = content[i];
+    if (quoted && i == 0)
+    {
+      if (c != "'" && c != '"')
+        return null;
+    }
+    if (c == "\\")
+      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)
+{
+  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.

+
+  let startIndex = match.index + match[0].length;
+  let content = 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]}.`));
+    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);
+  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)
+{
+  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 : "") +
+                ";");
+  }
+  styles.sort();
+  return styles.join(" ");
+}
+
+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.

+   */
+  *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);
+}
+
+HasSelector.prototype = {
+  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.

+   */
+  *getElements(prefix, subtree, styles)
+  {
+    let actualPrefix = (!prefix || prefixEndRegexp.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);
+      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))
+          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 = {
+  *getSelectors(prefix, subtree, styles)
+  {
+    for (let selector of findPropsSelectors(styles, prefix, this._regexp))
+      yield [selector, subtree];
+  }
+};
+
 function ElemHideEmulation(window, getFiltersFunc, addSelectorsFunc)
 {
   this.window = window;
   this.getFiltersFunc = getFiltersFunc;
   this.addSelectorsFunc = addSelectorsFunc;
 }
 
 ElemHideEmulation.prototype = {
-  stringifyStyle(style)
-  {
-    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 : "") +
-                  ";");
-    }
-    styles.sort();
-    return styles.join(" ");
-  },
-
   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;
     }
   },
 
-  findSelectors(stylesheet, selectors, filters)
-  {
-    // Explicitly ignore third-party stylesheets to ensure consistent behavior
-    // between Firefox and Chrome.
-    if (!this.isSameOrigin(stylesheet))
-      return;
-
-    let rules = stylesheet.cssRules;
-    if (!rules)
-      return;
-
-    for (let rule of rules)
-    {
-      if (rule.type != rule.STYLE_RULE)
-        continue;
-
-      let style = this.stringifyStyle(rule.style);
-      for (let pattern of this.patterns)
-      {
-        if (pattern.regexp.test(style))
-        {
-          let subSelectors = splitSelector(rule.selectorText);
-          for (let subSelector of subSelectors)
-          {
-            selectors.push(pattern.prefix + subSelector + pattern.suffix);
-            filters.push(pattern.text);
-          }
-        }
-      }
-    }
-  },
-
   addSelectors(stylesheets)
   {
     let selectors = [];
     let filters = [];
+
+    let cssStyles = [];
+
     for (let stylesheet of stylesheets)
-      this.findSelectors(stylesheet, selectors, filters);
+    {
+      // 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});
+      }
+    }
+
+    for (let patterns of this.selPatterns)
+      for (let selector of evaluate(patterns.selectors,
+                                    0, "", document, cssStyles))
+      {
+        selectors.push(selector);
+        filters.push(patterns.text);
+      }
+
     this.addSelectorsFunc(selectors, filters);
   },
 
   onLoad(event)
   {
     let stylesheet = event.target.sheet;
     if (stylesheet)
       this.addSelectors([stylesheet]);
   },
 
   apply()
   {
     this.getFiltersFunc(patterns =>
     {
-      this.patterns = [];
+      this.selPatterns = [];
+
       for (let pattern of patterns)
       {
-        let match = propertySelectorRegExp.exec(pattern.selector);
-        if (!match)
-          continue;
-
-        let propertyExpression = match[2];
-        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.patterns.push({
-          text: pattern.text,
-          regexp: new RegExp(regexpString, "i"),
-          prefix: pattern.selector.substr(0, match.index),
-          suffix: pattern.selector.substr(match.index + match[0].length)
-        });
+        let selectors = parseSelector(pattern.selector);
+        if (selectors != null && selectors.length > 0)
+          this.selPatterns.push({selectors, text: pattern.text});
       }
 
-      if (this.patterns.length > 0)
+      if (this.selPatterns.length > 0)
       {
         let {document} = this.window;
         this.addSelectors(document.styleSheets);
         document.addEventListener("load", this.onLoad.bind(this), true);
       }
     });
   }
 };