Opened 4 months ago

Closed 4 months ago

Last modified 3 months ago

#6848 closed change (fixed)

Add support to hide-if-contains for hiding a different ancestor of the element containing the search string

Reported by: mjethani Assignee: mjethani
Priority: P2 Milestone:
Module: Core Keywords: circumvention
Cc: kzar, rscott, Ross Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29851644/

Description (last modified by mjethani)

Background

The hide-if-contains snippet takes a CSS selector as its second argument. If an element that matches the selector contains the given search string, it hides the element. This sometimes is not enough, because the element to be hidden is in fact an ancestor of the element containing the search string.

For example:

<li>
  <span>Kool-Aid</span><span>Ad</span>
</li>

Here searching for the label "Ad" with the script hide-if-contains ad li doesn't work, even with regular expressions (#6847), because the text content is read out as "Kool-AidAd". The correct script for this would be hide-if-contains ad li 'li > span' where the third argument is the CSS selector to use while searching for the text.

What to change

Add a third argument called searchSelector to hide-if-contains, which should be a CSS selector that should be used to search for the text. Its value should default to the value of the selector argument.

Hints for testers

Test that both hide-if-contains and hide-if-shadow-contains work as described in #6809 and #6798 respectively (i.e. no regressions).

For this change, try the following test:

<!-- 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></div>";
  document.body.appendChild(result);
},
500);

The filter #$#hide-if-contains Ad div[id] span.label should hide the "Check this!" ad and not the "The Adventures of Tom Sawyer" content.

Change History (14)

comment:1 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:2 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:4 Changed 4 months ago by mjethani

  • Review URL(s) modified (diff)

comment:5 Changed 4 months ago by mjethani

  • Owner set to mjethani
  • Priority changed from Unknown to P2
  • Ready set

comment:6 Changed 4 months ago by mjethani

  • Status changed from new to reviewing

comment:7 Changed 4 months ago by abpbot

comment:8 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:9 Changed 4 months ago by mjethani

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:10 Changed 4 months ago by mjethani

  • Cc rscott Ross added

comment:11 Changed 3 months ago by abpbot

comment:12 Changed 3 months ago by Ross

Tested and working in Firefox so far.

comment:13 Changed 3 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Tested and working. Original behaviour for hide-if-contains and hide-if-shadow-contains described in #6809 and #6798 still works as expected. Have not noticed any regressions so far.

ABP 3.3.0.2117
Chrome 68 / 62 / 49 / Windows 10
Firefox 61 / 60 (ESR) / 51 / Windows 10

comment:14 Changed 3 months ago by sebastian

  • Sensitive unset
Note: See TracTickets for help on using tickets.