Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Skia][WASM] ListView's large scrolling optimization sometimes breaks scrolling and ScrollIntoView #16246

Open
ramezgerges opened this issue Apr 11, 2024 · 1 comment
Labels
area/skia ✏️ Categorizes an issue or PR as relevant to Skia kind/bug Something isn't working platform/wasm 🌐 Categorizes an issue or PR as relevant to the WebAssembly platform project/items 🧾 Categorizes an issue or PR as relevant to items (ItemsControl, ItemsRepeater, ...)

Comments

@ramezgerges
Copy link
Contributor

Current behavior

I was testing #15916 for #15971 and I noticed that ScrollIntoView wasn't scrolling to the correct target sometimes. After a lot more investigations, it turned out that scrolling itself is problematic. I'm not 100% sure if it's two separate problems or not, but removing the large scrolling optimization fixes both problems.

if (isLargeScroll)
{
// In this case, a majority of the materialized items are
// removed, let's clear everything and materialize from the
// new position.
if (this.Log().IsEnabled(LogLevel.Debug))
{
this.Log().LogDebug($"Large scroll Abs(delta):{Abs(delta)} AvailableBreadth:{AvailableBreadth}");
}
// Force the layout update below
unappliedDelta = 1;
// We know that the scroll is outside the available
// breath, so all lines can be recycled
ClearLines(clearContainer: false);
// Set the seed start to use the approximate position of
// the line based on the average line height.
var index = (int)(ScrollOffset / _averageLineHeight);
_dynamicSeedStart = index * _averageLineHeight;
_dynamicSeedIndex = Uno.UI.IndexPath.FromRowSection(index - 1, 0);
}

I've previously fixed it in #13409, but it's still broken (just less often so).

The problem with scrolling (that is separate from ScrollIntoView) is that sometimes large offsets will cause the internal state of the ListView to be corrupted, so you might see items at the wrong place, or not see some items at all, or seeing an item as expected but ListView internally doesn't see it (i.e. ContainerFromIndex(<some_index>) will return null even though the item at that index is actually visible in the UI).

Conceptually, the optimization is not sound. The idea of the optimization is that you can guess the container that will be at the top (if the orientation is vertical) of the viewport after the scroll is done by calculating the average height of the items currently materialized. While this is a good guess, we have no way of knowing how to correct it later. In other words, if you make a big scroll (i.e. large delta), you can't directly jump by a big distance, because you won't know which item to materialize and layout at a certain position, except by incrementally scrolling bit by bit and measuring each item as it materializes.

Expected behavior

No response

How to reproduce it (as minimally and precisely as possible)

There are 2 problems here. I managed to repro the scrolling problem in #16245 (When_ScrollIntoView_Containers_With_Varying_Heights).

For the ScrollIntoView problem, see the ListView_BringIntoView in the same PR and try to scroll to item 10. It will only be partially visible

Workaround

Remove the optimization

Works on UWP/WinUI

None

Environment

No response

NuGet package version(s)

No response

Affected platforms

No response

IDE

No response

IDE version

No response

Relevant plugins

No response

Anything else we need to know?

No response

@ramezgerges ramezgerges added kind/bug Something isn't working triage/untriaged Indicates an issue requires triaging or verification difficulty/tbd Categorizes an issue for which the difficulty level needs to be defined. labels Apr 11, 2024
@ramezgerges
Copy link
Contributor Author

ramezgerges commented Apr 11, 2024

cc. @Xiaoy312 @dr1rrb @jeromelaban

@MartinZikmund MartinZikmund added project/items 🧾 Categorizes an issue or PR as relevant to items (ItemsControl, ItemsRepeater, ...) platform/wasm 🌐 Categorizes an issue or PR as relevant to the WebAssembly platform area/skia ✏️ Categorizes an issue or PR as relevant to Skia and removed triage/untriaged Indicates an issue requires triaging or verification difficulty/tbd Categorizes an issue for which the difficulty level needs to be defined. labels Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/skia ✏️ Categorizes an issue or PR as relevant to Skia kind/bug Something isn't working platform/wasm 🌐 Categorizes an issue or PR as relevant to the WebAssembly platform project/items 🧾 Categorizes an issue or PR as relevant to items (ItemsControl, ItemsRepeater, ...)
Projects
None yet
Development

No branches or pull requests

2 participants