Opened 2 years ago

Closed 20 months ago

Last modified 5 months ago

#6193 closed change (fixed)

Applying exceptions to advanced pseudo-selector rules

Reported by: amrmak Assignee: hfiguiere
Priority: P2 Milestone: Adblock-Plus-3.1-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: hfiguiere, mapx, sebastian, kzar, arthur, mjethani, sergz Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29636648/

Description (last modified by kzar)

Environment

Windows 10 10.0.16299 (64-bit)
Chrome 63.0.3239.84 (64-bit) (same with Firefox 57.0.2 64-bit)
Adblock Plus 1.13.4 (same with ABP 3.0.2)

How to reproduce

  1. Go to Adblock Plus options page, and go to "Add your own filters tab"
  2. In the first text box, add the filter planet.fr#@#:-abp-properties(cursor: pointer;)
  3. Click "Add filter"

Observed behaviour

An Adblock Plus alert is triggered with the message:
"Line 1: ':-abp-properties(cursor: pointer;)' is not a valid CSS selector"

Expected behaviour

No alerts, and filter is added to custom filters.

Change History (21)

comment:1 Changed 2 years ago by amrmak

  • Platform changed from Unknown / Cross platform to Chrome

comment:2 Changed 2 years ago by amrmak

  • Platform changed from Chrome to Unknown / Cross platform

comment:3 Changed 2 years ago by hfiguiere

  • Cc hfiguiere added; hubert@… removed

comment:4 Changed 2 years ago by hfiguiere

  • Component changed from Core to Platform

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

this 1 is accepted

planet.fr#@?#:-abp-properties(cursor:pointer;)
Last edited 2 years ago by mapx (previous) (diff)

comment:6 Changed 2 years ago by mapx

  • Cc mapx added

comment:7 Changed 2 years ago by hfiguiere

  • Cc sebastian kzar added

comment:8 in reply to: ↑ 5 ; follow-up: Changed 2 years ago by hfiguiere

Replying to mapx:

this 1 is accepted

planet.fr#@?#:-abp-properties(cursor:pointer;)

This looks like a bug in core. It is parsed as a blocking filter.

comment:9 Changed 2 years ago by hfiguiere

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

comment:10 Changed 2 years ago by mapx

Is it necessary having that ugly form #?# ?
Why not just ## as for the normal hiding filters ?

The emulation filters are recognized using :-abp

Last edited 2 years ago by mapx (previous) (diff)

comment:11 Changed 2 years ago by arthur

  • Cc arthur added

comment:12 in reply to: ↑ 8 ; follow-ups: Changed 2 years ago by kzar

  • Cc sergz mjethani added
  • Component changed from Platform to Core
  • Description modified (diff)
  • Type changed from defect to change

Since this issue was marked as a defect under the wrong module and the review gave no / little more context I spent some time figuring out what it was about. Please could you give some more context in the review and issue next time, so that someone who isn't up to speed can get the idea more quickly? Also changing filter syntax requires some coordination and discussion.

The #@# syntax seems to be for regular element hiding exceptions, as in the opposite of a ## filter. By that logic shouldn't exceptions for advanced pseudo-selector rules have the syntax #@?#? Otherwise like mapx suggestions why do we both with the special #?# syntax at all, if we're going to check the filter for the string ":-abp-" anyway?

Replying to hfiguiere:

Replying to mapx:

this 1 is accepted

planet.fr#@?#:-abp-properties(cursor:pointer;)

This looks like a bug in core. It is parsed as a blocking filter.

Have you filed an issue for that?

comment:13 in reply to: ↑ 12 Changed 2 years ago by hfiguiere

Replying to kzar:

Since this issue was marked as a defect under the wrong module [...]

It really is a defect in the addon, not core. The problem is in the filter validation when added from the UI. The patch in review is fixing just that.

The #@# syntax seems to be for regular element hiding exceptions, as in the opposite of a ## filter. By that logic shouldn't exceptions for advanced pseudo-selector rules have the syntax #@?#? Otherwise like mapx suggestions why do we both with the special #?# syntax at all, if we're going to check the filter for the string ":-abp-" anyway?

It was decided that the exceptions filter applied to both ElementHiding and ElementHidingEmulation, hence no different syntax.

comment:14 in reply to: ↑ 12 Changed 2 years ago by hfiguiere

Replying to kzar:

Replying to hfiguiere:

Replying to mapx:

this 1 is accepted

planet.fr#@?#:-abp-properties(cursor:pointer;)

This looks like a bug in core. It is parsed as a blocking filter.

Have you filed an issue for that?

Filed https://issues.adblockplus.org/ticket/6234 now.

comment:15 Changed 2 years ago by kzar

  • Cc sergz removed
  • Component changed from Core to Platform

It really is a defect in the addon, not core. The problem is in the filter validation when added from the UI. The patch in review is fixing just that.

OK sorry, I've changed the module back.

It was decided that the exceptions filter applied to both ElementHiding and ElementHidingEmulation, hence no different syntax.

Could you provide some more detail? (E.g. how it was discussed to come to that decision.)

comment:16 Changed 2 years ago by hfiguiere

It was not explicitely discussed. Wladimir patch for https://issues.adblockplus.org/ticket/5287 was deliberately using the same syntax for exception. I don't see the need for a different syntax either.

comment:17 Changed 2 years ago by kzar

  • Priority changed from Unknown to P2
  • Ready set

Ah yea, you're right. Fair enough, I'll set this as ready.

comment:18 Changed 22 months ago by sergz

  • Cc sergz added

comment:19 Changed 20 months ago by abpbot

A commit referencing this issue has landed:
Issue 6193 - Allow element hiding emulation exceptions

comment:20 Changed 20 months ago by hfiguiere

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

comment:21 Changed 19 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Fixed. Element hiding emulation exceptions can be added and are functional.

ABP 3.0.4.2042
Firefox 59 / 55 / 51 / Windows 10
Chrome 66 / 58 / 49 / Windows 7
Opera 52 / 45 / 36 / Windows 10

Note: See TracTickets for help on using tickets.