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

scroll up an infiniteLoader that has scrollToIndex set bounces back to scrollToIndex #998

Closed
dcolens opened this issue Feb 2, 2018 · 16 comments · Fixed by #1154
Closed

Comments

@dcolens
Copy link
Contributor

dcolens commented Feb 2, 2018

repro steps:

  • have an infiniteloader with a list (or grid or ...) with scrollToIndex set to something like 10000 => the list shows starting at the corresponding line
  • scroll back up
  • after a few lines the list jumps back to 10000 (only a few lines for Firefox, a bit more for Chrome) => this is only triggered when fresh data is loaded.

some investigation:

It looks very similar to #595:

recomputeGridSize is called by InfiniteLoader once its new rows load. This in turn sets the _recomputeScrollTopFlag flag to true.

_recomputeScrollTopFlag is set to true in recomputeGridSize as follow:

this._recomputeScrollTopFlag = scrollToRow >= 0 && rowIndex <= scrollToRow;

I'm suspecting that this code is only valid when scrolling down but not up. If this is correct, recomputeGridSize should use this.state.scrollDirectionVertical to compute _recomputeScrollTopFlag and the same issue likely applies to horizontal scrolling too.

@bvaughn
Copy link
Owner

bvaughn commented Feb 3, 2018

Interested in contributing a new unit test that captures this behavior?

Better yet, a new test and a fix? 😄

@dcolens
Copy link
Contributor Author

dcolens commented Feb 4, 2018 via email

@bvaughn
Copy link
Owner

bvaughn commented Feb 4, 2018

Look at the existing tests. There are many. Should be plenty of examples of testing the individual things your bug report mentions.

Comment out your fix. Write a test that fails. Uncomment your fix and make sure the test passes. 🙂

You're obviously welcome to submit just the fix, but it has a better chance of getting merged (and would really help us out) is there was a test too.

dcolens pushed a commit to dcolens/react-virtualized that referenced this issue Feb 4, 2018
@dcolens dcolens mentioned this issue Feb 4, 2018
wuweiweiwu pushed a commit that referenced this issue May 8, 2018
* fix for #998

* add patch for scrollToColumn too

* prettier
wuweiweiwu added a commit that referenced this issue May 8, 2018
This reverts commit 6226f56.
wuweiweiwu added a commit that referenced this issue May 8, 2018
@dcolens
Copy link
Contributor Author

dcolens commented May 23, 2018

I was asked to write a fix, I did it.
I was asked to write tests, I did it.

I think I played fair. Not very motivating to contribute more.

@wuweiweiwu
Copy link
Contributor

@dcolens The reason it was reverted was because the test failed after Grid was updated to use gDSFP

Its on my list of TODOs. However, if you wan't to take some time to fix the test that is failing that'll be welcome :)

@mengdage
Copy link
Contributor

mengdage commented Jun 25, 2018

@wuweiweiwu Hi, it seems like people are too busy to fix this bug. I create #1154 with fixed tests.

@wuweiweiwu
Copy link
Contributor

@mengdage awesome! Ill take a look tonight!

wuweiweiwu pushed a commit that referenced this issue Jun 27, 2018
errendir added a commit to IdeaFlowCo/react-virtualized that referenced this issue Oct 24, 2018
* bvaughn/master: (54 commits)
  Update version and changelog for 9.21.0 release (bvaughn#1252)
  chore: update lockfile
  Update ci badge (bvaughn#1227)
  Allow users to override default table row styles (bvaughn#1175)
  Add onColumnClick to Table (bvaughn#1207)
  remove unused variable in Masonry.example.js (bvaughn#1218)
  Fix Table aria attributes (bvaughn#1208)
  Fix typo in CellMeasurer.DynamicHeightTableColumn.example.js (bvaughn#1190)
  Update usingAutoSizer.md (bvaughn#1186)
  Add an extra check for an e.target.className.indexOf function (bvaughn#1210)
  Fix broken Slack badge image (bvaughn#1205)
  docs(CellMeasurer): fix `import` statement (bvaughn#1187)
  Added new friend (bvaughn#1197)
  Fix createMultiSort bug (bvaughn#1051)
  adding new usecase example and fix some typos (bvaughn#1168)
  Updating version to 9.20.1
  Update changelog for the 9.20.1 release (bvaughn#1167)
  Prevent early debounceScrollEndedCallback when there is a slow render (bvaughn#1141)
  removing sideEffects (bvaughn#1163)
  fix for bvaughn#998 with test cases (bvaughn#1154)
  ...
@marsch
Copy link

marsch commented Jun 28, 2019

the merge was reverted - still having the issue - any workarounds known here?

@wazcodez
Copy link

Facing the same issue. Any potential workarounds?

@ryanwilliamquinn
Copy link

Same issue for me.

@jayeshpp
Copy link

Same issue for me too.
InfiniteLoader + WindowScroller + Grid

@bobalazek
Copy link

Experiencing the same issue. Why was the merge reverted?

@AleshaOleg
Copy link

For me downgrading to 9.20.0 worked

@wangbigmountain
Copy link

wangbigmountain commented Mar 11, 2020

Still have this issue InifiteLoader + List in version 9.21.0. @bvaughn is there a walkarounds?

@automatication
Copy link

@bvaughn Facing the same issue with InfinitLoader and Grid with 9.21.2, it would be amazing if this issue could be fixed.

@gigatesseract
Copy link

gigatesseract commented May 24, 2020

A tiny workaround

I was facing the same issue. I had to render 9k rows in total with a seek functionality for a particular row of interest. After seeking, scrolling up caused no issues, whereas when scrolling down, it automatically jumped to the already seeked row. (I am passing a state parameter to scrollToIndex prop which I am changing so that might rerender the list, but I could not explain why the issue does not come when scrolling up)
I solved the problem by resetting the scrollToIndex once scroll is complete.

this.setState({scrolled_index: index  + 20},  //this centers the row for my use case.    
          () =>{    
            this.setState({scrolled_index: -1})    
          });

I am passing this.state.scrolled_index to the List component.
I am currently working in a project with deadlines. Will get a plunker up and running as soon as possible. Will love to see a fix for this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet