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 missing Orientation check in VirtualizingStackPanel #17135

Merged

Conversation

dbriard
Copy link
Contributor

@dbriard dbriard commented Sep 26, 2024

What does the pull request do?

The PR is a fix for a missing orientation check in VirtualizingStackPanel's CalculateMeasureViewport method.
It should solve the following bug: #17125

What is the current behavior?

ListBoxBug.mp4

What is the updated/expected behavior with this PR?

ListBoxOk.mp4

How was the solution implemented (if it's not obvious)?

anchorU = orientation == Orientation.Horizontal ? _scrollToElement.Bounds.Left : _scrollToElement.Bounds.Top;
instead of
anchorU = _scrollToElement.Bounds.Top;

Checklist

Breaking changes

None

Obsoletions / Deprecations

Fixed issues

Fixes #17125

@avaloniaui-bot
Copy link

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

@MrJul MrJul added the bug label Sep 26, 2024
@MrJul
Copy link
Member

MrJul commented Sep 26, 2024

Thanks! Could you please add unit tests?

We already got ScrollIntoView_Correctly_Scrolls_Down_To_A_Page_Of_[Smaller/Larger]_Items for the vertical orientation, that can be used as a base for the horizontal ones.

@dbriard
Copy link
Contributor Author

dbriard commented Sep 26, 2024

Thanks! Could you please add unit tests?

We already got ScrollIntoView_Correctly_Scrolls_Down_To_A_Page_Of_[Smaller/Larger]_Items for the vertical orientation, that can be used as a base for the horizontal ones.

Ok, I'll have a look tomorrow.

…Smaller/Larger]_Items for the horizontal orientation
…age_Of_[Smaller/Larger]_Items for the horizontal orientation"

This reverts commit 9aa7ac2.
@avaloniaui-bot
Copy link

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

@MrJul
Copy link
Member

MrJul commented Sep 27, 2024

Thank you for adding the tests!
The failures seem to be caused by target.Orientation not being set in CreateUnrootedTarget().

@dbriard
Copy link
Contributor Author

dbriard commented Sep 27, 2024

Thank you for adding the tests! The failures seem to be caused by target.Orientation not being set in CreateUnrootedTarget().

Thank you for the clue. I tried to set the SV Template after changing the scroll Orientation, but that do not work better.

Unfortunatly, I have errors building the Avalonia project, so I cannot debug the Tests.

image

@MrJul
Copy link
Member

MrJul commented Sep 27, 2024

Thank you for the clue. I tried to set the SV Template after changing the scroll Orientation, but that do not work better.

Oh, it seems to only miss a target.Orientation = orientation (target being the VirtualizingStackPanel), sorry if that wasn't clear.

Unfortunatly, I have errors building the Avalonia project, so I cannot debug the Tests.

Ensure that the submodules are up-to-date with git submodule update --init --recursive

@dbriard
Copy link
Contributor Author

dbriard commented Oct 7, 2024

git submodule update --init --recursive

Thanks a lot @MrJul
The git command fixed the compilation issue :) and all the tests were green after setting the missing orientation on target!

@avaloniaui-bot
Copy link

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

@@ -0,0 +1,12 @@
{
Copy link
Member

@MrJul MrJul Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you've committed this file by mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, sorry about that, I fixed it.

@avaloniaui-bot
Copy link

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

Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@MrJul MrJul added the backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch label Oct 7, 2024
@MrJul MrJul added this pull request to the merge queue Oct 7, 2024
Merged via the queue into AvaloniaUI:master with commit 20db30d Oct 7, 2024
11 checks passed
maxkatz6 pushed a commit that referenced this pull request Oct 27, 2024
* Fix missing Orientation check in VirtualizingStackPanel > CalculateMeasureViewport

* Added UnitTests ScrollIntoView_Correctly_Scrolls_Right_To_A_Page_Of_[Smaller/Larger]_Items for the horizontal orientation

* Revert "Added UnitTests ScrollIntoView_Correctly_Scrolls_Right_To_A_Page_Of_[Smaller/Larger]_Items for the horizontal orientation"

This reverts commit 9aa7ac2.

* Added VirtualizingStackPanelTests horizontal unittests

* Changed SV orientation before settings Template

* fixed missing orientation on target

* Revert "fixed missing orientation on target"

This reverts commit 8f304d0.

* Fix missing orientation assignment.
@maxkatz6 maxkatz6 removed the backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch label Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants