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

Create AdGuard's :matches-attr #2329

Closed
8 tasks done
piquark6046 opened this issue Oct 18, 2022 · 11 comments
Closed
8 tasks done

Create AdGuard's :matches-attr #2329

piquark6046 opened this issue Oct 18, 2022 · 11 comments
Labels
enhancement New feature or request fixed issue has been addressed

Comments

@piquark6046
Copy link
Member

Prerequisites

  • I verified that this is not a filter list issue. Report any issues with filter lists or broken website functionality in the uAssets issue tracker.
  • This is not a support issue or a question. For any support, questions or help, visit /r/uBlockOrigin.
  • I performed a cursory search of the issue tracker to avoid opening a duplicate issue.
  • The issue is not present after disabling uBO in the browser.
  • I checked the documentation to understand that the issue I am reporting is not normal behavior.

I tried to reproduce the issue when...

  • uBO is the only extension.
  • uBO uses default lists and settings.
  • using a new, unmodified browser profile.

Description

Unlike AdGuard's :matches-attr, uBO's :watch-attr can only observe any changes of specific element attributes.
A filter developer cannot use a regexp in uBO's :watch-attr in order to match a randomized attribute.

Therefore, I request expansion of uBO's :watch-attr.

Related: AdguardTeam/AdguardFilters#132468

A specific URL where the issue occurs.

https://www.clien.net/service/board/news

Steps to Reproduce

  1. Visit the provided URL.
  2. Apply the following filter:
    clien.net#?#div[class*="_"]:watch-attr("class"="/[a-z]+_[a-z]+_[a-z]+/")
  3. Reload the website.

Expected behavior

Only one element is blocked correctly as AdGuard Browser Extension performs.

Actual behavior

All element having class attribute are blocked.

uBO version

1.44.4

Browser name and version

Mozilla Firefox Developer Edition 106.0b9

Operating System and version

Ubuntu 22.04.1 LTS

@krystian3w
Copy link

krystian3w commented Oct 18, 2022

So AdGuard forgot fix use filter without fallback: AdguardTeam/ExtendedCss#122 (broken unhide node action)

His implementation works fine in add-on 3.3.2 so don't added watch-attr, I don't checked in any version of apps for Windows/macOS/Android (maybe these need "pseudoclass").


In short I mean this is two different mechanisms.


To avoid break pages in outdated uBo better copy AdGuard name if worth for only few Korean pages (WP.pl in uBo no need implementation matches-attr/property - but hide ads is moved to uAssets team instead waiting to MajkiIT fixes on almost every day).

@gorhill
Copy link
Member

gorhill commented Oct 18, 2022

What you want is a new operator which matches text on attribute name and value. It's best to add this new operator :matches-attr() rather than trying to extend :watch-attr() which purpose is completely different than :matches-attr().

@piquark6046
Copy link
Member Author

@gorhill Great. Thanks a lot.

@piquark6046 piquark6046 changed the title Extend :watch-attr like AdGuard's :matches-attr Create AdGuard's :matches-attr Oct 18, 2022
@gwarser
Copy link

gwarser commented Oct 18, 2022

In the meantime, https://stackoverflow.com/questions/8308328/xpath-to-find-attributes-where-the-name-starts-with-a-given-value


Ah, and you don't need to specify attribute in :watch-attr(), this was a bug apparently: gorhill/uBlock@8aa379e

@gwarser gwarser added the enhancement New feature or request label Oct 18, 2022
@gorhill
Copy link
Member

gorhill commented Oct 21, 2022

I can find only two instances of matches-attr in all of AdGuard lists, and even the issue you link to is an instance of a matches-attr filter being removed. Just trying to assess priority of implementing this, which from what I see should be quite low.

gorhill added a commit to gorhill/uBlock that referenced this issue Dec 2, 2022
Related issue:
- uBlockOrigin/uBlock-issues#2329

The supported syntax is exactly as per AdGuard's documentation:
- https://kb.adguard.com/en/general/how-to-create-your-own-ad-filters#extended-css-matches-attr

Though recommended, the quotes are not mandatory in uBO if
the argument does not cause the parser to fail and if there
are no ambiguities.

Additionally, improved the code to better unquote pseudo-operator
arguments, and to bring it closer to how AdGuard does it as per
documentation. When using quotes, `"` and `\` should be escaped
to preserve these characters in the unquoted version of the
argument.

Additionally, it is now possible to have `:has-text()` match the
empty string by just quoting the empty string:

    ...##foo:has-text("")
@peace2000
Copy link
Member

peace2000 commented Dec 7, 2022

It seems this got implemented. Great!

Though as I'm testing it, it seems that if I put quotes around the attribute, it doesn't match. If I remove them, it matches.

Example:

https://www.k-rauta.fi/tuote/valmistasoite-hole-in-1-250ml/6408070100905

Putting this rule to the picker

##button:matches-attr(class="/[\w]{7}/")

Shows matches, but when I add the quotes around the attribute, matches are gone:

##button:matches-attr("class"="/[\w]{7}/")

I'm trying to match this element:

kuva

Did I understand something wrong or am I just being stupid now?

What I understood is that having quotes is better so that it's AG-compatible rule as well.

@gorhill
Copy link
Member

gorhill commented Dec 7, 2022

##button:matches-attr("class"="/[\w]{7}/")

I would expect this to work the same as ##button:matches-attr(class="/[\w]{7}/").

I will investigate.

gorhill added a commit to gorhill/uBlock that referenced this issue Dec 7, 2022
@peace2000
Copy link
Member

Thanks gorhill, the fix works.

Though I noticed another strange thing: uBO seems to accept a rule if it has a single square bracket before the last round bracket:

k-rauta.fi##button:matches-attr("class"="product-info-anchor"])

Shouldn't that be invalid?

It hides this element:

kuva

@gorhill
Copy link
Member

gorhill commented Dec 10, 2022

Shouldn't that be invalid?

Ok, it will be considered invalid in next release -- currently the [ is discarded.

@gorhill
Copy link
Member

gorhill commented Feb 21, 2023

Is it all good now? Can it be closed?

@piquark6046
Copy link
Member Author

Yup. It can be closed.

@uBlock-user uBlock-user added the fixed issue has been addressed label Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed issue has been addressed
Projects
None yet
Development

No branches or pull requests

6 participants