feat: support scrolling to hierarchical index

[tomivirkki]

Description

This change extends the grid.scrollToIndex(index) API to accept multiple indexes: grid.scrollToIndex(...indexes).

Relates to vaadin/flow-components#1016
Part of vaadin/flow-components#3505

Use example

• grid.scrollToIndex(5): this will still work as before on a non-hierarchical grid/grid with no items expanded = scrolls to the given row number (on the root level)

grid-scroll-to-index.mp4

• grid.scrollToIndex(5, 2, 1): when multiple indexes are passed to the method, each index represents the index number on an individual item hierarchy level. This would scroll to row 1 of the expanded row 2 of the expanded root level row 5.

grid-scroll-to-hierarchical-index.mp4

Under discussion

The first given index represents the row index on the root level only, whereas the old implementation didn't take hierarchy into account but scrolled to the "flat index" disregarding any preceding expanded items.

Some apps may theoretically find this a breaking change since now the first index ignores expanded items and operates on the root level.

On the other hand, this behavior was quite unpredictable and basically the reason for this update in the first place. Some could consider the existing behavior buggy.

Before	After
grid-scroll-to-index-before.mp4	grid-scroll-to-index-after.mp4

Options:

1. Consider this part as a bugfix, since scrolling to an index on a tree grid with expanded items worked unexpectedly before
2. Consider this part being a breaking change and instead, introduce a new API that's used for scrolling to indexes hierarchically.

If we choose option 1, on the Flow Grid API, we could also consider changing the API from public void scrollToIndex(int rowIndex) to public void scrollToIndex(int... indexes). However, while the use of the API would work as before, it might break binary compatibility. A solution would be to introduce the new API public void scrollToIndex(int... indexes) while keeping the old one as well.

Type of change

• Bugfix / Feature

[Ansku]

grid.scrollToIndex(5): this will still work as before, scrolls to the given row number (on the root level)

This is a bit misleading, since it only works as before if nothing is expanded before the given index. Which you do mention lower down, but it's easy to miss if you just skim the ticket. Also, what happens, if you use an index that is larger than your root item count? In my test project you end up with a bunch of empty rows, but it's possible that I didn't remember to exclude all my other changes from my test. This use case would easily happen e.g. with a small TreeGrid where all the items are within the cache and the default display mode is scrolled to bottom.

However, we'd probably recommend using the new API even for non-hierarchical Grid users, so this would effectively deprecate scrollToIndex.

This sounds a bit illogical to me. I would assume that it's more common to use a regular Grid than a TreeGrid, why should the Grid users need to learn the more complicated syntax?

Would it be possible or make any sense to consider this a breaking change but accept it anyway, add the new API without deprecating the old API, and add a configuration option that allows switching the old API to actually still use the old logic if someone finds that really important? And if you don't use the configuration option, it would just call the new API by default.

For the Flow side I'd also like to see scrollToItem(T) that calculates the necessary index array on its own (with the help of HierarchyMapper). It would also be possible to change Flow scrollToIndex and scrollToEnd to calculate the index array, although it might be confusing if Flow scrollToIndex behaves differently than the web component scrollToIndex -- unless of course the web component version would also attempt to do that...

[tomivirkki]

Thanks for the comments @Ansku

This is a bit misleading, since it only works as before if nothing is expanded before the given index

Good point, improved the description

Also, what happens, if you use an index that is larger than your root item count?

This shouldn't be an issue since the given indexes are clamped on each level (index exceeding the level size will be treated as the max index on that level). This case could be confusing, if the navigation continues to the child items of the last item. I'll add a test case for this tomorrow and fix if necessary.

This sounds a bit illogical to me

Good, point. Removed the note.

add a configuration option that allows switching the old API to actually still use the old logic if someone finds that really important

Maybe a feature flag if necessary but seems a bit overkill.

For the Flow side I'd also like to see scrollToItem(T)

Scroll to item has been discussed multiple times before and I believe it has the same issues with hierarchical structure as well #1951

[Ansku]

Scroll to item has been discussed multiple times before and I believe it has the same issues with hierarchical structure as well

It wouldn't work with all data sets ever, but perfect might be the enemy of good here and it could work for any that have reasonably limited size and fixed contents (outside of expanding/collapsing/filtering). Throwing an UnsupportedOperationException for inherently incompatible data sets would be a perfectly valid option in my mind, and as you said, it's been asked for a lot. And obviously that grid.scrollToIndex(dataProvider.getIndexOf(item)); suggested in that thread doesn't really work with hierarchical data, nor does the Viritin solution.

Access to a lot of the hierarchy handling has been made rather limited, so it's not a trivial feature to add to your application without built-in support, especially if you want to take into account what all has been expanded or filtered or whatnot without a lot of extra hassle. You'd preferably need a better method for querying the parent hierarchy than hierarchyMapper.getParentOfItem, though, since that parentIdMap handling is wasteful and unreliable if you haven't iterated over the entire data set (which is presumably why it's ignored by default), but should be doable -- and even trivial if the data provider happens to be a TreeDataProvider.

[tomivirkki]

Throwing an UnsupportedOperationException for inherently incompatible data sets would be a perfectly valid option in my mind

Not sure about adding a public scroll-to method which throws an exception if the Grid isn't configured certain way. This might help some users but I wonder how common it is to use an in-memory data provider with TreeGrid (vs lazy loading).

Maybe there could be a helper API somewhere (outside the Grid) that would provide you the index path to an item given the required parameters. That could accept a richer configuration which would enable it to work with data sets a simple grid.scrollToItem(item) couldn't be made to handle.

Anyways, this discussion is way out of the scope of this feature and can be continued, for example, in the issue I linked or a new feature proposal if you will.

[Ansku]

I wonder how common it is to use an in-memory data provider with TreeGrid (vs lazy loading)

Doesn't need to be in-memory, you only need to be able to query the direct children of each parent to figure out the index for each level. But yes, not really something to implement on the web component side.

[sissbruecker]

As a result of programmatic scrolling there is a very noticable UX issue when the user tries to scroll up. Scrolling up can result in lazy-loading previous expanded items, which then just "pop in" at your current scroll position, effectively moving whatever you were looking at out of the viewport. This isn't a new problem, but might have a higher impact now that scrolling through the hierarchy is supposed to be reliable. I think it's at least worth a discussion if we want to do something about it sooner rather than later.

[tomivirkki]

This isn't a new problem, but might have a higher impact now that scrolling through the hierarchy is supposed to be reliable.

The new scrollToIndex will still always end up with the correct item because it will keep repeating scrolling to the desired index until the grid has finished loading. When it comes to losing the scroll position when items auto-expand on manual scroll (upwards), there's this related issue #1256

[tomivirkki]

One more thing we could consider adding to the PR:

Currently, if the root level has 100 items and the last item (with 100 child items) is expanded, calling grid.scrollToIndex(9999, 9), will scroll to item 99, not 99-9 (which would be unexpected since it's the 10th child item of a parent you weren't actually targeting).

If we want to make the scrollToEnd() method in Flow TreeGrid always scroll to the very last item, we'd probably need to make Infinity work as an exception in this case so that the method could be implemented as getElement().executeJs("this.scrollToIndex(...Array(10).fill(Infinity))").

[czp13]

I feel this improvement will be helpful for some of my customers. The changes as I see are approved, any estimated time when these changes can be merged into the master and then released eventually? Thank you! 🙇

[tomivirkki]

@czp13 Thanks for the feedback. As of now, I'd expect this change to be delivered with Vaadin 24.1

[czp13]

@czp13 Thanks for the feedback. As of now, I'd expect this change to be delivered with Vaadin 24.1
Thanks a lot, @tomivirkki for responding and even giving a closer ETA, the plan sounds very nice to me!
Good luck with it, let me know if I can help anyway! 🤞

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
0.0% Duplication

[sirbris]

Will this be backported to V23? We can't change the requirement to use Java 17 for our customers this year

[web-padawan]

LGTM 📜

[web-padawan]

@sirbris I'm not entitled to make decisions, but there is a chance that we'll consider Vaadin 23.4 later this year.

[vaadin-bot]

This ticket/PR has been released with Vaadin 24.1.0.