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

Ensure BringIntoView request is not skipped for direct content of ScrollView #6889

Merged

Conversation

MartinZikmund
Copy link
Contributor

@MartinZikmund MartinZikmund commented Mar 27, 2022

Description

The logic in ScrollPresenter skips BringIntoView request if it is called for its direct content (even though it may be relevant if it has high Margin.

Motivation and Context

Fixes #6888

How Has This Been Tested?

Manually, added API test

Screenshots (if appropriate):

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Mar 27, 2022
@beervoley
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ojhad ojhad requested a review from RBrid March 28, 2022 23:31
@ojhad ojhad added area-Scrolling team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Mar 28, 2022
@ojhad
Copy link
Contributor

ojhad commented Mar 28, 2022

@RBrid could you take a look?

@MartinZikmund
Copy link
Contributor Author

Seems test is failing - I was not able to run tests locally with WinUI solution yet (only under Uno Platform), I will try again

Copy link
Contributor

@RBrid RBrid left a comment

Choose a reason for hiding this comment

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

The ScrollPresenter.cpp changes look good.
Thanks for fixing this!

dev/ScrollPresenter/APITests/ScrollPresenterTests.cs Outdated Show resolved Hide resolved
dev/ScrollPresenter/APITests/ScrollPresenterTests.cs Outdated Show resolved Hide resolved
dev/ScrollPresenter/APITests/ScrollPresenterTests.cs Outdated Show resolved Hide resolved
@MartinZikmund MartinZikmund force-pushed the user/mzikmund/scrollview-bringintoview branch from 476fc1b to 67fe50b Compare April 2, 2022 11:35
@MartinZikmund MartinZikmund requested a review from RBrid April 2, 2022 11:39
@beervoley
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@RBrid RBrid merged commit 7120c9e into microsoft:main Apr 4, 2022
@MartinZikmund MartinZikmund deleted the user/mzikmund/scrollview-bringintoview branch April 4, 2022 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Scrolling team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ScrollViewPresenter BringIntoView handling does not properly handle direct content
4 participants