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

lib/content/snippets.js

Index: lib/content/snippets.js
===================================================================
--- a/lib/content/snippets.js
+++ b/lib/content/snippets.js
@@ -900,8 +900,98 @@
     }
 
     return fetch_.apply(this, args);
   };
 }
 
 exports["strip-fetch-query-parameter"] = makeInjector(stripFetchQueryParameter,
                                                       toRegExp, regexEscape);
+

[Manish Jethani, 2019/04/17 13:54:26]

Nit: Maybe a good idea to group all the `hide-if-contains-*` snippets together?

[hub, 2019/04/17 17:25:06]

yes

+/**
+ * Hide an element matching a selector if it contains a specified visible text.

[Manish Jethani, 2019/04/17 13:54:25]

Nits or consistency:

 s/Hide/Hides/
 s/an element/any HTML element/
 s/a selector/a CSS selector/

 s/if it contains a specified visible text/<placeholder>/

<placeholder>: if the visible text content of the element contains a given
string.

[hub, 2019/04/17 17:25:06]

Done.

+ *
+ * @param {string} search the text 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} hideSelector the selector for the element that must be
+ *   hidden.
+ * @param {?string} [innerSelector] the selector relative to the element that
+ *   will select the text content to match.
+ */
+function hideIfContainsVisibleText(search, hideSelector, innerSelector = null)

[Manish Jethani, 2019/04/09 12:31:23]

I know the use case here is to target a node based on visible text, but I can
see the definition of "visible" changing both between releases and for different
sites. How about we make it an additional parameter to the
`hide-if-contains-and-matches-style` snippet? We could call it `textStyle`. In
addition to doing everything else it already does, it would also do what it does
here, i.e. check the style of the text.

If we do this, it would be more flexible but would not cover the case of `color
== background-color`, but filter list authors can just specify the actual color
on the site and it should work. Since regular expressions would be supported, it
should cover more cases.

To use the example in the Trac ticket:

  <style>
    .c978y4 {
      display: none;
    }
  </style>
  <div id="foo">
    <span>S</span><span class="c978y4">A</span><span>ponso</span><span
class="c978y4">x</span><span>red</span>
  </div>

The filter would look like this:

  example.com#$#hide-if-contains-and-matches-style Sponsored #foo #foo '' ''
'display: inline'

With named arguments:

  example.com#$#hide-if-contains-and-matches-style search=Sponsored
selector=#foo text-style='display: inline'

What do you think?

[hub, 2019/04/10 16:16:21]

I think this defeat the design of that snippet that establish the visibility:
the idea is that the filter authors don't have to deal with it, and therefor
don't need to update the filter when things change. We should instead ensure we
can cover all the bases here.

Also we have seen in the wild case were some of the text has opacity=0 and other
has font-size=0. In the same label. Which this snippet handles fine.

[Manish Jethani, 2019/04/10 16:39:23]

Instead of hard-coding the styles that would identify text that is "visible" (by
some definition of the term), what I am suggesting is that we make it dynamic so
that filter list authors can specify their own style. In the case of ABP
filters, we would then specify the styles ourselves - but not in the snippet's
code but in the filter. This way we can use it for multiple purposes to cover
more cases and still retain the flexibility of updating our technique to
identify visible text without having to do a whole new release.

What do you think is the downside to this approach exactly?

[Manish Jethani, 2019/04/10 17:15:17]

By the way, if you think we should add a `hide-if-contains-visible-text`
snippet, let's add that too, but let's route the implementation through
`hide-if-contains-and-matches-style`. This way if it gets circumvented at least
we don't have to wait for a new release, plus the text-style matching
functionality might come in handy in other cases.

Does this make sense?

[hub, 2019/04/10 19:40:05]

I'd be ok, despite being more complicated, to allow filter authors to specify
properties/values to either override or supplement the defaults. However this
mean we need to agree on a syntax and that can get complicated. But it is
important that this snippets works well with default values (I have added more
CSS properties than what I found used in the wild, and I'm ok to justify adding
more).

Also another thing that is not being handled, because I believe it is
complicated, but could fall into the area of this snippets, is the absolute
position of the element could out of the display to hide it. I have deliberately
left this out for the first version because of its inherent complexity. But if
it came it was used...

[hub, 2019/04/10 19:40:05]

I don't think it make sense to merge the code of these two snippets.

[Manish Jethani, 2019/04/10 19:48:20]

We don't have to invent anything new for the syntax, it can use the same syntax
as the `style` and `searchStyle` parameters? Isn't it?
Ack, but we can cross that bridge when we get to it.

[Manish Jethani, 2019/04/10 19:48:20]

What I'm saying is that the entire functionality that you have implemented here
can be implemented with _fewer_ lines of code _and_ combined with other
functionality in an existing snippet. What's wrong with this? Most of the
functionality is common, it's just an extra parameter and a few more lines of
code.

[Manish Jethani, 2019/04/10 19:58:52]

To give an example of what I'm saying, let's say there's a function like this:

  /**
    * Finds needle string in haystack string.
    */
  function indexOf(haystack: string, needle: string, fromIndex: int = 0);

Now we want to implement a `startsWith()` function. What I'm saying is that we
should extend the `indexOf()` function to take an additional parameter:

    function indexOf(haystack: string, needle: string, fromIndex: int = 0,
                     toIndex: int = -1);

If `toIndex` is 0, it stops the search at index 0.

Then we implement `startsWith()` like this:

  function startsWith(haystack: string, needle: string)
  {
    return indexOf(haystack, needle, 0, 0);
  }

What you're saying is that we repeat the code for searching for a string within
a string in `startsWith()`. I don't understand why. We might do it for
performance reasons, but in this case I don't see how such a reason would apply.
If the parameter is not passed then the function would not follow that path,
period.

[Manish Jethani, 2019/04/17 13:54:25]

Nit: parameter names for consistency with other snippets:

  search
  selector
  searchSelector

(All nits are mere suggestions.)

[hub, 2019/04/17 17:25:06]

They have a different meaning, but.... (see below)

+{
+  /**
+   * Determine if the text inside the element is visible.

[Manish Jethani, 2019/04/17 13:54:25]

Nit: s/Determine/Determines/

[hub, 2019/04/17 17:25:06]

Done.

+   * @param {Element} element the leaf element we are checking.

[Manish Jethani, 2019/04/17 13:54:26]

Nit: So far in this file the doc string starts with a capital letter, i.e. "The
leaf element ...".

[hub, 2019/04/17 17:25:06]

Done.

+   * @param {?CSSStyleDeclaration} [style] the computed style of element. If
+   *   falsey it will be queried.
+   * @returns {bool} whether the text is visible.
+   */
+  function isTextVisible(element, style)
+  {
+    if (!style)
+      style = window.getComputedStyle(element);

[Manish Jethani, 2019/04/17 13:54:26]

There's a local `getComputedStyle()` which covers all supported platforms.

[hub, 2019/04/17 17:25:06]

I only see `getComputedCSSText()` which calls`getComputedStyle()`. But here I'm
not interested in the text version of the computed style.

[Manish Jethani, 2019/04/17 18:33:28]

Sorry, my bad.

Ack.

+
+    if (style.getPropertyValue("opacity") == "0")
+      return false;
+    if (style.getPropertyValue("font-size") == "0px")
+      return false;
+    if (style.getPropertyValue("color") ==
+        style.getPropertyValue("background-color"))
+      return false;
+
+    return true;
+  }
+
+  /**
+   * Returns the visible text content from an element and its children.
+   * @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 "";
+
+    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);
+
+  new MutationObserver(() =>
+  {
+    for (let element of document.querySelectorAll(hideSelector))
+    {
+      let inners =
+          innerSelector ? element.querySelectorAll(innerSelector) : [element];

[Manish Jethani, 2019/04/17 13:54:26]

Here there's a second call to `querySelectorAll()`, whereas in
`hide-if-contains` there is only one call, even though the selector for hiding
vs. searching (inner) is also different over there. Maybe it's better to follow
the same pattern?

[hub, 2019/04/17 17:25:06]

Let's rework this in the next patch. (see comment about the argument names)

[Manish Jethani, 2019/04/17 18:33:28]

Just so I get this right, we'll have a follow-up patch which (1) makes this Type
1 snippet and (2) refactor the code a little bit. Yes?

[hub, 2019/04/17 18:40:34]

The current patch already refactor :-)

+      for (let inner of inners)
+      {
+        let content = getVisibleContent(inner);
+        if (re.test(content))
+          hideElement(element);
+      }
+    }
+  })
+  .observe(document, {childList: true, characterData: true, subtree: true});
+}
+
+exports["hide-if-contains-visible-text"] =
+  makeInjector(hideIfContainsVisibleText, hideElement);