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 18 matching lines @@
         selectors.push(selector.substring(start, i));
         start = i + 1;
       }
     }
   }
 
   selectors.push(selector.substring(start));
   return selectors;
 }
 
-// Return position of node from parent.
-// 1 base index like for :nth-child()

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

Nit: Here and below, if you add documentation you might just as well use proper
JSDoc syntax.

[hub, 2017/05/25 13:02:29]

Done.

+/** Return position of node from parent.
+ * @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)
 {
-  let parentNode = node ? node.parentNode : null;

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

I guess you should be consistent here:

if (!node)
  return 0;
let {parentNode} = node;
if (!parentNode)
  return 0;
let {children} = parentNode;
if (!children.length)
  return 0;

Note that ParentNode.children cannot return null, so you only have to check the
length. Or you can skip the check entirely because the length will be checked in
the loop below anyway.

[hub, 2017/05/16 21:35:54]

Done.

-  if (parentNode == null)
-    return 0;
-
-  let {children} = parentNode;
-  if (!children)
-    return 0;
-  let i = 0;
-  for (i = 0; i < children.length; i++)
+  let {children} = node.parentNode;
+  for (let i = 0; i < children.length; i++)
     if (children[i] == node)
-      break;
-  return i + 1;

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

I suggest that you simplify this, no need to declare the variable outside the
loop:

  for (let i = 0; i < children.length; i++)
    if (children[i] == node)
      return i + 1;
  return 0;

[hub, 2017/05/16 21:35:56]

Re-reading the code, it almost make it correct. If there was no children it
would have returned 1.... And length + 1 of not found....

-}
-
-function idValid(id)

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

Nit: how about isValidID() as function name?

[hub, 2017/05/16 21:35:53]

Acknowledged.

-{
-  if (!id)
-    return false;
-  if (id === "")

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

id cannot be an empty string at this point because !"" is true - the previous
check will return false already. In fact, !0 is also true, so the check above
will reject too much. I guess you actually want:

  return (id != null && id != "" && !/^(-?[0-9]|--)/.test(id));

But why are you singling out IDs starting with a dash? HTML5 says: "There are no
other restrictions on what form an ID can take; in particular, IDs can consist
of just digits, start with a digit, start with an underscore, consist of just
punctuation, etc."

[hub, 2017/05/16 21:35:51]

https://www.w3.org/TR/CSS2/syndata.html#value-def-identifier
https://www.w3.org/TR/CSS2/syndata.html#tokenization
Can start with a '-' but not '--', nor '-' followed by a digit.
This is CSS not HTML5. It the id valid in a selector.

Without that check if I try to select using an ID that, for example, starts with
a digit, querySelector() will throw a DOM exception.

All in all that thing is probably totally moot given the comment below.

-    return false;
-  if (id.match(/^(-?[0-9]|--)/))
-    return false;
-  return true;
+      return i + 1;
+  return 0;
 }
 
 function makeSelector(node, selector)
 {
-  if (node && idValid(node.id))
-  {
-    let newSelector = "#" + node.id;

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

I can see how this shortcut is tempting. The issue is however: even though the
id attribute is supposed to be unique, in practice nothing is stopping the
webpage from setting the same id on multiple elements. This selector will then
apply to all of them. Feel free to try that out.

Unfortunately, we have to go by position all the way up.

[hub, 2017/05/16 21:35:56]

:-/ This is really unfortunate.

-    if (selector != "")
-      newSelector += " > ";
-    return newSelector + selector;
+  if (!node.parentElement)
+  {
+    let newSelector = ":root";
+    if (selector)
+      newSelector += " > " + selector;
+    return newSelector;
   }
   let idx = positionInParent(node);
   if (idx > 0)

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

Usually, this will work because there is only one <html> element on the page.
However, the page is free to create more <html> elements and then this selector
will no longer be unique. Better:

  if (node == node.ownerDocument.documentElement)
    return `:root > ${selector}`;

  let idx = positionInParent(node);
  ...

Or maybe even:

  if (!node.parentElement)
    return `:root > ${selector}`;

And then you can reduce the number of checks in positionInParent() because you
already know that node is valid and has a parent element.

[hub, 2017/05/25 13:02:28]

Done.

   {
-    let newSelector = `${node.tagName}:nth-child(${idx}) `;
-    if (selector != "")
-      newSelector += "> ";

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

Nit: you should remove the trailing whitespace on line 101 and only add it here,
if there is a selector part to append. In fact, this should be:

  if (selector)
    newSelector += " > " + selector;

If we don't have an existing selector part then there is no point in
concatenating with it.

[hub, 2017/05/16 21:35:53]

Acknowledged.

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

Acknowledged but the code is unchanged :)

Well, I'm not going to insist that you address a nit.

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

it is done patch set 19, definitely.

[Wladimir Palant, 2017/06/07 08:35:19]

The trailing whitespace part - yes. Only doing the concatenation if we have a
non-empty selector to concatenate with - not so much.

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

Current code is:

    let newSelector = `${node.tagName}:nth-child(${idx})`;
    if (selector)
      newSelector += " > ";

Maybe I'm missing the obvious, but I only concatenate if we have a non empty
selector, don't I?

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

It's about the concatenation on the next line ;)

     let newSelector = `${node.tagName}:nth-child(${idx})`;
     if (selector)
       newSelector += " > " + selector;
     return makeSelector(node.parentNode, newSelector);

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

Ah !. 

But if selector is empty, it doesn't matter, it is like a no-op. I'll amend my
patch. Hear and above.

-    return makeSelector(node.parentNode, newSelector + selector);
+    let newSelector = `${node.tagName}:nth-child(${idx})`;
+    if (selector)
+      newSelector += " > " + selector;
+    return makeSelector(node.parentElement, newSelector);
   }
 
   return selector;
 }
 
-// return the regexString for the properties
-function parsePropSelPattern(propertyExpression)

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

This is logic which is only used by the PropSelector class. This function and
similar ones should be defined as private methods of that class, so that we have
this part of the functionality in one place.

[hub, 2017/05/16 21:35:54]

I'll move it to the PropsSelector constructor. That's the only place it is
called.

+function parseSelectorContent(content, startIndex)
+{
+  let parens = 1;
+  let quote = null;
+  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.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 = [];
+  if (match.index > 0)
+    selectors.push(new PlainSelector(selector.substr(0, match.index)));
+
+  let startIndex = match.index + match[0].length;
+  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 null;
+    selectors.push(hasSelector);
+  }
+  else
+  {
+    // this is an error, can't parse selector.
+    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 < 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 {
+    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 {StringifiedStyle[]} styles the stringified style objects.
+   */
+  *getSelectors(prefix, subtree, styles)
+  {
+    yield [prefix + this._selector, subtree];
+  }
+};
+
+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 {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 + " ",
+                          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(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 ", "}");

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

Nit: You need braces around the if body here. Also, please indent the second
line to indicate that this is the previous statement being continued. I think
that ESLint will flag both issues, please make sure to run it.

[hub, 2017/05/16 21:35:51]

ESLint doesn't flag this.

+      .replace("\\x7B ", "{").replace("\\x7D ", "}");
+  }
   else
     regexpString = filterToRegExp(propertyExpression);
-  return regexpString;

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

IMHO this should return the actual RegExp rather than expecting the caller to
convert the string into a RegExp instance.

[hub, 2017/05/16 21:35:55]

Acknowledged.

-}
-
-function parseSelector(selector)
-{
-  if (selector.length == 0)
-    return [];
-
-  let abpSelectorIndex = selector.indexOf(":-abp-");

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

How about you don't just go looking for anything with ":-abp-" but rather for
the specific prefixes we are interested in? This will also eliminate the
"parsing error" scenario that is adding unnecessary complexity.

  let match = /:-abp-(properties|has)\(/.exec(selector);
  if (!match)
    return [new PlainSelector(selector)];

  let startIndex = match.index + match[0].length;
  if (match[1] == "properties")
    ...
  else if (match[1] == "has")
    ...
  else
    throw new Error("Unexpected, should never get here");

Note how this approach avoids using magic numbers when calculating startIndex.

[hub, 2017/05/25 13:02:29]

I think I took the "don't use regexp" from the previous review too literally.
Will rework this accordingly.
I agree, magic numbers are bad.

Done

-  if (abpSelectorIndex == -1)
-    return [new PlainSelector(selector)];
-
-  let selectors = [];
-  if (abpSelectorIndex > 0)
-    selectors.push(new PlainSelector(selector.substr(0, abpSelectorIndex)));
-
-  let suffixStart = abpSelectorIndex;
-
-  if (selector.indexOf(":-abp-properties(", abpSelectorIndex) ==
-      abpSelectorIndex)
-  {
-    let startIndex = abpSelectorIndex + 17;
-    let endquoteIndex = selector.indexOf(selector[startIndex], startIndex + 1);
-    if ((endquoteIndex == -1) || (selector[endquoteIndex + 1] != ")"))
-      return null;
-
-    selectors.push(new PropsSelector(
-      selector.substr(startIndex + 1, endquoteIndex - startIndex - 1)));
-    suffixStart = endquoteIndex + 2;
-  }
-  else if (selector.indexOf(":-abp-has(", abpSelectorIndex) ==
-      abpSelectorIndex)
-  {
-    let startIndex = abpSelectorIndex + 10;
-    let parens = 1;
-    let i;
-    for (i = startIndex; i < selector.length; i++)
-    {
-      if (selector[i] == "(")
-        parens++;
-      else if (selector[i] == ")")
-        parens--;
-
-      if (parens == 0)
-        break;
-    }
-    if (parens != 0)
-      return null;
-
-    let hasSelector = new HasSelector(
-      selector.substr(startIndex, i - startIndex));
-    if (!hasSelector.ok())
-      return null;
-    selectors.push(hasSelector);
-    suffixStart = i + 1;

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

I'm not really happy with the way the contents of the pseudos are being parsed -
having two different parsing approaches making different assumptions about
contents is rather suboptimal. A generic approach isn't significantly more
complicated, something like this:

  let parens = 1;
  let quoted = null;
  let i;
  for (i = startIndex; i < selector.length; i++)
  {
    if (selector[i] == "\\")
      i++; // skip next character, it's escaped
    else if (quoted)
    {
      if (selector[i] == quoted)
        quoted = null;
    }
    else if (selector[i] == "'" || selector[i] == '"')
      quoted = selector[i];
    else if (selector[i] == "(")
      parens++;
    else if (selector[i] == ")")
    {
      parens--;
      if (parens == 0)
        break;
    }
  }
  if (parens != 0)
  {
    console.error(new SyntaxError("Failed parsing Adblock Plus selector " +
selector + ", didn't find closing parenthesis"));
    return null;
  }
  let contents = selector.substring(startIndex, i);
  let suffixStart = i + 1;
  if (match[1] == "properties")
    selectors.push(new PropsSelector(contents));
  else if (match[1] == "has")
    selectors.push(new HasSelector(contents));
  ...

Note that we shouldn't merely ignore invalid selectors, we should always report
an error so that filter list authors know they've made a mistake.

[hub, 2017/05/25 13:02:28]

They are parsed differently because -abp-properties() content is simply quoted,
ie we ignore everything between the quote. On the other hand, :-abp-has() is
unquoted becauase it is a selector. Also current parsing doesn't handle escapes
for the quotes. Reworked to a single parser.

As for the error reporting, I was not sure what's the best way to do it. I have
added a console.error() like you suggested in the right places.

-  }
-  else
-  {
-    // this is an error, can't parse selector.
-    return null;
-  }
-
-  let suffix = parseSelector(selector.substr(suffixStart));
-  if (suffix == null)
-    return null;
-
-  selectors.push(...suffix);
-
-  return selectors;
-}
-
-function matchStyleProps(style, rule, pattern, selectors, filters)

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

There is something wrong with this function. Please try to describe what it is
doing. IMHO that description will necessarily get very lengthy because it
doesn't isolate a single piece of functionality (matching) but combines it with
the processing of the results.

IMHO the matchStyleProps call below has to be replaced with something like:

  if (pattern.regexp.test(style))
    filters.push(...generateHidingSelectors(pattern, rule));

[hub, 2017/05/16 21:35:54]

I did merge that function into findPropsSelectors() because it is only called
from there, because to where it originally was.

-{
-  if (pattern.regexp.test(style))
-  {
-    let subSelectors = splitSelector(rule.selectorText);
-    for (let i = 0; i < subSelectors.length; i++)
-    {
-      let subSelector = subSelectors[i];

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

This should be a for..of loop.

[hub, 2017/05/16 21:35:54]

Acknowledged.

-      selectors.push(pattern.prefix + subSelector + pattern.suffix);
-      filters.push(pattern.text);
-    }
-  }
-}
-
-function findPropsSelectors(stylesheet, pattern, selectors, filters)
-{
-  let rules = stylesheet.cssRules;
-  if (!rules)
-    return;
-
-  for (let rule of rules)
-  {
-    if (rule.type != rule.STYLE_RULE)
-      continue;
-
-    let style = stringifyStyle(rule.style);
-    matchStyleProps(style, rule, pattern, selectors, filters);
-  }

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

We are still iterating through all stylesheets for each rule individually. In
particular, we will stringify the style rules for each property rule. This is
very inefficient.

[hub, 2017/05/31 02:16:49]

Done.

-}
-
-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, stylesheet)
-{
-  if (index >= chain.length)
-  {
-    yield prefix;
-    return;
-  }
-  for (let [selector, element] of
-       chain[index].getSelectors(prefix, subtree, stylesheet))
-    yield* evaluate(chain, index + 1, selector, element, stylesheet);
-}
-
-/*
- * getSelector() is a generator function returning a pair of selector
- * string and subtree.
- * getElements() is a generator function returning elements selected.

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

This should be two proper JSDoc comments on the respective methods.

[hub, 2017/05/16 21:35:54]

Acknowledged.

- */
-function PlainSelector(selector)
-{
-  this._selector = selector;
-}
-
-PlainSelector.prototype = {
-  *getSelectors(prefix, subtree, stylesheet)
-  {
-    yield [prefix + this._selector, subtree];
-  },
-
-  *getElements(prefix, subtree, stylesheet)

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

getElements() is never being called. On the other hand, there is no code
watching for DOM modifications either - so going with selectors only will hide
the wrong elements whenever the DOM changes. I guess that this is something you
still want to address?

[hub, 2017/05/26 12:42:46]

This is something that I still need to address.

[hub, 2017/05/31 02:16:49]

I removed the unused getElements. But I don't address the DOM changes yet.

I'll do that in either:
1. a new revision of this patch
2. another patch with a new review request (possibly holding this checkin back) 
Whichever is easiest to review.

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

We (Felix, Sebastian and me) discussed this in the office today. The consensus
is that we should go with the solution that is faster to implement

-  {
-    for (let selector of this.getSelectors(prefix, subtree, stylesheet))

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

Please use proper destructuring - the result isn't a selector but an array:

  for (let [selector, _] of this.getSelectors(prefix, subtree, stylesheet))

[hub, 2017/05/16 21:35:50]

It would be `let [selector] of `. The `, _` part create an unused variable `_`
and eslint complains about it.

Also fixed the line below too.

-      for (let element of subtree.querySelectorAll(selector[0]))
-        yield element;
+
+  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 this.findPropsSelectors(styles, prefix, this._regexp))
+      yield [selector, subtree];
   }
 };
 
-function HasSelector(selector)
-{
-  this._innerSelectors = parseSelector(selector);
-}
-
-HasSelector.prototype = {
-  ok()

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

Rename this into valid()?

[hub, 2017/05/16 21:35:55]

Acknowledged.

-  {
-    return this._innerSelectors != null;
-  },
-
-  *getSelectors(prefix, subtree, stylesheet)
-  {
-    for (let element of this.getElements(prefix, subtree, stylesheet))
-      yield [prefix + makeSelector(element, ""), subtree];

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

The prefix should be ignored here - the result of makeSelector() is already the
full selector, it shouldn't be concatenated with anything. Also, second element
of the array returned should be `element`, here you know which subtree you are
in so further refining of the selector should no longer search the entire
document.

[hub, 2017/05/16 21:35:52]

Done.

-  },
-
-  *getElements(prefix, subtree, stylesheet)
-  {
-    let elements = subtree.querySelectorAll(prefix ? prefix : "*");

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

This still won't do the right thing for something like "foo > :-abp-has(...)".
As I mentioned before, we need to append "*" to the prefix if it is empty or
ends with [\s>+~].

[hub, 2017/05/16 21:35:52]

Acknowledged.

-    for (let element of elements)
-    {
-      let newPrefix = makeSelector(element, "");
-      let iter = evaluate(this._innerSelectors, 0, "", element, stylesheet);

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

Why pass empty string rather than newPrefix + " " here? Won't this do the right
thing?

[hub, 2017/05/16 21:35:55]

I pass `element` as the subtree so from here I don't need the pefix for
evaluate.

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

But you need it so that you get the right selectors back. In particular, if you
have nested :has() selectors, concatenating prefix and the generated selector
will produce wrong results (generated selector is rooted). Mind you,
querySelectorAll() might be somewhat slower because you are passing in a longer
selector - this shouldn't be significant however.

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

Then below, line 299 I just pass "selector".

Done

-      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(selector)
-{
-  this._regexp = new RegExp(parsePropSelPattern(selector), "i");
-}
-
-PropsSelector.prototype = {
-  *getSelectors(prefix, subtree, stylesheet)
-  {
-    let selectors = [];
-    let filters = [];
-    let selPattern = {
-      prefix,
-      suffix: "",

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

Why do we still have the suffix here if it is always an empty string? Also, does
it still make sense to have this pattern object rather than merely passing a
regexp? You don't need to pass the prefix, you could rather add it to the
findPropsSelectors() results here - you are looping through them anyway.

[hub, 2017/05/16 21:35:55]

I think `suffix` left was an oversight from when I reworked the code. Removing
it as well as changing the code that still use it.

-      regexp: this._regexp
-    };
-
-    findPropsSelectors(stylesheet, selPattern, selectors, filters);

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

findPropsSelectors() should really be a generator yielding selector/filter
combinations. We no longer have to deal with arrays to store results here.

[hub, 2017/05/16 21:35:53]

we don't even need filter at the point. they are totally unused in that context.
Turned into a generator.

-    for (let selector of selectors)
-      yield [selector, subtree];
-  },
-
-  *getElements(prefix, subtree, stylesheet)
-  {
-    for (let [selector, element] of
-         this.getSelectors(prefix, subtree, stylesheet))
-      for (let subElement of element.querySelectorAll(selector))

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

Please don't pretend that element is meaningful here - it's always subtree for
this class. So it should be:

  for (let [selector, _] of ...)
    for (let element of subtree.querySelectorAll(selector))
      yield element;

In fact, it's exactly the same implementation as for PlainSelector.

[hub, 2017/05/16 21:35:50]

Done.

-        yield subElement;
-  }
-};
-
-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 = {
-

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

Nit: This empty line needs to go (ESLint should warn about that IMHO).

[hub, 2017/05/16 21:35:53]

Sadly it doesn't.

   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(stylesheet)
+  addSelectors(stylesheets)
   {
     let selectors = [];
-    let filters = [];
-
-    // Explicitly ignore third-party stylesheets to ensure consistent behavior
-    // between Firefox and Chrome.
-    if (!this.isSameOrigin(stylesheet))
-      return;
-
-    for (let patterns of this.selPatterns)
-      for (let selector of
-           evaluate(patterns.selectors, 0, "", document, stylesheet))
+    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)
       {
-        selectors.push(selector);
-        filters.push(patterns.text);
+        if (rule.type != rule.STYLE_RULE)
+          continue;
+
+        cssStyles.push(stringifyStyle(rule));
       }
-
-    this.addSelectorsFunc(selectors, filters);
+    }
+
+    let {document} = this.window;
+    for (let pattern of this.patterns)
+    {
+      for (let selector of evaluate(pattern.selectors,
+                                    0, "", document, cssStyles))
+      {
+        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, selectorFilters);
+    this.hideElemsFunc(elements, elementFilters);
   },
 
   onLoad(event)
   {
     let stylesheet = event.target.sheet;
     if (stylesheet)
-      this.addSelectors(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;
-        for (let stylesheet of document.styleSheets)
-          this.addSelectors(stylesheet);
+        this.addSelectors(document.styleSheets);
         document.addEventListener("load", this.onLoad.bind(this), true);
       }
+      reportError = oldReportError;
     });
   }
 };