Opened 5 months ago

Closed 3 months ago

Last modified 2 months ago

#6680 closed defect (fixed)

Element hiding emulation doesn't work with :-abp-properties() and dynamic DOM updates

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

https://codereview.adblockplus.org/29836555/

Description (last modified by mjethani)

Environment

ABP 3.1 on Chrome 66 OR ABP 3.1 on Firefox

Subscriptions: EasyList Germany+EasyList

How to reproduce

  1. Browse to http://www.pcgameshardware.de/
  2. Add the filters: (ad on top is alternating, hence the two filters)
pcgameshardware.de#?#div:-abp-properties(margin-left: 10px;) > img
pcgameshardware.de#?#div:-abp-properties(margin-left: 5px;) > img
  1. Refresh the page.

Observed behaviour

AdDefend ad on top is not hidden

Expected behaviour

AdDefend ad on top is hidden

Notes

Followed Manish's suggestion to try it on an older release, and it works as expected (tested with ABP 3.0.4 on Firefox 60.0.1).

Notes for testers

  • Testing in Firefox should be fine
  • Testing in Chrome, mentionned in comment 7 might be more tricky as it seems that even without AdblockPlus the ads don't display.

Here's a test:

<h1>Hello</h1>
<script>
setTimeout(() =>
{
  let styleElement = document.createElement("style");
  document.head.appendChild(styleElement);
  styleElement.sheet.insertRule("#toHide {background-color: #000}");
  setTimeout(() =>
  {
    let divElement = document.createElement("div");
    divElement.id = "toHide";
    divElement.textContent = "I should be gone!";
    divElement.style.color = "white";
    document.body.appendChild(divElement);
  },
  1000);
},
1000);
</script>

With the filter #?#:-abp-properties(background-color: rgb(0, 0, 0)) the div element "I should be gone!" should be hidden after a second or two.

Change History (17)

comment:1 Changed 5 months ago by hfiguiere

  • Cc hfiguiere added

comment:2 Changed 5 months ago by hfiguiere

  • Owner set to hfiguiere

comment:3 Changed 5 months ago by hfiguiere

  • Keywords circumvention added

comment:4 Changed 4 months ago by mjethani

  • Component changed from Unknown to Core
  • Priority changed from Unknown to P2

comment:5 Changed 3 months ago by hfiguiere

The regression is caused by https://hg.adblockplus.org/adblockpluscore/rev/40dcf08b35c0

However the case is as follow: the website insert styles manually. This is a test case not covered in our test suite (I have one now)

Last edited 3 months ago by hfiguiere (previous) (diff)

comment:6 Changed 3 months ago by hfiguiere

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

comment:7 Changed 3 months ago by hfiguiere

As of today, on Chrome 67.0.3396.99 (macOS), the ad no longer shows, even without Adblock Plus and in a private browser. Possibly a site specific issue.

On the other hand, it reproduces in Firefox as originally reported.

comment:8 Changed 3 months ago by kzar

I guess this is not a release blocker, since it is not a regression since the last release, so I agree on P2. It would be cool to include a fix however.

comment:9 Changed 3 months ago by abpbot

A commit referencing this issue has landed:
Issue 6680 - PropsSelector also depend on DOM modifications

comment:10 Changed 3 months ago by hfiguiere

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

comment:11 Changed 3 months ago by hfiguiere

  • Description modified (diff)

comment:12 Changed 3 months ago by mjethani

  • Summary changed from Element hiding emulation doesn't work as expected to Element hiding emulation doesn't work with :-abp-properties() and dynamic DOM updates

comment:13 Changed 3 months ago by mjethani

  • Cc kzar added
  • Ready set

comment:14 Changed 3 months ago by mjethani

  • Blocking 6437 added

comment:15 Changed 2 months ago by Ross

I've tried looking at this but am not sure if I'm actually seeing any AdDefend ads at this point. I also tried creating a small test page that adds a div with an img and the margin but I couldn't get a filter to match that either.

Is it possible to get a test case in the same vain as #6803 or confirmation AdDefend ads are still running on the site above?

ABP 3.2.0.2103

comment:16 Changed 2 months ago by mjethani

  • Description modified (diff)

comment:17 Changed 2 months ago by mjethani

Ross, I've added a test for this in the issue description.

Note: See TracTickets for help on using tickets.