[ComboBox] Regression: Endless items loading when opening the combo-box after selecting an item

vursen commented 2 years ago

Description

Seems to be a regression caused by something in 23.2 since the issue is not reproducible in 23.1.

Every once in a while, after selecting an item and opening the combo-box again, the combo-box gets stuck in the loading state. It must be noted though that the issue only occurs if you are scrolling the dropdown fast before making a selection.

https://user-images.githubusercontent.com/5039436/188892715-7503be39-3cad-41c4-84ef-61026789700b.mov

Expected outcome

I would expect the items to be rendered when opening the combo-box after selecting any item.

Minimal reproducible example

@Route(value = "combo-box")
public class ComboBoxView extends Div {
    public ComboBoxView() {
        ComboBox<String> comboBox = new ComboBox<>();
        List<String> items = new ArrayList<>();
        for (int i = 0; i < 1000; i++) {
            items.add("Item " + i);
        }
        comboBox.setItems(items);
        add(comboBox);
    }
}

Steps to reproduce

Open the dropdown.
Scroll down fast somewhere to the middle of the list.
Select an item.
Open the dropdown again.

Environment

Vaadin version(s): 23.2 and higher OS: Mac OS

Browsers

Issue is not browser related

vursen commented 2 years ago

I'm pretty sure it should not be related to:

Because the issue is not reproducible in 23.1 where both PRs are present.

vursen commented 2 years ago

This issue is related to the fact that ComboBox ensures the first page load and tries to scroll to the selected item (of course as long as focusedIndex is preserved) when opening. Since both things happen almost at the same time (with a requestAnimationFrame delay), the Flow ComboBox connector may end up sending more than one data range request within one round-trip which for some reason makes the server confused so that it either doesn't return anything at all or skips some pages that are apparently assumed to have been sent already:

We might probably want to investigate more deeply why the server-side works that way when it receives several range requests at once. I have a feeling that https://github.com/vaadin/flow-components/issues/3715 may be about the same problem.

vursen commented 2 years ago

On the other hand, there is another issue which is that scrolling to the selected item doesn't always work reliably with the Flow counterpart. In some cases, pageCallbacks are not empty by the moment ComboBox is opening (e.g they contain callbacks to the pages that were triggered to load before item selection), so the connector may try to cancel all the requests, including the request to the first page. This causes a scroller update. The scroller encounters placeholders from the first page and requests the first page again (in a requestAnimationFrame). By this time, ComboBox tries to scroll to the selected item. After a few cancellations, the request to the first page wins.

Reliable scrolling to the selected item is hard to implement when it comes to data providers / lazy loading. We discussed with @rolfsmeds that one of the options can be to disable that functionality altogether when using data providers. Instead, developers will be supposed to use the potential list-selected-items-at-top feature.

vursen commented 1 year ago

the Flow ComboBox connector may end up sending more than one data range request within one round-trip which for some reason makes the server confused so that it either doesn't return anything at all or skips some pages that are apparently assumed to have been sent already:

Further investigation confirmed that the reason the server doesn't respond with items is because it thinks that it has already sent them. For example, when you open ComboBox after selecting an item, the page containing the item has already been loaded from the server's perspective. When opening, currently ComboBox requests the first page and at the same time immediately jumps back to the selected item page which results in a sequence of setRequestedRange sent in one round trip e.g:

{"type":"publishedEventHandler","node":4,"templateEventMethodName":"setRequestedRange","templateEventMethodArgs":[0,50,""],"promise":3}
{"type":"publishedEventHandler","node":4,"templateEventMethodName":"setRequestedRange","templateEventMethodArgs":[950,50,""],"promise":2},

If the first page and selected item page are far apart (e.g. the selected item has the index 950), the connector will unload items during initial scrolling to maintain the client-side range. However, the server won't send items for the selected item page because it treats the above sequence as: we are on the selected item page as we were.

I tried to make the connector call DataCommunicator.reset() when jumping between pages to force the data communicator to resend the range but that breaks other use-cases. The reset command not only causes the server to resend the range but also resets some counters on the server-side.

At the end of the day, I guess the only reasonable thing would be to disable the initial scroll to the selected item when using a data provider. This is what will be done in https://github.com/vaadin/flow-components/issues/3470.

vaadin / flow-components