Opened on 08/20/2018 at 01:16:22 PM
Closed on 08/30/2018 at 03:57:53 PM
Last modified on 03/28/2019 at 09:53:13 AM
#6870 closed change (fixed)
Remove support for legacy :-abp-properties() syntax
| Reported by: | mjethani | Assignee: | jsonesen |
|---|---|---|---|
| Priority: | P2 | Milestone: | |
| Module: | Core | Keywords: | |
| Cc: | sergz, kzar, arthur, sebastian, jsonesen, hfiguiere | Blocked By: | |
| Blocking: | #6888 | Platform: | Unknown / Cross platform |
| Ready: | yes | Confidential: | no |
| Tester: | Ross | Verified working: | yes |
| Review URL(s): | |||
Description (last modified by mjethani)
Background
It has been more than a year since the legacy syntax for :-abp-properties() was deprecated. According to our internal Hub ticket no. 7571, none of the filter lists should be using this syntax any more. The additional parsing of the legacy syntax is responsible for the slowing down of the loading of the initial subscriptions (partially addressed by patch #29855590).
ABP should now stop supporting the legacy syntax.
What to change
Remove the additional parsing of the :-abp-properties() legacy syntax from Filter.fromText in lib/filterClasses.js and update any tests accordingly.
Hints for testers
See the :-abp-properties() section at https://adblockplus.org/development-builds/new-syntax-for-advanced-element-hiding-rules
The filter ##[-abp-properties="background-color: rgb(0, 0, 0)"] should now be a valid filter (even though it doesn't have an active domain) and should hide the element <div -abp-properties="background-color: rgb(0, 0, 0)">Ad</div> on any page.
Integration notes
After this is released, the filters documentation should be updated to say: "Note: The older syntax for the CSS property filters is no longer supported."
Attachments (0)
Change History (15)
comment:1 Changed on 08/20/2018 at 01:16:46 PM by mjethani
- Cc jsonesen added
comment:3 Changed on 08/20/2018 at 01:17:55 PM by mjethani
- Cc hfiguiere added
comment:4 in reply to: ↑ 2 Changed on 08/22/2018 at 09:24:13 AM by arthur
Replying to mjethani:
Arthur, is it OK to remove support for the legacy syntax now?
I think so but Sergei wanted to check that still as far as I know.
comment:5 Changed on 08/24/2018 at 10:37:50 AM by mjethani
I ran this script in adblockpluscore:
#!/bin/bash for url in $(sed -nE 's/.*url="(.*)".*/\1/p' chrome/content/ui/subscriptions.xml) do curl -sS "$url" | grep '\[-abp-' done
And I got nothing. It seems none of the lists we include by default are using the old syntax.
comment:6 Changed on 08/24/2018 at 10:38:37 AM by mjethani
- Priority changed from P4 to P2
- Ready set
comment:7 Changed on 08/24/2018 at 08:15:33 PM by jsonesen
Hey guys, I will take this task thanks for filing!
comment:8 Changed on 08/24/2018 at 08:15:45 PM by jsonesen
- Owner set to jsonesen
comment:9 Changed on 08/27/2018 at 03:56:03 PM by jsonesen
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:10 Changed on 08/27/2018 at 04:55:36 PM by hfiguiere
- Blocking 6888 added
comment:11 Changed on 08/27/2018 at 11:55:53 PM by abpbot
A commit referencing this issue has landed:
Issue 6870 - Remove support for legacy :-abp-properties() syntax
comment:12 Changed on 08/28/2018 at 12:19:35 AM by mjethani
- Description modified (diff)
comment:13 Changed on 08/30/2018 at 03:57:53 PM by jsonesen
- Resolution set to fixed
- Status changed from reviewing to closed
comment:14 Changed on 09/16/2018 at 11:57:58 AM by mjethani
- Description modified (diff)
comment:15 Changed on 10/24/2018 at 01:24:47 PM by Ross
- Tester changed from Unknown to Ross
- Verified working set
Done. Working as described.
ABP 3.3.2.2175
Firefox 62 / 51 / Windows 10
Chrome 69 / 49 / Windows 10
Opera 56 / 36 / Windows 10
Arthur, is it OK to remove support for the legacy syntax now?