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

Logger does not report [style="..."] #435

Closed
g3939434 opened this issue Jul 3, 2015 · 21 comments

Comments

@g3939434
Copy link

commented Jul 3, 2015

uBlock 0.9.9.3-dev.7, Firefox 39.0, Windows 8.1

Logger does not report [style="..."]

Steps to Reproduce:

Create filter:
google.com###lga[style="height:231px;margin-top:80px"]

Open logger and visit google.com. No dom is logged.

BTW, caret ^ is reported as asterisk * if it is not the last character of a filter

For example:
||google.com^images/srpr/logo11w.png is logged as ||google.com*images/srpr/logo11w.png

||google.com^*images/srpr/logo11w.png is logged as ||google.com*images/srpr/logo11w.png as well.

@gorhill

This comment has been minimized.

Copy link
Owner

commented Jul 3, 2015

For the time being, I do not intend to overload the inspector with too much extraneous information -- that's the point of it, to not mimic what the dev tools do, but to make it optimal to use for when the element picker is not enough. By the way, the example you give is not a good one, because the optimal filter for your example is google.com###lga -- there is no need for further narrowing.

@gorhill gorhill closed this Jul 3, 2015

@gorhill gorhill reopened this Jul 3, 2015

@gorhill

This comment has been minimized.

Copy link
Owner

commented Jul 3, 2015

It might make sense to report the style attribute if no other mean of narrowing are available (other than nth-of-type), and if the style attribute is enough in itself to discriminate one DOM element over another.

@g3939434

This comment has been minimized.

Copy link
Author

commented Jul 3, 2015

It looks like you misunderstand the topic. I'm not talking about the element picker but the logger.

The logger does not log a filter that is used if the filter contains [style="..."]

There is much examples in Easylist
###main-content > [style="padding:10px 0 0 0 !important;"]
##td[valign="top"] > .mainmenu[style="padding:10px 0 0 0 !important;"]

@gorhill

This comment has been minimized.

Copy link
Owner

commented Jul 3, 2015

It looks like you misunderstand the topic.

You're right, I misunderstood -- I do this often in the morning, sorry.

I could reproduce. Looking into it.

@gorhill

This comment has been minimized.

Copy link
Owner

commented Jul 3, 2015

Ok, that's a good, tricky one...

uBlock modifies the style of DOM elements it hides by adding the display: none !important style property. But modifying the style attribute causes style-based cosmetic filters to no longer match the same DOM element...

So side-effect of this is that the DOM inspector can't no longer find a matching DOM element for the style-based filter. Ideally, not modifying the style would solve this, however this would cause some cosmetic filters to start failing, because sometimes some elements have the display: block !important set by web sites. (Once I tried to remove uBlock's direct modification of the style attribute and issues were opened as a result -- issues which currently affect ABP for Chrome)

To make things even more complicated, just having uBlock add the display: none !important property causes the style attribute to be regenerated from scratch by the browser apparently. With Chromium I can see the style attribute becoming style="height: 231px; margin-top: 80px; [...]" in the DOM, i.e. there are extra spaces in there, so even removing the display style property does not put back the DOM element in its original, filter-matching state.

Quite the puzzle...

@g3939434

This comment has been minimized.

Copy link
Author

commented Jul 3, 2015

Thanks for your investigation. And how about the second one, is that an issue?

caret ^ is reported as asterisk * if it is not the last character of a filter

@gorhill

This comment has been minimized.

Copy link
Owner

commented Jul 3, 2015

how about the second one, is that an issue?

Not really. From uBlock's point of view, ^ is equivalent to *. If ever this causes issue I could make the necessary changes to support explicitly ^, but so far there is no issue with equating ^ to *, and any edge case could probably be resolved with modifying the filter. There are efficiency benefits when equating ^ with *.

@gorhill gorhill added the fixing label Jul 4, 2015

@gorhill

This comment has been minimized.

Copy link
Owner

commented Jul 4, 2015

The root problem here is also behind another issue I have seen raised elsewhere about the logger not reporting all cosmetic filters which have been applied to a page.

I have what appears so far as a very elegant solution for this, with really virtuous side effects (like not relying anymore on CSS styling to hide DOM elements), however this requires shadow DOM, which unfortunately is not supported out of the box in Firefox (support is hidden behind a config flag).

@gorhill gorhill removed the fixing label Jul 5, 2015

gorhill added a commit that referenced this issue Jul 6, 2015
@g3939434

This comment has been minimized.

Copy link
Author

commented Jul 6, 2015

A reminder that you might already know

Chrome is currently having issue with shadow DOM on some of Google's websites - Chromium bug #496055

@gorhill

This comment has been minimized.

Copy link
Owner

commented Jul 6, 2015

I couldn't reproduce the described issue with uBlock. Note that I use shadow DOM completely differently than what the bug describes, i.e. I do not use it to re-style elements down the tree, using a shadow root for the html node.

I use shadow DOM nodes to void the rendering itself of whatever element needs to be hidden without resorting to CSS styling at all. This means that now the hiding code is resistant to those web pages which would use js code to re-apply an inline display: block !important to defeat cosmetic filtering (though they could go one step further and fight at shadow level, but I doubt there is anyone doing this as of now). So uBlock will be at an advantage for a while.

The bug was opened by one of ABP developer, and looking at ABP's code, I see it creates a shadow DOM node at the root document level (the html tag), whereas I use one shadow DOM specifcally for each node which is to be hidden.

Still, I consider this new way of hiding elements experimental, as this will need testing to find out whether there are unforeseen side effects. Firefox does not support shadow DOM out of the box, and if enabled, it does not work as well as with Chromium, so the old way of hiding elements is still in there as fallback mechanism.

I couldn't think of any ways to solve the issue without resorting to this new shadow DOM approach, so Firefox will still suffer the issue here, until it supports well enough shadow DOM.

Edit: I tried the various issues reported in ABP's tracker and did not see anything wrong with uBlock. Another significant difference with ABP I forgot to mention is that ABP creates its shadow root node unconditionally for every page (and frames on that page), whereas uBlock create one shadow DOM node only for whatever element needs to be cosmetically filtered.

@gorhill

This comment has been minimized.

Copy link
Owner

commented Jul 6, 2015

About those other uBlock related issues mentioned above: using the detailed test case:

  • No hidden elements are reported for the page in the popup UI, while it should be 1.
  • Turning cosmetic filtering off does not reveal the hidden element (logo).
  • The cosmetic filter is not reported in the DOM inspector.

Using shadow DOM approach also solves these issues.

@CrisBRM

This comment has been minimized.

Copy link

commented Jul 6, 2015

@gorhill So FF will benefit from it, if you enable dom.webcomponents.enabled, correct?

@gorhill

This comment has been minimized.

Copy link
Owner

commented Jul 6, 2015

So FF will benefit from it, if you enable dom.webcomponents.enabled, correct?

Yes, enabling dom.webcomponents.enabled will cause the shadow DOM code path to be taken.

It seems to work at first, but more testing is definitely needed.

I can toggle cosmetic filtering fine in Google search page. However for whatever reasons, I can't toggle nodes in the DOM inspector (they disappear but do not come back) -- this is where I have noticed FF failing with shadow DOM. I find this puzzling since I can toggle the same nodes with the cosmetic filtering switch in the popup UI.

I am thinking if it fails there it might also fail under other circumstances in normal usage.

@gorhill

This comment has been minimized.

Copy link
Owner

commented Jul 6, 2015

Ok I see.. It's pointless to enable dom.webcomponents.enabled (other than to test if the fallback code works), since the shadow DOM code does not work at all in FF. The injected styles do all the job.

@gorhill

This comment has been minimized.

Copy link
Owner

commented Jul 6, 2015

Never mind, I could make it work in Firefox by slightly changing the approach (simpler), committed in e3e4d57.

@lewisje

This comment has been minimized.

Copy link

commented Jul 6, 2015

From uBlock's point of view, ^ is equivalent to *. If ever this causes issue I could make the necessary changes to support explicitly ^, but so far there is no issue with equating ^ to *, and any edge case could probably be resolved with modifying the filter.

I can think of a couple potential problems (although they may be "edge cases"):

  1. The set of TLDs is not prefix-free, so ||example.co^pixel.gif would not match "http://example.com/pixel.gif" or "http://example.codes/pixel.gif" but ||example.co*pixel.gif would.
  2. The caret does not consume path components, so ||example.co^pixel.gif would not match "http://example.co/img/pixel.gif" but ||example.co*pixel.gif would.

Then again, it's not common to use the caret except as the last character, because its purpose was to allow matching a specific domain name, regardless of whether a port or a path component immediately follows the domain, and if it's not the last character, you have to start making a choice of whether to match a specific port or the default port; because uBlock Origin doesn't equate caret to asterisk if it's the last character in the URL pattern, it captures the typical use case.

TL;DR: I felt like explaining further why @gorhill had the right idea, as unintuitive as it is.

@g3939434

This comment has been minimized.

Copy link
Author

commented Jul 6, 2015

Then again, it's not common to use the caret except as the last character, because its purpose was to allow matching a specific domain name, regardless of whether a port or a path component immediately follows the domain, and if it's not the last character, you have to start making a choice of whether to match a specific port or the default port; because uBlock Origin doesn't equate caret to asterisk if it's the last character in the URL pattern, it captures the typical use case.

I got confused after reading this part of @lewisje, your comment, especially the last sentence.

uBlock Origin doesn't equate caret to asterisk if it's the last character in the URL pattern, it captures the typical use case.

Since @gorhill said,

From uBlock's point of view, ^ is equivalent to *.

I asked that question was because the filter showed in the logger was not identical to the filter in lists. And, there are many, for instance ||example.com^*/ad/, are used in Easylist. I also have a habit to put ^* instead of /* or * after a domain because it can be blocking a wrong domain in a rare case or not blocking when there is a port.

I did not find anything specific about ^ in wiki - Filter syntax extensions, so at that moment I thought it was the same function as Adblock Plus.

Here is what Adblock Plus explains about ^

Often you need to accept any separator character in a filter. For example, you might write a filter that blocks http://example.com/ and http://example.com:8000/ but not http://example.com.ar/. Here the symbol ^ can be used as a placeholder for a single separator character: http://example.com^ (requires Adblock Plus 1.1 or higher).

Separator character is anything but a letter, a digit, or one of the following: _ - . %. The end of the address is also accepted as separator. In the following example all separator characters are shown in red: http://example.com:8000/foo.bar?a=12&b=%D1%82%D0%B5%D1%81%D1%82. So this address can be blocked with the filter ^example.com^ or ^%D1%82%D0%B5%D1%81%D1%82^ or ^foo.bar^.

@lewisje

This comment has been minimized.

Copy link

commented Jul 6, 2015

I forgot that the caret could match a question mark or octothorpe; anyway, when I said

because uBlock Origin doesn't equate caret to asterisk if it's the last character in the URL pattern, it captures the typical use case.

I was referring to what you had said earlier.

caret ^ is reported as asterisk * if it is not the last character of a filter

You just brought up a good use for a caret in the middle of a filter.

@Halibut80

This comment has been minimized.

Copy link

commented Jul 8, 2015

@gorhill

Shadow DOM support is disabled by default in Firefox. You can enable it by setting dom.webcomponents.enabled to true in about:config.

This is not good recommendation: this option breaks many site. New Google Music, for example.

@gorhill

This comment has been minimized.

Copy link
Owner

commented Jul 8, 2015

This is not good recommendation

I added a warning.

@gorhill

This comment has been minimized.

Copy link
Owner

commented Jul 18, 2015

Fixed with febb181.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.