Issue 5249 - Implement :-abp-contains()

Issue 5249 - Implement :-abp-contains()

[hub, 2017-05-25 13:05:09]

This is a preliminary implementation, in-flux as it relies on issue 3143. Not
ready for review.

[hub, 2017-06-21 21:32:13]

Here are two design choice:

- we do an exact match otherwise the element selection is either too greedy or
require a traversal to get the lowest match... If you really think this should
be the case, it can be done. It is a matter of going down the tree until it no
longer match (children then siblings)

- we use textContent to determine the text content. We could use innerText that
would deal with styling (aka ignoring content marked as hidden) that could be a
more resilient to counter measures. But might be slower too.

[Wladimir Palant, 2017-06-28 10:47:25]

https://codereview.adblockplus.org/29448560/diff/29470694/chrome/content/elem...
File chrome/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29448560/diff/29470694/chrome/content/elem...
chrome/content/elemHideEmulation.js:253: if (element.textContent == this._text)
An exact match is definitely not what we want here, especially if you think
about whitespace. Just confirmed by looking at the implementation in Sizzle: a
case-sensitive substring match is the expected behavior. In other words:

  if (element.textContent.contains(this._text))

As to your concerns about this behavior being greedy: yes, if we have a selector
like `:contains(Advertisement)` without any further qualifiers we'll potentially
hide the document element. While it doesn't make a whole lot of sense, this is
the correct behavior IMHO. If overshooting like this isn't the idea, the
selector should have additional qualifiers to limit the number of potentially
matching elements.

I think that we can catch this issue by rejecting chains consisting only of a
single ContainsSelector instance - these never make sense. On the top level,
such filters will either hide nothing (text not found on the page) or the entire
document element. Inside an :-abp-has() pseudo they don't make sense either,
:-abp-has() should simply be replaced by :-abp-contains() to achieve the same
effect. Selectors like `#sidebar > :contains(Advertisement)` or
`:contains(Advertisement) + .block` are valid however as they will only apply to
specific elements (the latter being quite suboptimal performance-wise but
still).

As to your considerations about textContent vs. innerText, getting the text "as
rendered" is a very complicated affair and switching to a bogus IE-compat
feature won't save us here. A quick test in Firefox shows inconsistent handling
of display:none (hidden nodes as children are ignored but element.innerText of a
hidden node returns text content). Also, tricks like `font-size:0` or `::before`
pseudo aren't being considered here. So dealing with potential circumvention
isn't trivial, let's wait and see what shows up in the wild first.

https://codereview.adblockplus.org/29448560/diff/29470694/test/browser/elemHi...
File test/browser/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29448560/diff/29470694/test/browser/elemHi...
test/browser/elemHideEmulation.js:416: ["div > div > :-abp-contains(to hide)"]
How about "#parent > div:-abp-contains(to hide)"? This is a slightly less
trivial scenario, sibling and toHide should be hidden.

[hub, 2017-06-28 14:39:11]

updated patch

https://codereview.adblockplus.org/29448560/diff/29470694/chrome/content/elem...
File chrome/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29448560/diff/29470694/chrome/content/elem...
chrome/content/elemHideEmulation.js:253: if (element.textContent == this._text)
On 2017/06/28 10:47:24, Wladimir Palant wrote:
> An exact match is definitely not what we want here, especially if you think
> about whitespace. Just confirmed by looking at the implementation in Sizzle: a
> case-sensitive substring match is the expected behavior. In other words:
> 
>   if (element.textContent.contains(this._text))
> 
> As to your concerns about this behavior being greedy: yes, if we have a
selector
> like `:contains(Advertisement)` without any further qualifiers we'll
potentially
> hide the document element. While it doesn't make a whole lot of sense, this is
> the correct behavior IMHO. If overshooting like this isn't the idea, the
> selector should have additional qualifiers to limit the number of potentially
> matching elements.
> 
> I think that we can catch this issue by rejecting chains consisting only of a
> single ContainsSelector instance - these never make sense. On the top level,
> such filters will either hide nothing (text not found on the page) or the
entire
> document element. Inside an :-abp-has() pseudo they don't make sense either,
> :-abp-has() should simply be replaced by :-abp-contains() to achieve the same
> effect. Selectors like `#sidebar > :contains(Advertisement)` or
> `:contains(Advertisement) + .block` are valid however as they will only apply
to
> specific elements (the latter being quite suboptimal performance-wise but
> still).
> 
> As to your considerations about textContent vs. innerText, getting the text
"as
> rendered" is a very complicated affair and switching to a bogus IE-compat
> feature won't save us here. A quick test in Firefox shows inconsistent
handling
> of display:none (hidden nodes as children are ignored but element.innerText of
a
> hidden node returns text content). Also, tricks like `font-size:0` or
`::before`
> pseudo aren't being considered here. So dealing with potential circumvention
> isn't trivial, let's wait and see what shows up in the wild first.

Done.

[Wladimir Palant, 2017-06-28 14:53:58]

https://codereview.adblockplus.org/29448560/diff/29470694/test/browser/elemHi...
File test/browser/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29448560/diff/29470694/test/browser/elemHi...
test/browser/elemHideEmulation.js:416: ["div > div > :-abp-contains(to hide)"]
On 2017/06/28 10:47:24, Wladimir Palant wrote:
> How about "#parent > div:-abp-contains(to hide)"? This is a slightly less
> trivial scenario, sibling and toHide should be hidden.

Did you overlook this comment?

https://codereview.adblockplus.org/29448560/diff/29476555/chrome/content/elem...
File chrome/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29448560/diff/29476555/chrome/content/elem...
chrome/content/elemHideEmulation.js:253: if
(element.textContent.indexOf(this._text) != -1)
Nit: Please use .includes() rather than .indexOf().

[hub, 2017-06-28 16:11:36]

updated patch

https://codereview.adblockplus.org/29448560/diff/29470694/test/browser/elemHi...
File test/browser/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29448560/diff/29470694/test/browser/elemHi...
test/browser/elemHideEmulation.js:416: ["div > div > :-abp-contains(to hide)"]
On 2017/06/28 14:53:56, Wladimir Palant wrote:
> On 2017/06/28 10:47:24, Wladimir Palant wrote:
> > How about "#parent > div:-abp-contains(to hide)"? This is a slightly less
> > trivial scenario, sibling and toHide should be hidden.
> 
> Did you overlook this comment?

I did. but "#parent > div:-abp-contains(to hide)" will only select #sibling
since #tohide is not a direct child of #parent.

However "#parent div:-abp-contains(to hide)" will.

https://codereview.adblockplus.org/29448560/diff/29476555/chrome/content/elem...
File chrome/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29448560/diff/29476555/chrome/content/elem...
chrome/content/elemHideEmulation.js:253: if
(element.textContent.indexOf(this._text) != -1)
On 2017/06/28 14:53:58, Wladimir Palant wrote:
> Nit: Please use .includes() rather than .indexOf().

Done.

[Wladimir Palant, 2017-06-28 17:48:30]

LGTM