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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Feb 22, 2016
+239 −199
Merged

Fixes slow (mouse) wheel scrolling #110

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
24d0002
Potentially addressing Firefox+Windows slow wheel scrolling
bvaughn Feb 17, 2016
10c89ad
bumped package for rc build
bvaughn Feb 17, 2016
32adaa1
Ammended approach to scroll left/top update to potentially address FF…
bvaughn Feb 17, 2016
575ea26
Merged master
bvaughn Feb 18, 2016
0c4b47a
Refactored scroll-position-updating
bvaughn Feb 18, 2016
f31a18c
Resolves scroll-interruption issue for Safari but at the cost of intr…
bvaughn Feb 21, 2016
d644396
Re-added :key for Grid children but used a 0-based key instead of a r…
bvaughn Feb 22, 2016
c6ab15e
Updated package and CHANGELOG in anticipation of a release
bvaughn Feb 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions dist/react-virtualized.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/react-virtualized.js.map
View file
Open in desktop

Large diffs are not rendered by default.

12 changes: 9 additions & 3 deletions es/Grid/Grid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,13 +282,19 @@ var Grid = (function (_Component) {
var scrollToRow = _props2.scrollToRow;
var width = _props2.width;
var _state = this.state;
var isScrolling = _state.isScrolling;
var scrollLeft = _state.scrollLeft;
var scrollTop = _state.scrollTop;

// Make sure any changes to :scrollLeft or :scrollTop get applied
if (scrollLeft >= 0 && scrollLeft !== prevState.scrollLeft || scrollTop >= 0 && scrollTop !== prevState.scrollTop) {
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) {
Copy link
Owner Author

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 with ScrollSync)
  • Scroll adjustments based on reduced row/column count (to prevent over-scrolling)

Copy link

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.

Copy link
Owner Author

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. :)

if (scrollLeft >= 0 && scrollLeft !== prevState.scrollLeft && scrollLeft !== this.refs.scrollingContainer.scrollLeft) {
this.refs.scrollingContainer.scrollLeft = scrollLeft;
}
if (scrollTop >= 0 && scrollTop !== prevState.scrollTop && scrollTop !== this.refs.scrollingContainer.scrollTop) {
this.refs.scrollingContainer.scrollTop = scrollTop;
}
}

// Update scrollLeft if appropriate
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"description": "React components for efficiently rendering large, scrollable lists and tabular data",
"author": "Brian Vaughn <[email protected]>",
"user": "bvaughn",
"version": "5.1.1",
"version": "5.1.2-rc1",
"homepage": "https://github.com/bvaughn/react-virtualized",
"main": "dist/react-virtualized.js",
"jsnext:main": "es/index.js",
Expand Down
24 changes: 17 additions & 7 deletions source/Grid/Grid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,15 +220,25 @@ export default class Grid extends Component {

componentDidUpdate (prevProps, prevState) {
const { columnsCount, columnWidth, height, rowHeight, rowsCount, scrollToColumn, scrollToRow, width } = this.props
const { scrollLeft, scrollTop } = this.state
const { isScrolling, scrollLeft, scrollTop } = this.state

// Make sure any changes to :scrollLeft or :scrollTop get applied
if (
(scrollLeft >= 0 && scrollLeft !== prevState.scrollLeft) ||
(scrollTop >= 0 && scrollTop !== prevState.scrollTop)
) {
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) {
if (
scrollLeft >= 0 &&
scrollLeft !== prevState.scrollLeft &&
scrollLeft !== this.refs.scrollingContainer.scrollLeft
) {
this.refs.scrollingContainer.scrollLeft = scrollLeft
}
if (
scrollTop >= 0 &&
scrollTop !== prevState.scrollTop &&
scrollTop !== this.refs.scrollingContainer.scrollTop
) {
this.refs.scrollingContainer.scrollTop = scrollTop
}
}

// Update scrollLeft if appropriate
Expand Down