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

Fix VirtualizingStackPanel ScrollIntoView #15449

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

grokys
Copy link
Member

@grokys grokys commented Apr 19, 2024

What does the pull request do?

Fixes VirtualizingStackPanel.ScrollIntoView to prevent duplicate items and elements not being correctly scrolled into view with variable sized items.

This problem often shows up when scrolling with the keyboard because keyboard navigation uses ScrollIntoView internally. What was happening was the following:

  1. The item to be scrolled to is realized and placed into the scroll viewer at its estimated position, based on the currently realized elements
  2. The scroll viewer is scrolled to make this "scroll to" element visible
  3. A layout pass is run to realize a page of items at this new scroll position
  4. The first item in the viewport is calculated (this element is called the "anchor")
  5. Containers are realized around the anchor
  6. But the sizes of elements at the new scroll position are different to the elements at the previous scroll position
  7. Meaning that the "scroll to" element doesn't fit in the viewport
  8. The "scroll to" element is left as a orphan element

This PR fixes this by using the "scroll to" element as the "anchor" in step 4; that is, realization is started from the "scroll to" element rather than the first element visible in the viewport. This ensures that the "scroll to" element is guaranteed to be realized and visible. In addition when we're waiting for a viewport update and have a "scroll to" element, we use that element's position to calculate our desired size.

I looked through the open issues for ItemsControl but didn't find an open one that reported this exact problem with a repro, though various comments suggested that they were hitting it.

Take into account the element we're scrolling to when calculating the anchor element for realization.
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0047511-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@grokys grokys marked this pull request as ready for review April 22, 2024 19:39
@grokys
Copy link
Member Author

grokys commented Apr 22, 2024

This PR is now ready for review.

@maxkatz6 maxkatz6 added backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch labels Apr 22, 2024
@maxkatz6 maxkatz6 merged commit 91646f1 into master Apr 22, 2024
11 checks passed
@maxkatz6 maxkatz6 deleted the fixes/virtualizing-scrollintoview branch April 22, 2024 23:05
@grokys grokys added the bug label Apr 23, 2024
maxkatz6 pushed a commit that referenced this pull request Apr 24, 2024
* Add more tests for ScrollIntoView.

* Improve ScrollIntoView.

Take into account the element we're scrolling to when calculating the anchor element for realization.
@maxkatz6 maxkatz6 added backported-11.1.x and removed backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch labels Apr 24, 2024
grokys added a commit that referenced this pull request Jun 11, 2024
* Add more tests for ScrollIntoView.

* Improve ScrollIntoView.

Take into account the element we're scrolling to when calculating the anchor element for realization.
@grokys grokys added backported-11.0.x and removed backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch labels Jun 11, 2024
grokys added a commit that referenced this pull request Jun 12, 2024
* Add more tests for ScrollIntoView.

* Improve ScrollIntoView.

Take into account the element we're scrolling to when calculating the anchor element for realization.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants