Opened 5 years ago

Closed 4 years ago

#1282 closed defect (fixed)

"Block element" creates filter for whitelisted elements

Reported by: passbrains Assignee: sebastian
Priority: P3 Milestone: Adblock-Plus-1.8.12-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: sergz, sebastian, mapx, philll, greiner, kzar, trev Blocked By: #1853
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/5225119261655040

Description (last modified by sebastian)

Adapted from https://www.passbrains.com/dashboard/view-ticket.php?ticket_no=AOP-18

Environment

Chrome/Opera/Safari

How to reproduce

  1. Go to https://en.wikipedia.org/wiki/Portable_Network_Graphics
  2. Click the ABP icon and choose "Block Element"
  3. Click on the example image in the right column
  4. Save the filter
  5. Reload the page

Observed behaviour

A request blocking filter for following URL is added:

https://upload.wikimedia.org/wikipedia/commons/thumb/9/9a/PNG_transparency_demonstration_2.png/280px-PNG_transparency_demonstration_2.png

However the image isn't blocked because following exception rule in EasyList:

@@||upload.wikimedia.org/wikipedia/

That is because exception rules always override blocking rules.

Expected behaviour

No filter which won't have any effect because an exception rule should be suggested.

Attachments (1)

4733_1409161082_AdblockPlus_001.png (355.8 KB) - added by passbrains 5 years ago.

Download all attachments as: .zip

Change History (19)

Changed 5 years ago by passbrains

comment:1 Changed 5 years ago by sebastian

  • Cc sebastian added
  • Component changed from Unknown to Platform
  • Description modified (diff)
  • Platform changed from Opera to Chrome
  • Priority changed from Unknown to P3
  • Ready set
  • Summary changed from Block element feature is not fully functional for .PNG elements. to "Block element" creates filter for whitelisted elements

comment:2 Changed 5 years ago by sebastian

  • Description modified (diff)

comment:3 Changed 5 years ago by mapx

  • Cc mapx added

comment:4 follow-up: Changed 5 years ago by sebastian

  • Cc philll greiner kzar trev added
  • Ready unset

Removing ready for now, since we don't seem to agree, how to address this issue.

So far it seems that the majority is in the favor to let user defined filters override whitelisting filters.

comment:5 Changed 5 years ago by sergz

  • Cc sergz added

comment:6 in reply to: ↑ 4 Changed 5 years ago by greiner

Replying to sebastian:

So far it seems that the majority is in the favor to let user defined filters override whitelisting filters.

Yes, that would be the best user experience IMHO. However, not sure yet how well this can be integrated with the existing filter matching mechanism since we currently don't take a filter's origin (i.e. in which filterlist(s) it was defined) into consideration.

comment:7 follow-up: Changed 5 years ago by sebastian

Letting the user override exception rules from filter lists would certainly be nice. But doesn't seem to be trivial to implement, and might also impair the performance.

Alternatively, we could extend the filter validation (#491) to prevent users from adding filters that are overridden by exception rules. The downside with that approach is that exception rules added to filter lists can still conflict with existing custom filters without noticing. But this case can probably be neglected.

For reference, this issue has been reported 5 times within the past 2 weeks via passbrains.

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

comment:8 in reply to: ↑ 7 Changed 5 years ago by greiner

Replying to sebastian:

Alternatively, we could extend the filter validation (#491) to prevent users from adding filters that are overridden by exception rules.

This would only work for element hiding filters since the exception rule for a hiding filter is basically the same filter with just minor differences. For blocking filters, however, we require a context to determine which filters match and since a user can freely edit the filters we cannot rely on the context the suggested filters were initially based on.

Therefore I'd suggest not to suggest filters in the first place which we know would have no effect because of an existing exception rule.

comment:9 Changed 5 years ago by sebastian

Sure, the "Block element" dialog shouldn't suggest any filters which won't have any affect, as described in the issue description. But I guess we should address it in the filter validation as well, otherwise we will have the same issue when users add custom filters.

comment:10 Changed 5 years ago by philll

If we are really serious about letting the user have control, we should imo rather go through a complex process of implementing the feature to have overwrites.

comment:11 Changed 5 years ago by sebastian

I just hacked together a proof of concept. Apparently it was easier than expected, and also simpler than the alternatives, to make user-defined filters override filter lists. However, I'd like to know what trev thinks about that patch. Whether this change is reasonable, given that additional code has to run for every filter match, and might have an effect on the performance.

comment:12 Changed 5 years ago by trev

Mind you, the solution for Firefox was a much more pragmatic one: you cannot create filters for whitelisted elements. And you aren't really solving the reported issue here because the next issue will say "Block element creates filter for user-whitelisted elements". I'm not saying that we shouldn't let user-defined filters have higher priority, it's merely an entirely different issue - not even the same component.

comment:13 Changed 5 years ago by sebastian

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

You are right, letting users override filter lists with custom filters, is in fact a separate issue. And letting the user do so, won't fix this issue in scenarios where a user-defined exception rule is whitelisting the element. So lets just not generate filters conflicting with exception rules.

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

comment:14 Changed 5 years ago by sebastian

  • Owner set to sebastian

comment:15 Changed 5 years ago by sebastian

  • Description modified (diff)

comment:16 Changed 4 years ago by sebastian

  • Blocked By 1853 added

comment:17 Changed 4 years ago by greiner

#1936 is also related to the discussion about prioritizing user filters. I couldn't find a canonical issue about it, though, so if you created one in the past please provide a link to it here and mark #1936 as duplicate.

comment:18 Changed 4 years ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Safari-next
  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.