Fix Shell Navigating event not firing on ShellContent change#34351
Fix Shell Navigating event not firing on ShellContent change#34351jpd21122012 wants to merge 4 commits intodotnet:mainfrom
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34351Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34351" |
|
Hey there @@jpd21122012! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
Hi, just checking if this PR could be reviewed for the upcoming 10.6 servicing release. This fix ensures that the Shell.Navigating event is correctly triggered when the ShellContent changes, restoring expected navigation lifecycle behavior. The change is minimal and scoped to the navigation event flow, without affecting public APIs, so it should be safe for servicing. Let me know if any additional validation or scenarios should be covered 👍 |
kubaflo
left a comment
There was a problem hiding this comment.
Could you please add a test? You can use the write-test agent
Yes sure, let me work on it |
There was a problem hiding this comment.
Pull request overview
This PR addresses a behavioral gap in .NET MAUI Shell on Windows where switching ShellContent within the same ShellSection does not raise the Shell.Navigating event, leading to inconsistent event sequencing (Navigated without Navigating). It also adds a UI test case intended to validate the corrected event ordering.
Changes:
- Update
ShellSection.OnCurrentItemChangedto invoke the navigating pipeline before callingUpdateCurrentStateforShellNavigationSource.ShellContentChanged. - Add a HostApp reproduction (
Issue34318) to switchShellContentand surface a signal whenNavigatingfires. - Add an Appium-based UI test (
Issue34318) that taps a button and asserts the app observed theNavigatingevent.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/Controls/src/Core/Shell/ShellSection.cs | Adds a HandleNavigating call and cancellation check when ShellSection.CurrentItem changes. |
| src/Controls/tests/TestCases.HostApp/Issues/Issue34318.cs | Adds a Shell-based repro page that attempts to broadcast when Shell.Navigating fires and switches ShellContent. |
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue34318.cs | Adds a UI test that taps a button and asserts the “Navigating” signal is observed. |
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34351 | Raise HandleNavigating during ShellSection.OnCurrentItemChanged before updating current state; add UI repro test |
⏳ PENDING (Gate) | src/Controls/src/Core/Shell/ShellSection.cs, src/Controls/tests/TestCases.HostApp/Issues/Issue34318.cs, src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue34318.cs |
Original PR; requested gate platform is Android even though issue is Windows-only |
Issue: #34318 - [Windows] Shell.Navigating Event Not Triggered on Windows When Switching Tab Contents
PR: #34351 - Fix Shell Navigating event not firing on ShellContent change
Platforms Affected: Windows (issue report); iOS requested for this review run
Files Changed: 1 implementation, 2 test
Key Findings
- The linked issue is explicitly reported and validated as Windows-only; this review run used iOS per the requested platform.
- The PR changes
src/Controls/src/Core/Shell/ShellSection.csand adds a HostApp/UI test pair for issue 34318. - Inline review feedback raises unresolved concerns about navigation-state completeness, event ordering, and cancellation semantics in
OnCurrentItemChanged. - A prior agent review exists in PR discussion (MauiBot summary on 2026-03-22) and recommended requesting changes; no local phase artifacts were present in
CustomAgentLogsTmp/PRState/34351/PRAgent/at the start of this run.
Edge Cases From Discussion
- Proposed navigation state currently passes
nullfor both navigation and modal stacks, which can makeTargetincomplete when stack or modal state is present. - Raising
Navigatingafter appearance/disappearing side effects have already begun weakens expected event ordering. - Cancellation is handled after
CurrentItemhas already changed, which can leave Shell state inconsistent if a handler cancels.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34351 | Raise HandleNavigating from ShellSection.OnCurrentItemChanged before UpdateCurrentState; add HostApp/UI repro for issue 34318 |
PENDING (Gate) | src/Controls/src/Core/Shell/ShellSection.cs, src/Controls/tests/TestCases.HostApp/Issues/Issue34318.cs, src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue34318.cs |
Original PR |
🚦 Gate — Test Verification
Gate Result: ❌ FAILED
Platform: android
Mode: Full Verification
- Tests FAIL without fix: ✅
- Tests PASS with fix: ❌
Notes
- Verification artifact:
CustomAgentLogsTmp/PRState/34351/PRAgent/gate/verify-tests-fail/verification-report.md - The Android UI test times out in
Issue34318.NavigatingFiresWhenShellContentChanges()while waiting for"Navigating"after tappingChangeContentButton. - Page-source evidence from
CustomAgentLogsTmp/UITests/NavigatingFiresWhenShellContentChanges-Android-UITestBaseTearDown-PageSource-*.txtshowsResultLabelstill displaysWaiting, so the test never observes the event. - This aligns with review comments that the HostApp repro page is likely broken (mismatched
MessagingCentersender type and incorrectParent is ShellSectionassumption), so gate failure is not reliable evidence that the product fix itself is correct or incorrect on Android.
Gate FAILEDResult:
Platform: ios
Mode: Full Verification
- Tests FAIL without fix:
- Tests PASS with fix:
Notes:
- Verification artifact:
CustomAgentLogsTmp/PRState/34351/PRAgent/gate/verify-tests-fail/verification-report.md - The verifier reported
VERIFICATION FAILEDon iOS: tests failed both without the fix and with the fix. CustomAgentLogsTmp/UITests/test-output.logshowsIssue34318.NavigatingFiresWhenShellContentChanges()failing withSystem.TimeoutException: Timed out waiting for element...atsrc/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue34318.cs:24while waiting forResultLabelafter tappingChangeContentButton.- The captured page source (
CustomAgentLogsTmp/UITests/NavigatingFiresWhenShellContentChanges-iOS-UITestBaseTearDown-PageSource-845dc644e8184f71907c6a44c112fc61.txt) shows the app onPageBwithPageBLabel, not on the page containingResultLabel, so the added UI test does not successfully observe the intended signal on iOS.
🔧 Fix — Analysis & Comparison
Gate Result: ❌ FAILED
Platform: android
Mode: Full Verification
- Tests FAIL without fix: ✅
- Tests PASS with fix: ❌
Notes
- Verification artifact:
CustomAgentLogsTmp/PRState/34351/PRAgent/gate/verify-tests-fail/verification-report.md - The Android UI test times out in
Issue34318.NavigatingFiresWhenShellContentChanges()while waiting for"Navigating"after tappingChangeContentButton. - Page-source evidence from
CustomAgentLogsTmp/UITests/NavigatingFiresWhenShellContentChanges-Android-UITestBaseTearDown-PageSource-*.txtshowsResultLabelstill displaysWaiting, so the test never observes the event. - This aligns with review comments that the HostApp repro page is likely broken (mismatched
MessagingCentersender type and incorrectParent is ShellSectionassumption), so gate failure is not reliable evidence that the product fix itself is correct or incorrect on Android.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Fire from a callback on ; redesign the repro so the visible page reports whether the event fired | PASS | 3 files | Best overall: pre-commit interception with true cancellation semantics and a passing iOS validation |
| 2 | try-fix | Keep the fix in but move ahead of appearance/disappearing side effects; redesign the repro to assert from the visible page | PASS | 3 files | Simpler than callback-based interception, but still happens after the property mutation |
| 3 | try-fix | Keep the PR hook point and add explicit rollback to the previous CurrentItem when Navigating is FAIL |
1 file | Validation was derailed by iOS toolchain mismatch during the attempt; no clean passing result | cancelled |
| 4 | try-fix | Use coerceValue plus reflection-based reset of the accumulated-navigation flag before proposing FAIL |
1 file | More invasive and still failed to trigger reliable event observation | navigation |
| 5 | try-fix | Use plus direct with full state construction before the renderer sees the change; cancel in the repro to keep PageA visible | PASS | 3 files | Works, but is more complex than Candidate #1 and the test shape is less representative of the original issue |
| PR | PR #34351 | Raise HandleNavigating from ShellSection.OnCurrentItemChanged; add HostApp/UI repro for issue FAILED (Gate) |
3 files | iOS gate failed because the added UI test timed out waiting for ResultLabel after navigation |
34318 |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 1 | Yes | Candidate #1 implemented and passed |
| claude-sonnet-4.6 | 1 | Yes | Candidate #2 implemented and passed |
| gpt-5.3-codex | 1 | Yes | Candidate #3 implemented; validation failed under toolchain blocker |
| gemini-3-pro-preview | 1 | Yes | Candidate #4 implemented and failed |
| claude-opus-4.6 | 2 | Yes | Suggested iOS native selection delegate gating |
| claude-sonnet-4.6 | 2 | Yes | Suggested iOS native selection delegate gating |
| gpt-5.3-codex | 2 | Yes | Suggested SetValueCore interception for CurrentItemProperty |
| gemini-3-pro-preview | 2 | Yes | Suggested higher-level IShellController.ProposeNavigation routing |
| claude-sonnet-4.6 | 2b | Yes | Candidate #5 implemented from the higher-level proposal-path idea and passed |
| claude-opus-4.6 | 3 | Yes | Suggested iOS renderer/handler-side gating before native transition |
| claude-sonnet-4.6 | 3 | No | NO NEW IDEAS |
| gpt-5.3-codex | 3 | No | NO NEW IDEAS |
| gemini-3-pro-preview | 3 | Yes | Suggested centralizing HandleNavigating inside Shell.UpdateCurrentState |
Exhausted: Yes (max 3 cross-pollination rounds reached)
Selected Fix: Candidate # it passed on iOS, provides the cleanest pre-commit interception point, preserves true cancellation semantics, and is less invasive than the coerce/reflection alternatives.1
📋 Report — Final Recommendation
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Fire Navigating in Shell.UpdateCurrentState; repair HostApp/UI test wiring |
✅ PASS | 3 files | Broader than the PR, but event fires later than property validation |
| 2 | try-fix | Fire Navigating from ShellSection.CurrentItemProperty validateValue; repair HostApp/UI test wiring |
✅ PASS | 3 files | Best candidate: pre-commit interception with atomic cancellation semantics |
| 3 | try-fix | Call existing ProposeNavigation(...) from ShellSection.OnCurrentItemChanged; repair HostApp/UI test wiring |
❌ FAIL | 3 files | Still timed out waiting for ResultLabel to show Navigating |
| 4 | try-fix | Use coerceValue on ShellSection.CurrentItemProperty to call ProposeNavigation(...); repair HostApp/UI test wiring |
✅ PASS | 3 files | Passed, but verification strategy depended on canceling navigation in the repro |
| 5 | try-fix | Revert-and-route through Shell.GoToAsync from OnCurrentItemChanged; repair HostApp/UI test wiring |
❌ FAIL | 3 files | Deadlocked on Android via PendingNavigationTask; not viable from property-changed callback |
| PR | PR #34351 | Call HandleNavigating from ShellSection.OnCurrentItemChanged; add UI repro test |
❌ FAILED (Gate) | 3 files | Gate failed on Android because the added UI test never observed the event |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 1 | Yes | Candidate #1 implemented and passed |
| claude-sonnet-4.6 | 1 | Yes | Candidate #2 implemented and passed |
| gpt-5.3-codex | 1 | Yes | Candidate #3 implemented and failed verification |
| gemini-3-pro-preview | 1 | Yes | Candidate #4 implemented and passed |
| claude-opus-4.6 | 2 | Yes | Suggested central navigation request path; distilled into candidate #5 |
| claude-sonnet-4.6 | 2 | Yes | Similar centralized GoToAsync/navigation-manager idea; covered by candidate #5 |
| gpt-5.3-codex | 2 | Yes | Suggested handler-level interception, but Android already implements pre-navigation in ShellSectionRenderer.OnPageSelected |
| gemini-3-pro-preview | 2 | No | NO NEW IDEAS |
| claude-opus-4.6 | 3 | No | NO NEW IDEAS |
| claude-sonnet-4.6 | 3 | No | NO NEW IDEAS |
| gpt-5.3-codex | 3 | No | NO NEW IDEAS |
| gemini-3-pro-preview | 3 | No | NO NEW IDEAS |
Exhausted: Yes
Selected Fix: Candidate #2 — it passed empirically, preserves true pre-commit cancellation semantics, avoids GoToAsync re-entrancy problems, and does not depend on altering the repro to cancel navigation.
Final Recommendation: REQUEST CHANGES
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight COMPLETE | Linked issue is Windows-specific; this review run validated on requested iOS | |
| FAILED | iOS full verification failed because tests failed without fix and with fix | Gate |
| Try-Fix COMPLETE | 5 candidate fixes evaluated, 3 passing; cross-pollination exhausted after 3 rounds | |
| Report COMPLETE |
Summary
PR #34351 should not be approved as-is. On the requested iOS platform, the gate failed both before and after the fix because the newly added HostApp/UI test does not reliably observe the event it is trying to validate. The try-fix phase found multiple alternatives that passed on iOS, including stronger options than the PR's current OnCurrentItemChanged implementation.
Root Cause
- The PR's validation is unreliable on iOS. After tapping
ChangeContentButton, the UI test waits forResultLabelon the original page, but the app has already transitioned toPageB, so the assertion times out even with the PR applied. - The product fix is placed too late in the lifecycle. Calling
HandleNavigatingfromShellSection.OnCurrentItemChangedmeans the property mutation and some lifecycle side effects have already started, which weakens both ordering and cancellation behavior.
Fix Quality
The try-fix phase found better alternatives than the PR approach. The best candidate was a validateValue interception on ShellSection.CurrentItemProperty, which fires Navigating before the property commits, supports true cancellation semantics, and passed iOS verification. A second passing option reordered HandleNavigating earlier inside OnCurrentItemChanged, and a third passing option used coerceValue plus direct HandleNavigating before the renderer saw the change. All three outperformed the PR's current implementation on the requested platform.
kubaflo
left a comment
There was a problem hiding this comment.
Looks like the test is failing
…erage - Ensure Navigating event is triggered when ShellContent changes - Add UITest to validate expected behavior - Replace MessagingCenter usage with direct Shell.Navigating subscription - Fix parent traversal to correctly resolve ShellSection - Remove unused variable to avoid warnings-as-errors
Fixes #34318
Problem
When switching between
ShellContentitems within the sameShellSection(for example A1 → A2), theNavigatingevent is not fired on Windows.Only the
Navigatedevent is raised.This creates inconsistent behavior compared to other navigation scenarios where
Navigatingis expected to fire before the navigation completes.Root Cause
ShellSection.OnCurrentItemChangeddirectly calls:UpdateCurrentState(ShellNavigationSource.ShellContentChanged)
without first invoking the navigation pipeline through
ShellNavigationManager.HandleNavigating.Because of this, the
Navigatingevent is skipped.Fix
Trigger the navigation pipeline before updating the current state:
NavigationManager.HandleNavigatingUpdateCurrentStateResult
Switching between
ShellContentitems now correctly raises:Navigating → Navigated
with
ShellNavigationSource.ShellContentChanged.Tested
Reproduced with the sample from the issue:
Navigatingnow fires correctly.