[Windows] Fix for Shell.FlyoutVerticalScrollMode="Disabled" does not disable scrolling#32516
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a Windows-specific bug where setting Shell.FlyoutVerticalScrollMode="Disabled" does not actually disable vertical scrolling in the flyout menu. The fix adds the missing property mapping and ensures the scroll mode is updated when the Shell loads.
Key Changes
- Added
FlyoutVerticalScrollModeto the Shell handler property mapper for Windows - Added
UpdateValuecall forFlyoutVerticalScrollModein theOnLoadedevent handler - Created UI test with HostApp page and test case to verify the fix
Reviewed Changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ShellHandler.cs | Adds FlyoutVerticalScrollMode to Windows-specific property mapper |
| ShellHandler.Windows.cs | Calls UpdateValue for FlyoutVerticalScrollMode on Shell load |
| Issue32416.cs (HostApp) | Test page demonstrating disabled scroll mode with 20 menu items |
| Issue32416.cs (Tests) | NUnit test that scrolls and verifies via screenshot |
| VerifyFlyoutVerticalScrollModeDisabled.png (3 files) | Platform-specific screenshot baselines |
| for (int i = 1; i <= 20; i++) | ||
| { | ||
| Items.Add(new MenuItem | ||
| { | ||
| Text = $"Item {i}" | ||
| }); | ||
| } |
There was a problem hiding this comment.
MenuItem elements are missing AutomationId attributes required for UI test automation. According to the UI Testing Guidelines, all interactive controls must have AutomationId for reliable test automation. Update each MenuItem to include AutomationId like this: AutomationId = $\"Item {i}\". Example of proper usage exists in the codebase at ShellBackButtonBehavior.cs line 21: Items.Add(new MenuItem() { Text = \"Flyout Open\", AutomationId = \"Flyout Open\" });
2b4aecc to
a0178c1
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #32516 | Register Shell.FlyoutVerticalScrollMode in the Shell handler mapper and trigger a Windows update on OnLoaded; add a UI screenshot test for a locked flyout with many menu items. |
PENDING (Gate) | src/Controls/src/Core/Handlers/Shell/ShellHandler.cs, src/Controls/src/Core/Handlers/Shell/ShellHandler.Windows.cs, src/Controls/tests/TestCases.HostApp/Issues/Issue32416.cs, src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue32416.cs |
Original PR |
🚦 Gate — Test Verification
Gate Result: PASSED
Result: PASSED
Platform: windows
Mode: Full Verification
- Tests FAIL without fix:
- Tests PASS with fix:
Evidence
- Verification used the required
verify-tests-fail-without-fixskill via task agent. - Broken-baseline run failed
Issue32416.VerifyFlyoutVerticalScrollModeDisabledwith a visual snapshot mismatch (~2.98% difference), proving the test catches the regression. - Restored-fix run passed the same test (
1/1passing), proving the PR fix satisfies the verification gate on Windows.
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Bypass the mapper on Windows by applying FlyoutVerticalScrollMode directly from ShellHandler.Windows.cs during template/pane events and PropertyChanged. |
PASS | 1 file | Works, but adds custom Windows-only property-change plumbing instead of using the standard mapper flow. |
| 2 | try-fix | Keep the mapper entry, but apply UpdateValue(nameof(Shell.FlyoutVerticalScrollMode)) from OnApplyTemplateFinished instead of OnLoaded. |
PASS | 2 files | Viable and precise, but not clearly better than the PRs reuse of the existing OnLoaded initialization path. |
| 3 | try-fix | Call MapFlyoutVerticalScrollMode(this, VirtualView) directly during OnPaneOpening only, without adding a mapper entry. |
FAIL | 1 file | Repeated Windows runs still showed visual diffs; pane-opening alone is insufficient. |
| 4 | try-fix | Override UpdateValue(string? property) in ShellHandler.Windows.cs to manually dispatch MapFlyoutVerticalScrollMode for initial and runtime updates. |
PASS | 2 files | Functional, but introduces a Windows public API change and PublicAPI maintenance cost. |
| 5 | try-fix | Cache the desired flyout scroll mode on MauiNavigationView and apply it inside OnApplyTemplate() when the ScrollViewer exists. |
PASS | 3 files | Strong root-cause alternative, but broader than necessary because it adds control-side state. |
| 6 | try-fix | Set ScrollViewer.VerticalScrollMode on the MauiNavigationView root and rely on WinUI inheritance to reach the internal flyout scroll viewer. |
FAIL | 2 files | WinUI inheritance did not affect the internal scroll viewer. |
| 7 | try-fix | Suppress scroll input on the internal MenuItemsScrollViewer via wheel/view-changing event handlers. |
FAIL | 2 files | The saved test log still failed the screenshot assertion, so this cannot count as a pass. |
| 8 | try-fix | If the ScrollViewer is missing, defer application via LayoutUpdated, then apply once and unsubscribe. |
PASS | 2 files | Works, but adds event-subscription timing logic that is more complex than the PRs OnLoaded approach. |
| PR | PR #32516 | Register Shell.FlyoutVerticalScrollMode in the shared Shell mapper and trigger a Windows update on OnLoaded. |
PASSED (Gate) | 4 code files + snapshots | Original PR; minimal change that aligns with existing Windows handler initialization. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 1 | Yes | Direct Windows-only application path that bypasses the mapper. |
| claude-sonnet-4.6 | 1 | Yes | Use the shared mapper, but apply from OnApplyTemplateFinished instead of OnLoaded. |
| gpt-5.3-codex | 1 | Yes | Try direct pane-opening application only; empirically failed. |
| gemini-3-pro-preview | 1 | Yes | Override UpdateValue on Windows to manually route this property to the mapper. |
| claude-opus-4.6 | 2 | No | NO NEW IDEAS |
| claude-sonnet-4.6 | 2 | Yes | Move timing responsibility into MauiNavigationView via cached scroll mode applied during template creation. |
| gpt-5.3-codex | 2 | No | NO NEW IDEAS |
| gemini-3-pro-preview | 2 | No | NO NEW IDEAS |
| claude-opus-4.6 | 3 | Yes | Suppress wheel/gesture scrolling directly on the internal ScrollViewer. |
| claude-sonnet-4.6 | 3 | No | NO NEW IDEAS |
| gpt-5.3-codex | 3 | No | NO NEW IDEAS |
| gemini-3-pro-preview | 3 | Yes | Defer application through LayoutUpdated until the internal ScrollViewer exists. |
Exhausted: Yes
Selected Fix: PR #32516 It passes the gate, stays within existing handler patterns, avoids public API expansion, and is simpler/less invasive than the passing alternatives.
📋 Report — Final Recommendation
Final Recommendation: APPROVE
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | COMPLETE | Issue/PR context gathered; bug is Android+Windows, PR change is Windows-only. |
| Gate | PASSED | Windows full verification proved the test fails without the fix and passes with it. |
| Try-Fix | COMPLETE | 8 attempts explored across 3 cross-pollination rounds; multiple alternatives passed, but PR fix remained the best tradeoff. |
| Report | COMPLETE |
Summary
PR #32516 fixes the Windows Shell flyout scrolling bug by wiring Shell.FlyoutVerticalScrollMode into the existing Shell property mapper and reapplying it during the Windows handler initialization path. The gate passed on Windows: the new test failed against the broken baseline and passed with the PR fix.
The mandatory try-fix exploration found several working alternatives, but each was less attractive than the PR for at least one reason: extra Windows-only plumbing, broader control-side state changes, added event timing complexity, or a public API change. The PR's implementation is the smallest change that preserves the normal mapper-based design and solves the verified scenario.
Root Cause
The Windows flyout scroll mode property was not registered in the Shell handler mapper, so UpdateValue(nameof(Shell.FlyoutVerticalScrollMode)) was a no-op. In practice, the disabled scroll mode needed both mapper participation and a post-template reapplication point so the internal flyout ScrollViewer actually received the setting.
Fix Quality
The PR addresses the missing mapper registration and uses an existing Windows initialization hook (OnLoaded) to reapply the value once the platform view is ready. That keeps the fix local, avoids introducing new public API surface, and aligns with the existing pattern already used in OnLoaded for other flyout-related properties.
A non-blocking note from inline review remains: the added HostApp test page uses menu item text lookup instead of explicit AutomationId values. Gate verification showed the test works today, so this is not blocking the recommendation.
…disable scrolling (#32516) <!-- 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 Shell.FlyoutVerticalScrollMode is set to "Disabled", the flyout items should not allow vertical scrolling. However, on Windows, the flyout still scrolls even when the mode is set to "Disabled". ### Root Cause of the issue - Although UpdateValue(nameof(Shell.FlyoutVerticalScrollMode)) is called in the OnPaneOpening event, the MapFlyoutVerticalScrollMode method is never executed because the property is not registered in the PropertyMapper, which prevents the scroll mode from being disabled. ### Description of Change - Added mapping for the Shell.FlyoutVerticalScrollMode property in the handler property map to enable platform-specific updates. - Ensured that the flyout vertical scroll mode is updated when the Shell is loaded on Windows by calling UpdateValue(nameof(Shell.FlyoutVerticalScrollMode)) in OnLoaded. <!-- Enter description of the fix in this section --> ### Issues Fixed <!-- Please make sure that there is a bug logged for the issue being fixed. The bug should describe the problem and how to reproduce it. --> Fixes #32416 ### Tested the behaviour in the following platforms - [x] - Window - [ ] - Android - [x] - iOS - [x] - Mac ### Output | Before | After | |----------|----------| | <video src="https://github.com/user-attachments/assets/6a406c6a-8efe-46fc-9e7a-432c836d689d"> | <video src="https://github.com/user-attachments/assets/1a59f24f-cd50-4f94-8981-640eb52bc009"> | <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
…disable scrolling (dotnet#32516) <!-- 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 Shell.FlyoutVerticalScrollMode is set to "Disabled", the flyout items should not allow vertical scrolling. However, on Windows, the flyout still scrolls even when the mode is set to "Disabled". ### Root Cause of the issue - Although UpdateValue(nameof(Shell.FlyoutVerticalScrollMode)) is called in the OnPaneOpening event, the MapFlyoutVerticalScrollMode method is never executed because the property is not registered in the PropertyMapper, which prevents the scroll mode from being disabled. ### Description of Change - Added mapping for the Shell.FlyoutVerticalScrollMode property in the handler property map to enable platform-specific updates. - Ensured that the flyout vertical scroll mode is updated when the Shell is loaded on Windows by calling UpdateValue(nameof(Shell.FlyoutVerticalScrollMode)) in OnLoaded. <!-- Enter description of the fix in this section --> ### Issues Fixed <!-- Please make sure that there is a bug logged for the issue being fixed. The bug should describe the problem and how to reproduce it. --> Fixes dotnet#32416 ### Tested the behaviour in the following platforms - [x] - Window - [ ] - Android - [x] - iOS - [x] - Mac ### Output | Before | After | |----------|----------| | <video src="https://github.com/user-attachments/assets/6a406c6a-8efe-46fc-9e7a-432c836d689d"> | <video src="https://github.com/user-attachments/assets/1a59f24f-cd50-4f94-8981-640eb52bc009"> | <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
…disable scrolling (#32516) <!-- 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 Shell.FlyoutVerticalScrollMode is set to "Disabled", the flyout items should not allow vertical scrolling. However, on Windows, the flyout still scrolls even when the mode is set to "Disabled". ### Root Cause of the issue - Although UpdateValue(nameof(Shell.FlyoutVerticalScrollMode)) is called in the OnPaneOpening event, the MapFlyoutVerticalScrollMode method is never executed because the property is not registered in the PropertyMapper, which prevents the scroll mode from being disabled. ### Description of Change - Added mapping for the Shell.FlyoutVerticalScrollMode property in the handler property map to enable platform-specific updates. - Ensured that the flyout vertical scroll mode is updated when the Shell is loaded on Windows by calling UpdateValue(nameof(Shell.FlyoutVerticalScrollMode)) in OnLoaded. <!-- Enter description of the fix in this section --> ### Issues Fixed <!-- Please make sure that there is a bug logged for the issue being fixed. The bug should describe the problem and how to reproduce it. --> Fixes #32416 ### Tested the behaviour in the following platforms - [x] - Window - [ ] - Android - [x] - iOS - [x] - Mac ### Output | Before | After | |----------|----------| | <video src="https://github.com/user-attachments/assets/6a406c6a-8efe-46fc-9e7a-432c836d689d"> | <video src="https://github.com/user-attachments/assets/1a59f24f-cd50-4f94-8981-640eb52bc009"> | <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
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
Root Cause of the issue
Description of Change
Issues Fixed
Fixes #32416
Tested the behaviour in the following platforms
Output
Before_fix.mp4
After_fix.mp4