Opened 4 months ago

Closed 4 months ago

Last modified 3 months ago

#6847 closed change (fixed)

Add regular expression support to hide-if-contains and hide-if-shadow-contains snippets

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

https://codereview.adblockplus.org/29852577/

Description (last modified by mjethani)

Background

Currently the hide-if-contains and hide-if-shadow-contains snippets accept a search string as their first argument. This works when the text is fixed and sufficiently unique, but it doesn't work when the text is more generic and can appear in non-ad items.

For example, the filter #$#hide-if-contains ad li can end up hiding elements that contain the word "adventure" or "advocacy", which could be part of the content of the document.

What to change

Let hide-if-contains and hide-if-shadow-contains treat the search string as a regular expression if the string is surrounded by /.

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 /\\bAd\\b/ div[id] should hide the "Check this!" ad and not the "The Adventures of Tom Sawyer" content.

Also try the following test:

<!-- test.html -->
<!-- Content --> 
<div>
  Lorem ipsum dolor sit amet, consectetur
  adipiscing elit, sed do eiusmod tempor
  incididunt ut labore et dolore magna aliqua.
</div>
<!-- Ad -->
<div id="qvdqv3k4hxe">
  <div>Best ad ever!</div>
  <div></div>
</div>
<!-- Advocacy -->
<div id="zc34r93jglg">
  <div>Support the Open Web</div>
  <div></div>
</div>
<script src="script.js"></script>
/* script.js */
setTimeout(() =>
{       
  let labelElement =
    document.getElementById("qvdqv3k4hxe").lastElementChild;
  let shadowRoot =
    labelElement.attachShadow({mode: "closed"});
  let span = document.createElement("span");
  span.innerText = "Ad";
  shadowRoot.appendChild(span);

  labelElement =
    document.getElementById("zc34r93jglg").lastElementChild;
  shadowRoot =
    labelElement.attachShadow({mode: "closed"});
  span = document.createElement("span");
  span.innerText = "Advocacy";
  shadowRoot.appendChild(span);
},
500);

The filter #$#hide-if-shadow-contains /\\bAd\\b/ div[id] should hide the ad "Best ad ever!" but not the content "Support the Open Web".

Additional notes

The \ (backslash) is an escape character in snippet filters (see #6538), which means it needs to be escaped itself to be taken literally. This is why the regular expression \bAd\b must be specified as /\\bAd\\b/ in a snippet filter.

The regular expression passed to the snippet cannot include the /i (ignore case) flag, nor any other flags; support for flags may be added in the future.

Change History (12)

comment:1 Changed 4 months ago by mjethani

  • Priority changed from Unknown to P2
  • Ready set

comment:2 Changed 4 months ago by mjethani

  • Owner set to mjethani
  • Review URL(s) modified (diff)

comment:3 Changed 4 months ago by mjethani

  • Status changed from new to reviewing

comment:4 Changed 4 months ago by abpbot

A commit referencing this issue has landed:
Issue 6847 - Add regular expression support to snippets

comment:5 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:6 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:7 Changed 4 months ago by mjethani

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

comment:8 Changed 3 months ago by abpbot

A commit referencing this issue has landed:
Issue 6847 - Add regular expression support to snippets

comment:9 Changed 3 months ago by mjethani

  • Cc Ross rscott added

comment:10 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:11 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:12 Changed 3 months ago by sebastian

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