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 408 matching lines @@
  */
 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);
-      }
+      let closest = element.closest(selector);
+      if (closest && match(element, 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.
@@ skipping 25 matching lines @@
  *   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
+   * @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"))
+
+    let color = style.getPropertyValue("color");
+    // if color is transparent...
+    if (color == "rgba(0, 0, 0, 0)")
       return false;
+    if (style.getPropertyValue("background-color") == color)
+      return false;
 
     return true;
+  }
+
+  /**
+   * Check if an element is visible
+   * @param {Element} element The element to check visibility of.
+   * @param {?CSSStyleDeclaration} style The computed style of element. If
+   *   falsey it will be queried.
+   * @param {?Element} closest The closest parent to reach.
+   * @return {bool} Whether the element is visible.
+   */
+  function isVisible(element, style, closest)
+  {
+    if (!style)
+      style = window.getComputedStyle(element);
+
+    if (style.getPropertyValue("display") == "none")
+      return false;
+    let visibility = style.getPropertyValue("visibility");
+    if (visibility == "hidden" || visibility == "collapse")
+      return false;
+
+    if (!closest || element == closest)
+      return true;
+
+    let parent = element.parentElement;
+    if (!parent)
+      return true;
+
+    return isVisible(parent, null, closest);
   }
 
   /**
    * Returns the visible text content from an element and its descendants.
    * @param {Element} element The element whose visible text we want.
+   * @param {Element} closest The closest parent to reach while checking
+   *   for visibility.
    * @returns {string} The text that is visible.
    */
-  function getVisibleContent(element)
+  function getVisibleContent(element, closest)
   {
     let style = window.getComputedStyle(element);
-    if (style.getPropertyValue("display") == "none")
-      return "";
-    let visibility = style.getPropertyValue("visibility");
-    if (visibility == "hidden" || visibility == "collapse")
+    if (!isVisible(element, style, closest))
       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);
+          text += getVisibleContent(node, element);
           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.

 
-  hideIfMatches(element => re.test(getVisibleContent(element)),
-                selector, searchSelector);
+  hideIfMatches(
+    (element, closest) => re.test(getVisibleContent(element, closest)),
+    selector, searchSelector);
 }
 
 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
@@ skipping 441 matching lines @@
 
       args[0] = url.href;
     }
 
     return fetch_.apply(this, args);
   };
 }
 
 exports["strip-fetch-query-parameter"] = makeInjector(stripFetchQueryParameter,
                                                       toRegExp, regexEscape);