Opened 7 weeks ago

Closed 6 weeks ago

Last modified 5 weeks ago

#6919 closed change (fixed)

Implement hide-if-contains-and-matches-style snippet

Reported by: mjethani Assignee: mjethani
Priority: P2 Milestone: Adblock-Plus-3.3.2-for-Chrome-Opera
Module: Core Keywords: circumvention
Cc: amrmak, Ross Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29873558

Description (last modified by mjethani)

Background

The hide-if-contains snippet is documented in #6809, #6847, and #6848. This snippet looks for some given text within an element and hides the element if the text is found.

Sometimes an element contains the text, but the text is hidden, which leads to a false positive. In the case of search ads, for example, all the items in the search results may contain the text "Ad" but it is only displayed for the ads and hidden for the rest of the results. We need to be able to specify an additional constraint: hide the element only if the text is visible.

To make this more generic, the filter list author may want to look for any specific computed style of an element.

This could be an additional parameter to the hide-if-contains snippet, but for backwards compatibility and to avoid false positives we must make it a new snippet instead.

What to change

Implement a new snippet called hide-if-contains-and-matches-style with the following signature:

function hideIfContainsAndMatchesStyle(search, selector = "*", searchSelector = null,
                                       style = null, searchStyle = null)

The first three parameters should have exactly the semantics of the hide-if-contains snippet.

The fourth parameter, style, should match the selector element's window.getComputedStyle() and hide the selector element only if the computed style matches.

The fifth parameter, searchStyle, should similarly match the style for the searchSelector element.

Similar to the value of the search parameter, the values of the style and searchStyle parameters should be treated as plain text unless surrounded by / (slash), in which case they should be treated as regular expressions. These values should be compared against the values of the cssText properties of the return values of their respective elements' window.getComputedStyle().

Hints for testers

Make sure that when the snippet is invoked with the first three arguments only, it behaves exactly like the hide-if-contains snippet so far (aggregate of #6809, #6847, and #6848).

To test the fourth and fifth arguments, try the following:

<!-- test.html -->
<div>
  Lorem ipsum dolor sit amet, consectetur
  adipiscing elit, sed do eiusmod tempor
  incididunt ut labore et dolore magna aliqua.
</div>
<script src="script.js"></script>
/* script.js */
setTimeout(() =>
{
  let ad = document.createElement("div");
  ad.id = Math.random().toString(36).substring(2);
  ad.innerHTML = "<div><span>Check this!</span><span class='label'>Ad</span></div>";
  document.body.appendChild(ad);

  let result = document.createElement("div");
  result.id = Math.random().toString(36).substring(2);
  result.innerHTML = "<div><span>The Adventures of Tom Sawyer</span><span class='label' style='display: none'>Ad</span></div>";
  document.body.appendChild(result);
},
500);

The following filters, each one on its own, should hide the "Check this!" ad and not the "The Adventures of Tom Sawyer" content:

  • #$#hide-if-contains-and-matches-style Ad div[id] span.label /./ 'display: inline;'
  • #$#hide-if-contains-and-matches-style Ad div[id] span.label ';' /\\bdisplay:\ inline\;/

Known limitations

In the current implementation, the fourth and fifth parameters to the snippet do not work on Firefox (no match, target element not hidden) due to Firefox bug #137687 (see discussion about Chrome vs. Firefox).

Change History (18)

comment:1 Changed 7 weeks ago by mjethani

  • Description modified (diff)

comment:2 Changed 7 weeks ago by mjethani

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:3 Changed 7 weeks ago by abpbot

A commit referencing this issue has landed:
Issue 6919 - Implement hide-if-contains-and-matches-style snippet

comment:4 Changed 6 weeks ago by mjethani

  • Description modified (diff)
  • Ready set

comment:5 Changed 6 weeks ago by mjethani

  • Description modified (diff)

comment:6 Changed 6 weeks ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:7 Changed 6 weeks ago by mjethani

  • Description modified (diff)

comment:8 Changed 6 weeks ago by mjethani

  • Description modified (diff)

comment:9 Changed 6 weeks ago by abpbot

A commit referencing this issue has landed:
Issue 6919 - Implement hide-if-contains-and-matches-style snippet

comment:10 follow-up: Changed 6 weeks ago by Ross

This works in Chrome as described, with both three arguments and five.

However I cannot get this snippet to work in Firefox with the extra arguments. The previous snippets still work, and this snippet works with three (and hides both the ad div and the tom sawyer div - as expected I think?). But using the examples above doesn't work in Firefox like Chrome.

So if I understand correctly, the filters above are saying "Hide a div that has an id and has any style, if any of its children are span.labels that both contain the text Ad and have display:inline in their style"?

comment:11 in reply to: ↑ 10 Changed 6 weeks ago by mjethani

Replying to Ross:

So if I understand correctly, the filters above are saying "Hide a div that has an id and has any style, if any of its children are span.labels that both contain the text Ad and have display:inline in their style"?

Exactly, except it's descendants rather than just children (i.e. children, grandchildren, and onwards).

I'm looking into this now.

comment:12 Changed 6 weeks ago by mjethani

  • Description modified (diff)

comment:13 Changed 6 weeks ago by mjethani

Ross, I have updated the issue description with a "Known limitations" section. I'll have to think about how to implement this efficiently on Firefox, I don't think we can do that for this release.

So:

  1. The snippet works as expected on Chrome
  2. The snippet does not work on Firefox (specifically the fourth and fifth parameters), due to bug #137687, but it has no negative effect either (the web page is not broken in any way, there are no errors in the console)

If the above limitation is acceptable, let's go with it and file a separate issue for Firefox compatibility. Let me know what you think.

comment:14 Changed 6 weeks ago by Ross

Just working on Chrome for this ticket is acceptable (if Sebastian and filter list authors agree). Filing a separate issue for Firefox compatibility makes sense to me. Shall I do that?

comment:15 Changed 6 weeks ago by mjethani

Ross, sure, please go ahead and file a ticket for Firefox compatibility.

comment:16 Changed 6 weeks ago by Ross

Ticket for Firefox compatibility: #6938

comment:17 Changed 6 weeks ago by sebastian

  • Milestone set to Adblock-Plus-3.3.2-for-Chrome-Opera
  • Sensitive unset

comment:18 Changed 5 weeks ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

This was fine, barring the Firefox issue in #6938.

ABP 3.3.1.2125
Chrome 68 / 62 / 49 / Windows 10

Note: See TracTickets for help on using tickets.