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

uBO does not unhide nodes no longer matching procedural cosmetic filters #341

Closed
4 of 8 tasks
krystian3w opened this issue Dec 14, 2018 · 56 comments
Closed
4 of 8 tasks
Labels
enhancement New feature or request fixed issue has been addressed

Comments

@krystian3w
Copy link

krystian3w commented Dec 14, 2018

Prerequisites

  • I checked the documentation to understand that the issue I report is not a normal behavior
  • I verified that this is not a filter issue
  • This is not a support issue or a question
  • I performed a cursory search of the issue tracker to avoid opening a duplicate issue
    • Your issue may already be reported.
  • I tried to reproduce the issue when...
    • uBlock Origin is the only extension
    • uBlock Origin with default lists/settings
    • using a new, unmodified browser profile
  • I am running the latest version of uBlock Origin

Description

e.g.

##.nytc---modal-window---isShown:if-not(.nytc---largepicturemodal---contentBody > .nytc---x---x.nytc---largepicturemodal---xBtn[role="button"])

filter cut to much in uBO on cooking.nytimes.com.

A specific URL where the issue occurs

https://cooking.nytimes.com/recipes/1018016-pot-roast
https://cooking.nytimes.com/recipes/1650-italian-pot-roast-stracotto

uBlockOrigin/uAssets#3708
AdguardTeam/AdguardFilters#24339

cooking.nytimes.com#$#html.nytc---modal-window---noScroll { overflow: visible!important; }
cooking.nytimes.com#$#body.nytc---modal-window---noScroll { overflow: visible!important; }
cooking.nytimes.com##.nytc---modal-window---isShown:not(:has(.nytc---largepicturemodal---contentBody > .nytc---x---x.nytc---largepicturemodal---xBtn[role="button"]))

uBO rewrite last filter:

cooking.nytimes.com##.nytc---modal-window---isShown:if-not(.nytc---largepicturemodal---contentBody > .nytc---x---x.nytc---largepicturemodal---xBtn[role="button"])

Steps to Reproduce

  1. Open website
  2. block pop-up with AdGuard rewrite filter
  3. button (link) login no works - no open Window login with ":x:" button.

Expected behavior:

filter works same as in AdGuard with :if-not() construction on uBO.

Actual behavior:

filter no works same as in AdGuard with :if-not() construction on uBO.

Your environment

  • uBlock Origin version: uBO 1.17.4
  • Browser Name and version: Firefox 64 x64
  • Operating System and version: Windows 7 Pro x64
@gorhill
Copy link
Member

gorhill commented Dec 14, 2018

I don't understand what you are reporting. Your filter matches one element on the page, the overlay box. Seems to me this is what you would want? What does "cut to much in uBO" mean?

@gorhill gorhill added the unable to reproduce cannot reproduce the issue label Dec 14, 2018
@krystian3w
Copy link
Author

krystian3w commented Dec 14, 2018

Try block first pop-up and no block normal login pop-up.

Use this same partent html element.

First no have ❌ button, secound have.

@gorhill
Copy link
Member

gorhill commented Dec 14, 2018

first pop-up

What first pop-up? I get a "Join NYT Cooking".

Please be detailed, clear, applied, concise and provide screenshots if needed, I just can't understand what you are reporting.

@gorhill
Copy link
Member

gorhill commented Dec 14, 2018

Also, you did not check the box "I verified that this is not a filter issue". Why? If you want assistance with filter issues, this is not the place.

@krystian3w
Copy link
Author

krystian3w commented Dec 14, 2018

First annoye pop-up:

obraz

Secound from login link obraz :

obraz

have ":x:"

@uBlock-user
Copy link
Contributor

Are you saying that you cannot close the login popup with those filters ?

@krystian3w
Copy link
Author

krystian3w commented Dec 14, 2018

No posible hide (and close first pop-up) without analize JavaScirpt or blocking login for all users.

@krystian3w
Copy link
Author

krystian3w commented Dec 14, 2018

AdGuard have workaround with :if-not() - no use helped attrib for html emements with JavaScript :has() filter?

@gorhill
Copy link
Member

gorhill commented Dec 14, 2018

After I click "Log in", I get no close button even without uBO, why should that be a uBO issue?

@uBlock-user
Copy link
Contributor

uBlock-user commented Dec 14, 2018

I disabled cosmetic filtering and the X still doesn't appear on refresh, I think they removed on purpose or the website is broken.

@krystian3w
Copy link
Author

Or Login FAKE wall.

@gorhill
Copy link
Member

gorhill commented Dec 14, 2018

AdGuard have workaround

Why do you want a workaround when the filter you provide seems to work fine? I am at a lost to understand exactly what issue you are reporting here.

@uBlock-user
Copy link
Contributor

He thinks uBO removed the X button from the login popup, which is not the case.

@krystian3w
Copy link
Author

Nope, uBO no analize again code to remove attrib if X button exsist in hidden element.

Website use same code to show two pop-up's.

@uBlock-user
Copy link
Contributor

I don't see the X button if I disable uBO from the extensions page, so uBO is not responsible for that/

@gorhill
Copy link
Member

gorhill commented Dec 14, 2018

Ok, I tried Adguard filter used for that site in that linked issue:

##.nytc---modal-window---isShown:not(:has(.nytc---largepicturemodal---contentBody > .nytc---x---x.nytc---largepicturemodal---xBtn[role="button"]))

And indeed, this is not a supported syntax yet in uBO.

So if I understand correctly, the issue here is asking to increase compatibility with the above Adguard filter?

This is what I was planning to do with gorhill/uBlock#3683.

@krystian3w
Copy link
Author

a bit like it is possible to easily cheat login walls without script inject and analyze javascript website.

@gorhill gorhill reopened this Dec 14, 2018
@gorhill
Copy link
Member

gorhill commented Dec 14, 2018

I prefer to keep open as I can work on that specific unsupported case.

Both uBO and Adguard essentially do run javascript when enforcing the above filter, because it's not natively supported by the browser.

@uBlock-user
Copy link
Contributor

Adguard's filter brings back the X button I suppose, so guessing he wants that wit uBO filter too.

@krystian3w
Copy link
Author

krystian3w commented Dec 14, 2018

No come back only ❌, comeback all pop-up if click in obraz.

uBO cannot same action without analize JavaScript website end test injections.

@uBlock-user uBlock-user added the enhancement New feature or request label Dec 14, 2018
@uBlock-user uBlock-user added the fixed issue has been addressed label Dec 14, 2018
@gwarser
Copy link

gwarser commented Dec 14, 2018

There are two login/register dialogs. One appears when you load page, without "X", blocking access to page content. @krystian3w want to remove this dialog and uses ##dialog:if-not(x-button) (pseudocode) and it works fine. Second dialog appears when you click on "YOUR RECIPE BOX, Log In" in top right corner. This one have "X" in top right corner and must be accessible/visible if you want to login into page. This dialog should not match ##dialog:if-not(x-button) but uBO hides it anyway.

@gorhill gorhill reopened this Dec 14, 2018
@gorhill
Copy link
Member

gorhill commented Dec 14, 2018

Thanks for the clarification, now I get it.

Well I fixed something here (increased a bit compatibility with Adguard filters), but what you described still occurs. I need to inevstigate why.

@gwarser
Copy link

gwarser commented Dec 14, 2018

I vaguely remember discussion about something like this - conclusion was uBO stops watching hidden nodes for performance reasons (or something like that).

@gorhill
Copy link
Member

gorhill commented Dec 24, 2018

Will need a new solution for this, but it can't be installing an attribute observer by default, everywhere all the time. An idea would be to opt-in to attribute watching to solve sites for which it is necessary:

vivrehome.pl##:watch-attributes(class)

A sort of configuration operator to expand how the cosmetic filterer works on a page.

@gorhill
Copy link
Member

gorhill commented Dec 24, 2018

It seems Adguard is also not listening to attribute changes, so maybe we could discuss a common solution. @ameshkov

@gorhill
Copy link
Member

gorhill commented Dec 25, 2018

I have a working prototype to solve the vivrehome.pl case, and that would be the filter to solve it:

vivrehome.pl##.js-popup-register:has(.js-title-default.is-hidden:watch-attributes(class))

:watch-attributes is a bit long, I am opened to a more concise name.

@kulfoon
Copy link

kulfoon commented Dec 25, 2018

:watch-att

@mapx-
Copy link
Contributor

mapx- commented Dec 25, 2018

:wattr

@ameshkov
Copy link

@gorhill hi, thanks for the tip!

I can't say that I like the proposed solution with a new pseudo-class, but I'll keep watching this issue and make sure AG will support this kind of rules if you decide so.

Tbh, I don't think the overhead is that serious/considerable in the case of an attributes observer, although I have not tested it. I am talking about doing smth like this (pseudo-code):

    // prevent infinite loops in the case of a page's own observer changing these attributes
    const MAX_REEVALUATION_COUNT = 10;
    const reevaluateObserverOptions = {
        attributes: true,
        attributeFilter: ['style', 'class', 'id']
    }

    function applyStyle(node) {
        let reevaluateObserve = new MutationObserver(reevaluate);
        reevaluateObserve.observe(node, reevaluateObserverOptions);
        // Adds an expando to the observer to protect us to protect ourselves from inifinite loops
        reevaluateObserve.reevaluateProtectionCount = 0;
        return reevaluateObserve;
    }

    function reevaluate(mutations, observer) {
        if (!mutations.length) {
            return;
        }
        observer.disconnect();
        ....
        HERE WE RE-EVALUATE THE NODE
        ....
        if (++observer.reevaluateProtectionCount < MAX_REEVALUATION_COUNT) {
            observer.observe(target, reevaluateObserverOptions);
        }
    }

@uBlock-user
Copy link
Contributor

uBlock-user commented Dec 25, 2018

@ameshkov The test case you uploaded at https://ameshkov.github.io/web/watchattr.html, is it specific to Adguard ?

@ameshkov
Copy link

@uBlock-user the test rule should be ameshkov.github.io###testdiv:has(p) to make it cross-blocker :)

@uBlock-user
Copy link
Contributor

uBlock-user commented Dec 25, 2018

@ameshkov Yes, I did add that on uBO's dev build and the red div doesn't appear when I click the button to change div style after a page refresh, so uBO's not affected ?

@ameshkov
Copy link

That's the bug, it should've appeared

@gorhill
Copy link
Member

gorhill commented Dec 25, 2018

@ameshkov

That solution would not work for the case at hand, vivrehome.pl, you would need to also watch for changes in the descendants, and this is where is gets worrying -- just think of Facebook or Twitter DOM, and how likely it is for DOM elements to have their attributes changed, which would translate in having to constantly re-evaluate the procedural filters.

I still have this case in mind and this is why I consider re-evaluating to be avoided as much as possible.

A well targeted :watch-attrs operator would accomplish this: no added overhead other than where is it used, and a well-targeted one (only the node(s) of interest, only the attribute(s) of interest) would ensure minimum overhead to the point of immeasurability.

@ameshkov
Copy link

ameshkov commented Dec 25, 2018

@gorhill

That solution would not work for the case at hand, vivrehome.pl, you would need to also watch for changes in the descendants, and this is where is gets worrying

Yup, makes sense, thank you. I'll think a bit more about it tonight, it's just something disturbs me about this new operator.

On a side note, how does uBO react on other DOM changes in the current version? I did some tests and it's hard to trigger. Also, it seems that unhiding does not happen even if reevaluation is triggered. I can file a bug report if it's not something known.

@gorhill
Copy link
Member

gorhill commented Dec 25, 2018

how does uBO react on other DOM changes in the current version

Using childList with subtree, i.e. anything added or removed will trigger the re-evaluation. The fix for unhiding is in the dev build -- related to the original issue here.

@krystian3w
Copy link
Author

This website vivrehome.pl could be helped by the creation of cookies, but in AdGuard it can be done by a limited number of lists (eg. AdGuard Annoyance), in uBO each list with modified userResourcesLocation.

But it is difficult to require that someone modify userResourcesLocation or self-merge several files as he already uses some.

In addition, it is known that it can be dangerous or undesirable that someone generates some cookies or localstorage to cut something.

gorhill added a commit to gorhill/uBlock that referenced this issue Dec 26, 2018
Related issue:
- #3683

This commit further increases uBO's procedural cosmetic filters
Adguard's cosmetic filter syntax -- specifically those procedural
cosmetic filters where plain CSS selectors appeared following
a procedural oeprator (this was rejected as invalid by uBO).

Also, experimental support for `:watch-attrs` procedural
operator, as discussed in <uBlockOrigin/uBlock-issues#341 (comment)>.
Support may be dropped before next release depending on whether
a better solution is suggested.

Additionally, the usual opportunistic refactoring toward ES6
syntax.
@gorhill
Copy link
Member

gorhill commented Dec 26, 2018

I've added "experimental" support in 1.17.5rc1 for :watch-attrs, just to have a sense of the fix. I use "experimental" as meaning I will drop support if a better solution is proposed. But I do like the minimal overhead of the one here. I expect that this operator will be used very rarely (it took long for such issues to be reported) -- there is no point using it if it doesn't solve issues seen in the two following examples (one of which is a proof of concept):

Site: https://www.vivrehome.pl/

  • Filter: vivrehome.pl##.js-popup-register:has(.js-title-default.is-hidden:watch-attributes(class))
  • Purpose: to block the "Register" overlay when first visiting the site, but yet allow the "Register" overlay when clicking "rejestracja".

Site: https://ameshkov.github.io/web/watchattr.html

  • Filter: ameshkov.github.io###testdiv:watch-attrs(id):has(p)
  • Purpose: to detect id changes.

:watch-attrs is a pass-through filter, i.e. it matches all the elements at the point where it is evaluated. It does however modify the behavior of the procedural cosmetic filter engine by forcing re-evaluation when one or more attribute changes on the matched elements. The argument is a comma-separate list of attribute names. No argument means watch changes of any one attribute.

hawkeye116477 pushed a commit to hawkeye116477/uBlock-for-firefox-legacy that referenced this issue Jun 28, 2020
hawkeye116477 pushed a commit to hawkeye116477/uBlock-for-firefox-legacy that referenced this issue Jun 28, 2020
hawkeye116477 pushed a commit to hawkeye116477/uBlock-for-firefox-legacy that referenced this issue Jun 28, 2020
hawkeye116477 pushed a commit to hawkeye116477/uBlock-for-firefox-legacy that referenced this issue Jun 28, 2020
JustOff added a commit to gorhill/uBlock-for-firefox-legacy that referenced this issue Jul 4, 2020
hawkeye116477 added a commit to hawkeye116477/uBlock-for-firefox-legacy that referenced this issue Jul 4, 2020
Related issue:
gorhill/uBlock #3683

This commit further increases uBO's procedural cosmetic filters
Adguard's cosmetic filter syntax -- specifically those procedural
cosmetic filters where plain CSS selectors appeared following
a procedural operator (this was rejected as invalid by uBO).

Also, experimental support for `:watch-attrs` procedural
operator, as discussed in <uBlockOrigin/uBlock-issues#341 (comment)>.
Support may be dropped before next release depending on whether
a better solution is suggested.

Additionally, the usual opportunistic refactoring toward ES6
syntax.

Co-authored-by:  gorhill <585534+gorhill@users.noreply.github.com>
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

8 participants