#3969 closed defect (fixed)

Hits are not counted for custom CSS property rules

Reported by: Ross Assignee: trev
Priority: P3 Milestone: Adblock-Plus-2.8-for-Firefox
Module: Adblock-Plus-for-Firefox Keywords:
Cc: sebastian Blocked By: #3999, #4000, #4138
Blocking: Platform: Firefox
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29345639/

Description (last modified by trev)

Environment

ABP 2.7.2.4166
Firefox 45 / Windows 8

How to reproduce

  1. Install ABP dev build.
  2. Add the custom filter: testpages.adblockplus.org##.abp-logo
  3. Navigate to https://testpages.adblockplus.org
  4. Observe logo is hidden (expected). Observe hit is incremented against rule (expected).
  5. Disable the .abp-logo filter added in Step 2.
  6. Add the custom filter: testpages.adblockplus.org##[-abp-properties='width: 64px;']
  7. Refresh the https://testpages.adblockplus.org tab.
  8. Observe the logo is hidden (expected). Observe the hit counter for the rule stays at 0 (unexpected).

Observed behaviour

Hits are not counted against CSS property rules.

Expected behaviour

Hits to be counted as with other rules.

Background

As CSS property filters were initially implemented for Chrome, there is no hit counting functionality.

What to change

Update dependency on adblockpluscore to revision f7f491edb1b7 (imports #4000). Update dependency on adblockplusui to revision f0daa0b325ed (imports #3999). Make sure to consider the second parameter in the addSelectorFunc callback - increase hit counts for these filters (unless it's a private browsing window) and add these hits to the list of blockable items.

Hints for testers

The adblockplusui dependency update imports a whole lot of additional changes, mostly related to the new options page. None of these should be relevant for the Firefox build. However, retesting the functionality of the first-run page would be a good idea.

Change History (11)

comment:1 Changed 18 months ago by trev

  • Component changed from Adblock-Plus-for-Firefox to Core
  • Description modified (diff)
  • Priority changed from Unknown to P3
  • Ready set

comment:2 Changed 18 months ago by trev

  • Component changed from Core to Adblock-Plus-for-Firefox

This will require changes to both Core and UI code in addition to Firefox, so let's turn this into a Firefox bug and file the other ones as blockers.

comment:3 Changed 18 months ago by trev

  • Blocked By 3999 added

comment:4 Changed 18 months ago by trev

  • Blocked By 4000 added

comment:5 Changed 18 months ago by trev

  • Description modified (diff)
  • Owner set to trev

comment:6 Changed 17 months ago by trev

  • Blocked By 4138 added

comment:7 Changed 17 months ago by trev

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

comment:8 Changed 17 months ago by abpbot

A commit referencing this issue has landed:
Issue 3969 - Hits are not counted for custom CSS property rules

comment:9 Changed 17 months ago by trev

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

comment:10 Changed 14 months ago by sebastian

  • Cc sebastian added
  • Resolution fixed deleted
  • Status changed from closed to reopened

If you now use a CSS property filter that is translated into a CSS selector which doesn't match anything (e.g. testpages.adblockplus.org##foo [-abp-properties='width: 64px;']) it is still counted as a filter hit and shows up as "hidden" in blockable items.

Not sure if there is any other way to fix that for Firefox, but also considering the devtools panel on Chrome, I think we'd have to make the addSelectorsFunc take a mapping of selectors to filters.

comment:11 Changed 12 months ago by trev

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

What you are proposing requires significant changes to Core. While it does make sense, we should definitely deal with it in a separate issue. I created #4515, the issue here is resolved.

Note: See TracTickets for help on using tickets.