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

Make VirtualizingStackPanel better handle container size changes #16168

Merged
merged 7 commits into from
Jul 8, 2024

Conversation

grokys
Copy link
Member

@grokys grokys commented Jun 29, 2024

What does the pull request do?

As described in #15712, if a container changes size after layout then glitches occur when scrolling.

To fix this:

  • At the start of a measure pass, check if any realized container's U size has changed; if it has changed then StartU is no longer valid because the average element size will have changed, so mark is as unstable
  • Use the desired size of measured containers instead of the bounds: a layout pass may not had completed on the containers yet, so the bounds may not be up-to-date. It was easier to move the estimation methods out of RealizedStackElements and into VirtualizingStackPanel itself in order to do this, and arguably makes more sense
  • During layout, ensure that if there is a focused element outside the visible viewport, then it is correctly measured and arranged

Fixed issues

Fixes #15712

grokys added 4 commits June 29, 2024 12:46
If any container U size has changed since the last layout pass then `StartU` must be considered unstable as the average container height will have changed.
If the focused element has been moved outside the visible viewport due to a realized container size change, then we need to ensure it's positioned correctly.
@avaloniaui-bot
Copy link

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

@grokys grokys added the bug label Jun 29, 2024
@grokys grokys marked this pull request as draft June 29, 2024 20:28
@grokys
Copy link
Member Author

grokys commented Jun 29, 2024

Marking this as a draft because I've discovered that it's not working quite as intended.

@grokys grokys marked this pull request as ready for review July 1, 2024 07:18
@grokys
Copy link
Member Author

grokys commented Jul 1, 2024

Marked this PR as ready for review again. It isn't perfect but it's better than before and a better fix will be more involved. The problems are twofold:

  1. The problem called out in the above inline comment
  2. When containers are resized the scroll offset isn't updated so it's quite possible that the currently visible/selected item will be scrolled out of view. This may or may not be expected, depending on the use-case. The fix for this is also more involved, but could be remedied in user code.

@Whiletru3
Copy link
Contributor

Hello, I still have issue with the _focusedElement if you zoom out, select one element (to set the _focusedElement) then if you scroll to make it disappear of the viewbox, then zoom in/scroll , you'll have a ghost item representing the _focusedElement.
Like in this gif :
focusedElement virtualization

grokys added 3 commits July 2, 2024 22:44
And revert the expected results for another test to the way they were at the beginning of this PR.
Use the desired size of _measured_ containers instead of the bounds: a layout pass may not had completed on the containers yet, so the bounds may not be up-to-date. Was easier to move the estimation methods out of `RealizedStackElements` and into `VirtualizingStackPanel` itself in order to do this, and arguably makes more sense.
@grokys
Copy link
Member Author

grokys commented Jul 2, 2024

Thanks for testing @Whiletru3 - I've updated the PR to address some of the concerns I had with the original naïve approach and hopefully this should have fixed the issue you were seeing.

@avaloniaui-bot
Copy link

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

@Whiletru3
Copy link
Contributor

Hello @grokys , Using your last changes, I can't reproduce the issue :)
I can now zoom in/out, add, remove, d&d elements. Great !

Now, I just have to tackle the issue of keeping the offset when zoom in or out.

@Whiletru3
Copy link
Contributor

Hello @maxkatz6 and @grokys,
Will it be backported to 11.0.x ? or is there an ETA for the final 11.1.0 ?

Thanks :)

@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 Jul 8, 2024
@maxkatz6
Copy link
Member

maxkatz6 commented Jul 8, 2024

@Whiletru3 this PR will go into 11.1.1. But it might be backported to the 11.0.x, I will lable it for the consideration.

@maxkatz6 maxkatz6 merged commit cc082f9 into master Jul 8, 2024
12 checks passed
@maxkatz6 maxkatz6 deleted the fixes/15712-virtualizing-item-size-change branch July 8, 2024 23:16
grokys added a commit that referenced this pull request Jul 24, 2024
)

* Add a failing test for #15712.

* Validate StartU at the start of a measure pass.

If any container U size has changed since the last layout pass then `StartU` must be considered unstable as the average container height will have changed.

* Correctly position focused element.

If the focused element has been moved outside the visible viewport due to a realized container size change, then we need to ensure it's positioned correctly.

* We can skip check if StartU is already unstable.

* Don't invalidate virt. panels more than necessary.

* Add another virt panel test.

And revert the expected results for another test to the way they were at the beginning of this PR.

* Tweak container size estimation.

Use the desired size of _measured_ containers instead of the bounds: a layout pass may not had completed on the containers yet, so the bounds may not be up-to-date. Was easier to move the estimation methods out of `RealizedStackElements` and into `VirtualizingStackPanel` itself in order to do this, and arguably makes more sense.
grokys added a commit that referenced this pull request Aug 2, 2024
)

* Add a failing test for #15712.

* Validate StartU at the start of a measure pass.

If any container U size has changed since the last layout pass then `StartU` must be considered unstable as the average container height will have changed.

* Correctly position focused element.

If the focused element has been moved outside the visible viewport due to a realized container size change, then we need to ensure it's positioned correctly.

* We can skip check if StartU is already unstable.

* Don't invalidate virt. panels more than necessary.

* Add another virt panel test.

And revert the expected results for another test to the way they were at the beginning of this PR.

* Tweak container size estimation.

Use the desired size of _measured_ containers instead of the bounds: a layout pass may not had completed on the containers yet, so the bounds may not be up-to-date. Was easier to move the estimation methods out of `RealizedStackElements` and into `VirtualizingStackPanel` itself in order to do this, and arguably makes more sense.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch backported-11.1.x bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Listbox virtualized with zoom in/out (using LayoutTransformControl) issues
4 participants