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

[johanvos]

When the size of a ListCell is changed and a scrollTo method is invoked without having a layout calculation in between, the old (wrong) size is used to calculcate the total estimate. This happens e.g. when the size is changed in the updateItem method.
This PR will immediately resize the cell and sets the new value in the cache containing the cellsizes.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/712/head:pull/712
$ git checkout pull/712

Update a local copy of the PR:
$ git checkout pull/712
$ git pull https://git.openjdk.org/jfx pull/712/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 712

View PR using the GUI difftool:
$ git pr show -t 712

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/712.diff

[bridgekeeper]

👋 Welcome back jvos! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge]

Webrevs

• 07: Full - Incremental (585119de)
• 06: Full - Incremental (67b351ac)
• 05: Full - Incremental (c7d722d3)
• 04: Full - Incremental (aa08ba26)
• 03: Full - Incremental (2b1b4bdc)
• 02: Full - Incremental (a931ba75)
• 01: Full - Incremental (05935373)
• 00: Full (d1ab8afb)

[FlorianKirmaier]

I've just tested it with the TestProgram of the ticket, and it is now less wrong, but still not correct.
There is a small empty are, of the size of about 5 pixels.

This happens with the 1, 2, and 3rd button, but not with the 4th button. The order of the clicks doesn't seem to matter.

[FlorianKirmaier]

I've added a new button to the test programm. The 5th button now literally jumps into the "Nothingness"

.

[johanvos]

There was a problem in ListView, in which cells are shifted down in case there are empty cells being rendered while there are leading cells available.
I also added a testcase for this that fails before and succeeds after.

[kevinrushforth]

It looks like there are some failing unit tests now.

[johanvos]

It looks like there are some failing unit tests now.

I fixed them.

[FlorianKirmaier]

I've added another testbutton to the testclass.
When we jump to somewhere in the middle, the selected index is not at the top, but somewhere else.
Depending on the application, this can be quite a bit.
In the affected application - it sometimes still jumps to places closer to the wrong cell compared to the right cell.

The cell #2 should be at the top, but isnt.

[FlorianKirmaier]

After some investigating, it also doesn't properly scroll to the Bottom.
I've added a minimal modified button to the TestApplication.

Is this conceptional doable, to get properly working scrollTo implementation for varying cell sizes?

[eduardsdv]

I don't believe that it is possible to position the content and scrollbar exactly, if the VirtualFlow estimates the size of the cells.
For that case we need an external cell size provider, that I, as an application developer, can provide to the e.g. ListView.

I thought of something like this. The existing estimation logic can then be implemented as a default cell size provider.

interface CellSizeProvider {

 /**
  * Returns the size of all cells.
  * IMPORTANT: Be carefully by implementing this method. It can drastically degrade the performance of a component.
  */
 double getTotalSize();

 /**
  * Returns the size of the cell for an item given by its index. 
  * IMPORTANT: Be carefully by implementing this method. It can drastically degrade the performance of a component.
  */
 double getCellSize(int);

 /**
  * The callback which is set by the VirtualFlow. It allows to calculate the cell size for an item given by its index. 
  * IMPORTANT: This callback returns the exact value, but it may be slow.
  */
 void setCellSizeCalculator(Function<Integer, Double> cellSizeCalculator);

}

[johanvos]

I agree with that observation. The mathematical perfect situation would be to pre-calculate the height of all items, so that the scrolbar position can be exact, and the content placing can be exact as well. That would be at the price of a major performance overhead for large lists. For small lists, this overhead is more acceptable.

I agree that this is something that could be rather application-defined instead of having heuristics in the ListView.

As a historical note, before https://bugs.openjdk.java.net/browse/JDK-8089589 was fixed, all items were considered of equali height for the position of the scrollbar. In the current case, if that precondition holds, the estimated size will be the real size, so in that case there is no need to calculate the height of every item before getting the correct total size.
This is again something the application knows best -- even with non-fixed cell heights, the expected variations might be small enough and if they are randomly spread, the estimate will soon be "good enough".

[FlorianKirmaier]

@johanvos
As requested, we've made a unit test, which tests this bug.
It's based on your test and our original test class. It can be added to the ListViewTest.
You can find it, in the JDK ticket.

Btw, adding the cells incrementally seems to make a difference - which is why the new test class tests both cases.

It accepts various inputs of cell sizes - and should work with any inputs.
It should catch all the cases from the original test class.
Because it works with all possible inputs - one could even make a random test-cases generator to make sure it works in every case.

It basically only works well, when the sizes are only 20s - in most other cases it fails.

[martinfrancois]

I noticed this bug as well, but in my case even when the CellHeight stays the same (so I'm not sure if it's the same or something else)... A minimal repro example:

public class ApplicationUI extends VBox {
    private final ObservableList<String> elements = FXCollections.observableArrayList();
    public ApplicationUI() {
        Button button = new Button("Add New Element");
        ListView<String> listView = new ListView<>(elements);
        setVgrow(listView, Priority.ALWAYS);
        getChildren().addAll(listView, button);
        button.setOnAction(event -> {
            elements.add(String.valueOf(elements.size()));
        });
        elements.addListener((ListChangeListener<String>) c -> {
            while (c.next()) {
                listView.scrollTo(c.getFrom());
            }
        });
    }
}

Click the button until there is a scroll bar, it always seems to scroll down to the second last element in the list, instead of the last.

Interestingly, changing the line from listView.scrollTo(c.getFrom()); to listView.scrollTo(c.getFrom()-1); or listView.scrollTo(c.getFrom()+1); results in the same behavior, however changing it to listView.scrollTo(c.getFrom()-2); does scroll correctly to the last item in the list?

Just thought I would share this in case this would help find the issue.

[johanvos]

The later reported issues were due to the fact that the estimatedSize got updated during the layoutChildren call. That makes things much more complicated and leads to discrepancies. I disabled the updates to the estimated size during the layoutChildren call, so that all operations that are happening in that call are using a consistent value of the estimated size. We now pass all the tests added by @FlorianKirmaier on the JBS issue . Those tests are great regression tests.

[FlorianKirmaier]

@johanvos
Hi Johan,
Great to see all these unit tests getting green so fast!
Unfortunately, the provided Test Application still behaves very buggy - I would even say it still is equally buggy, which is quite a suprise.
Nevertheless, this is a great step forward.

So there must be something the tests are missing. I haven't found out yet what.
Maybe they will behave different, if they are executed as a system test, with a real window?
I will keep you posted, when we've updated the tests.

[FlorianKirmaier]

Hi @johanvos,

I've updated the TestClass once more. You can find it on the ticket.

There are 2 cases that seem to not work properly yet.

1.) If the elements are very big, the tests seem to fail - but it only happens if the scene is layouted again.

2.) On Click, the element gets selected and the VirtualFlow "jumps around".
I've adapted the tests to simulate the click with the selectionModel.
I've also added another assert, to check whether our cell is visible - which seems to be not always the case.

Both cases were also reproducible with the original test application.

Btw, can I somehow become an official reviewer for this PR?

[johanvos]

To give some background: the reason for this PR is to fix (real or perceived) regression after https://bugs.openjdk.java.net/browse/JDK-8089589 was fixed.
The original problem was this: in case a ListView contains items with different heights, the scrollbar can show weird jumps as it considers all cells to have the same height.
The "easy" solution to this would be to pre-calculate the height of all cells, but that would be a potential performance issue.

The approach taken in JDK-8089589 is to gradually improve the information about the different sizes of the cells, and it fixes the weird jumps (at the very least, it avoids jumping upwards when scrolling down and vice versa).

However, this gradual improvements can cause other visual disturbances. Initially, we improved the parameters whenever a layoutChildren was invoked. However, this leads to undesired effects in case layoutChildren is invoked again after a specific goal is asked (e.g. scroll to a cell with a specific index). A consequence of the gradual improvement is that the algorithm realizes the first cell need to move upwards or downwards a bit, but that will destroy the state where a cell should be positioned exactly at a given position.
The latest commit fixes this, by modifying the parameters (absoluteOffset and position) in such a way that they lead to improvements, yet keep the visible cells intact.
This broke a number of tests, as there are cases where keeping these cells intact is not what we want. When there is no cached estimate, or when the absoluteOffset has a bogus value, we do not want to maintain the previous situation.

I'm moving this PR to draft until I have more confirmation about the (perceived) stability.

[FlorianKirmaier]

Thank you for the update!

We've tested it, and we made good progress.
In our Application, we often resize the Cells, after the content is loaded.
We've polished the unit test for the following 3 cases.
(the changes are in this comment)

1.) Not jumping to the last cell.
Sometimes, when the last cell is very big, the ListView jumps to one cell above it.
This was supposed to be caught by the previous tests, but due to a bug in the tests, it was missed.
With this new definition of "shouldScrollToBottom" the bug is caught by the unit test:

boolean isLastIndex = scrollToIndex == heights.length - 1;
boolean shouldScrollToBottom = (sizeBelow < viewportLength) || (isLastIndex && lastCellSize >= viewportLength);

2.) Jumps on height changes
When the heights of the cells changes, "everything jumps around".
Would it be possible to have the invariant, that the topmost visible element shouldn't move?
Or alternatively: The selected element should be stable if it is visible.
If this would be implementable, this would make the VirtualFlow behave much more "calm".

3.) Jumps on click after height changes
When the heights have changed, and an element is selected, then the position jumps around again.
This is clearly a bug, because selecting a visible element shouldn't have the effect that cells "fly around".
If they should move due to the height changes, this should happen when the height is changed, not when a cell is selected.

For 2 and 3, I have some test-code, which can be added to the end of the method "verifyListViewScrollTo":

    // Additional Tests:
    double previousLayoutY = scrollToCell.getLayoutY();
    System.out.println("previousLayoutY: " + previousLayoutY);
    if(previousLayoutY == 0) {
        System.out.println("ADDITIONAL CHECKS");
        // Upper cell shouldn't move after heights are changed
        List<Integer> alternateHeights = Arrays.stream(heights).map(x -> x + 250).collect(Collectors.toList());
        listView.getItems().setAll(alternateHeights);
        listView.requestLayout();
        Toolkit.getToolkit().firePulse();
        assertEquals("Upper cell shouldn't move after changing heights", previousLayoutY, scrollToCell.getLayoutY(), 1.);

        // 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.);
    }

[christianheilmann]

Hi Johan,

thanks for the update of the PR. From the "scrollTo" perspective, everything seems to work now. Big thanks.
We coming close to the final solution
We found now one issue with scrolling down where it jumps to wrong position.
After a scrollTo method call, the scrolling behaves very weird. It then goes up, when scrolling down. It feels like, after the first scrolling it jump up have a cell - which is half the screen, if the cells are big.
Even after resizing and relayouting, the positions in our applications seems stable and reasonable.
After a small scrolling, they tend to jump.
Maybe there is a minor fix missing?
I attached you a video showing you the bug in the scroll behavior:
list-view-scrolling.m4v look at #18 when we scroll down it jumps up to #17. With this you maybe able to find the bug.

Hope you find the last remaining glitches.

Thanks

list-view-scrolling.mp4

[kevinrushforth]

I guess this should be closed in favor of #808 ?

[johanvos]

Closed in favor of #808