Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#4394 closed change (fixed)

Create a new filter class for element hiding emulation filters

Reported by: fhd Assignee: fhd
Priority: P2 Milestone:
Module: Core Keywords:
Cc: trev, kzar, mario, Lain_13, mapx, greiner, rach, sergz Blocked By:
Blocking: #3143, #4592, #4592, #4658, #4659 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29361668/

Description (last modified by fhd)

Background

Element hiding filters hide elements specified via standard CSS selectors. Standard CSS selectors are however not always powerful enough, hence we extended the syntax with CSS property filters, which are element hiding filters that can also match elements based on the CSS properties they have. #3143 aims to further extend the syntax by supporting the new CSS4 :has() pseudo class.

We need a single filter class for all such non-standard extensions to the element hiding syntax because those extensions need to work in conjunction, they cannot be mutually exclusive.

They are quite similar anyway: They do not get treated like other element hiding rules, they need a content script to actually apply them. Because of this, they add runtime overhead and should never be generic (i.e. not domain-specific).

The new filter class should have flags to mark which extensions are actually used in which filter, to make it easier for the content scripts to apply them.

The best name we could come up with for this type of filter is Element Hiding Emulation Filter.

What to change

  1. Replace the CSSPropertyFilter class by the new ElemHideEmulationFilter class.
  2. The new filter class needs to have flags that are set based on which extensions are being used in the filter. For now, one flag for CSS properties and one for :has would suffice.
  3. The CSS property filters content script needs to be updated so it ignores filters that don't have the CSS properties flag set.

The :has() pseudo-class doesn't actually have to be supported at this point, that's what #3143 is for.

Please note that the Chrome/Firefox specific changes will happen in dedicated issues that will be created once this has landed.

Change History (17)

comment:1 Changed 3 years ago by mario

  • Cc mario added

comment:2 Changed 3 years ago by mapx

  • Cc Lain_13 mapx added

comment:3 Changed 3 years ago by fhd

Given how some ElemHideEmulationFilters might actually only use extensions that do not need to be emulated on some browsers (i.e. already supported new CSS features), I think it might be best to do feature detection before deciding whether or not a filter is a plain ElemHideFilter or actually an ElemHideEmulationFilter. That way, the content scripts won't even have to bother with those.

Wladimir, what do you think? (Also in general, I'm reluctant to put much energy into this before it's set to Ready.)

comment:4 Changed 3 years ago by trev

Given that :has() isn't supported by any browsers whatsoever this is a purely theoretical discussion at the moment. Of course we could do feature detection, and we've done it in the past. It's merely a temporary solution however, and right now it's not necessary.

comment:5 Changed 3 years ago by trev

  • Ready set

comment:6 Changed 3 years ago by greiner

  • Cc greiner added

comment:7 Changed 3 years ago by rach

  • Cc rach added

comment:8 Changed 3 years ago by fhd

  • Description modified (diff)

comment:9 Changed 3 years ago by fhd

  • Blocked By 4592 added

comment:10 Changed 3 years ago by fhd

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

comment:11 Changed 3 years ago by abpbot

comment:12 Changed 3 years ago by fhd

  • Blocking 4658 added

comment:13 Changed 3 years ago by fhd

  • Blocking 4659 added

comment:14 Changed 3 years ago by fhd

  • Blocked By 4592 removed
  • Blocking 4592 added
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:15 Changed 3 years ago by fhd

  • Blocking

comment:16 Changed 2 years ago by sergz

  • Cc sergz added

comment:17 Changed 2 years ago by Ross

Tested this change as part of #4658 and it appears to be working okay.

(Leaving ticket open until Chrome changes).

ABP 2.8.2.4246-beta
Firefox 51 / 53 / Beta (54.0b2)
Windows 7

Note: See TracTickets for help on using tickets.