Fixed [Windows] KeepScrollOffset Behavior Not Working as Expected in CarouselView - #29800
Conversation
|
Hey there @@Shalini-Ashokan! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
dotnet-policy-service
Bot
added
the
partner/syncfusion
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the Windows implementation of ItemsUpdatingScrollMode.KeepScrollOffset in CarouselView by explicitly preserving the current item index when the collection changes and updates the handler logic accordingly. It also adds a UI test and host app sample for the issue.
- Added a flag in the Windows handler to maintain the scroll offset on collection changes.
- Updated
OnCollectionItemsSourceChangedandCarouselScrolledto use the new flag. - Introduced a shared UI test and a HostApp page to validate the fix (currently guarded on Windows).
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue29421.cs | Added a UI test for KeepScrollOffset behavior (guarded) |
| src/Controls/tests/TestCases.HostApp/Issues/Issue29421.cs | Added HostApp sample page with CarouselView and Add button |
| src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs | Added _isCollectionChanged flag and updated scroll logic |
Comments suppressed due to low confidence (1)
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue29421.cs:1
- Since this PR resolves the KeepScrollOffset issue on Windows, remove the
#if TEST_FAILS_ON_WINDOWSguard so the test runs on Windows and validates the fix.
#if TEST_FAILS_ON_WINDOWS // Related issue for windows: https://github.com/dotnet/maui/issues/29245
| _isCollectionChanged = true; | ||
| } | ||
|
|
||
| if (e.Action == NotifyCollectionChangedAction.Remove) |
There was a problem hiding this comment.
[nitpick] Consider checking ItemsView.ItemsUpdatingScrollMode == KeepScrollOffset before setting _isCollectionChanged so the flag is only used when needed and avoids unnecessary state changes.
| _isCollectionChanged = true; | |
| } | |
| if (e.Action == NotifyCollectionChangedAction.Remove) | |
| if (ItemsView.ItemsUpdatingScrollMode == KeepScrollOffset) | |
| { | |
| _isCollectionChanged = true; | |
| } | |
| } | |
| if (e.Action == NotifyCollectionChangedAction.Remove && ItemsView.ItemsUpdatingScrollMode == KeepScrollOffset) |
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
PureWeen
added
platform/windows
area-controls-collectionview
|
/rebase |
5a4e52b to
cd79344
Compare
5ef9af8 to
32e4ac0
Compare
MauiBot
added
s/agent-review-incomplete
s/agent-reviewed
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Could you please review the AI's summary?
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 29800Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 29800" |
@kubaflo, I addressed the ai concerns. |
MauiBot
added
s/agent-review-incomplete
and removed
s/agent-changes-requested
|
/azp run maui-pr-uitests , maui-pr-devicetests |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/review -b feature/enhanced-reviewer -p windows |
github-actions
Bot
added
the
s/agent-review-in-progress
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 1 findings
See inline comments for details.
| } | ||
|
|
||
| var position = e.CenterItemIndex; | ||
| if (_isCollectionChanged && ItemsView.ItemsUpdatingScrollMode == ItemsUpdatingScrollMode.KeepScrollOffset) |
There was a problem hiding this comment.
[major] Windows CarouselView lifecycle/correctness — _isCollectionChanged is a persistent bool that is set during Add/Remove but is only cleared when a later CarouselScrolled event runs while ItemsUpdatingScrollMode == KeepScrollOffset. If no scroll event is raised immediately after the collection mutation, or if the mutation happens under another scroll mode and the mode is later changed to KeepScrollOffset, the stale flag causes the next real user scroll to be overwritten with ItemsView.Position and swallowed. Scope this state to the specific KeepScrollOffset mutation and clear it deterministically, e.g. only set it when the mode is KeepScrollOffset and reset it on disconnect / after the relevant scroll callback is consumed.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@Shalini-Ashokan — new AI review results are available based on this last commit:
872b326. To request a fresh review after new comments or commits, comment/review rerun.
Review Sessions — click to expand
Gate — Test Before & After Fix
Gate Result: ❌ FAILED
Platform: WINDOWS · Base: main · Merge base: bb4e7040
🩺 Test does not reproduce the bug — ran the same in both states (PASS without fix, PASS with fix). The repro test is not exercising the issue. Strengthen the test before reviewing the fix.
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
🖥️ Issue29421 Issue29421 |
❌ PASS — 593s | ❌ FAIL — 491s |
🔴 Without fix — 🖥️ Issue29421: PASS ❌ · 593s
Determining projects to restore...
Restored D:\a\1\s\src\Controls\src\Core\Controls.Core.csproj (in 50.03 sec).
Restored D:\a\1\s\src\Controls\Maps\src\Controls.Maps.csproj (in 49.9 sec).
Restored D:\a\1\s\src\Controls\Foldable\src\Controls.Foldable.csproj (in 365 ms).
Restored D:\a\1\s\src\BlazorWebView\src\Maui\Microsoft.AspNetCore.Components.WebView.Maui.csproj (in 4.05 sec).
Restored D:\a\1\s\src\Graphics\src\Graphics.Win2D\Graphics.Win2D.csproj (in 25 ms).
Restored D:\a\1\s\src\Essentials\src\Essentials.csproj (in 18 ms).
Restored D:\a\1\s\src\Core\src\Core.csproj (in 66 ms).
Restored D:\a\1\s\src\Core\maps\src\Maps.csproj (in 23 ms).
Restored D:\a\1\s\src\Graphics\src\Graphics\Graphics.csproj (in 3.95 sec).
Restored D:\a\1\s\src\Controls\src\Xaml\Controls.Xaml.csproj (in 27 ms).
Restored D:\a\1\s\src\Controls\tests\TestCases.HostApp\Controls.TestCases.HostApp.csproj (in 718 ms).
3 of 14 projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Graphics -> D:\a\1\s\artifacts\bin\Graphics\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Essentials -> D:\a\1\s\artifacts\bin\Essentials\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Graphics.Win2D -> D:\a\1\s\artifacts\bin\Graphics.Win2D\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Graphics.Win2D.WinUI.Desktop.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Core -> D:\a\1\s\artifacts\bin\Core\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.dll
Controls.BindingSourceGen -> D:\a\1\s\artifacts\bin\Controls.BindingSourceGen\Debug\netstandard2.0\Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Controls.Core -> D:\a\1\s\artifacts\bin\Controls.Core\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.dll
Maps -> D:\a\1\s\artifacts\bin\Maps\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Maps.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Controls.Xaml -> D:\a\1\s\artifacts\bin\Controls.Xaml\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.Xaml.dll
Microsoft.AspNetCore.Components.WebView.Maui -> D:\a\1\s\artifacts\bin\Microsoft.AspNetCore.Components.WebView.Maui\Debug\net10.0-windows10.0.19041.0\Microsoft.AspNetCore.Components.WebView.Maui.dll
Controls.Foldable -> D:\a\1\s\artifacts\bin\Controls.Foldable\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.Foldable.dll
Controls.Maps -> D:\a\1\s\artifacts\bin\Controls.Maps\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.Maps.dll
Controls.TestCases.HostApp -> D:\a\1\s\artifacts\bin\Controls.TestCases.HostApp\Debug\net10.0-windows10.0.19041.0\win-x64\Controls.TestCases.HostApp.dll
Build succeeded.
0 Warning(s)
0 Error(s)
Time Elapsed 00:06:13.93
Determining projects to restore...
Restored D:\a\1\s\src\TestUtils\src\VisualTestUtils\VisualTestUtils.csproj (in 862 ms).
Restored D:\a\1\s\src\TestUtils\src\UITest.NUnit\UITest.NUnit.csproj (in 1.59 sec).
Restored D:\a\1\s\src\TestUtils\src\UITest.Core\UITest.Core.csproj (in 18 ms).
Restored D:\a\1\s\src\TestUtils\src\UITest.Appium\UITest.Appium.csproj (in 1.89 sec).
Restored D:\a\1\s\src\TestUtils\src\VisualTestUtils.MagickNet\VisualTestUtils.MagickNet.csproj (in 4.7 sec).
Restored D:\a\1\s\src\TestUtils\src\UITest.Analyzers\UITest.Analyzers.csproj (in 7.19 sec).
Restored D:\a\1\s\src\Controls\tests\CustomAttributes\Controls.CustomAttributes.csproj (in 6 ms).
Restored D:\a\1\s\src\Controls\tests\TestCases.WinUI.Tests\Controls.TestCases.WinUI.Tests.csproj (in 8.81 sec).
7 of 15 projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Graphics -> D:\a\1\s\artifacts\bin\Graphics\Debug\net10.0\Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Essentials -> D:\a\1\s\artifacts\bin\Essentials\Debug\net10.0\Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Controls.CustomAttributes -> D:\a\1\s\artifacts\bin\Controls.CustomAttributes\Debug\net10.0\Controls.CustomAttributes.dll
Core -> D:\a\1\s\artifacts\bin\Core\Debug\net10.0\Microsoft.Maui.dll
Controls.Core.Design -> D:\a\1\s\artifacts\bin\Controls.Core.Design\Debug\net472\Microsoft.Maui.Controls.DesignTools.dll
Controls.BindingSourceGen -> D:\a\1\s\artifacts\bin\Controls.BindingSourceGen\Debug\netstandard2.0\Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Controls.Core -> D:\a\1\s\artifacts\bin\Controls.Core\Debug\net10.0\Microsoft.Maui.Controls.dll
UITest.Core -> D:\a\1\s\artifacts\bin\UITest.Core\Debug\net10.0\UITest.Core.dll
UITest.Appium -> D:\a\1\s\artifacts\bin\UITest.Appium\Debug\net10.0\UITest.Appium.dll
VisualTestUtils -> D:\a\1\s\artifacts\bin\VisualTestUtils\Debug\netstandard2.0\VisualTestUtils.dll
UITest.NUnit -> D:\a\1\s\artifacts\bin\UITest.NUnit\Debug\net10.0\UITest.NUnit.dll
VisualTestUtils.MagickNet -> D:\a\1\s\artifacts\bin\VisualTestUtils.MagickNet\Debug\netstandard2.0\VisualTestUtils.MagickNet.dll
UITest.Analyzers -> D:\a\1\s\artifacts\bin\UITest.Analyzers\Debug\netstandard2.0\UITest.Analyzers.dll
Controls.TestCases.WinUI.Tests -> D:\a\1\s\artifacts\bin\Controls.TestCases.WinUI.Tests\Debug\net10.0\Controls.TestCases.WinUI.Tests.dll
Test run for D:\a\1\s\artifacts\bin\Controls.TestCases.WinUI.Tests\Debug\net10.0\Controls.TestCases.WinUI.Tests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (x64)
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
NUnit Adapter 4.5.0.0: Test execution started
Running selected tests in D:\a\1\s\artifacts\bin\Controls.TestCases.WinUI.Tests\Debug\net10.0\Controls.TestCases.WinUI.Tests.dll
NUnit3TestExecutor discovered 1 of 1 NUnit test cases using Current Discovery mode, Non-Explicit run
>>>>> 6/16/2026 2:38:52 PM FixtureSetup for Issue29421(Windows)
>>>>> 6/16/2026 2:39:01 PM VerifyCarouselViewKeepScrollOffsetAdd Start
>>>>> 6/16/2026 2:39:04 PM VerifyCarouselViewKeepScrollOffsetAdd Stop
Passed VerifyCarouselViewKeepScrollOffsetAdd [2 s]
NUnit Adapter 4.5.0.0: Test execution complete
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.12] Discovering: Controls.TestCases.WinUI.Tests
[xUnit.net 00:00:00.35] Discovered: Controls.TestCases.WinUI.Tests
Results File: D:\a\1\s\CustomAgentLogsTmp\UITests\TestResults\Issue29421.trx
Test Run Successful.
Total tests: 1
Passed: 1
Total time: 35.1593 Seconds
>>> TRX_RESULT_FILE: D:\a\1\s\CustomAgentLogsTmp\UITests\TestResults\Issue29421.trx
🟢 With fix — 🖥️ Issue29421: FAIL ❌ · 491s
Determining projects to restore...
All projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Graphics -> D:\a\1\s\artifacts\bin\Graphics\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Graphics.Win2D -> D:\a\1\s\artifacts\bin\Graphics.Win2D\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Graphics.Win2D.WinUI.Desktop.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Essentials -> D:\a\1\s\artifacts\bin\Essentials\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Core -> D:\a\1\s\artifacts\bin\Core\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.dll
Controls.BindingSourceGen -> D:\a\1\s\artifacts\bin\Controls.BindingSourceGen\Debug\netstandard2.0\Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Controls.Core -> D:\a\1\s\artifacts\bin\Controls.Core\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Microsoft.AspNetCore.Components.WebView.Maui -> D:\a\1\s\artifacts\bin\Microsoft.AspNetCore.Components.WebView.Maui\Debug\net10.0-windows10.0.19041.0\Microsoft.AspNetCore.Components.WebView.Maui.dll
Controls.Xaml -> D:\a\1\s\artifacts\bin\Controls.Xaml\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.Xaml.dll
Controls.Foldable -> D:\a\1\s\artifacts\bin\Controls.Foldable\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.Foldable.dll
Maps -> D:\a\1\s\artifacts\bin\Maps\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Maps.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Controls.Maps -> D:\a\1\s\artifacts\bin\Controls.Maps\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.Maps.dll
Controls.TestCases.HostApp -> D:\a\1\s\artifacts\bin\Controls.TestCases.HostApp\Debug\net10.0-windows10.0.19041.0\win-x64\Controls.TestCases.HostApp.dll
Build succeeded.
0 Warning(s)
0 Error(s)
Time Elapsed 00:06:07.25
Determining projects to restore...
All projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Graphics -> D:\a\1\s\artifacts\bin\Graphics\Debug\net10.0\Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Essentials -> D:\a\1\s\artifacts\bin\Essentials\Debug\net10.0\Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Core -> D:\a\1\s\artifacts\bin\Core\Debug\net10.0\Microsoft.Maui.dll
Controls.CustomAttributes -> D:\a\1\s\artifacts\bin\Controls.CustomAttributes\Debug\net10.0\Controls.CustomAttributes.dll
Controls.Core.Design -> D:\a\1\s\artifacts\bin\Controls.Core.Design\Debug\net472\Microsoft.Maui.Controls.DesignTools.dll
Controls.BindingSourceGen -> D:\a\1\s\artifacts\bin\Controls.BindingSourceGen\Debug\netstandard2.0\Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14389899
Controls.Core -> D:\a\1\s\artifacts\bin\Controls.Core\Debug\net10.0\Microsoft.Maui.Controls.dll
UITest.Core -> D:\a\1\s\artifacts\bin\UITest.Core\Debug\net10.0\UITest.Core.dll
UITest.NUnit -> D:\a\1\s\artifacts\bin\UITest.NUnit\Debug\net10.0\UITest.NUnit.dll
VisualTestUtils -> D:\a\1\s\artifacts\bin\VisualTestUtils\Debug\netstandard2.0\VisualTestUtils.dll
UITest.Appium -> D:\a\1\s\artifacts\bin\UITest.Appium\Debug\net10.0\UITest.Appium.dll
VisualTestUtils.MagickNet -> D:\a\1\s\artifacts\bin\VisualTestUtils.MagickNet\Debug\netstandard2.0\VisualTestUtils.MagickNet.dll
UITest.Analyzers -> D:\a\1\s\artifacts\bin\UITest.Analyzers\Debug\netstandard2.0\UITest.Analyzers.dll
Controls.TestCases.WinUI.Tests -> D:\a\1\s\artifacts\bin\Controls.TestCases.WinUI.Tests\Debug\net10.0\Controls.TestCases.WinUI.Tests.dll
Test run for D:\a\1\s\artifacts\bin\Controls.TestCases.WinUI.Tests\Debug\net10.0\Controls.TestCases.WinUI.Tests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (x64)
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
NUnit Adapter 4.5.0.0: Test execution started
Running selected tests in D:\a\1\s\artifacts\bin\Controls.TestCases.WinUI.Tests\Debug\net10.0\Controls.TestCases.WinUI.Tests.dll
NUnit3TestExecutor discovered 1 of 1 NUnit test cases using Current Discovery mode, Non-Explicit run
>>>>> 6/16/2026 2:47:01 PM FixtureSetup for Issue29421(Windows)
>>>>> 6/16/2026 2:47:10 PM VerifyCarouselViewKeepScrollOffsetAdd Start
>>>>> 6/16/2026 2:47:14 PM VerifyCarouselViewKeepScrollOffsetAdd Stop
>>>>> 6/16/2026 2:47:14 PM Log types:
Failed VerifyCarouselViewKeepScrollOffsetAdd [4 s]
Error Message:
VisualTestUtils.VisualTestFailedException :
Snapshot different than baseline: VerifyCarouselViewKeepScrollOffsetAdd.png (0.56% difference)
If the correct baseline has changed (this isn't a a bug), then update the baseline image.
See test attachment or download the build artifacts to get the new snapshot file.
More info: https://aka.ms/visual-test-workflow
Stack Trace:
at VisualTestUtils.VisualRegressionTester.Fail(String message) in /_/src/TestUtils/src/VisualTestUtils/VisualRegressionTester.cs:line 162
at VisualTestUtils.VisualRegressionTester.VerifyMatchesSnapshot(String name, ImageSnapshot actualImage, String environmentName, ITestContext testContext) in /_/src/TestUtils/src/VisualTestUtils/VisualRegressionTester.cs:line 123
at Microsoft.Maui.TestCases.Tests.UITest.<VerifyScreenshot>g__Verify|13_0(String name, <>c__DisplayClass13_0&) in /_/src/Controls/tests/TestCases.Shared.Tests/UITest.cs:line 477
at Microsoft.Maui.TestCases.Tests.UITest.VerifyScreenshot(String name, Nullable`1 retryDelay, Nullable`1 retryTimeout, Int32 cropLeft, Int32 cropRight, Int32 cropTop, Int32 cropBottom, Double tolerance, Boolean includeTitleBar) in /_/src/Controls/tests/TestCases.Shared.Tests/UITest.cs:line 309
at Microsoft.Maui.TestCases.Tests.Issues.Issue29421.VerifyCarouselViewKeepScrollOffsetAdd() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue29421.cs:line 21
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
NUnit Adapter 4.5.0.0: Test execution complete
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.13] Discovering: Controls.TestCases.WinUI.Tests
[xUnit.net 00:00:00.35] Discovered: Controls.TestCases.WinUI.Tests
Results File: D:\a\1\s\CustomAgentLogsTmp\UITests\TestResults\Issue29421.trx
Total tests: 1
Failed: 1
Test Run Failed.
Total time: 29.5225 Seconds
>>> TRX_RESULT_FILE: D:\a\1\s\CustomAgentLogsTmp\UITests\TestResults\Issue29421.trx
⚠️ Failure Details
- ❌ Issue29421 PASSED without fix (should fail) — tests don't catch the bug
- ❌ Issue29421 FAILED with fix (should pass)
VerifyCarouselViewKeepScrollOffsetAdd [4 s]VisualTestUtils.VisualTestFailedException : Snapshot different than baseline: VerifyCarouselViewKeepScrollOffsetAdd.png (0.56% difference) If the correct baseline has changed (this isn't a a bug), ...
📁 Fix files reverted (2 files)
eng/pipelines/ci-copilot.ymlsrc/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
UI Tests — CarouselView,CollectionView
Detected UI test categories: CarouselView,CollectionView
❌ Deep UI tests — 355 passed, 1 failed across 2 categories on platform-pool agent (replaces in-process counts above).
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
CarouselView |
37/38 (1 ❌) | — |
CollectionView |
318/329 ✓ | — |
❌ CarouselView — 1 failed test
VerifyCarouselViewWithIsSwipeDisable
OpenQA.Selenium.WebDriverArgumentException : $Contact moves cannot take place before contact down: {contactId}
at OpenQA.Selenium.WebDriver.UnpackAndThrowOnError(Response errorResponse, String commandToExecute)
at OpenQA.Selenium.WebDriver.ExecuteAsync(String driverCommandToExecute, Dictionary`2 parameters)
at OpenQA.Selenium.WebDriver.Execute(String driverCommandToExecute, Dictionary`2 parameters)
at OpenQA.Selenium.Appium.AppiumDriver.Execute(String driverCommandToExecute, Dictionary`2 parameters)
at OpenQA.Selenium.Appium.AppiumDriver.PerformActions(IList`1 actionSequenceList)
at UITest.Appium.AppiumScrollActions.PerformActions(AppiumDriver driver, Int32 startX, Int32 startY, Int32 endX, Int32 endY, ScrollStrategy strategy, Int32 swipeSpeed, String elementId) in /_/src/TestUtils/src/UITest.Appium/Actions/AppiumScrollActions.cs:line 342
at UITest.Appium.AppiumScrollActions.ScrollToRight(AppiumDriver driver, AppiumElement element, ScrollStrategy strategy, Doubl
...
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs)
Pre-Flight — Context & Validation
Issue: #29421 - KeepScrollOffset Not Working as Expected in CarouselView
PR: #29800 - Windows CarouselView KeepScrollOffset collection-change fix
Platforms Affected: Windows
Files Changed: 1 implementation, 7 test/snapshot
Key Findings
- PR modifies the active Windows
Items/CarouselView handler; Items2 has no Windows implementation. - The PR's approach uses a handler-level
_isCollectionChangedflag to override the next scroll-derived position with the existingItemsView.PositionforKeepScrollOffsetafter Add/Remove. - Gate was already completed by the caller and failed; this phase did not re-run gate verification.
- GitHub CLI is unauthenticated and public API became rate-limited, so PR comments/reviews/CI were only partially available.
Code Review Summary
Verdict: NEEDS_CHANGES
Confidence: low
Errors: 1 | Warnings: 1 | Suggestions: 0
Key code review findings:
- Error
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue29421.cs:21: regression test does not prove the fix because the gate result says tests did not behave as expected. - Warning
src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs:538:_isCollectionChangedcan become stale because it is set outside theKeepScrollOffsetmode but only cleared inside that mode's scroll callback branch.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #29800 | Track a collection-change flag and ignore the next Scrolled center index for KeepScrollOffset, preserving ItemsView.Position. |
❌ FAILED (Gate) | CarouselViewHandler.Windows.cs, UI test/snapshots |
Original PR; gate already failed and code review found stale-flag risk. |
Code Review — Deep Analysis
Code Review — PR #29800
Independent Assessment
What this changes: Windows CarouselViewHandler adds _isCollectionChanged state so the next Scrolled callback after an Add/Remove under KeepScrollOffset preserves the existing ItemsView.Position instead of accepting WinUI's reported center index. It also adds a UI screenshot regression test for issue 29421.
Inferred motivation: Prevent Windows CarouselView from changing logical Position/CurrentItem when the underlying collection changes while ItemsUpdatingScrollMode.KeepScrollOffset is selected.
Reconciliation with PR Narrative
Author claims: GitHub PR narrative was unavailable because gh is unauthenticated and public PR API rate-limited. Available local evidence links the change to "KeepScrollOffset Not Working as Expected in CarouselView" / issue 29421.
Agreement/disagreement: The code intent matches the local issue/test naming. However, available local verification shows the new test does not validate the fix.
Prior Review Reconciliation
| Prior Error Finding | Source | Status | Evidence |
|---|---|---|---|
| GitHub prior review surfaces unavailable | gh pr view, pull comments API, issue comments API |
Tool unavailable | gh returned "please run: gh auth login"; public API became rate-limited. |
No prior Error findings were found in available local artifacts.
Blast Radius Assessment
- Runs for all instances: Yes, all Windows
CarouselViewinstances using observable item sources subscribe inCreateCollectionViewSource; the new flag only affects Add/Remove + scroll callbacks. - Startup impact: No.
- Static/shared state: No static state; new state is per handler, but it can persist across later callbacks if not cleared.
CI Status
- Required-check result: tool-unavailable / undetermined
- Classification: undetermined
- Action taken:
gh pr checks 29800 --repo dotnet/maui --requiredfailed due missing GitHub CLI authentication; confidence capped low. User-provided gate result says gate failed: tests did not behave as expected.
Findings
Error — Regression test does not prove the fix
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue29421.cs:21
The added test only taps AddButton and calls VerifyScreenshot(). The user-provided gate result says tests did not behave as expected. The test should be strengthened to fail without the fix and pass with the fix, ideally by asserting the expected visible item/position/current item after insertion rather than relying only on the current screenshot baseline.
Warning — _isCollectionChanged can become stale
src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs:538
_isCollectionChanged is set for Add and all Remove actions, regardless of the current ItemsUpdatingScrollMode, but it is only cleared inside CarouselScrolled when the mode is KeepScrollOffset. If a collection change occurs under KeepItemsInView or KeepLastItemInView, or if no scroll callback follows the collection change, the flag can survive and later suppress an unrelated user scroll after the mode changes to KeepScrollOffset. Scope the flag to KeepScrollOffset updates or ensure it is cleared when the collection-change handling no longer needs it.
Failure-Mode Probing
- Collection changes while mode is not
KeepScrollOffset:_isCollectionChangedmay be set but not cleared by the next scroll callback because the guard is mode-specific. - No
Scrolledcallback after Add/Remove: the flag remains true until a future scroll, which may be user-initiated and unrelated. - Disconnect/reconnect after a pending flag: subscriptions are removed, but the per-handler flag is not reset; a reused handler could carry stale state.
- Null/default startup values: no new startup path or static initializer; existing event unsubscription mostly limits callback-after-disconnect risk.
Verdict: NEEDS_CHANGES
Confidence: low. The handler change is Windows platform UI plumbing, CI required-check state is unavailable, and the user-provided gate result says the tests did not behave as expected.
Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Direct non-animated ScrollTo after collection change for KeepScrollOffset. |
❌ FAIL | 1 file | Build passed; UI screenshot mismatch 0.56%. |
| 2 | try-fix | Dispatcher-deferred logical state reconciliation after WinUI collection/layout side effects. | 1 file | UI test passed; expert review found transient wrong events and disconnect crash risk. | |
| 3 | try-fix | Reuse _isInternalPositionUpdate to suppress scroll while collection handler is active. |
1 file | UI test passed; expert review found delayed WinUI scroll callbacks can arrive after the flag clears. | |
| 4 | try-fix | One-shot pending retained position consumed by CarouselScrolled, with dispatcher cleanup. |
1 file | UI test passed; expert review found same-position cleanup race across rapid operations. | |
| 5 | try-fix | Token-scoped pending retained position with token-matched cleanup. | 1 file | UI test passed; expert review found cleanup can still run before deferred WinUI scroll callback. | |
| PR | PR #29800 | Persistent _isCollectionChanged flag suppresses next scroll-derived position. |
❌ FAILED (Gate) | 1 implementation + tests | Gate failed; stale flag risk identified in pre-flight review. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| gpt-5.5 | 1 | Yes | Avoid immediate native scroll; use deferred logical reconciliation. |
| gpt-5.5 | 2 | Yes | Suppress CarouselScrolled only while _isInternalPositionUpdate is true. |
| gpt-5.5 | 3 | Yes | Use a scoped one-shot pending expected position consumed in CarouselScrolled. |
| gpt-5.5 | 4 | Yes | Add an operation token so older cleanup cannot clear newer pending state. |
| gpt-5.5 | 5 | No | Remaining alternatives require a reliable WinUI post-layout/scroll-complete signal or stronger regression oracle; trivial variations of pending state/dispatcher cleanup remain vulnerable. |
Exhausted: Yes
Selected Fix: None — no alternative both passed tests and cleared expert review. Candidate 5 is the strongest explored direction but still has a major timing concern, so it is not demonstrably better than the PR's fix.
try-fix-1
Approach
Direct native reposition: after the collection-change handler computes the retained carousel position, update CurrentItem/Position and call non-animated ItemsView.ScrollTo(carouselPosition, Center) only for KeepScrollOffset.
This differs from PR #29800 because it does not suppress a later Scrolled callback with persistent handler state; it tries to keep WinUI's native view aligned immediately after the mutation.
Diff
diff --git a/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs b/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
index 6b08e60c65..f8331f5b90 100644
--- a/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
+++ b/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
@@ -612,6 +612,11 @@ namespace Microsoft.Maui.Controls.Handlers.Items
SetCarouselViewCurrentItem(carouselPosition);
SetCarouselViewPosition(carouselPosition);
+
+ if (ItemsView.ItemsUpdatingScrollMode == ItemsUpdatingScrollMode.KeepScrollOffset)
+ {
+ ItemsView.ScrollTo(carouselPosition, position: ScrollToPosition.Center, animate: false);
+ }
}
finally
{Test Results
❌ Fail — pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform windows -TestFilter "FullyQualifiedName~Issue29421" exited non-zero.
Failure excerpt:
Snapshot different than baseline: VerifyCarouselViewKeepScrollOffsetAdd.png (0.56% difference)
Total tests: 1
Failed: 1
Failure Analysis
The direct ScrollTo approach builds successfully but does not satisfy the current screenshot regression. It likely executes too early relative to WinUI item realization, or the screenshot baseline/test itself is not a reliable oracle (matching the pre-flight gate failure). This teaches the next attempt to avoid relying on immediate native scrolling from within collection-change handling.
Expert Review
reviewer-findings.json is []; no concrete major finding was produced for this final diff.
try-fix-2
Approach
Dispatcher-deferred logical reconciliation: after collection change, compute and apply the retained logical position, then enqueue a one-shot DispatcherQueue callback that revalidates mode/position and reapplies CurrentItem/Position after WinUI collection/layout side effects.
This differs from PR #29800 by avoiding the persistent _isCollectionChanged flag and differs from try-fix-1 by not forcing native ScrollTo.
Diff
diff --git a/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs b/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
index 6b08e60c65..a585234c3f 100644
--- a/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
+++ b/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
@@ -610,8 +610,32 @@ namespace Microsoft.Maui.Controls.Handlers.Items
carouselPosition = 0;
}
+ var shouldReconcileKeepScrollOffset =
+ ItemsView.ItemsUpdatingScrollMode == ItemsUpdatingScrollMode.KeepScrollOffset
+ && (e.Action == NotifyCollectionChangedAction.Add || e.Action == NotifyCollectionChangedAction.Remove)
+ && IsValidPosition(carouselPosition);
+
SetCarouselViewCurrentItem(carouselPosition);
SetCarouselViewPosition(carouselPosition);
+
+ if (shouldReconcileKeepScrollOffset)
+ {
+ var expectedPosition = carouselPosition;
+ ListViewBase?.DispatcherQueue?.TryEnqueue(() =>
+ {
+ if (ItemsView == null || ListViewBase == null)
+ return;
+
+ if (ItemsView.ItemsUpdatingScrollMode != ItemsUpdatingScrollMode.KeepScrollOffset)
+ return;
+
+ if (!IsValidPosition(expectedPosition))
+ return;
+
+ SetCarouselViewCurrentItem(expectedPosition);
+ SetCarouselViewPosition(expectedPosition);
+ });
+ }
}
finally
{Test Results
✅ Pass — pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform windows -TestFilter "FullyQualifiedName~Issue29421" passed 1/1 UI tests.
Passing excerpt:
Passed VerifyCarouselViewKeepScrollOffsetAdd [2 s]
Test Run Successful.
Total tests: 1
Passed: 1
Failure Analysis
Although the test passed, expert review found this candidate is not safe enough to select: a wrong PositionChanged/CurrentItemChanged event can still be raised before the deferred correction, and the deferred callback can touch typed ItemsView after disconnect. This makes it less robust than required despite the green test.
Expert Review
reviewer-findings.json contains 2 findings: one major transient-event correctness issue and one critical dispatcher lifecycle issue.
try-fix-3
Approach
Synchronous collection-update scroll suppression: in CarouselScrolled, return during KeepScrollOffset if the existing _isInternalPositionUpdate flag is true.
This differs from PR #29800 because it reuses a scoped flag already set only during OnCollectionItemsSourceChanged, rather than adding a persistent _isCollectionChanged field. It also avoids try-fix-1's forced scroll and try-fix-2's deferred dispatcher callback.
Diff
diff --git a/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs b/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
index 6b08e60c65..283c72f0f5 100644
--- a/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
+++ b/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
@@ -535,6 +535,11 @@ namespace Microsoft.Maui.Controls.Handlers.Items
var position = e.CenterItemIndex;
+ if (_isInternalPositionUpdate && ItemsView.ItemsUpdatingScrollMode == ItemsUpdatingScrollMode.KeepScrollOffset)
+ {
+ return;
+ }
+
if (position == -1)
{
return;Test Results
✅ Pass — pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform windows -TestFilter "FullyQualifiedName~Issue29421" passed 1/1 UI tests.
Passing excerpt:
Passed VerifyCarouselViewKeepScrollOffsetAdd [1 s]
Test Run Successful.
Total tests: 1
Passed: 1
Failure Analysis
The UI test passed, but expert review found a meaningful timing gap: if WinUI posts the problematic scroll event after OnCollectionItemsSourceChanged returns, _isInternalPositionUpdate is already false and the drift can still occur. This candidate is simpler than the PR but not demonstrably robust enough.
Expert Review
reviewer-findings.json contains 1 major finding about deferred WinUI scroll callbacks.
try-fix-4
Approach
Scoped one-shot pending position: set _pendingKeepScrollOffsetPosition only for valid KeepScrollOffset Add/Remove operations, consume it in CarouselScrolled before trusting WinUI's shifted e.CenterItemIndex, clear it on disconnect, and schedule cleanup if no scroll arrives.
Diff
diff --git a/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs b/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
index 6b08e60c65..f05bd11a72 100644
--- a/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
+++ b/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
@@ -30,6 +30,7 @@ namespace Microsoft.Maui.Controls.Handlers.Items
bool _isCarouselViewReady;
bool _isInternalPositionUpdate;
int _gotoPosition = -1;
+ int _pendingKeepScrollOffsetPosition = -1;
NotifyCollectionChangedEventHandler _collectionChanged;
readonly WeakNotifyCollectionChangedProxy _proxy = new();
@@ -52,6 +53,8 @@ namespace Microsoft.Maui.Controls.Handlers.Items
protected override void DisconnectHandler(ListViewBase platformView)
{
+ _pendingKeepScrollOffsetPosition = -1;
+
if (ItemsView != null)
ItemsView.Scrolled -= CarouselScrolled;
@@ -535,6 +538,18 @@ namespace Microsoft.Maui.Controls.Handlers.Items
var position = e.CenterItemIndex;
+ if (_pendingKeepScrollOffsetPosition != -1)
+ {
+ var pendingPosition = _pendingKeepScrollOffsetPosition;
+ _pendingKeepScrollOffsetPosition = -1;
+
+ if (ItemsView.ItemsUpdatingScrollMode == ItemsUpdatingScrollMode.KeepScrollOffset && IsValidPosition(pendingPosition))
+ {
+ SetCarouselViewPosition(pendingPosition);
+ return;
+ }
+ }
+
if (position == -1)
{
return;
@@ -610,6 +625,22 @@ namespace Microsoft.Maui.Controls.Handlers.Items
carouselPosition = 0;
}
+ var shouldKeepScrollOffset =
+ ItemsView.ItemsUpdatingScrollMode == ItemsUpdatingScrollMode.KeepScrollOffset
+ && (e.Action == NotifyCollectionChangedAction.Add || e.Action == NotifyCollectionChangedAction.Remove)
+ && IsValidPosition(carouselPosition);
+
+ if (shouldKeepScrollOffset)
+ {
+ _pendingKeepScrollOffsetPosition = carouselPosition;
+ var expectedPosition = carouselPosition;
+ ListViewBase?.DispatcherQueue?.TryEnqueue(() =>
+ {
+ if (_pendingKeepScrollOffsetPosition == expectedPosition)
+ _pendingKeepScrollOffsetPosition = -1;
+ });
+ }
+
SetCarouselViewCurrentItem(carouselPosition);
SetCarouselViewPosition(carouselPosition);
}Test Results
✅ Pass — pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform windows -TestFilter "FullyQualifiedName~Issue29421" passed 1/1 UI tests.
Failure Analysis
This is the strongest direction so far, but expert review found a race: cleanup matched only by retained position can clear a later operation that retained the same position. The next iteration adds an operation token.
Expert Review
reviewer-findings.json contains 1 major cleanup-race finding.
try-fix-5
Approach
Token-scoped pending position: set _pendingKeepScrollOffsetPosition and increment _pendingKeepScrollOffsetUpdate only for valid KeepScrollOffset Add/Remove operations. CarouselScrolled consumes the pending expected position before accepting WinUI's shifted e.CenterItemIndex. Dispatcher cleanup clears only if its captured operation token is still current, and disconnect clears/increments the token.
This differs from PR #29800 because it tracks the expected position rather than a stale-prone bool, scopes state only to KeepScrollOffset, and protects cleanup from older operations clearing newer pending state. It differs from try-fix-4 by adding the operation token.
Diff
diff --git a/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs b/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
index 6b08e60c65..2c399a7714 100644
--- a/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
+++ b/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
@@ -30,6 +30,8 @@ namespace Microsoft.Maui.Controls.Handlers.Items
bool _isCarouselViewReady;
bool _isInternalPositionUpdate;
int _gotoPosition = -1;
+ int _pendingKeepScrollOffsetPosition = -1;
+ int _pendingKeepScrollOffsetUpdate;
NotifyCollectionChangedEventHandler _collectionChanged;
readonly WeakNotifyCollectionChangedProxy _proxy = new();
@@ -52,6 +54,9 @@ namespace Microsoft.Maui.Controls.Handlers.Items
protected override void DisconnectHandler(ListViewBase platformView)
{
+ _pendingKeepScrollOffsetPosition = -1;
+ _pendingKeepScrollOffsetUpdate++;
+
if (ItemsView != null)
ItemsView.Scrolled -= CarouselScrolled;
@@ -535,6 +540,18 @@ namespace Microsoft.Maui.Controls.Handlers.Items
var position = e.CenterItemIndex;
+ if (_pendingKeepScrollOffsetPosition != -1)
+ {
+ var pendingPosition = _pendingKeepScrollOffsetPosition;
+ _pendingKeepScrollOffsetPosition = -1;
+
+ if (ItemsView.ItemsUpdatingScrollMode == ItemsUpdatingScrollMode.KeepScrollOffset && IsValidPosition(pendingPosition))
+ {
+ SetCarouselViewPosition(pendingPosition);
+ return;
+ }
+ }
+
if (position == -1)
{
return;
@@ -610,6 +627,22 @@ namespace Microsoft.Maui.Controls.Handlers.Items
carouselPosition = 0;
}
+ var shouldKeepScrollOffset =
+ ItemsView.ItemsUpdatingScrollMode == ItemsUpdatingScrollMode.KeepScrollOffset
+ && (e.Action == NotifyCollectionChangedAction.Add || e.Action == NotifyCollectionChangedAction.Remove)
+ && IsValidPosition(carouselPosition);
+
+ if (shouldKeepScrollOffset)
+ {
+ var update = ++_pendingKeepScrollOffsetUpdate;
+ _pendingKeepScrollOffsetPosition = carouselPosition;
+ ListViewBase?.DispatcherQueue?.TryEnqueue(() =>
+ {
+ if (_pendingKeepScrollOffsetUpdate == update)
+ _pendingKeepScrollOffsetPosition = -1;
+ });
+ }
+
SetCarouselViewCurrentItem(carouselPosition);
SetCarouselViewPosition(carouselPosition);
}Test Results
✅ Pass — pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform windows -TestFilter "FullyQualifiedName~Issue29421" passed 1/1 UI tests.
Passing excerpt:
Passed VerifyCarouselViewKeepScrollOffsetAdd [2 s]
Test Run Successful.
Total tests: 1
Passed: 1
Failure Analysis
The token removes try-fix-4's same-position cleanup race, but expert review still found a broader timing flaw: cleanup may run before WinUI emits the deferred scroll callback. That means the original drift can still reproduce if the pending state is cleared too early. A robust version would need a concrete platform signal for the relevant scroll/layout pass rather than a blind dispatcher cleanup.
Expert Review
reviewer-findings.json contains 1 major finding about cleanup running before the deferred WinUI scroll callback.
Report — Final Recommendation
Comparative Report — PR #29800
Decision
Winner: try-fix-5
try-fix-5 is the strongest available candidate because it passed the Windows issue UI test and has the most narrowly scoped state model among the passing alternatives: it tracks an expected retained position and protects cleanup with an operation token. It still has a major expert-review timing concern, so this is a least-risk selection rather than a clean approval. Per the required ranking rule, candidates with failed regression evidence (pr, pr-plus-reviewer, try-fix-1) are ranked below candidates that passed.
Candidate ranking
| Rank | Candidate | Regression result | Expert/code-review result | Assessment |
|---|---|---|---|---|
| 1 | try-fix-5 |
Passed | Major timing concern: dispatcher cleanup can run before the deferred WinUI scroll callback | Best of the passing candidates. Token-scoped pending state fixes try-fix-4's same-position race and is more targeted than the raw PR bool, but still needs a stronger platform signal to be fully robust. |
| 2 | try-fix-4 |
Passed | Major cleanup race when two operations retain the same position | Good direction, but older cleanup can clear newer pending state. Superseded by try-fix-5. |
| 3 | try-fix-3 |
Passed | Major timing gap if WinUI scroll callback arrives after _isInternalPositionUpdate clears |
Simple and low surface area, but only covers synchronous callbacks during collection handling. |
| 4 | try-fix-2 |
Passed | Major transient event correctness issue and critical disconnect/lifecycle risk | Deferred reconciliation passes the visual test but can emit wrong logical events before correction and can run after disconnect. |
| 5 | pr-plus-reviewer |
Not re-run; treated as inheriting PR gate failure | Fixes the stale-flag reviewer finding by scoping suppression to KeepScrollOffset and clearing on disconnect |
Safer than raw PR, but it preserves the same Add + KeepScrollOffset path that failed the gate, so it cannot outrank passing candidates. |
| 6 | try-fix-1 |
Failed | No concrete review finding | Direct ScrollTo failed the screenshot regression, likely because it fires too early relative to WinUI item realization. |
| 7 | pr |
Failed gate | Major stale-flag finding; test does not prove fix | Raw submitted PR failed the required gate and leaves stale handler state that can suppress later legitimate scrolls. |
Notes on pr and pr-plus-reviewer
The raw PR uses _isCollectionChanged as persistent state. The expert reviewer found this stale-prone because it is set during Add/Remove and only cleared in the KeepScrollOffset branch of CarouselScrolled.
The pr-plus-reviewer sandbox candidate scopes that flag to Add/Remove operations already in KeepScrollOffset and clears it on disconnect. That resolves the specific stale-state feedback, but because the original failed gate was exactly an Add operation with KeepScrollOffset, this adjustment does not provide evidence that the PR behavior now passes.
Final recommendation
Use try-fix-5 as the winning candidate for manifest purposes because it is the highest-ranked candidate with passing regression evidence. Before merge, it should still be hardened against the remaining WinUI timing concern, ideally by tying pending-state cleanup to a concrete post-layout/scroll-complete signal instead of a blind dispatcher enqueue.
Future Action — alternative fix proposed (try-fix-5)
Automated review — alternative fix proposed
The expert-reviewer evaluation compared the PR fix against automatically generated candidates and selected try-fix-5 as the strongest fix.
Why: try-fix-5 won because it passed the Windows regression test and is the strongest reviewed passing alternative: token-scoped pending state avoids the raw PR's stale bool and fixes try-fix-4's same-position cleanup race. It still has a timing concern, but failed-regression candidates must rank below candidates that passed.
Please consider applying the candidate diff below (or use it as guidance). Once you push an update, this workflow will re-trigger and re-evaluate.
Candidate diff (try-fix-5)
diff --git a/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs b/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
index 6b08e60c65..2c399a7714 100644
--- a/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
+++ b/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
@@ -30,6 +30,8 @@ namespace Microsoft.Maui.Controls.Handlers.Items
bool _isCarouselViewReady;
bool _isInternalPositionUpdate;
int _gotoPosition = -1;
+ int _pendingKeepScrollOffsetPosition = -1;
+ int _pendingKeepScrollOffsetUpdate;
NotifyCollectionChangedEventHandler _collectionChanged;
readonly WeakNotifyCollectionChangedProxy _proxy = new();
@@ -52,6 +54,9 @@ namespace Microsoft.Maui.Controls.Handlers.Items
protected override void DisconnectHandler(ListViewBase platformView)
{
+ _pendingKeepScrollOffsetPosition = -1;
+ _pendingKeepScrollOffsetUpdate++;
+
if (ItemsView != null)
ItemsView.Scrolled -= CarouselScrolled;
@@ -535,6 +540,18 @@ namespace Microsoft.Maui.Controls.Handlers.Items
var position = e.CenterItemIndex;
+ if (_pendingKeepScrollOffsetPosition != -1)
+ {
+ var pendingPosition = _pendingKeepScrollOffsetPosition;
+ _pendingKeepScrollOffsetPosition = -1;
+
+ if (ItemsView.ItemsUpdatingScrollMode == ItemsUpdatingScrollMode.KeepScrollOffset && IsValidPosition(pendingPosition))
+ {
+ SetCarouselViewPosition(pendingPosition);
+ return;
+ }
+ }
+
if (position == -1)
{
return;
@@ -610,6 +627,22 @@ namespace Microsoft.Maui.Controls.Handlers.Items
carouselPosition = 0;
}
+ var shouldKeepScrollOffset =
+ ItemsView.ItemsUpdatingScrollMode == ItemsUpdatingScrollMode.KeepScrollOffset
+ && (e.Action == NotifyCollectionChangedAction.Add || e.Action == NotifyCollectionChangedAction.Remove)
+ && IsValidPosition(carouselPosition);
+
+ if (shouldKeepScrollOffset)
+ {
+ var update = ++_pendingKeepScrollOffsetUpdate;
+ _pendingKeepScrollOffsetPosition = carouselPosition;
+ ListViewBase?.DispatcherQueue?.TryEnqueue(() =>
+ {
+ if (_pendingKeepScrollOffsetUpdate == update)
+ _pendingKeepScrollOffsetPosition = -1;
+ });
+ }
+
SetCarouselViewCurrentItem(carouselPosition);
SetCarouselViewPosition(carouselPosition);
}
MauiBot
removed
the
s/agent-review-in-progress
|
/review rerun |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
Tests Failure Analysis
Test Failure Review: Likely unrelated - click to expandOverall verdict: Likely unrelated All 5 unique test failures and all build/infrastructure failures point to pre-existing CI instability, SSL network errors, missing MSBuild task files, and flaky/timeout UI tests in areas (Shell, WebView, BottomSheet) and on platforms (Android, iOS, macOS) not touched by this Windows-only CarouselView scroll offset fix. The base branch (
Recommended actionNo action needed on failures — all are infrastructure, pre-existing, or platform/area mismatches. The PR can proceed to human review on its code merits. Evidence detailsPR scope: 1 source file changed ( Build 1474424 (maui-pr):
Build 1474426 (maui-pr-devicetests):
Build 1474425 (maui-pr-uitests):
Limitations: AzDO was not authenticated; authenticated test-run APIs were skipped. Helix work-item details for the Windows device tests were inaccessible (404). Log excerpts for individual UI test job logs had empty |
…CarouselView (#29800) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details When ItemsUpdatingScrollMode is set to KeepScrollOffset, the CurrentItem position gets reset incorrectly. ### Root Cause The CurrentItem position is incorrectly updated by the scrolled event when the scroll mode is set to KeepScrollOffset. ### Description of Change Explicitly set the current item index when the scroll mode is KeepScrollOffset to maintain the correct position and item selection. Validated the behavior in the following platforms - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Issues Fixed Fixes #29421 ### Output ScreenShot |Before|After| |--|--| | <video src="https://github.com/user-attachments/assets/5ab2bfc4-363a-415c-90f4-b5083813d337" >| <video src="https://github.com/user-attachments/assets/773a46a6-3f49-44b8-a770-edc5136e5e59">| --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…CarouselView (#29800) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details When ItemsUpdatingScrollMode is set to KeepScrollOffset, the CurrentItem position gets reset incorrectly. ### Root Cause The CurrentItem position is incorrectly updated by the scrolled event when the scroll mode is set to KeepScrollOffset. ### Description of Change Explicitly set the current item index when the scroll mode is KeepScrollOffset to maintain the correct position and item selection. Validated the behavior in the following platforms - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Issues Fixed Fixes #29421 ### Output ScreenShot |Before|After| |--|--| | <video src="https://github.com/user-attachments/assets/5ab2bfc4-363a-415c-90f4-b5083813d337" >| <video src="https://github.com/user-attachments/assets/773a46a6-3f49-44b8-a770-edc5136e5e59">| --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…CarouselView (#29800) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details When ItemsUpdatingScrollMode is set to KeepScrollOffset, the CurrentItem position gets reset incorrectly. ### Root Cause The CurrentItem position is incorrectly updated by the scrolled event when the scroll mode is set to KeepScrollOffset. ### Description of Change Explicitly set the current item index when the scroll mode is KeepScrollOffset to maintain the correct position and item selection. Validated the behavior in the following platforms - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Issues Fixed Fixes #29421 ### Output ScreenShot |Before|After| |--|--| | <video src="https://github.com/user-attachments/assets/5ab2bfc4-363a-415c-90f4-b5083813d337" >| <video src="https://github.com/user-attachments/assets/773a46a6-3f49-44b8-a770-edc5136e5e59">| --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…CarouselView (#29800) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details When ItemsUpdatingScrollMode is set to KeepScrollOffset, the CurrentItem position gets reset incorrectly. ### Root Cause The CurrentItem position is incorrectly updated by the scrolled event when the scroll mode is set to KeepScrollOffset. ### Description of Change Explicitly set the current item index when the scroll mode is KeepScrollOffset to maintain the correct position and item selection. Validated the behavior in the following platforms - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Issues Fixed Fixes #29421 ### Output ScreenShot |Before|After| |--|--| | <video src="https://github.com/user-attachments/assets/5ab2bfc4-363a-415c-90f4-b5083813d337" >| <video src="https://github.com/user-attachments/assets/773a46a6-3f49-44b8-a770-edc5136e5e59">| --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…CarouselView (#29800) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details When ItemsUpdatingScrollMode is set to KeepScrollOffset, the CurrentItem position gets reset incorrectly. ### Root Cause The CurrentItem position is incorrectly updated by the scrolled event when the scroll mode is set to KeepScrollOffset. ### Description of Change Explicitly set the current item index when the scroll mode is KeepScrollOffset to maintain the correct position and item selection. Validated the behavior in the following platforms - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Issues Fixed Fixes #29421 ### Output ScreenShot |Before|After| |--|--| | <video src="https://github.com/user-attachments/assets/5ab2bfc4-363a-415c-90f4-b5083813d337" >| <video src="https://github.com/user-attachments/assets/773a46a6-3f49-44b8-a770-edc5136e5e59">| --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…CarouselView (#29800) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details When ItemsUpdatingScrollMode is set to KeepScrollOffset, the CurrentItem position gets reset incorrectly. ### Root Cause The CurrentItem position is incorrectly updated by the scrolled event when the scroll mode is set to KeepScrollOffset. ### Description of Change Explicitly set the current item index when the scroll mode is KeepScrollOffset to maintain the correct position and item selection. Validated the behavior in the following platforms - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Issues Fixed Fixes #29421 ### Output ScreenShot |Before|After| |--|--| | <video src="https://github.com/user-attachments/assets/5ab2bfc4-363a-415c-90f4-b5083813d337" >| <video src="https://github.com/user-attachments/assets/773a46a6-3f49-44b8-a770-edc5136e5e59">| --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…CarouselView (#29800) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details When ItemsUpdatingScrollMode is set to KeepScrollOffset, the CurrentItem position gets reset incorrectly. ### Root Cause The CurrentItem position is incorrectly updated by the scrolled event when the scroll mode is set to KeepScrollOffset. ### Description of Change Explicitly set the current item index when the scroll mode is KeepScrollOffset to maintain the correct position and item selection. Validated the behavior in the following platforms - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Issues Fixed Fixes #29421 ### Output ScreenShot |Before|After| |--|--| | <video src="https://github.com/user-attachments/assets/5ab2bfc4-363a-415c-90f4-b5083813d337" >| <video src="https://github.com/user-attachments/assets/773a46a6-3f49-44b8-a770-edc5136e5e59">| --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…CarouselView (#29800) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details When ItemsUpdatingScrollMode is set to KeepScrollOffset, the CurrentItem position gets reset incorrectly. ### Root Cause The CurrentItem position is incorrectly updated by the scrolled event when the scroll mode is set to KeepScrollOffset. ### Description of Change Explicitly set the current item index when the scroll mode is KeepScrollOffset to maintain the correct position and item selection. Validated the behavior in the following platforms - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Issues Fixed Fixes #29421 ### Output ScreenShot |Before|After| |--|--| | <video src="https://github.com/user-attachments/assets/5ab2bfc4-363a-415c-90f4-b5083813d337" >| <video src="https://github.com/user-attachments/assets/773a46a6-3f49-44b8-a770-edc5136e5e59">| --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Issue Details
When ItemsUpdatingScrollMode is set to KeepScrollOffset, the CurrentItem position gets reset incorrectly.
Root Cause
The CurrentItem position is incorrectly updated by the scrolled event when the scroll mode is set to KeepScrollOffset.
Description of Change
Explicitly set the current item index when the scroll mode is KeepScrollOffset to maintain the correct position and item selection.
Validated the behavior in the following platforms
Issues Fixed
Fixes #29421
Output ScreenShot
BeforeFix-29421.mp4
AfterFix-29421.mp4