uBO doesn't reject cosmetic filters with invalid pseudo-classes/pseudo-elements

[gwarser]

Discussed in #2440

^{Originally posted by u-RraaLL January 3, 2023}
Before CSSTree, if I made a mistake with a pseudo class - e.g. a typo, the whole line would be invalidated. Now, the editors will accept anything that doesn't match the name of a procedural filter or an action operator <- these will throw errors until properly finished e.g. :remove -> :remove(), :matches-path -> :matches-path(/^/$/).

This, however, will not throw any errors in "My filters" :matchess-path(/^/$/). It will in Picker, though, which, I'm guessing, verifies the presence of brackets better.

Invalid pseudo classes don't throw errors in neither of the places. They just won't have a match, which in Picker's case will usually prevent creation of the filter.

At first I thought Picker would accept ##a, b:randomstringhere, allowing for matches or a, but it seems it looks at the invalid pseudo classes in the procedural manner ##a, b:has(c), not finding a match for either.

It will still accept them if placed inside :is(), :where() or :matches() though. Which makes sense since they're using forgiving selectors list. So I understand the errors won't be thrown for these, but I do think they should be thrown in the other cases.

---

I probably should've filed it as a bug report, but I'd be forced to lie on the template to send it in.

[gorhill]

This, however, will not throw any errors in "My filters" :matchess-path(/^/$/)

It does if writing ##:matchess-path(/^/$/), otherwise without ## it's parsed as a network filter.

Invalid pseudo classes don't throw errors in neither of the places

##b:randomstringhere is also accepted as valid in other blocker extensions.

Before CSSTree, uBO would use the browser's DOM API to validate selectors, and after years of patching cases to make it better at detecting (often breaking something else as a result) while ending up with a difficult to maintain pile of code, this all broke down when :has() became native in Chromium.

The only sensible way out of the creaking pile of code once and for all was to rely on a solid library for parsing CSS, hence CSS tree. This also made it possible to compile cosmetic filters outside the browser environment, as is required for MV3 since filters are pre-compiled in Nodejs.

Frankly, after almost 8 9 years at this, I don't see myself piling on more and more tasks to tie all my free time on this project to catch perfectly every single instance of bad filter. I don't think it's too untenable when crafting filters to be careful at it beyond what uBO will already warn as invalid.

[u-RraaLL]

Yeah, I wanted to follow-up in the discussion, but you removed your reply before I got back online so I thought you perhaps changed your mind about at least some part of my message.

It does if writing ##:matchess-path(/^/$/), otherwise without ## it's parsed as a network filter.

True, but I originally ran into the issue when working with an :upward() filter - it doesn't throw errors when other procedurals are present, it seems.
Strange thing is, it did throw me errors in Picker yesterday, but no longer does now. So I thought there was a difference between the way it works in the two places.

##b:randomstringhere is also accepted as valid in other blocker extensions.

That's fine, but since ##a, b = ##a and ##b, should a typo e.g. ##a, b:only-chld really stop a from finding a match?

[gorhill]

:upward() should fail when there is no argument, I need to fix this.

[u-RraaLL]

That wasn't actually what I meant, but I'm glad this led you to finding this issue.

What I meant was that while this will fail ##:matchess-path(/) b, this will not ##:matchess-path(/) b:upward(a). Same goes for any other invalid procedural operators when a valid one is present. E.g. ##a:upward(b):Has-text(c)

[gorhill]

Ok, that is weird, I will look into this. (actually off the top of my head I think I know why this is happening, this needs fixing)

[gorhill]

I don't see myself piling on more and more tasks

Sometimes I am more tired than usual and I see any task as being a mountain. Looking at this again, I think it's feasible to validate pseudo-classes using the list at https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-classes and reject anything not in there.

[gwarser]

Experimental stuff can be problematic, like https://blog.shahednasser.com/how-to-style-a-video-player-and-create-a-custom-player/#video-pseudo-element-selectors People asked for this earlier.

[gorhill]

These are pseudo-elements, I wouldn't touch these for now (CSSTree distinguishes these from pseudo-classes), and if ever I want to address them I would probably just ignore trying to validate peudo-elements prefixed with -.

[krystian3w]

Chromium pseudo-classes #1247

• :single-button
• :horizontal
• :vertical
• :increment
• :decrement

(but I don't implemented this in Polish Annoyance Filters at all to today so mabye nobody use after fix)

[gorhill]

document.querySelector('body:single-button') works in Chromium, but throws in Firefox, meaning a filter using :single-button would break cosmetic filtering in Firefox, unless properly gated using !#if .... Maybe it's worth not supporting something not valid in all uBO-supported browsers to avoid the risk of breaking cosmetic filtering.

[krystian3w]

Polish Annoyance Filters have if:

FiltersHeroes/PolishAnnoyanceFilters@4a0c854#diff-c38964498cebfed5aee1796a25d579fba58b7772476b0cd2e9d3dd5977b91d93R37 (4 years)

but I don't use gwarser script to research whole public filter lists.

[krystian3w]

The former :if-not( still don't rejects the filter line (redish background) - works like "comment" and hide element in procedural method (have two random attributes).

The same is true for :if( - this one makes the filter not work but doesn't have redish background.

[u-RraaLL]

Maybe not the right place, but didn't want to create a new issue over such a small thing.

##body>div:upward(1) :is(a) works fine until uBO1.51, doesn't in 1.52.

It now requires the use of the asterisk ##body>div:upward(1) *:is(a)

Is that by mistake or is it necessary due to some change and we should document it somewhere?

Edit: It seems to be happening with all procedural filters (e.g. body:has(div) :is(a)), not just upward.

[gorhill]

It's a regression due to gorhill/uBlock@fbc7a0e0ae.