Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#616 closed change (fixed)

Add $generichide filter option that can be used to disable all generic element hiding filters for a website

Reported by: Crits Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-for-1.9.4-Chrome-Opera-Safari
Module: Core Keywords: 2015q3
Cc: famlam, mapx, arthur, trev, manvel, greiner, sergz, ben Blocked By: #2738
Blocking: Platform: Unknown
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/5840485868371968/
https://codereview.adblockplus.org/5991150368325632/
https://codereview.adblockplus.org/5138680696012800/
https://codereview.adblockplus.org/6439460933730304/
https://codereview.adblockplus.org/29325987/

Description (last modified by kzar)

Background

We need a way to disable all generic element hiding rules for a given website. At the moment we can only disable each generic filter individually which is time consuming and verbose!

What to change

Add the new $generichide filter option to the lib/filter... code in the adblockplus repository.

It should act similarly to the existing $elemhide option but instead of matching all element hiding rules it should only match the generic ones.

When a $generichide filter is applied to a page it should also recursively apply to all child frames.

Generic filter rules count as rules that:

  • Do not have a domain specified. "Hide this element on all domains"
  • Have only domain exceptions specified. "Hide this element on all domains except example.com"

Example generic filters:

  • ~example.com##.ad

We also need to update the adblockplus dependency for the adblockpluschrome repository from bdd5cd8bca3a to 893426c6a6ab. The following unrelated changes to adblockplus have happened since then:

  • The first simply fixes a mistake in an Indonesian translation, this should not matter to us.
  • The second is simply adding a mapping for an ABB related graphic, again not relevant to us.
  • Finally the last five focus on releasing version 2.6.11 and should not cause us any problems either.

Finally we need to update the adblockplustests dependency for the adblockpluschrome repository from ae6c7a2cc723 to 449058480108. The following unrelated changes to adblockplustests have happened since then:

  • The first, second and fifth changes were to tests that aren't included by the Chrome extension test suite. (synchroniser, icon_position and policy respectively)
  • The third change was to the metadata.gecko file which again is not used by the Chrome tests.
  • The fourth change was to fix a problem with cleanup after some of the asynchronous unit tests. I don't think the issue showed up in any of the Chrome tests, but if anything adding this change should make things run more reliably.

I have also tested that these changes and no additional tests are failing as a result. (9 CSS escaping tests are currently failing but that is unrelated)

Change History (49)

comment:1 Changed 5 years ago by mapx

  • Cc mapx added

comment:2 follow-up: Changed 5 years ago by arthur

  • Cc arthur added
  • Priority changed from Unknown to P3
  • Ready set

Thanks for creating this issue. I will also create one for generic blocking rules as well.

comment:3 in reply to: ↑ 2 Changed 5 years ago by Crits

Replying to arthur:

Thanks for creating this issue. I will also create one for generic blocking rules as well.

That would be sooooo great!

comment:4 Changed 5 years ago by arthur

  • Priority changed from P3 to P2

comment:5 Changed 5 years ago by Crits

  • Component changed from Unknown to Core

comment:6 Changed 5 years ago by trev

  • Cc trev added
  • Platform set to Unknown

comment:7 Changed 5 years ago by saroyanm

  • Cc manvel@… added

comment:8 follow-up: Changed 5 years ago by greiner

Rather than introducing a new option, what about reusing elemhide for that? In such a case <domain>$elemhide would apply to all element hiding filters while <domain>$elemhide=~<domain> would only apply to filters that are not specific to that domain. Would that be sufficient?

However, there might be backwards-compatibility problems so <domain>$elemhide=~<domain> would probably be equal to <domain>$elemhide in older versions that don't support this syntax. Although implementing $generichide would probably have similar issues in this regard.

@trev: Regarding the technical feasibility of this feature, I wonder whether this would be blocked on #521.

comment:9 Changed 5 years ago by trev

Although implementing $generichide would probably have similar issues in this regard.

Currently, the typical approach is writing rules like foo$generichide,image,~image to avoid this. For older versions (ones that don't know generichide) this is a rule not matching any types. For newer versions it is equivalent to foo$generichide.

@trev: Regarding the technical feasibility of this feature, I wonder whether this would be blocked on #521.

No, #521 doesn't affect this any more than it affects usual element hiding exceptions for example.

comment:10 in reply to: ↑ 8 Changed 5 years ago by famlam

Rather than introducing a new option, what about reusing elemhide for that? In such a case <domain>$elemhide would apply to all element hiding filters while <domain>$elemhide=~<domain> would only apply to filters that are not specific to that domain. Would that be sufficient?

Although I would prefer the re-use of options, I'd rather suggest the use of boolean options. $elemhide=... would give a lot of possibilities that all have to be taken care of, e.g. <domain>$elemhide=~<domain2> or <domain>$elemhide=<domain> and all kind of combinations such as <domain.com>$elemhide=~sub.domain.com|domain.com|~com if we want to exclude certain rules only. Besides, it's slightly confusing as it has a completely different interpretation than the similar-syntax $domain= option.

I can think of a couple of those boolean options. The simplest is ~elemhide, which only purpose currently could be that ~elemhide is shorthand for ~image,image (e.g., match all resource types), although I cannot recall any occurrence of that so far.
Another possibility is ~third-party: adding this would get rid of all domain-specific (=first party) rules, which would be more easily to understand, even though the meaning is slightly different than ~third-party for regular rules (as hiding rules are of course by definition first-party, except for in subframes)
Another option is to completely replace the present elemhide with the newly suggested elemhide. The need for the current behavior is rare: you don't want to whitelist specific rules if you can as well remove those (or, in case of an external filter list that doesn't want to cooperate, you could eventually whitelist them with #@# rules). Requiring the current elemhide behavior on third-party subframes is even more rare, I cannot think of a realistic use case for that. Furthermore, elemhide is only known to list authors and the few users who have read the time to learn the syntax. It's not an option that can be added from within the extension (e.g. EHH or the block-a-resource dialog).

My first and second suggestion can also be ported to blocking rules (e.g. ~document, issue #647). The third suggestion would be specific to hiding rules.

However, there might be backwards-compatibility problems so <domain>$elemhide=~<domain> would probably be equal to <domain>$elemhide in older versions that don't support this syntax. Although implementing $generichide would probably have similar issues in this regard.

Wasn't this dealt with in a way that filters with unknown options were completely discarded, such that we don't even need the $image,~image syntax anymore? At least in EL we do no longer use $image,~image for $popup rules...

comment:11 Changed 5 years ago by trev

  • Priority changed from P2 to P3

comment:12 Changed 5 years ago by fhd

  • Keywords 2014q4 added
  • Priority changed from P3 to P2

comment:13 Changed 5 years ago by greiner

  • Cc greiner added

comment:14 follow-up: Changed 5 years ago by greiner

Replying to famlam:

Besides, it's slightly confusing as it has a completely different interpretation than the similar-syntax $domain= option.

Isn't that the general problem with the $elemhide filter option? While other options are specifying when a filter should be applied, $elemhide specifies what should be applied. On top of that it defines element hiding behavior within a blocking filter. Adding another element hiding-specific option to blocking filters would make it even more confusing than it already is.

Therefore I'm leaning towards either extending $elemhide in some way or introducing filter options to element hiding filters. While I could also imagine a completely new type of filter for domain-specific options (e.g. $document, $elemhide) rather than resource-specific ones (e.g. $image, $collapse), it appears out of scope for this issue.

My first and second suggestion can also be ported to blocking rules (e.g. ~document, issue #647).

While this might be the way of least resistance in regard to implementation, I can imagine this to make it even more difficult to comprehend what the keyword elemhide actually means (e.g. @@||example.com$elemhide disables all element hiding filters; @@||example.com$~elemhide disables only generic element hiding filters). The same issue applies to $~document.

comment:15 Changed 5 years ago by famlam

And what about completely replacing the current $elemhide behavior? As explained in my previous post, I don't think there is much reason to have the present behavior over the suggested behavior. (And it would hardly, if any at all, cause compatibility issues with currently existing filters). Furthermore, you would prevent that all filter lists have to update their filters to the new variant, as well as keeping an old rule for older ABP versions (until most people have updated).

But if you really want to extend $elemhide with options ($elemhide=X), I wouldn't choose the domain as it is rare that $elemhide occurs in a filter not containing a domain already (e.g. @@/ads/*$elemhide would always be way too generic, compared to @@||DOMAIN/ads/$elemhide or @@/ads/*$elemhide,domain=DOMAIN, and @@||site.com^$elemhide=~anyOtherSite.com would be meaningless). That way, you would only add duplicate information to a filter. Better (IMO) might in that case be something like $elemhide=generic (or some other fixed-value-option). I guess code-technically it would also be beneficial for speed, as you have to match the full filter anyway, but have no additional comparisons for the domain of $elemhide.

comment:16 in reply to: ↑ 14 ; follow-up: Changed 5 years ago by trev

Replying to greiner:

Isn't that the general problem with the $elemhide filter option? While other options are specifying when a filter should be applied, $elemhide specifies what should be applied.

We actually have both kinds of options. We have those making the filter more specific (type options, $domain and similar) and those activating non-default behavior ($document, $collapse).

On top of that it defines element hiding behavior within a blocking filter.

Since it applies to URLs rather than selectors, it actually makes sense.

Therefore I'm leaning towards either extending $elemhide in some way

That would overload the meaning here and make things extremely confusing.

or introducing filter options to element hiding filters.

That would make no sense whatsoever - as I said already, this isn't about selectors.

Actually, both this and #647 could be implemented by means of a new $generic option: @@||example.com^$generic (disable generic filters of any kind on example.com domain).

Replying to famlam:

And what about completely replacing the current $elemhide behavior? As explained in my previous post, I don't think there is much reason to have the present behavior over the suggested behavior. (And it would hardly, if any at all, cause compatibility issues with currently existing filters).

This is indeed an option. It has the advantage that we wouldn't need to support three different whitelisting schemes for element hiding rules ($document, $elemhide and $generic), and conventional filter lists indeed rarely intend to override element hiding rules that have been targeted specifically at a particular domain. The only place where this would become problematic is the Acceptable Ads list, it would have to use specific exceptions for some rules (not sure how many) if we changed $elemhide behavior. My impression is that this would make maintaining that list harder.

comment:17 follow-up: Changed 5 years ago by famlam

Actually, both this and #647 could be implemented by means of a new $generic option: >@@||example.com^$generic (disable generic filters of any kind on example.com domain).

The problem I see with $generic is that it would also more-or-less kill EasyPrivacy and other non-ads-lists on websites where basically only generic element hiding rules have to be disabled.
Many cases where we have to add $elemhide rules due to Anti-Adblock measurements, we only need a few blocking whitelists, and often enough those blocking whitelists are for resources of which the server is blocked (e.g., rules starting with ||, which according to the discussion-in-which-I-do-not-participate in #647 would be non-generic). It's very rare to find Anti-Anti-Tracking or Anti-Anti-Social measurements on websites though, so there is no need to whitelist them altogether.

The only place where this would become problematic is the Acceptable Ads list, it would have to use specific exceptions for some rules (not sure how many) if we changed $elemhide behavior.

Just to sketch the impact: according to my redundancy checker there are ~125 specific hiding rules (in EL+ELD) being made redundant by $elemhide rules in the Acceptable Ads list, which means ~125 #@# rules have to be added to the AA list (+ ones that are triggered by specific rules in other regional lists) if this method is chosen. (Furthermore, off topic, it would allow using custom hiding rules on those AA domains, which is currently impossible).

Last edited 5 years ago by famlam (previous) (diff)

comment:18 in reply to: ↑ 16 Changed 5 years ago by famlam

[sorry for posting twice, I cannot update my previous post]

Replying to trev:

The only place where this would become problematic is the Acceptable Ads list, it would have to use specific exceptions for some rules (not sure how many) if we changed $elemhide behavior.

Just to sketch the impact: according to my redundancy checker there are ~125 specific hiding rules (in EL+ELD) being made redundant by $elemhide rules in the Acceptable Ads list, which means ~125 #@# rules have to be added to the AA list (+ ones that are triggered by specific rules in other regional lists) if this method is chosen. (Furthermore, off topic, it would allow using custom hiding rules on those AA domains, which is currently impossible).

Version 0, edited 5 years ago by famlam (next)

comment:19 in reply to: ↑ 17 Changed 5 years ago by trev

Replying to famlam:

Just to sketch the impact: according to my redundancy checker there are ~125 specific hiding rules (in EL+ELD) being made redundant by $elemhide rules in the Acceptable Ads list, which means ~125 #@# rules have to be added to the AA list (+ ones that are triggered by specific rules in other regional lists) if this method is chosen.

The main problem here isn't the one-time effort of adding these rules, it's keeping them in sync with EasyList & Co. that's problematic.

comment:20 Changed 5 years ago by sergz

  • Cc sergz added

comment:21 follow-up: Changed 5 years ago by Lain_13

Hi, I'm not sure it have to be a part of this issue, so feel free to create a new one but I think elemhide have to disable collecting hit statistics in FireFox for specific site since it reveals presence of active AdBlock through property -moz-binding (example here: iphones.ru search for ".getPropertyValue('-moz-binding').indexOf('about:')" in the page code). Maybe it have to be a separate option but it will be a fix specific only for Firefox then.

comment:22 in reply to: ↑ 21 Changed 5 years ago by greiner

Replying to Lain_13:

Hi, I'm not sure it have to be a part of this issue, so feel free to create a new one but I think elemhide have to disable collecting hit statistics in FireFox for specific site since it reveals presence of active AdBlock through property -moz-binding (example here: iphones.ru search for ".getPropertyValue('-moz-binding').indexOf('about:')" in the page code). Maybe it have to be a separate option but it will be a fix specific only for Firefox then.

Sorry but I couldn't find that in the page code. Please create a new issue if you can provide more details about it. Generally, example code and pages would be quite helpful.

comment:23 Changed 5 years ago by kzar

  • Owner set to kzar

comment:24 Changed 5 years ago by kzar

I'm not clear how this ticket differs from #647? (Maybe one of them should be closed?)

Also there are lots of different approaches suggested in the comments of this ticket, could you check the description is accurate and maybe update it @palant / @greiner? Just so it's clear what exactly we're doing before I start. So far there are a couple of quotes in the description, describing things that would be nice but nothing concrete that says exactly what we're going to do.

comment:25 Changed 5 years ago by kzar

  • Ready unset

comment:26 Changed 5 years ago by arthur

@kzar
#647 is for blocking rules, this one is for hiding rules.

comment:27 Changed 5 years ago by greiner

  • Description modified (diff)

I reflected the common consens in the issue description.

comment:28 Changed 5 years ago by kzar

  • Description modified (diff)
  • Summary changed from Disable generic hiding rules only + lessen the effect of $elemhide to Add $generichide filter option that can be used to disable all generic filters for a website

comment:29 Changed 5 years ago by kzar

  • Description modified (diff)
  • Summary changed from Add $generichide filter option that can be used to disable all generic filters for a website to Add $generichide filter option that can be used to disable all generic element hiding filters for a website

comment:30 Changed 5 years ago by kzar

  • Ready set

comment:31 Changed 5 years ago by kzar

  • Description modified (diff)

comment:32 Changed 5 years ago by kzar

  • Description modified (diff)

comment:33 Changed 5 years ago by kzar

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

comment:34 Changed 5 years ago by kzar

  • Review URL(s) modified (diff)

comment:35 Changed 5 years ago by kzar

  • Review URL(s) modified (diff)

comment:36 Changed 5 years ago by kzar

Quick update: I've implemented $generichide + $genericblock in Firefox + Chrome and added some tests. All changes are now just awaiting review.

comment:37 Changed 4 years ago by trev

  • Blocked By 2738 added

comment:38 Changed 4 years ago by fhd

  • Keywords 2015q3 added; 2014q4 removed
  • Tester set to Unknown

comment:39 Changed 4 years ago by arthur

  • Cc manvel ben added; manvel@… removed

comment:40 Changed 4 years ago by kzar

  • Review URL(s) modified (diff)

comment:43 Changed 4 years ago by kzar

  • Blocked By 3080 added

comment:44 Changed 4 years ago by kzar

  • Blocked By 3080 removed

comment:45 Changed 4 years ago by kzar

  • Description modified (diff)

comment:47 Changed 4 years ago by kzar

  • Description modified (diff)

comment:48 Changed 4 years ago by kzar

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

comment:49 Changed 4 years ago by kzar

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Safari-next
Note: See TracTickets for help on using tickets.