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

Nested procedural filtering into :has() did not works #2228

Closed
8 tasks done
krystian3w opened this issue Aug 23, 2022 · 47 comments
Closed
8 tasks done

Nested procedural filtering into :has() did not works #2228

krystian3w opened this issue Aug 23, 2022 · 47 comments
Labels
bug Something isn't working

Comments

@krystian3w
Copy link

krystian3w commented Aug 23, 2022

Prerequisites

I tried to reproduce the issue when...

  • uBO is the only extension
  • uBO with default lists/settings
  • using a new, unmodified browser profile

Description

Nested has like:

example.*##div:has(h1:has-text(Example))

will no longer works with Chrome 105 after stable release if Google community no implement better native has in CSS.

So I reccomend mediation with Big "corporations" after bugs form console no looks to fix in Addons. I leave it free to throw this into the chromium and Firefox bugtrackers.

AdguardTeam/ExtendedCss#149


This can be too regresion after implement mix has with style #382 or fix Yuki issue #2170


AdGuard Base sometimes use domain.tld##foo:has(bar:has-text(word)); I don't checked this in uBo stock list or Regional.

A specific URL where the issue occurs

example.org / example.com

Steps to Reproduce

  1. Install Chromium Browser 105+ or Opera 91+
  • layout.css.has-selector.enabled in Firefox have same troubles - so can break filters in 2023/2024 after publish function as stable (for now have some glitches, so maybe Moz://a implement something to change nothing in uBo code)
  1. Paste filter from description to My Filters (or elmenet picker (with omit domain) - as later step).

  2. Open one of two demo Internet pages.

Expected behavior

Website should be "blank".

Actual behavior

Website isn't "blank".

uBlock Origin version

1.44.1b1

Browser name and version

Firefox 103, Opera 91+, Chrome 105+

Operating System and version

Windows 7/10

@gwarser
Copy link

gwarser commented Aug 23, 2022

@krystian3w this should be detected. Probably new implementation does not throw errors when fed with syntax which is not valid natively. Y:

image

@gwarser gwarser added the bug Something isn't working label Aug 23, 2022
@gwarser
Copy link

gwarser commented Aug 23, 2022

image

@gorhill
Copy link
Member

gorhill commented Aug 23, 2022

MDN says:

:has( <forgiving-relative-selector-list> )

So :has() is forgiving now, meaning it won't throw when invalid selectors are passed to it.

@gorhill gorhill reopened this Aug 23, 2022
@gorhill
Copy link
Member

gorhill commented Aug 23, 2022

Why close? This needs fixing.

@krystian3w
Copy link
Author

I meant no possible fix on addon level.

@gorhill
Copy link
Member

gorhill commented Aug 23, 2022

I need to investigate what can be done.

@krystian3w
Copy link
Author

If I correct checked latest stable 105 release of Chrome has landed with enabled has for CSS - so can back e.g. Instagram sponsored posts.

So Google community tries test of implementation on whole people (not on only beta, dev and canary level).

@gwarser gwarser added the fixed issue has been addressed label Aug 31, 2022
@gwarser gwarser closed this as completed Aug 31, 2022
@gwarser gwarser reopened this Aug 31, 2022
@gwarser
Copy link

gwarser commented Aug 31, 2022

##body:has(div:has(h1:has-text(Example))) does not work? Nightly with layout.css.has-selector.enabled, uBO 1.44.1b2

@krystian3w
Copy link
Author

krystian3w commented Aug 31, 2022

Also both no implement read :scope as visible in path - bad info for Nordic list.

Firefox also no implemented:

(> (maybe too + and ~)

Chrome ignore/no implemented:

body:has(body > as fallback to ":scope" if these are correct (e.g. in Firefox).

@gwarser gwarser removed the fixed issue has been addressed label Aug 31, 2022
@Yuki2718
Copy link

Will uBO uses native :has if possible?

@gorhill
Copy link
Member

gorhill commented Aug 31, 2022

Yes, that's the goal, it's already the case and it's actually why the issue exist here given that :has(...) has been spec'ed as a "forgiving" operator -- this makes sorting out prrocedural vs. declarative very difficult for uBO.

@gorhill
Copy link
Member

gorhill commented Aug 31, 2022

I didn't expect Chrome 105 to reach stable already. I will have to do something to fix this and publish a 1.44.2 revision.

gorhill added a commit to gorhill/uBlock that referenced this issue Aug 31, 2022
Related issue:
- uBlockOrigin/uBlock-issues#2228

Using `#?#` (instead of `##` for a procedural cosmetic filter will
prevent uBO from trying to convert the filter into a declarative
one.
@gorhill
Copy link
Member

gorhill commented Aug 31, 2022

The chosen solution is for filter authors to force the parsing of a filter as procedural by using #?# instead of ##. If a :has(...) filter does not result in the expected behavior for browsers supporting :has(...), force parsing as a uBO procedural by using #?#.


That said, ##body:has(div:has(h1:has-text(Example))) is kind of a contrived example since the filter would be better crafted as ##body:has(div h1:has-text(Example)), but it still is a valid filter and it will now work when using #?#.

@gothic-bum
Copy link

The chosen solution is for filter authors to force the parsing of a filter as procedural by using #?# instead of ##.

Will #?# also apply to other procedural filters that have native browser support (e.g. :not(...) ), or is it just for :has(...)?

@gorhill
Copy link
Member

gorhill commented Aug 31, 2022

Any potentially procedural operator, so also :not(...). Normally uBO tries to compile them as declarative, #?# will prevent that.

@gorhill
Copy link
Member

gorhill commented Aug 31, 2022

I do expect the need to use #?# to be rare, but we will see.

@MasterKia
Copy link
Member

What about older rules? Do they all need to be tested again?

Can uBO warn in the syntax highlighter that this rule might need #?# instead of ##?

@krystian3w

This comment was marked as outdated.

@krystian3w
Copy link
Author

krystian3w commented Sep 6, 2022

I detected teach browser be add-on how read child's:

firefox_udQvjabKc5.mp4
! https://codepen.io/bramus/pen/MWEvKEg
cdpn.io#?#figure:has(figcaption) img:style(outline: 10px dashed red;)

Without these teach, Mozilla fail don't stylize second image.

@gorhill
Copy link
Member

gorhill commented Sep 6, 2022

I can reproduce the issue with Firefox with your filter once layout.css.has-selector.enabled is set to true -- not sure why as I stepped in the code once and all seemed to execute as expected -- this needs more investigation.

Without these teach

I don't understand that sentence, what is teach?

@krystian3w
Copy link
Author

krystian3w commented Sep 6, 2022

Mozilla based on devtools ignored has selector and matches second image without figcaption parent.

So uBo temporary teach demo how don't match false positives.

This maybe glitch similar to unhide node.

@Yuki2718
Copy link

Yuki2718 commented Sep 6, 2022

So ##selector1:has(something) selecotor2 can cause FP on Firefox with layout.css.has-selector.enabled: true.

@krystian3w
Copy link
Author

But hopefully not enabled for any groups:

  • stable
  • beta and oldie Aurora renamed to developer
  • nightly

We can still fight for this piece of bread. But I don't see light to implement error for nesting.

@krystian3w
Copy link
Author

krystian3w commented Sep 7, 2022

I detected new regression in works :not() after enforce filter as procedural or in AdGuard style syntax in uBo 1.44.2:

! works simple negation
private-company-webapp.org##table td:has-text(employee1) ~ td:not([class*="day_off"]):style(border-top: 5px solid green)
!
! Both have broken `:not()`
private-company-webapp.org#?#table td:has-text(employee1) ~ td:not([class*="day_off"]):style(border-top: 5px solid green)
private-company-webapp.org#$?#table td:has-text(employee1) ~ td:not([class*="day_off"]) { border-top: 5px solid green }

The negation in enforced procedural and AdGuard syntax are cleared to empty brackets in use (possible see this in uBo logger). Also then no works disable broken filter by Author mode form advance settings (due no match with raw database?).

Minimal demo (PoC due company app is not public):

https://jsbin.com/gejiqan

IMO maybe possible fix instead rewrite filter or back to mixed declarative state.

@Nomes77
Copy link

Nomes77 commented Sep 9, 2022

I do expect the need to use #?# to be rare, but we will see.

I'm on FireFox 104.0.1 (layout.css.has-selector.enabled enabled) and with the latest stable uBo update and I only had to change one rule (procedural operator) of all rules I have, namely:
github.com##div.push > .body > :has([href="/apps/github-actions"]) to github.com#?#div.push > .body > :has([href="/apps/github-actions"]), but strangely github.com##div.push > [class="body"] > :has([href="/apps/github-actions"]) also worked.

Nomes77 added a commit to Nomes77/AdBlockFilters that referenced this issue Sep 9, 2022
JohnyP36 added a commit to JohnyP36/Personal-List that referenced this issue Sep 10, 2022
Except one rule, because that will otherwise cause an error.
Yuki2718 referenced this issue in uBlockOrigin/uAssets Sep 11, 2022
gorhill added a commit to gorhill/uBlock that referenced this issue Sep 23, 2022
The new parser no longer uses the browser DOM to validate
that a cosmetic filter is valid or not, this is now done
through a JS library, CSSTree.

This means filter list authors will have to be more careful
to ensure that a cosmetic filter is really valid, as there is
no more guarantee that a cosmetic filter which works for a
given browser/version will still work properly on another
browser, or different version of the same browser.

This change has become necessary because of many reasons,
one of them being the flakiness of the previous parser as
exposed by many issues lately:

- uBlockOrigin/uBlock-issues#2262
- uBlockOrigin/uBlock-issues#2228

The new parser introduces breaking changes, there was no way
to do otherwise. Some current procedural cosmetic filters will
be shown as invalid with this change. This occurs because the
CSSTree library gets confused with some syntax which was
previously allowed by the previous parser because it was more
permissive.

Mainly the issue is with the arguments passed to some procedural
cosmetic filters, and these issues can be solved as follow:

Use quotes around the argument. You can use either single or
double-quotes, whichever is most convenient. If your argument
contains a single quote, use double-quotes, and vice versa.

Additionally, try to escape a quote inside an argument using
backslash. THis may work, but if not, use quotes around the
argument.

When the parser encounter quotes around an argument, it will
discard them before trying to process the argument, same with
escaped quotes inside the argument. Examples:

Breakage:

    ...##^script:has-text(toscr')

Fix:

    ...##^script:has-text(toscr\')

Breakage:

    ...##:xpath(//*[contains(text(),"VPN")]):upward(2)

Fix:

    ...##:xpath('//*[contains(text(),"VPN")]'):upward(2)

There are not many filters which break in the default set of
filter lists, so this should be workable for default lists.

Unfortunately those fixes will break the filter for previous
versions of uBO since these to not deal with quoted argument.
In such case, it may be necessary to keep the previous filter,
which will be discarded as broken on newer version of uBO.

THis was a necessary change as the old parser was becoming
more and more flaky after being constantly patched for new
cases arising, The new parser should be far more robust and
stay robist through expanding procedural cosmetic filter
syntax.

Additionally, in the MV3 version, filters are pre-compiled
using a Nodejs script, i.e. outside the browser, so validating
cosmetic filters using a live DOM no longer made sense.

This new parser will have to be tested throughly before stable
release.
@krystian3w
Copy link
Author

krystian3w commented Oct 10, 2022

I see breakage in uBo dev with >, ~ and + in html filtering (filters clear body from nodes/dom tree):

  • Error:

    Uncaught DOMException: Element.querySelectorAll: ' > a' is not a valid selector
    html-filtering.js:201
  • Cited code line:

    return Array.from(root.querySelectorAll(this.selector));
  • Examples:

    gorhill.github.io##^#phf #a2 .fail:has(> a > b)
    gorhill.github.io##^#phf #a12 .fail:has(+ a)
    gorhill.github.io##^#phf #a13 .fail:has(~ a:has(b))

Workaround can be use upward.

IMO not related with experimental layout.css.has-selector.enabled - Those willing can check whether CSStree in html filtering (and the future implementation of has by browser engine).

Also no checked use "…", '…' or many backslashes to enforce as procedural (I already prefer to use upward).

Maybe works Fine in Firefox 104 and 106 (so can be 105 bug).

gorhill added a commit to gorhill/uBlock that referenced this issue Oct 10, 2022
@krystian3w krystian3w changed the title Nested procedural filtering into :has() no works Nested procedural filtering into :has() did not works Oct 10, 2022
mneunomne pushed a commit to mneunomne/AdNauseam that referenced this issue Oct 17, 2022
mneunomne pushed a commit to mneunomne/AdNauseam that referenced this issue Oct 17, 2022
Related issue:
- uBlockOrigin/uBlock-issues#2228

Using `#?#` (instead of `##` for a procedural cosmetic filter will
prevent uBO from trying to convert the filter into a declarative
one.
mneunomne pushed a commit to mneunomne/AdNauseam that referenced this issue Oct 17, 2022
@krystian3w
Copy link
Author

krystian3w commented Oct 19, 2022

Latest uBo dev have if condition when found:

foo.who##a, b:has(c)

Because simple CSS hide any a without test exist c inside (only b are checked and no possible check elements above like b:has(body c)) - for joining recommended will be use: :is(a, b):has(c) as the shortest.

In short I mean CSS didn't followed content blockers decision.

@gorhill
Copy link
Member

gorhill commented Oct 19, 2022

uBO should follow the expected result in CSS. With the previous parser there was not much motivation to do so because of all the added code complications, but this comes free with the new parser.

At this point I think I prefer to leave this to another dev build cycle though.

@krystian3w
Copy link
Author

krystian3w commented Oct 26, 2022

I think this can be regression in deprecated syntax:

  1. Open on PC gorhill/uBlock@2f646db
  2. Into picker paste ##.pl-s:if-not(*)
  3. Filter matches ≈24 elements but only ≈four (4) are without childs, may works fine in ##.pl-s:not(:has(*))

If page don't have .pl-s maybe needed is enable sidebar with catalogs for commits.

In real cases regression will break pages like kobieta.wp.pl with MajkiIT oldie filter line (blank/blinks very important div in dom tree).

@gorhill
Copy link
Member

gorhill commented Oct 26, 2022

Into picker paste ##.pl-s:if-not(*)

if-not is no longer documented as being valid, it should not be used as a case to fix.

@krystian3w
Copy link
Author

krystian3w commented Oct 26, 2022

So somebody must virtual kicked / prodded MajkiIT to update filter before new parser are stable.

Similar case like #2316 to remember update in filter file (if idea #2328 too much break html filtering).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests