Issue 7450 - Implement hide-if-contains-visible-text snippet

lib/content/snippets.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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 390 matching lines @@
     }
   });
 }
 
 exports["hide-if-shadow-contains"] = makeInjector(hideIfShadowContains,
                                                   toRegExp, regexEscape,
                                                   hideElement);
 
 /**
  * Hides any HTML element or one of its ancestors matching a CSS selector if
+ * it matches the provided condition.
+ *
+ * @param {function} match The function that provides the matching condition.
+ * @param {string} selector The CSS selector that an HTML element must match
+ *   for it to be hidden.
+ * @param {?string} [searchSelector] The CSS selector that an HTML element
+ *   containing the given string must match. Defaults to the value of the
+ *   <code>selector</code> argument.
+ */
+function hideIfMatches(match, selector, searchSelector)
+{
+  if (searchSelector == null)
+    searchSelector = selector;
+
+  let callback = () =>
+  {
+    for (let element of document.querySelectorAll(searchSelector))
+    {
+      if (match(element))
+      {
+        let closest = element.closest(selector);
+        if (closest)
+          hideElement(closest);
+      }
+    }
+  };
+  new MutationObserver(callback)
+    .observe(document, {childList: true, characterData: true, subtree: true});

[a.giammarchi, 2019/04/26 13:33:58]

I might make a similar observation we had before (so that this is maybe a
broader issue whenever we use MutationObserver).

A CSS selector might have `a[data-whatever]` condition in it, but it'll never
have anything able to reach the text content (only the `match` callback might be
interested, not the `querySelectorAll` result)

see:
https://developer.mozilla.org/en-US/docs/Web/API/MutationObserverInit/charact...

Accordingly, are we sure we don't want to improve our CSS selector abilities so
that if it does contain a square bracket we should also watch for attribute
changes?

'cause speaking of CSS selectors, I find attributes change more relevant than
unreachable text content which never interests the CSS selector itself
(different story for XPath where this point was initially made).

What do you think?

[hub, 2019/04/27 17:32:05]

The current code is just refactoring of the other snippet "hide-if-contains".
It's a valid point but I'd rather take it as a separate issue as to make sure we
don't impact the other snippet.

[a.giammarchi, 2019/04/29 07:27:25]

Acknowledged.

[hub, 2019/04/29 15:20:55]

BTW I filed https://gitlab.com/eyeo/adblockplus/adblockpluscore/issues/12

+  callback();
+}
+
+/**
+ * Hides any HTML element or one of its ancestors matching a CSS selector if
  * the text content of the element contains a given string.
  *
  * @param {string} search The string to look for in HTML elements. If the
  *   string begins and ends with a slash (<code>/</code>), the text in between
  *   is treated as a regular expression.
  * @param {string} selector The CSS selector that an HTML element must match
  *   for it to be hidden.
- * @param {string?} [searchSelector] The CSS selector that an HTML element
+ * @param {?string} [searchSelector] The CSS selector that an HTML element
  *   containing the given string must match. Defaults to the value of the
  *   <code>selector</code> argument.
  */
 function hideIfContains(search, selector = "*", searchSelector = null)
 {
-  if (searchSelector == null)
-    searchSelector = selector;
+  let re = toRegExp(search);
+
+  hideIfMatches(element => re.test(element.textContent),
+                selector, searchSelector);
+}
+
+exports["hide-if-contains"] = hideIfContains;
+
+/**
+ * Hides any HTML element matching a CSS selector if the visible text content
+ * of the element contains a given string.
+ *
+ * @param {string} search The string to match to the visible text. Is considered
+ *   visible text that isn't hidden by CSS properties or other means.
+ *   If the string begins and ends with a slash (<code>/</code>), the
+ *   text in between is treated as a regular expression.
+ * @param {string} selector The CSS selector that an HTML element must match
+ *   for it to be hidden.
+ * @param {?string} [searchSelector] The CSS selector that an HTML element
+ *   containing the given string must match. Defaults to the value of the
+ *   <code>selector</code> argument.
+ */
+function hideIfContainsVisibleText(search, selector, searchSelector = null)
+{
+  /**
+   * Determines if the text inside the element is visible.
+   * @param {Element} element The element we are checking.
+   * @param {?CSSStyleDeclaration} [style] The computed style of element. If
+   *   falsey it will be queried.

[a.giammarchi, 2019/04/26 13:33:58]

typo? did you mean `falsy` ?

[hub, 2019/04/27 17:32:05]

Both are acceptable, and we use `falsey` elsewhere in the code base.

[a.giammarchi, 2019/04/29 07:27:25]

Acknowledged.

+   * @returns {bool} Whether the text is visible.
+   */
+  function isTextVisible(element, style)
+  {
+    if (!style)
+      style = window.getComputedStyle(element);
+
+    if (style.getPropertyValue("opacity") == "0")
+      return false;
+    if (style.getPropertyValue("font-size") == "0px")
+      return false;
+    if (style.getPropertyValue("color") ==

[a.giammarchi, 2019/04/26 13:33:57]

shouldn't `color` be checked against `transparent` too, which might not be the
same of having a transparent background?

[hub, 2019/04/27 17:32:05]

good point. I knew there were things I'm missing.

Done

+        style.getPropertyValue("background-color"))
+      return false;
+
+    return true;
+  }
+
+  /**
+   * Returns the visible text content from an element and its descendants.
+   * @param {Element} element The element whose visible text we want.
+   * @returns {string} The text that is visible.
+   */
+  function getVisibleContent(element)
+  {
+    let style = window.getComputedStyle(element);
+    if (style.getPropertyValue("display") == "none")
+      return "";
+    let visibility = style.getPropertyValue("visibility");
+    if (visibility == "hidden" || visibility == "collapse")
+      return "";
+

[a.giammarchi, 2019/04/26 13:51:06]

shouldn't `clientWidth` and `clientHeight` be also checked against 0? (either is
fine)

that should already screen a lot of fancy CSS use cases, right?

[hub, 2019/04/27 17:32:05]

Good point. I'll look into it.

+    let text = "";
+    for (let node of element.childNodes)
+    {
+      switch (node.nodeType)
+      {
+        case Node.ELEMENT_NODE:
+          text += getVisibleContent(node);
+          break;
+        case Node.TEXT_NODE:
+          if (isTextVisible(element, style))
+            text += node.nodeValue;
+          break;
+      }
+    }
+    return text;
+  }
 
   let re = toRegExp(search);

[a.giammarchi, 2019/04/26 13:33:58]

this feature is overall OK but I have one concern: it's easy with CSS to
capitalize or upper case text, but this technique seems to suffer a general
issue we have with our `toRegExp` function that doesn't accept flags as in
`/find-me/i`

I think the ignorecase is crucial for this scenario, so I wonder if we're
working on this already, or we'll need to manually address all case-sensitive
variants of any text we're looking for.

[hub, 2019/04/27 17:32:05]

Whichever effect applies to the text with CSS, the content (that we check
against) doesn't change. The 'i' flag would be useful to have the filter more
tolerant to changes.

[a.giammarchi, 2019/04/29 07:27:25]

If I understand correctly, we might check for "Ads" but the content might be
"aDs" instead, or "adS", or "ADS", or "AdS", or "aDS" and each char visually
normalized by CSS.

In this scenrio, the `i` flag does make the difference, isn't it?

[hub, 2019/04/29 13:03:47]

I see where this is going. Good point.

Because this impact other snippets, I have filed
https://gitlab.com/eyeo/adblockplus/adblockpluscore/issues/11

[a.giammarchi, 2019/04/29 15:31:10]

Acknowledged.

 
-  new MutationObserver(() =>
-  {
-    for (let element of document.querySelectorAll(searchSelector))
-    {
-      if (re.test(element.textContent))
-      {
-        let closest = element.closest(selector);
-        if (closest)
-          hideElement(closest);
-      }
-    }
-  })
-  .observe(document, {childList: true, characterData: true, subtree: true});
+  hideIfMatches(element => re.test(getVisibleContent(element)),
+                selector, searchSelector);
 }
 
-exports["hide-if-contains"] = hideIfContains;
+exports["hide-if-contains-visible-text"] = hideIfContainsVisibleText;
 
 /**
  * Hides any HTML element or one of its ancestors matching a CSS selector if
  * the text content of the element contains a given string and, optionally, if
  * the element's computed style contains a given string.
  *
  * @param {string} search The string to look for in HTML elements. If the
  *   string begins and ends with a slash (<code>/</code>), the text in between
  *   is treated as a regular expression.
  * @param {string} selector The CSS selector that an HTML element must match
@@ skipping 438 matching lines @@
 
       args[0] = url.href;
     }
 
     return fetch_.apply(this, args);
   };
 }
 
 exports["strip-fetch-query-parameter"] = makeInjector(stripFetchQueryParameter,
                                                       toRegExp, regexEscape);