Add scrollToIndex support in WindowScroller

[leoasis]

Hi!

This is my attempt to tackle #540. Since this is still not done I didn't add any tests nor updated the documentation, but will do when we decide this fix is correct and works as expected.

I modified the WindowScroller example to have the same Scroll to index input as the List example, so that we can test this.

The main code change is in WindowScroller now passing an extra field in the children callback that is onChildScroll, which should be wired to the child collection's onScroll property.

The WindowScroller handles that event and tries to scroll the scrollElement according to the children's scrollTop.

Notice however that I ran into a weird timing issue, which is why you see the setTimeout with 0 in the code there. Obviously I would like to know of a way to remove it if possible. When I set the scrollIndex prop in the list, and the onChildScroll callback is called, I try to change the scroll position of the scroll element. This works during that tick, but then a scroll event fires and I'm not sure why the scroll position is updated again to a value that is near the top.

The weird thing is that that doesn't happen if I imperatively do list.scrollToRow(index) (I set a ref in the example and attached it to window.listEl so you can test this, no need to do setTimeout in this case), which is why it makes me thing that some other DOM stuff may be going on, or maybe I don't understand how something works.

I know you said you don't have much time to look at this, but just wanted to create the PR and see if there's anybody (either you or anybody) that could point me to what I'm doing wrong, or what I'm missing. Hopefully I'm close to solving this issue.

If you'd like further information that may help you actually take a look at this without taking much time from you, let me know as well.

[leoasis]

Added a couple of gifs to show what's going on with the implementation:

This example shows that everything works without the setTimeout when using a ref and calling scrollToRow imperatively:

This example shows how it doesn't work without the setTimeout as before, but using the scrollToIndex prop instead of imperatively calling scrollToRow:

This example shows how imperative style still works if I add a setTimeout:

Finally, this example shows how the declarative example with a prop now works with the setTimeout:

Check the console to see the value of window.scrollY after scrolling, and how it behaves with and without the setTimeout. Not sure what is causing the position to change in the synchronous version, that's where I could use some help or knowledge about either RV internals that may be causing this issue, or how scroll threading in the browser may be messing up.

[leoasis]

I've been playing around with this a little more, and it seems the culprit is not inside react-virtualized, but in the example!

The input is causing the scroll position to be screwed up, but if I do a setTimeout in the onChange handler and setState inside the timeout, it all works as expected.

Still haven't delved into the real issue with the input, but probably it's the browser trying to focus it? Maybe it's React's fault?

In any case, that research can be done separately, will try to complete this PR with tests, since now I don't need the timeout anymore (at least not in the library code).

[leoasis]

Alright, pushed some changes, updated the docs and added the corresponding tests. I think the PR should be ready to review 😄

Regarding the issue with the scroll event and the input change, will try to do some research, but I think it's unrelated to this PR (unless you think otherwise)

[leoasis]

Ok did some research about the scroll issue with the input, and it was indeed the focus behavior of the browser.

I made this codepen that shows the same issue: https://codepen.io/anon/pen/mWZwdq
Unless you blur the input or delay the scrollTo call with a setTimeout, the scroll position of the window will keep the input visible.

So, besides contributing something to this library, I learned something!! 😄

[leoasis]

Alright, this is now rebased with latest code in master, and should be ready to be reviewed whenever you have time. Thanks for your time!

[bvaughn]

Hoping to find time soon! :)

[bvaughn]

I feel so bad about not taking the time to review this PR yet. It's just big enough- and I use WindowScroller infrequently enough- that I haven't felt comfortable taking on the risk of a big change.

I just checked out the branch and tested the updated WindowScroller.example. It looks like if I type "1" -> "11" -> "111" -> "1111" (which gets capped to 999) -> "11111" (still 999) the page scrolls back to the top (the first row) instead of the staying at the bottom. Also if I type "2" -> "22" there's no scrolling, even though (on my screen) only about 15 rows are visible. The page doesn't scroll until I type "222". (In my case the page doesn't scroll until I enter a value >= 24 but this is presumably tied to the size of my browser window.)

Not sure what's going on there as I didn't dig in to much, just noted it.

If you're willing to resume work on this- and I would totally understand if you weren't, given the delay- I think we'd need to figure out why the above tests aren't working. I'd also like to see the new WindowScroller.jest tests verify behavior of the scrollToIndex prop more directly rather than the lower-level onScroll test.

Something that may be worth pointing out as an alternative to this approach is the new getOffsetForRow method (in List and Table) or the new getOffsetForCell (in Grid). This method gives the exact (calculated) offset of a specific row. This could be used to scroll the element/window to that offset in application code. Would be similar to what you're doing but might have fewer timing edge-cases?

[leoasis]

Hey! Absolutely no worries for not having time or energy to review this before. I follow your work in open source and I am more than grateful for what you are doing both in React and here. Wish I could be more helpful without requiring more time from you.

Regarding those edge cases you detected, will definitely take a look and see what's the problem there, I think I saw it before but for some reason didn't look deeper to see what was going on, my fault there.

I'd also like to see the new WindowScroller.jest tests verify behavior of the scrollToIndex prop more directly rather than the lower-level onScroll test.

Do you mean you want to have tests setting scrollToIndex prop in addition to these tests? Or just change these tests so that I only act on the index prop?

Something that may be worth pointing out as an alternative to this approach is the new getOffsetForRow method (in List and Table) or the new getOffsetForCell (in Grid). This method gives the exact (calculated) offset of a specific row. This could be used to scroll the element/window to that offset in application code. Would be similar to what you're doing but might have fewer timing edge-cases?

Yeah I think I prefer the imperative approach when scrolling since in general you need that to be a one-time thing. I ended up using something like that in my code, but just wanted to provide support for this use case. Anyway, with this fix it would all be done automatically instead of doing scrollToRow and also scrolling the window. I know it's not that much more, but it's what you'd expect.

[leoasis]

Ok I found some interesting stuff by analyzing the edge cases you mentioned. It turns out that the case where you pressed "2" -> "22" was not working due to the height of the Grid being set to the window.innerHeight (by WindowScroller). This makes the Grid believe that it doesn't need to change its scrollTop because there is enough height to make that item visible, so it doesn't do anything. What it doesn't take into account is the visible part of the Grid in the window.

I think this is something we need to decide if we want to support only for WindowScroller or if it should also be supported by Grid when it's used standalone. The question is what to do if the Grid is partially visible in the window, and it receives a scrollToRow (or its imperative version scrollToPosition) and the row is not currently visible, but rendered because it has enough height. Should the Grid scroll the window in this case? Or is that be something that should live outside the Grid?

[jasongonzales23]

Hey, I am in need of this update and would be willing to help. Let me know what I can do! That said I am going to pull in @leoasis fork and use it in my app to see how it goes.

[bvaughn]

Hey @jasongonzales23,

Testing is great! If you can confirm that it works well for you that would be great.

[jasongonzales23]

Hi @leoasis and @bvaughn, thanks for your effort to fix this feature! I've included a video of the feature working in declarative fashion for me. If you could merge this ASAP, I know I would find it quite helpful as I am trying to ship this feature and would feel better about shipping it once it's in the main branch and packaged. I'd argue it's good to ship soon since the feature is currently broken when using List as a child of WindowScroller. Additionally, it hasn't introduced any regressions and now makes library work as documented. With this fix the code is simple and elegant in keeping with React-Redux patterns: user clicks the point (which conveniently has the index of the corresponding tile in the list below), and the state is updated with that index. When the state is updated that value is passed to the scrollToIndex property and just works. Beautiful. Let me know if there is more I can do to help get this merged!

[jasongonzales23]

hey, hopefully I helped, but I ended up having problems not at all related to your library and have opted for a solution just using plain ol window.scroll() good luck with the PR!

[bvaughn]

@leoasis Thanks a ton for submitting this PR and for your patience as it took me forever to review it. 😄 Merged! Will hopefully deploy a new release with this feature today or tomorrow

[bvaughn]

I just published this feature to NPM as [email protected], tagged next. If you'd like to test it pre-release you can via:

npm install react-virtualized@next

Assuming no problems are reported with the RC, this feature will go out with 9.8.0 sometime this weekend. 😁

[leoasis]

@bvaughn thanks for taking the time! I (think I) understand the effort it takes to maintain this and a lot other projects while also working on something else, so no worries on the timing.

How do you want to move forward with the edge cases you found and I also verified here? Is there anything you would like me to work on to do a follow up PR to fix? Or should we wait until an issue appears and there's enough interest from other users?

[bvaughn]

@leoasis If you're willing to do follow-up PRs to fix, that's great. I don't really use this component so I probably won't encounter those issues directly and so I'm probably not the best one to own them.

Have you smoke tested the RC yet? Does it seem ok?

[leoasis]

Hey @bvaughn sorry for not answering before hehe, was on vacation and then I got back and was super busy. Yeah I've tested this version in our app and works for our needs! We didn't ran into those edge cases you found, but it works.

I'll see if I can find some time to work on those follow ups, and check for related issues in this repo. Anyway, if you find other issues related to this, feel free to mention me so I can take a look if you don't have enough time.

[scf4]

I have a setup like this:

<LargeHeader />
<WindowScrollerList />

and I've come across a couple of bugs:

• scrollToRow(0) never scrolls, even if row 0 isn't in view.

• scrollToRow(anyRow) doesn't work if the entire list is shorter than the browser, even if it isn't in view.

Are these the edge cases you talked about @leoasis ?

[leoasis]

Yeah maybe they could be edge cases related to what I found. Check this #643 (comment) and try to see if you find this is the issue. Or if you can give me a minimal example (in a JSFiddle or somewhere) so I can verify.