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

Combining procedural selector :has() and :style() #382

Closed
shesek opened this issue Jan 18, 2019 · 32 comments
Closed

Combining procedural selector :has() and :style() #382

shesek opened this issue Jan 18, 2019 · 32 comments
Labels
enhancement New feature or request fixed issue has been addressed

Comments

@shesek
Copy link

shesek commented Jan 18, 2019

This is a very useful combination, but it seems like they don't like working together?

@uBlock-user
Copy link
Contributor

Put back the template and fill it if you want to file a bug.

@uBlock-user uBlock-user added the invalid not a uBlock issue label Jan 18, 2019
@gorhill
Copy link
Member

gorhill commented Jan 18, 2019

I reopen because I am aware of this, this is a TODO. Many AdGuard filters are discarded because of this. Repro steps:

  • Open the logger
  • Select "All" in the logger
  • Set filter expression to "cosmetic filter" in the logger
  • Enable (or force a reload of) AdGuard Base Filters

Result: the output of the logger will show many such cosmetic filters being rejected.

@gorhill gorhill reopened this Jan 18, 2019
@gorhill gorhill removed the invalid not a uBlock issue label Jan 18, 2019
@uBlock-user uBlock-user added the TODO todo label Jan 18, 2019
@shesek
Copy link
Author

shesek commented Jan 19, 2019

Apologies for not properly using the template, I wasn't really sure whether its a bug report or I'm simply missing something. Posting this to /r/uBlockOrigin as a question first would probably be a better way to go at this. Sorry!

I'm not sure whether uBlockOrigin cares about enabling non-ad-blocking use-cases, but FWIW, what I was trying to do is reduce the visibility of facebook posts that contain a certain hashtag:

facebook.com##div[role=article]:has(.userContentWrapper .userContent a[href^="/hashtag/XYZ?"]):style(opacity:0.5)

@krystian3w
Copy link

As for me, it's a bit of a strained example, you can do something like this with ViolentMonkey / GreasyMonkey / TamperMonkey also.

@Grossdm
Copy link

Grossdm commented Feb 14, 2019

Has :style() ever been chainable (allowed on procedural cosmetic filters)? I thought that it was, and I've struggled in creating some filters recently.

If it is at all feasible, implementing this would be useful.

A common need arises in unsticking headers (very helpful for mobile browsing).

If I can find the correct DOM node with the element selector, often the selector is one that seems likely either to not apply on other pages of the site or to not last very long (i.e., small site changes likely to break the selector).

Typically, I can create a procedural selector with either:

  1. :has() because the "header" (only a div, not a header tag) contains a class named similar to "nav".

  2. :matches-css() in some easy cases because the stuck header is simply styled position: fixed, but unfortunately lacks an id or class that a regular CSS selector could match.

Thanks for everyone's work on uBlock Origin!

@yourduskquibbles
Copy link

Has :style() ever been chainable (allowed on procedural cosmetic filters)? I thought that it was, and I've struggled in creating some filters recently.

Not that I've found. See gorhill's first comment on it that I could find - gorhill/uBlock#781 (comment)

I would also find this useful (and have a real use case I would use this on as well)

@gwarser gwarser changed the title Combining :has() and :style() Combining procedural selector :has() and :style() Nov 26, 2019
@peace2000
Copy link
Member

peace2000 commented May 18, 2020

Found this old request and want to express my interest as well for having this feature.

It's harder to make rules as element name randomizing has become more popular.

@nuchi
Copy link

nuchi commented Jul 27, 2020

I got this working locally. If you don't mind I'll open a PR later tonight. No expectation, of course. I saw that you occasionally accept PRs, but this might be a slightly larger change than you ordinarily accept, and I don't know enough about the performance implications to know whether there would be an issue.

@krystian3w
Copy link

As long as you have permissions to send PR, the author prefers to take care of the quality of the code, and you will probably be able to see your version with a lot of patches.

@gorhill
Copy link
Member

gorhill commented Jul 27, 2020

The reason why this is not fixed yet is not to implement supporting :style() itself, which is quite easy to add at this point, the reason is because of all other aspects that have to be revisited to work with the fix: element picker (preview mode), DOM inspector, cosmetic logger and whatever else I am not thinking at the moment, and I don't want to fix this just to see an avalanche of opened issues for all those other aspects which are expected to work seamlessly with procedural :style() operators.

Now that said, @nuchi if you have something that work, let me inspect the code change in your repo -- I couldn't find any uBO repo in your GitHub page.

@nuchi
Copy link

nuchi commented Jul 27, 2020

Thanks! All those details are great to hear about. I took care of excluding the element picker from being styled, because the code for that was right next to the code I was touching, but the other things you mentioned are things I didn't know about. (And am happy to work on, now that you've brought them to my attention, if it makes it easier to accept a contribution.)

I just pushed my changes.
https://github.com/gorhill/uBlock/compare/master...nuchi:hn--add-procedural-styles?expand=1

@nuchi
Copy link

nuchi commented Aug 6, 2020

I just pushed three changes:

  1. a fix to a bug I had in the existing code, and
  2. element picker implementation (i.e. preview mode)
  3. an adjustment to DOMFilterer.getAllSelectors_ so that logging doesn't get spurious entries
    I verified the element picker correctly handles each of the following on example.com:
##a
##a:style(font-style: italic)
##a:upward(2):style(font-style: italic)
##a:upward(2)

You can see those three changes in the three commits in the link above. I hope this might make it easier to accept my contribution, though as before, no expectation.

@mvastola
Copy link

mvastola commented Aug 18, 2020

Is there any news on this? This feature would be insanely helpful.

Also, ideally this will be moot, but its very confusing that such rules currently silently fail, and the fact that they're not compatible is buried in the wiki. This just drove me crazy thinking my CSS or syntax was off.

gorhill added a commit to gorhill/uBlock that referenced this issue Sep 7, 2020
Related issue:
- uBlockOrigin/uBlock-issues#382

Additionally, remnant code for pseudo-user stylesheets
has been removed. Related commit:
- 5c68867
@gorhill
Copy link
Member

gorhill commented Sep 7, 2020

@nuchi I appreciate your efforts to solve the issue here but this is something I had to go through myself. The reason is that when I have to fix such core feature, I think long before I write any code about what would be the best approach to fix the issue, and most of the time this involves some level of needed refactoring -- which was the case here.

@uBlock-user uBlock-user added enhancement New feature or request fixed issue has been addressed and removed TODO todo labels Sep 7, 2020
@imba-tjd
Copy link

imba-tjd commented Sep 11, 2020

I think I found a not-working case. FF 81.0b9

This works:

example.com##html:style(background: red)

But these are not:

example.com##html:has-text(Example):style(background: red)
example.com##html:has(body):style(background: red)
example.com##html:has(div):style(background: red)

Though these works:

example.com##body:has(div):style(background: red !important)
example.com##body:has-text(Example):style(background: red !important)

Well, now I think my case is a has issue. Because example.com##html works but example.com##html:has(body) fails.

I've opened #1240

@krystian3w
Copy link

krystian3w commented Sep 11, 2020

It looks like a protection against hiding <html> tag inherited from procedural filters.

AdGuard no have that protection.

@krystian3w
Copy link

krystian3w commented Sep 14, 2020

AdGuard syntax may no translated if have regex with {, } (also simplest string).

  • example.*

    example.*#$?#body > div > h1:has-text(/[\w\W]{1,24}/) { color: red !important; }

    I try colorize <H1> if have 1-24 characters.

  • https://jsfiddle.net/67Lhj4gy/

    jsfiddle.net,fiddle.jshell.net#$?#body > div > h1:has-text({ text }) { color: red !important; }

    I try colorize <H1> if have { text }.


The only problem will be if someone is stubborn with the AGuard syntax.


related AdguardTeam/ExtendedCss#114

@gorhill
Copy link
Member

gorhill commented Sep 14, 2020

Enabling AdGuard Base, AdGuard Mobile, AdGuard Annoyance, I get the following cosmetic filters reported as invalid:

! adguard-mobile:
ktuu.com#?#aside > .visible-xs-block > .row:has(> .col-xs-24 > a.ptr[onclick^="trackNativeLink('https://www.ktuu.com/content/sponsored/"]) + hr

! adguard-generic:
oxforddictionaries.com#$#@media (min-width: 1024px) { .main-content > div.container > aside[class="sidebar extend"] { margin: 0 0 20px -300px!important; } }
hm-news.co.il#$?#a[href][data-item^='{"sources":['][data-item*='Video Ad'] { remove: true; }

! adguard-annoyance:
protanki.tv#$#.ant-modal-mask { display: none!important; } { display: none!important; }
buzzhand.com#$?#.mash-lightbox[-ext-has="div"] { display: none!important; }
itemci.com#$#html { oberflow: visible!important; }
itemci.com#$#body { oberflow: visible!important; }
511tactical.com#$##.modal-backdrop { display:none!important; }

Fixing your case (...:has-text({ text })...) also fixes parsing the following AdGuard filter:

hm-news.co.il#$?#a[href][data-item^='{"sources":['][data-item*='Video Ad'] { remove: true; }

The following filters were correctly reject, they have mistakes in them:

oxforddictionaries.com#$#@media (min-width: 1024px) { .main-content > div.container > aside[class="sidebar extend"] { margin: 0 0 20px -300px!important; } }
protanki.tv#$#.ant-modal-mask { display: none!important; } { display: none!important; }
itemci.com#$#html { oberflow: visible!important; }
itemci.com#$#body { oberflow: visible!important; }
511tactical.com#$##.modal-backdrop { display:none!important; }

This leaves those two as improperly parsed:

ktuu.com#?#aside > .visible-xs-block > .row:has(> .col-xs-24 > a.ptr[onclick^="trackNativeLink('https://www.ktuu.com/content/sponsored/"]) + hr
buzzhand.com#$?#.mash-lightbox[-ext-has="div"] { display: none!important; }

For first one of these two, the parsing error is uBO improperly matching parenthesis. For the other ome, I have to investigate as I am not seeing anything wrong with it.

@gorhill
Copy link
Member

gorhill commented Sep 14, 2020

Actually one is a @media directive, it's not supported by uBO.

@krystian3w
Copy link

krystian3w commented Sep 14, 2020

And someting like: DandelionSprout/adfilt#63 (comment)

::-webkit-scrollbar-button:single-button:vertical:increment


I reported ktuu.com as outdated due redirect to: alaskasnewssource.com

@krystian3w
Copy link

buzzhand.com#$?#.mash-lightbox[-ext-has="div"] { display: none!important; }

Maybe fixed if server filters.adtidy.org serve as:

buzzhand.com##.mash-lightbox:has(div):style(display: none!important;)

in the feature.

@gorhill
Copy link
Member

gorhill commented Sep 14, 2020

::-webkit-scrollbar-button:single-button:vertical:increment

Is that valid CSS pseudo-element syntax?

@gorhill
Copy link
Member

gorhill commented Sep 14, 2020

Maybe fixed if server filters.adtidy.org serve as:

I fixed that one in uBO, along with the case where { appears in a pseudo operator.

@mapx-
Copy link
Contributor

mapx- commented Sep 14, 2020

::-webkit-scrollbar-button:single-button:vertical:increment

Is that valid CSS pseudo-element syntax?

https://css-tricks.com/custom-scrollbars-in-webkit/

@krystian3w
Copy link

krystian3w commented Sep 14, 2020

Nope for:

getComputedStyle(document.querySelector('html::-webkit-scrollbar-button:single-button:vertical:increment'), null);

recommend in the past to read CSS values for :matches-css*(*: *) - I often use to read RGB/RGBA colors or property long background-image value.

@gorhill
Copy link
Member

gorhill commented Sep 14, 2020

While investigating, it looks like there is a regression. Trying to find existing filter, this one in uBlock filters:

enterinit.com##*::selection:style(background-color:#338FFF!important)

The style is no longer applied, the change here causes uBO to always apply display: none !important when there is a pseudo-element.

gorhill added a commit to gorhill/uBlock that referenced this issue Sep 14, 2020
Related issue:
- uBlockOrigin/uBlock-issues#382

Additionally, fix minor parsing issues with AdGuard's
cosmetic filters.
@yourduskquibbles
Copy link

yourduskquibbles commented Oct 5, 2020

@gorhill I think there may be another case where this doesn't work consistently - when filters targeting multiple DOM elements are consolidated into one filter and one of them includes the :upward(n) uBO extended filter syntax.

test link: https://www.nytimes.com/live/2020/10/01/us/trump-vs-biden

After scrolling down the page a bit, try to create the following consolidated filters (applying CSS to three elements with one filter) using the element picker:

  1. The filter below does NOT work to pin the multiple floating header components (when :upward(n) is in the first or middle consolidated filter position element picker won't highlight elements and won't allow create):
    ###NYT_TOP_BANNER_REGION:upward(1), #NYT_TOP_BANNER_REGION, #NYT_TOP_BANNER_REGION > div > .interactive-size-medium.interactive-content:style(position: static !important;)

nytimes01

  1. But the filter below WORKS to pin the multiple floating header components (when :upward(n) is in the last consolidated filter position):
    ###NYT_TOP_BANNER_REGION, #NYT_TOP_BANNER_REGION > div > .interactive-size-medium.interactive-content, #NYT_TOP_BANNER_REGION:upward(1):style(position: static !important;)

nytimes02

The filter that you can create with element picker also works on page reload, but the non-working filter doesn't work even if you add it to My Filters without going through element picker.

Tested on both firefox and chrome latest uBO dev builds.

@gorhill
Copy link
Member

gorhill commented Oct 5, 2020

@yourduskquibbles You can't use a procedural operator in a list of selectors, you will need to separate #NYT_TOP_BANNER_REGION:upward(1) from the rest.

When you use a procedural operator in a selector, the whole selector is parsed as a procedural one, and in such case this results in an invalid selector when using a comma-separated list of CSS selector. When not using a procedural operator, you end up with a valid list of plain CSS selectors and uBO does not try to parse this, it just declaratively inject the style.

Note that the second form works because the prepended selector passes the querySelector() test, #NYT_TOP_BANNER_REGION, #NYT_TOP_BANNER_REGION > div > .interactive-size-medium.interactive-content, #NYT_TOP_BANNER_REGION is a valid plain CSS selector, it can be querySelector-ed.

In the first form, uBO tries to test , #NYT_TOP_BANNER_REGION, #NYT_TOP_BANNER_REGION > div > .interactive-size-medium.interactive-content against querySelector, and this fails.

@krystian3w
Copy link

AdGuard maybe support:

example.*#$#body:has(div[style]), html:has(div[style]) { overflow: visible !important }

but upward/xpath now supported only at end of filter due Sizzle (jQuery?) limitation in own environment.

yourduskquibbles added a commit to yourduskquibbles/webannoyances that referenced this issue Oct 6, 2020
Per uBlockOrigin/uBlock-issues#382 (comment)

`:upward(n)` procedural filter needs to ALWAYS be on its own line and never combined with comma separated filters, even when using `:style()` modifications
@gorhill
Copy link
Member

gorhill commented Oct 6, 2020

Actually, I need to investigate why the first form does not show as an error, I would have expected so.

gorhill added a commit to gorhill/uBlock that referenced this issue Oct 6, 2020
@gorhill
Copy link
Member

gorhill commented Oct 6, 2020

In the next dev build, following selector will be marked and rejected as erroneous:

*###a:upward(1), #b:style(position: static !important;)

While the following one will be accepted as valid:

*###a, #b:upward(1):style(position: static !important;)

However, the last one has to be understood as result of #a, #b being fed to :upward(1):style(...); not as #a:style(...) and #b:upward(1):style(...).

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