Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support new syntax Adblock Plus #1752

Closed
dimisa-RUAdList opened this issue Jun 24, 2016 · 8 comments
Closed

Support new syntax Adblock Plus #1752

dimisa-RUAdList opened this issue Jun 24, 2016 · 8 comments
Labels

Comments

@dimisa-RUAdList
Copy link

In the subscription RuAdList we started to use a New CSS property filter syntax. uBO will support this syntax?

https://hg.adblockplus.org/ruadlist/rev/8f672cd4f007#l1.12
https://adblockplus.org/development-builds/new-css-property-filter-syntax

@gorhill
Copy link
Owner

gorhill commented Jun 24, 2016

Duplicate of #139.

I believe the rationale for these filters was to take care of ads such as the AdDefend ones, seen on wetter.com for example. So far, uBO's ability to inject scriptlets has done a better job in my opinion, as this prevents the javascript code injecting these ads from even running: scriptlets can fix the source of the problem, rather than the symptoms.

What is the exact issue at http://go.mail.ru/search?fr=main&q=%D1%88%D0%B0%D0%BF%D0%BA%D0%B0? When I go to this URL, I do not see any ads with uBO + default filters (no RU AdList).

@dimisa-RUAdList
Copy link
Author

Perhaps the advertising is not visible because of the geolocation. Details can be found here: http://rgho.st/7QwgZSZtm

Separately to block scripts without breaking functionality here will not work, we know how to do it, but here it's impossible. The main question - whether the uBO in the future to support the new syntax ABP or not? At the moment we declare that the RU AdList supports this extension, so the answer is very important.

@TheFinalCut83
Copy link

dimisa-RUAdList is right. With russian IP I do see ads from http://go.mail.ru/search?fr=main&q=%D1%88%D0%B0%D0%BF%D0%BA%D0%B0 in Cyberfox 47.0.1 / Firefox 48b3 with uBO 1.7.6,
Filters:
uBlock filters
uBlock filters – Badware risks‎
uBlock filters – Privacy‎
uBlock filters – Unbreak‎
Adblock Warning Removal List
Anti-Adblock Killer | Reek
EasyList
EasyPrivacy‎
Malvertising filter list by Disconnect‎
RUS: Adguard Russian Filter
RUS: BitBlock List
RUS: RU AdList
ubo up
ubo down

With ABP 2.7.3 ads isn't shown
abp up
abp down

@gorhill
Copy link
Owner

gorhill commented Jun 25, 2016

@TheFinalCut83 Yes, I can see this without a VPN.

I'm evaluating whether to support -abp-properties, and/or other solutions for this, such as :has() filters (for which I wrote prototype code a long while ago).

I do worry about the overhead introduced by -abp-properties, as it requires to iterate through all CSS rules on a page (411 CSS rules in the current case), and for each to match against a regex, and all this to reverse lookup the CSS selector(s) to use for filtering. I also observe that the rules are not available for when the stylesheet is from another origin.

I lean more toward an implementation of :has filter, which implementation will use native calls such as querySelectorAll to only iterate through the nodes which are matches -- i.e. go.mail.ru##.responses__wrapper > div > div:has([href^="http://an.yandex.ru/"]) in the current case.

In the current case, querySelectorAll(".responses__wrapper > div > div") returns only 3 nodes, so we would have to further iterate through these 3 nodes only (again using native call querySelector()) to confirm whether the node has to be hidden, instead of iterating and regex-matching through 411 CSS rules. Once a target node is a match, uBO could derive a CSS rule from it. Also, with this approach uBO can apply the display: none property directly to the matching nodes, rather than rely only on a style, which is often overriden by web pages using inline styling.

@dimisa-RUAdList
Copy link
Author

dimisa-RUAdList commented Jun 25, 2016

Until then, until the decision is made to support the new syntax, I specify the extension as uBO partially supported RU AdList Filters: https://forums.lanik.us/viewtopic.php?f=102&t=22512&p=69843

I hope this is temporary.

@gorhill
Copy link
Owner

gorhill commented Jun 26, 2016

I hope this is temporary.

I will decline.

I have a working prototype for an implementation of :has() filters, and for the cases I have where it's really needed[1], the amount of overhead is significantly lower than implementing -abp-properties, there is just no comparison. For example, querying the DOM with .responses__wrapper > div > div returns only two nodes for the case here, to compare to the 400+ plus CSS rules which are visited by ABP, where for each one there are test against regexp(s), array creation, and string concatenation involved.

Given that I consider efficiency a primary of uBO, supporting -abp-properties does not fit in here.

Another advantage: once (and if) the :has() selector is implemented by the various browsers, the existing filters will work natively.

I may revisit my decision in the future if there are many cases where it's advantageous to support -abp-properties versus other solutions, but currently I don't see this.

[1] opensubtitles.org##div[itemscope] > fieldset:has([href="/support#vip"]), yandex.ru##.serp-item:has(.label_color_yellow), go.mail.ru##.responses__wrapper > div > div:has(a[href^="http://an.yandex.ru/"]) (note that ABP's -abp-properties can't take care of the yandex.ru case)

@gorhill
Copy link
Owner

gorhill commented Jun 30, 2016

The new cosmetic filter operators seem to work reasonably well to me. But as of now they are specific to uBO. If I made it possible for filter list maintainers to use them in a way that do not make a filter list incompatible with other blockers, would you use the new operators?

For example, a well-thought side of the new -abp-properties is that these cosmetic filters do not break other blockers using EasyList (though uBO discards them at compile time to avoid the injection of selectors which accomplish nothing on their own). If I was to use a similar approach for the new :has/:xpath/:style operators, would filter list maintainers be willing to use these even though they won't be immediately enforced by ABP or other blockers? If their use shows that they indeed are helpful to filter list maintainers, this might encourage adoption by other blockers.

For example:

example.com##[-ubo-xpath='...']
example.com###eleId > div[-ubo-has='...']
example.com##.video[-ubo-style='...']

@gorhill
Copy link
Owner

gorhill commented Aug 2, 2016

Google Translate of "pikabu.ru - EasyList Forum":

In any case, on my proposal to support the new syntax ATS uBO developer refused ... Ignore if the syntax used by editors - it's something amazing ...

I explained why I refused to support that syntax: it's quite inefficient. Here is a benchmark, which consisted in re-loading the page 5 times, with cache enabled (EasyList + RUAdlist + EasyPrivacy):

a

Now keep in mind this page is atypically simple, so dealing with the -abp-properties syntax is a best case scenario with this page, and yet it requires more processing power than dealing with the new cosmetic filter options in uBO 1.8.0. On more complex pages, like focus.de things get worse quickly efficiency-wise (and just as in the current case, ads were not hidden on focus.de), because it requires visiting all CSS rules in a document.

The code in ABP that specifically deals with -abp-properties is more demanding on the CPU than the most expensive entry in uBO, 66 ms vs. 48 ms.

Also note the (program) timing in the benchmark above: aside its more expensive content script code, ABP will also cause the browser to work harder: ABP = 1347 ms, uBO = 745 ms.

What is in the best interests of users is to promote solutions which are efficient and actually work. I proposed above solutions which would address the issue here efficiently, without breaking ABP, but you never answered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants