gorhill / uBlock

uBlock Origin - An efficient blocker for Chromium and Firefox. Fast and lean.
GNU General Public License v3.0
46.98k stars 3.14k forks source link

Using user stylesheets produces flickering not present before. #3160

Closed kasper93 closed 7 years ago

kasper93 commented 7 years ago

Describe the issue

After 6112a68faf46d89c58defdd9e14d4342747c9523 flickering is visible on every page reload. Before only on first load flicker was visible and cached afterwards.

It is not big deal, but probably this minor visual annoyance will be noticed by some people. But if with current solution it is not possible to do flicker-free blocking, I guess we will live with it.

One or more specific URLs where the issue occurs

http://raymondhill.net/ublock/tiles1.html

Steps for anyone to reproduce the issue

Steps to test are described on the page...

Your settings

gorhill commented 7 years ago

There was flickering before... It has nothing to do with the "current solution", which is a vast improvement from before (uBO no longer lookup all matching nodes when user stylesheets are supported).

It's a case of the page being so simple and small that it loads before the styles can be injected.

gorhill commented 7 years ago

Insert

domFilterer.commit(true);

At line 1293 of contentscript.js, see if it makes a difference. This would equate exactly what was done before. But as said, on my side I always has flickering before for that page. When I test, I use real cases such as Google Search page, and there is no flickering with the refactored code.

kasper93 commented 7 years ago

There was flickering before...

On my end without this commit 6112a68 I DO NOT see any flicker on this site.

At line 1293 of contentscript.js, see if it makes a difference.

No.

Okay, I have another example of flicker that happens now and was eliminated after first load before. Placeholder hide flicker always now.

You can test on this page https://www.hltv.org/ranking/teams with ||www.hltv.org/img/static/egbranking.png you can see placeholder when reloading, before it was only visible on first load on my system. (make sure it is not hidden with some other filter)

gorhill commented 7 years ago

No.

How many cosmetic filters are in effect in your configuration? Specifically, how many generic cosmetic filters (subtract cosmetic filter count with and without by using the "Ignore generic cosmetic filters" checkbox).

Placeholder hide flicker always now.

Ok, so this answers my question:

// TODO: Is it *really* worth to cache selectors of collapsed resources?
//       This adds code complexity and I am having doubts about the
//       benefits. Investigate.
kasper93 commented 7 years ago

I think I have most of the list enabled. (Fanboy’s Annoyance List alone have over 20k of cosmetic filter...)

84,987 cosmetic filters 53,872 specific cosmetic filters 31,115 generic cosmetic filters

Ok, so this answers my question:

I see. Are the generic filters still cached per site basis like before?

gorhill commented 7 years ago

That's a lot of generic filters. It's roughly like before, except that all high generics are unconditionally injected. As detailed in the issue, these high generics are a minority relative to all generics.

For any new issues/concerns/questions related to #2984, it's best to add to the issue, not open a new one for everything you see as a regression -- the two issues you opened would have been best added to #2984, as whatever I do as a result is part of the refactoring. Once I officially publish a version (dev or stable), then new issue can be opened.

gorhill commented 7 years ago

Can you tell me the other lists you have aside default and Fanboy Annoyances which contribute toward your high cosmetic filtering count? I am currently at ~66K total cosmetic filters with default and Annoyances.

kasper93 commented 7 years ago

Adguard Base Filters EasyList Adguard’s Annoyance List​​ Fanboy’s Annoyance List

Will give you 78,066 cosmetic filters.

I have every list enabled except regional ones and Fanboy’s Social Blocking List because this gives me some annoying false positives. So I have 84k. I know it is overkill, but it works for me. I'm interested in content not "fireworks" that we have everywhere now a days. I might actually try disabling Fanboy’s Annoyance List because I think it might be too heavy for the benefit it gives.

For any new issues/concerns/questions related to #2984, it's best to add to the issue, not open a new one for everything you see as a regression

Ok, got it!

gorhill commented 7 years ago

I know it is overkill, but it works for me.

You don't have to justify yourself, I am just trying to understand what is causing you to see a regression.

I re-arranged things a bit to ensure that the highly generic cosmetic filters do not get in the way of committing asap the more targeted cosmetic filters into the page. Note that the high generics already had lower priority, they are not injected if the ready state of the document is still loading. However for that one test page, the page loads so fast that I suspect that the document is already in its interactive or done phase when uBO is handling the specific cosmetic filters near document_start, thus causing the high generics to be injected at the same time as the more targeted ones.

This way things are closer to how they were (there was no highly generic cosmetic filters injected at page load before, though there was a price paid later in having to survey the DOM for these highly generics, that price is now removed).

There is still flickering on my side, but as said there was also before for me, though sometimes I do not see the flickering at all with the re-arrangement.

There is one other potential improvement for Firefox I was keeping for the future, made possible by the refactoring, which is to avoid the roundtrip between the main process and the content script before injection. The user styles can only be injected from the main process, and they currently are injected by having the content script send the styles to the main process (after they were gotten from the main process...). This also worked like this before (with uBO/legacy they did not have to be sent to the main process). But the refactoring now allows me to avoid the roundtrip and inject the styles immediately upon them being requested. That something I could work on sooner than expected, but for now I really just want to stabilize all this to be ready to ship before Firefox 57 becomes stable. At the moment, it appear the issue is only affecting the proof-of-concept page on my server.

Regarding the filckering collapsed placeholders, this is fixed.

gorhill commented 7 years ago

Well, I'm sorry, the whole time I thought the test case was http://raymondhill.net/ublock/adbox.html, while I realize now the issue was about http://raymondhill.net/ublock/tiles1.html.

Yes, using http://raymondhill.net/ublock/tiles1.html I do observe a difference. It's quite probably because as said above I had commented out the code to investigate whether using cosmetic filtering for collapsing blocked resources was worth it.

kasper93 commented 7 years ago

Regarding the filckering collapsed placeholders, this is fixed.

Thanks! This is resolved.

I do observe a difference. It's quite probably because as said above I had commented out the code to investigate whether using cosmetic filtering for collapsing blocked resources was worth it.

To be clear I'm talking about the generic cosmetic filtering: <div> removed box from the beginning. Sorry if I wasn't clear enough. I still see the flicker, red background before it is blocked. With older uBO I NEVER see it on Firefox (except first page load). No big deal for me, but I'm trying to report things that might be considered regression after recent changes.

gorhill commented 7 years ago

I still see the flicker, red background before it is blocked.

That defies explanation, as currently the styles are injected asap just like before. The highly generic ones are delayed until the next animation frame, so they can no longer delay the injection of the specific ones.

Edit: I tried again, and I see absolutely no difference with the div flickering as with 1.14.16. Sometime I see it, often I don't with both versions (using the "go back" button on the page).

kasper93 commented 7 years ago

I tried again. And indeed I was wrong old version also flickers, but very rarely. I mean if I go back and open page again it almost never flicker, it does on reload rarely. With new version it is the opposite, it flickers most of the time and rarely it does not.

I made short comparison: NEW 3 reloads - 3 flickers 2 page loads - 2 flickers OLD 5 reloads - 1 flicker 2 page loads - 0 flicker

I know it is small amounts of tries, didn't want to make long video. But the results are consistent. With OLD it is hard to trigger flicker, with NEW it is hard not to trigger.

Like I said I'm only trying to describe my observations about new version. Not saying it is worst, but I can see clear difference. Also I don't see much difference if I disable all filter list except Easylist.

EDIT: I tried to stress test OLD version and for 100 reloads it flickered 6 times. EDIT2: NEW version per 100 reloads flickered 65 times. (with some nice 10 not flicker in row). I noticed if I reload faster it flickers less, but I tried to keep some intervals between reloads to emulate more normal usage.

gorhill commented 7 years ago

I can only speculate. Maybe just having all the high generics part of the first message is causing slightly extra delay not there before when going through WebExtensions messaging API. I will investigate sooner than later the potential improvement of injecting in the main process to avoid roundtrip. I would expect this not only to help, but to provide even better results than 1.14.16.

gorhill commented 7 years ago

I created a branch noroundtrip in which I implemented the idea of avoiding the roundtrip when injecting the specific selectors (including the cached ones). Would you be willing to build and try from that branch? (my understanding is that you are building from the repo).

kasper93 commented 7 years ago

Yep, now it is as before if not better. I reloaded page 100 times and noticed only 3 flickers. Good job.

I wanted to see on my other test page, but details is undefined in vapi-usercss.js:128

gorhill commented 7 years ago

but details is undefined vapi-usercss.js:128

Oops, yes, details is optional, I forgot to account for this.

Now I wonder if I should do the same for the high generic ones -- I want to experiment with this. One thing to keep in mind is that ABP injects all of the 18,000+ generic cosmetic filters (from only EasyList), so having uBO injecting the 2-4% most generic out of the 18,000+ shouldn't be an issue, but this needs testing.

kasper93 commented 7 years ago

I tested some more and it works ok. I noticed that procedural filters are also affected by this new delay. On 1.14.16 on some sites I almost didn't notice flicker, but now I do. I made quick test with

raymondhill.net##div:has(> #ADSLOT_1)

On 1.14.16 div is visible for 1 frame before collapsing. On noroundtrip (e1acf6d) div is visible for 11-12 frames before collapsing.