Opened 21 months ago

Closed 16 months ago

Last modified 6 weeks 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 21 months ago by amrmak

  • Platform changed from Unknown / Cross platform to Chrome

comment:2 Changed 21 months ago by amrmak

  • Platform changed from Chrome to Unknown / Cross platform

comment:3 Changed 21 months ago by hfiguiere

  • Cc hfiguiere added; hubert@… removed

comment:4 Changed 21 months ago by hfiguiere

  • Component changed from Core to Platform

comment:5 follow-up: Changed 21 months ago by mapx

this 1 is accepted

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

comment:6 Changed 21 months ago by mapx

  • Cc mapx added

comment:7 Changed 21 months ago by hfiguiere

  • Cc sebastian kzar added

comment:8 in reply to: ↑ 5 ; follow-up: Changed 21 months 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 21 months ago by hfiguiere

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

comment:10 Changed 21 months 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 21 months ago by mapx (previous) (diff)

comment:11 Changed 20 months ago by arthur

  • Cc arthur added

comment:12 in reply to: ↑ 8 ; follow-ups: Changed 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 18 months ago by sergz

  • Cc sergz added

comment:19 Changed 16 months ago by abpbot

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

comment:20 Changed 16 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 16 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.