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 for uBO media queries #70

Open
scripthunter7 opened this issue Dec 17, 2022 · 7 comments
Open

Support for uBO media queries #70

scripthunter7 opened this issue Dec 17, 2022 · 7 comments
Assignees
Labels

Comments

@scripthunter7
Copy link
Member

scripthunter7 commented Dec 17, 2022

uBO now supports media queries. The tsurlfilter's converter does not support it yet.

Reference: https://github.com/gorhill/uBlock/wiki/Procedural-cosmetic-filters#subjectmatches-mediaarg

Examples:

  • example.com###target-3 > .target-4:matches-media((min-width: 1920px) and (min-height: 930px)):style(color: red !important)

    should be converted to

    example.com#$#@media (min-width: 1920px) and (min-height: 930px) { #target-3 > .target-4 { color: red !important; } }
  • github.com##pre:matches-media(print):style(white-space:pre-line !important;)

    should be converted to

    github.com#$#@media (print) { pre { white-space:pre-line !important; } }

FIXME:

  • This is typically an element hiding rule

    example.com###target-1 > .target-2:matches-media((min-width: 800px))

    and perhaps it can be converted to

    example.com#$#@media (min-width: 800px) { #target-1 > .target-2 { display: none !important; } }

    but I'm not sure about that

Reference: https://kb.adguard.com/en/general/how-to-create-your-own-ad-filters#examples-9

@slavaleleka Do you have a better idea for the last case? 🙂

@ameshkov ameshkov added the enhancement New feature or request label Dec 18, 2022
@adguard-bot adguard-bot added Version: AdGuard v4.3 enhancement New feature or request and removed enhancement New feature or request labels Dec 18, 2022
@ameshkov
Copy link
Member

@maximtop @scripthunter7

Copy/pasting implementation ideas from the comment in a duplicate issue:

Possible implementations:

  1. Support it uBO-like, i.e. natively (parse the rule, check if there's :matches-media, take it into account when composing a rule).
  2. Via rules converter

Tbh, I am not sure which one is better.

Also, we should definitely file it to CoreLibs as well.

@ameshkov
Copy link
Member

ameshkov commented Apr 14, 2023

Regarding the converter way, I am not entirely sure about it.

Of course we could convert them into a simple CSS rule, i.e. ##.banner:matches-media(xxx) --> #$#@media xxx { .banner { display: none; } }. Is it the best way for a filters maintainer?

With how things done now element hiding rules with @media support cannot be implemented.

One more idea would be to introduce a new cosmetic modifier "media" and write those rules this way:
[media="xxx"]##.banner

In this case the rules converter will do a simple ##.banner:matches-media(xxx) --> [media="xxx"]##.banner conversion.

@scripthunter7
Copy link
Member Author

scripthunter7 commented Apr 14, 2023

One more idea would be to introduce a new cosmetic modifier "media" and write those rules this way:
[media="xxx"]##.banner

In my opinion, it makes more sense to use the native CSS syntax at the engine level and convert the other rules.

With how things done now element hiding rules with @media support cannot be implemented.

It seems this is currently a very rare case and can be solved with a CSS rule:

#$#@media(media query list) { css selector list { display: none !important; } }

By the way, this case is a bit similar to when scriptlets get a path constraint (see #65). For example ##+js(scriptlet):matches-path(/path) -> [$path=/path]#%#//scriptlet('scriptlet')

@ameshkov
Copy link
Member

In my opinion, it makes more sense to use the native CSS syntax at the engine level and convert the other rules.

I am more or less okay with both approaches.

IMO, [media="..."] modifier has the following advantages:

  1. Simplifies cosmetic rule parsing & validation, no need to handle @media() { { } } case.
  2. It will be easier to group rules by their media value, i.e. if there're multiple rules.

Both simplifications are really minor and can be easily achieved with the native CSS syntax as well.

On the other hand, using native CSS syntax has a very huge and clear advantage over that new modifier, we don't need to bother implementing anything new.

@scripthunter7
Copy link
Member Author

scripthunter7 commented Apr 16, 2023

Well, if necessary, we can directly parse a CSS media query list (e.g. with CSSTree), so for me the modifier also seems like a good idea, and syntactically it is easy to use. For my part, I simply prefer native CSS, but your idea is more flexible:)

If I understand everything correctly, the following two rules are both valid on this way:

  • #$#@media (min-width: 1000px) and (max-width: 2000px) { body { padding: 0 !important; } }
  • [$media=(min-width: 1000px) and (max-width: 2000px)]#$#body { padding: 0 !important; }

(Although in the case of CSS injections, the native CSS syntax seems more natural to me, and I don't see any complications in terms of parsing either)

And we can simplify
#$#@media (min-width: 1000px) and (max-width: 2000px) { .element-to-hide { display: none !important; } }
to
[$media=(min-width: 1000px) and (max-width: 2000px)]##.element-to-hide

Other aspects:

  • Should this media modifier have an effect on any other rules in addition to the element hiding and CSS injection rules? Does that make sense? app, path and domain are generic, but media cannot affect HTML filtering rules, for example
  • What are the possible limitations? For example, does the Safari Content Blocker handle the media query?
  • Statistics:
    • uAssets:
      • CSS injection + media queries: only 3 rules
      • element hiding + media queries: 0 rules
    • AdGuard Filters:
      • CSS injection + media queries: only 22 rules
      • CSS injection as element hiding (display: none): 1 rule

@ameshkov
Copy link
Member

ameshkov commented Apr 16, 2023

Thank you!

Actually, there are only a few dozens of these rules in the known filter lists so probably bothering with implementing a new modifier is not feasible anyway.

What are the possible limitations? For example, does the Safari Content Blocker handle the media query?

In Safari these CSS rules are only applied via web extension, Safari Content Blocker does not provide media queries support.

Off-topic:

It seems we don't highlight it properly in the VS code extension.

@scripthunter7
Copy link
Member Author

It seems we don't highlight it properly in the VS code extension.

It has already been fixed, but we haven't released a new version of the extension since then. Linguist uses the improved version (last } tokenized fine):

#$#@media (min-width: 1px) { .ad { padding: 1; } }

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