bvaughn / react-window

React components for efficiently rendering large lists and tabular data
https://react-window.now.sh/
MIT License
15.73k stars 783 forks source link

mouse wheel scrolling broken in chrome #221

Closed Spenc84 closed 5 years ago

Spenc84 commented 5 years ago

This issue can be seen from your demo page when viewed in a Chrome browser.

When scrolling with the mouse wheel in a chrome browser you will see empty white space as the javascript tries to catch up with the browsers scroll position. This only seems to occur in chrome and only when using the mouse wheel instead of the scroll bar.

This is likely the result of Chrome's recent decision to make onwheel events passive by default. related issue

If it turns out that this issue is a byproduct of the passive listener issue, then I'm not sure what the best approach to resolving it would be prior to React releasing a passive listener API. Maybe a native onScroll handler could be added on componentDidMount instead of using React's onScroll jsx attribute? Or would that approach have unintended side effects?

bvaughn commented 5 years ago

Maybe a native onScroll handler could be added on componentDidMount instead of using React's onScroll jsx attribute? Or would that approach have unintended side effects?

Hm. React batches updates that happen inside of its own event handlers, so this might cause more renders. (We could batch inside of our own handler, but we'd be outside of the batch of anything else that happened to listen to React's scroll event.)

I think I'd prefer to not try solving this within react-window. I'd rather wait for an idiomatic React solution. I'll try to remember to bring it up during the next React+Chrome weekly sync though.

Spenc84 commented 5 years ago

So I've been toying with this issue a bit more and am curious to know what was the reasoning behind adding the style property will-change: transform to the grid's containing element? I don't understand that property well enough to know precisely what optimizations it attempts to apply, or what problems it attempts to solve, but when removed it does seem to eliminate the wheel scrolling issues in chrome.

Here is a forked version of your demo's sandbox comparing two different instances of your grid component, one with the will-change property applied, and one without it: https://codesandbox.io/s/9vj156m7o

You can see that the grid with the will-change property applied seems to have trouble keeping up with scrolling via the mouse wheel, while the other doesn't.

In both cases Mozilla seems to do fine when scrolling with the mouse wheel, but if you click on the scroll bar and scroll by dragging it around then it completely whites out. Any suggestions for that?

bvaughn commented 5 years ago

Hey @Spenc84, sorry for the slow response time. I struggle to keep up with projects lately.

Admittedly it's been a long time since I added the will-change style to react-virtualized. When I created this library, I kind of just copied it over without much thought. Looking back in my notes when I added the style:

Without this property, Chrome repaints the entire Grid any time a new row or column is added. Firefox only repaints the new row or column (regardless of this property). Safari and IE don't support the property at all.

Looking at the MDN docs again, maybe a value of will-change: contents; would make more sense to begin with? Or maybe the property is not really that helpful given that this is a windowing library and I'm actively managing element positions and rendering.

I don't have a lot of time right this moment to do any performance profiling to compare with and without this property. Would you be interested in lending a hand on this? I'd like to be confident before removing this style that it won't cause a perf regression.

Spenc84 commented 5 years ago

Admittedly my experience with profiling is fairly limited, so I'm not sure how much help I would be in that regard. It is on my list of topics to delve further into however, so I'd be happy to lend a hand whenever that time comes, but I can't be sure how far down the road that will be.

Whenever I do get that opportunity however I'll be sure to use this as a test case if you haven't already addressed it by that point.

chrishtr commented 5 years ago

In Chromium, will-change: transform on an element with overflow:scroll will cause the scroller to be composited, and hence raster into a separate texture. Because it's composited, it now also has threaded scrolling by default, meaning that scrolling will happen independently of the main thread, and scroll events will be sent to the main thread asynchronously.

This is why there is different behavior between the will-change:transform version vs not.

Also, the mouse-wheel passive event listener default only has effect for composited + threaded scrollers. (The concept of passiveness only makes sense for scrollers which might be scrolled on the compositor thread in parallel with the main thread.)

Spenc84 commented 5 years ago

That's incredibly useful information, and makes a lot of sense. Thank you for taking the time to share that.

bvaughn commented 5 years ago

Thanks for the added context, @chrishtr 🙇

I'm not entirely sure how to act on this information. It sounds like will-change: transform is serving a useful function for this library in most case, as threaded scrolling seems desirable. The wheel event performance behavior reported seems pretty unfortunate though.

I guess as a stop gap, the will-change style can be easily overridden using the style prop (if that seems like the right trade off for a particular app/use case).

chrishtr commented 5 years ago

Yes, threaded scrolling is likely desirable, because it will allow scrolling to not jank if any main-thread javascript, DOM rendering, or other tasks are running. The downside however, is that you lose a certain amount of control of per-frame rendering. In particular, if the scrolling is fast enough, the javascript thread slow enough, or content complex enough, scrolling will get ahead of the content drawn by the virtual scroller and it will show white (in Chrome rendering parlance, we call this "checkerboarding").

The async wheel event behavior is only one instance of this. Touch scroll gestures will also have this threading behavior, unless you explicitly cancel the entire gesture at start and handle it yourself (which is definitely a bad idea, because it means you'll have to implement a ton of user agent built- in behavior such as fling curves, not to mention loss of the good behavior in the presence of slow scripts or DOM rendering).

The best solution is to optimize the virtual scroller so that it is able to keep up, and de-jank the main thread. However, that is not easy sometimes either. Another solution is to force the event listener to be non-passive by adding passive:false to the event listener arguments.

bvaughn commented 5 years ago

Thanks for confirming!

I'm going to close this issue, because I don't intend to take any action on it for the time being. Happy to continue discussion and brainstorming though!

sibelius commented 3 years ago

what is the proper fix for this?