Opened 8 months ago

Closed 7 months ago

Last modified 3 months ago

#4603 closed defect (fixed)

Block element tool highlighting is broken by certain element hiding filters

Reported by: xqwztis Assignee: kzar
Priority: P3 Milestone: Adblock-Plus-1.13-for-Chrome-Opera
Module: Platform Keywords:
Cc: kzar, sebastian, mapx, Lain_13 Blocked By:
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29365526/

Description (last modified by kzar)

Environment

Chrome 54, Adblock Plus 1.12.4

No filter subscriptions at all, only this filter:

##div[class^="_"][style*="absolute"][style*="position"][style*="z-index"]

How to reproduce

  1. Browse to https://reddit.com (or any other website)
  2. Click the Adblock Plus icon and then "Block element".
  3. Roll the cursor over the page to select an element to block.

Observed behaviour

No elements are highlighted as the mouse passes over them.

Expected behaviour

Blockable elements should be highlighted yellow.

Change History (15)

comment:1 Changed 8 months ago by xqwztis

Environment

Extensions: 13

Apps: 5

Last edited 8 months ago by xqwztis (previous) (diff)

comment:2 Changed 8 months ago by mapx

  • Cc kzar sebastian mapx added

comment:3 Changed 7 months ago by mapx

well, all this strange behaviour is due to this filter in ruadlist:

##div[class^="_"][style*="absolute"][style*="position"][style*="z-index"]

whitelisting it all is ok.

The question is: how could a hiding filter provoke such weird behaviour ?

Last edited 7 months ago by mapx (previous) (diff)

comment:4 Changed 7 months ago by mapx

  • Cc Lain_13 added

comment:6 Changed 7 months ago by dimisa

comment:7 Changed 7 months ago by kzar

  • Component changed from Unknown to Platform
  • Description modified (diff)
  • Priority changed from Unknown to P3
  • Ready set
  • Summary changed from Adblock Plus doesn't highlight elements to be blocked (mac OS Sierra) to Block element tool highlighting is broken by certain element hiding filters

Can also reproduce this on Linux so seems unrelated to operating system.

comment:8 Changed 7 months ago by kzar

  • Owner set to kzar

comment:9 Changed 7 months ago by kzar

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

The filter composer highlights elements by creating an overlay on top of them which is translucent yellow / red. The troublesome filter was simply hiding those elements!

Turns out our code had a typo, it was giving the overlay elements the style of display: inline-box instead of display: inline-block, but I also needed to add !important in order to fix this problem.

(What made this a little trickier to debug was that a code comment was out of date and the code was a little confusing. (We used to sometimes highlight elements by adding styles to them, instead of always using overlays. But that is no longer the case.) I added another review to at least improve the comments.)

comment:10 Changed 7 months ago by abpbot

comment:11 Changed 7 months ago by kzar

  • Milestone set to Adblock-Plus-for-Chrome-Opera-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:12 Changed 7 months ago by mapx

well, the solution is unfortunately based on the inability of ABP for chrome to hide the elements with style's !important keyword.

if I use Lain's userscript (in tampermonkey) "it's not important" the issue is still not fixed.
https://greasyfork.org/en/scripts/14720-it-s-not-important

This userscript is used exactly to fight the increasingly large number of sites using the !important trick.

comment:13 Changed 7 months ago by Lain_13

I'd actually recommend to give an element a random string as an ID and operate it through a STYLE element. This way it will have only an ID and will never be hidden by complex style-based filters.
Please, don't use static prefixes or suffixes since some sites attempt to exploit it in order to break user's ability to hide ads on their page in an easy way.

comment:14 Changed 7 months ago by kzar

Well I considered using the random ID / class approach, but that would add quite a bit of complexity for almost no gain. The case of element hiding filters matching our overlays is already rare enough, the combination of that with the use of Lain's userscript is even rarer.

At some point in the future it would be nice to tidy up some of the block element code further, perhaps then we could improve this too, but in the mean time I think the !important workaround is good enough, sorry.

Last edited 7 months ago by kzar (previous) (diff)

comment:15 Changed 3 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Fixed. Element hiding is improved compared to 1.12.4.

ABP 1.12.4.1739
Chrome 49 / 56 / Windows 10
Chrome 56 / OS X 10.12
Chrome 56 / Ubuntu 16.04
Opera 37 / 41 / Windows 7
Safari 10 / OS X 10.12

Note: See TracTickets for help on using tickets.