-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Fixes slow (mouse) wheel scrolling #110
Conversation
…+Windows interrupt issue
this.refs.scrollingContainer.scrollLeft = scrollLeft; | ||
this.refs.scrollingContainer.scrollTop = scrollTop; | ||
// Don't re-apply while a scroll is in progress; this causes slow scrolling for certain OS/browser combinations (eg. Windows and Firefox) | ||
if (!isScrolling) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not happy with this approach as-is. I think it would work for almost all cases but since it depends on timing... it has potential to break.
I guess this is more of a proof-of-concept. If it resolves the issue then I'll think of a more elegant way to differentiate between the types of scrolling:
- User scrolls (with or without mouse-wheel) via the browser
- Incoming scroll-to-row/column property
- Incoming scroll left/top via
setScrollPosition
function (or property, as withScrollSync
) - Scroll adjustments based on reduced row/column count (to prevent over-scrolling)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds much better to me. But actually you just need to differentiate between two sources of scroll position changes: observation of browser scrolling, and intentional adjustments. So instead of isScrolling, I'd call that part of the state scrollPosUpdateReason, with the two possible values "observed" and "requested".
And in the comment, instead of "this causes slow scrolling for certain OS/browser combinations (eg. Windows and Firefox)" I'd write:
"Assigning to scrollLeft / scrollTop tells the browser to interrupt any running scroll animations, and to discard any pending async changes to the scroll position that may have happened in the meantime (e.g. on a separate scrolling thread). So we only set the element's scrollLeft/scrollTop when we really require an adjustment of the scroll position."
I found out that the old code altered scrolling behavior even on OS X with touchpad scrolling (i.e. without any scroll animations), just less noticeably so. What happened was that the scroll event fired, so the current scroll position was recorded in the state. But Grid's _setNextState processes state changes asynchronously, so before componentDidUpdate was called with that state, the scroll position could have changed again, and then we'd overwrite the new scroll position with the one that we recorded in the state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion @mstange! What you propose sounds solid. :)
…oducing a React warning message about children without keys
Fixes slow (mouse) wheel scrolling
Resolves #2.
The gist of this fix for the mouse-wheel issue reported against react-virtualized on Firefox is that
componentDidUpdate
no longer adjustsscrollLeft
/scrollTop
in reaction to observed scroll events. This was interrupting the scroll animation.