Fix issue with StartBringIntoView scrolling for items already visible #3167
Conversation
msft-github-bot
added
the
needs-triage
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ranjeshj
added
area-ItemsRepeater
team-Controls
|
Not exactly sure what made the test fail, but it failed to start scrolling the item into view. Updated the test and added some debug information in case of failure. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| { | ||
| // The anchor is not necessarily laid out yet. Its position should default | ||
| // to zero and the layout origin is expected to change once layout is done. | ||
| // Until then, we need a window that's going to protect the anchor from | ||
| // getting recycled. | ||
|
|
||
| // Also, we only want to mess with the realization rect iff the anchor is not inside it. |
There was a problem hiding this comment.
only [](start = 20, length = 4)
nit: Only and Iff is a bit redundant
There was a problem hiding this comment.
Don't bother fixing this unless you need to make another change
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@chingucoding these test failures look real, do you need anything to address this? |
|
The test is running perfectly fine on my machine. Looking at the very logs, it seems that the scrolling was started, however not finished at all when trying to evaluate how much was scrolled (usually the scrollviewer scrolled between 0 and 50px). It could be that the scrolling is not finished yet, however a vertical offset of 0 is not uncommon, and shouldn't IdleSynchronizer.Wait prevent us from continuing while such operations are still going on? The only immediate "fixes" I can think of are running the ChangeView code multiple times or wait a certain amount of times, but those are just guesses and will definitely leave us with unreliable tests to some degree. |
|
@RBrid do you know if there is anything we are currently doing in the test app to work around the issue Marcel is describing? |
|
The ScrollViewer.ViewChanged event with e.IsIntermediate==False is typically used to detect the end of a view change. See "private void BringElementInNestedScrollViewersIntoView" for an example. |
|
Thanks for the hint @RBrid, updated the test to use the event now. Hopefully that fixes the scrolling issue. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
marcelwgn
deleted the
user/chingucoding/repeater-bringintoview-ofitemsinview
branch
Description
The issue was that the ViewPortManager fiddled with our viewport when there was an anchorelement. In theory this was a good idea, however this makes no sense if the anchor is already in the realization rect since moving that realization rect results in the repeater and layout to think that the item actually is not visible, and the scrollviewer to scroll to the start because of that.
@grokys Thank you for the detailed repo code (that I could simply paste in the app) and the investigations in this issue, that helped a lot!
Motivation and Context
Fixes #1423 and fixes #1425
How Has This Been Tested?
Add new API test.
Screenshots (if appropriate):