RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v3]

[Florian Kirmaier]

On Mon, 11 Jul 2022 09:11:58 GMT, Johan Vos <jvos at openjdk.org> wrote:

>> In order to fix the issues reported in JDK-8089589, the VirtualFlow now uses the real sizes of the elements that are displayed. This allows for a more natural way of scrolling through lists that contain items of very different sizes.
>> The algorithm that is used, does not calculate the size of each individual cell upfront, as that would be a major performance overhead (especially for large lists). Instead, we estimate the total size and the size of the individual cells based on the real measured size of a (growing) number of cells. 
>> 
>> There are a number of key variables that depend on this estimated size:
>> * position
>> * aboluteOffset
>> * currentIndex
>> 
>> As a consequence, when the estimated size is changing, the absoluteOffset may change which may lead to a new currentIndex. If this happens during a layout phase, or a "complex" operation (e.g. scroll to an item and select it), this may lead to visually unexpected artifacts. While the rendering becomes more "correct" when the estimated size is improving, it is more important that we do not create visual confusion. 
>> 
>> The difficulty is that there are a large number of ways to manipulate the VirtualFlow, and different entry points may have different expectations about which variables should be kept constant (or only changed gradually).
>> 
>> In this PR, we focus on consistency in the scrollTo method, where we want to keep the top-cell constant. In case the estimated size is improved during the scrollTo execution, or the next layoutChildren invocation, this implies keeping the result of computeCurrentIndex() constant so that after scrolling to a cell and selecting it, the selected cell is indeed the one that is scrolled to.
>> 
>> This PR contains a number of tests for this scrollTo behaviour, that should also be considered as regression test in case we want to add more invariants later.
>
> Johan Vos has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add link to issue about ignored test
>   Reverse expected and actual args in some tests.

I've retested this version of the PR, and I can confirm, that it fixes the bug in the real-world application! So no regression happened when moving to the new PR! So great work!

Not sure whether the first two tests are still necessary.
The tests I wrote, basically used these test as a template and tried to generalize/parametrize it. But I guess more tests don't do any harm.

There are 2 issues, we shouldn't forget about **after** this PR:

1.) In my original tests I had a snippet which is removed in this version. I still think the test is correct but it fails. So we should investigate it.

// After clicking, the cells shouldn't move
listView.getSelectionModel().select(scrollToIndex);
listView.requestLayout();
Toolkit.getToolkit().firePulse();
assertEquals("Upper cell shouldn't move after changing heights and clicking", previousLayoutY, scrollToCell.getLayoutY(), 1.);


2.)
In the previous [PR](https://github.com/openjdk/jfx/pull/712)  we reported issues when scrolling after jumping to an index - including a video. As discussed with Johan we would like to investigate this further after this PR.

-------------

PR: https://git.openjdk.org/jfx/pull/808