diff --git a/.github/agent-pr-session/pr-32289.md b/.github/agent-pr-session/pr-32289.md new file mode 100644 index 000000000000..33c9e80c1071 --- /dev/null +++ b/.github/agent-pr-session/pr-32289.md @@ -0,0 +1,202 @@ +# PR Review: #32289 - Fix handler not disconnected when removing non visible pages using RemovePage() + +**Date:** 2026-01-07 | **Issue:** [#32239](https://github.com/dotnet/maui/issues/32239) | **PR:** [#32289](https://github.com/dotnet/maui/pull/32289) + +## โœ… Status: COMPLETE + +| Phase | Status | +|-------|--------| +| Pre-Flight | โœ… COMPLETE | +| ๐Ÿงช Tests | โœ… COMPLETE | +| ๐Ÿšฆ Gate | โœ… PASSED | +| ๐Ÿ”ง Fix | โœ… COMPLETE | +| ๐Ÿ“‹ Report | โœ… COMPLETE | + +--- + +
+๐Ÿ“‹ Issue Summary + +**Problem:** When removing pages from a NavigationPage's navigation stack using `NavigationPage.Navigation.RemovePage()`, handlers are not properly disconnected from the removed pages. However, using `ContentPage.Navigation.RemovePage()` correctly disconnects handlers. + +**Root Cause (from PR):** The `RemovePage()` method removes the page from the navigation stack but does not explicitly disconnect its handler. + +**Regression:** Introduced in PR #24887, reproducible from MAUI 9.0.40+ + +**Steps to Reproduce:** +1. Push multiple pages onto a NavigationPage stack +2. Call `NavigationPage.Navigation.RemovePage()` on a non-visible page +3. Observe that the page's handler remains connected (no cleanup) + +**Workaround:** Manually call `.DisconnectHandlers()` after removing the page + +**Platforms Affected:** +- [x] iOS +- [x] Android +- [x] Windows +- [x] MacCatalyst + +
+ +
+๐Ÿ“ Files Changed + +| File | Type | Changes | +|------|------|---------| +| `src/Controls/src/Core/NavigationPage/NavigationPage.cs` | Fix | +4 lines | +| `src/Controls/src/Core/NavigationPage/NavigationPage.Legacy.cs` | Fix | +4 lines | +| `src/Controls/src/Core/NavigationProxy.cs` | Fix | -1 line (removed duplicate) | +| `src/Controls/src/Core/Shell/ShellSection.cs` | Fix | +1 line | +| `src/Controls/tests/Core.UnitTests/NavigationUnitTest.cs` | Unit Test | +63 lines | +| `src/Controls/tests/Core.UnitTests/ShellNavigatingTests.cs` | Unit Test | +25 lines | + +**Test Type:** Unit Tests + +
+ +
+๐Ÿ’ฌ PR Discussion Summary + +**Key Comments:** +- Copilot flagged potential duplicate disconnection logic between NavigationProxy and NavigationPage +- Author responded by removing redundant logic from NavigationProxy and updating ShellSection +- StephaneDelcroix requested unit tests โ†’ Author added them +- rmarinho confirmed unit tests cover both `useMaui: true` and `useMaui: false` scenarios + +**Reviewer Feedback:** +- Comments about misleading code comments (fixed) +- Concern about duplicate `DisconnectHandlers()` calls (resolved by moving from NavigationProxy to implementations) +- StephaneDelcroix: Approved after unit tests added +- rmarinho: Approved - confirmed tests cover NavigationPage and Shell scenarios + +**Maintainer Approvals:** +- โœ… StephaneDelcroix (Jan 7, 2026) +- โœ… rmarinho (Jan 7, 2026) + +**Disagreements to Investigate:** +| File:Line | Reviewer Says | Author Says | Status | +|-----------|---------------|-------------|--------| +| NavigationPage.cs:914 | Duplicate disconnection with NavigationProxy | Removed from NavigationProxy, now only in NavigationPage | โœ… RESOLVED | +| NavigationPage.Legacy.cs:257 | Same duplicate concern | Same resolution | โœ… RESOLVED | + +**Author Uncertainty:** +- None noted + +
+ +
+๐Ÿงช Tests + +**Status**: โœ… COMPLETE + +- [x] PR includes unit tests +- [x] Tests follow naming convention +- [x] Unit tests cover both useMaui: true/false paths +- [x] Unit tests cover Shell navigation + +**Test Files (from PR - Unit Tests):** +- `src/Controls/tests/Core.UnitTests/NavigationUnitTest.cs` (+63 lines) + - `RemovePageDisconnectsHandlerForNonVisiblePage` - Tests removing middle page from 3-page stack (both useMaui: true/false) + - `RemovePageDisconnectsHandlerForRemovedRootPage` - Tests removing root page when another page is on top +- `src/Controls/tests/Core.UnitTests/ShellNavigatingTests.cs` (+25 lines) + - `RemovePageDisconnectsHandlerInShell` - Tests Shell navigation scenario + +**Unit Test Coverage Analysis:** +| Code Path | useMaui: true | useMaui: false | Shell | +|-----------|---------------|----------------|-------| +| Remove middle page | โœ… | โœ… | โœ… | +| Remove root page | โœ… | โœ… | - | + +Coverage is adequate - tests cover all modified code paths. + +
+ +
+๐Ÿšฆ Gate - Test Verification + +**Status**: โœ… PASSED + +- [x] Tests FAIL without fix (bug reproduced) +- [x] Tests PASS with fix + +**Result:** PASSED โœ… + +**Verification:** Unit tests from PR cover all code paths: +- `RemovePageDisconnectsHandlerForNonVisiblePage(true)` - Maui path (Android/Windows) +- `RemovePageDisconnectsHandlerForNonVisiblePage(false)` - Legacy path (iOS/MacCatalyst) +- `RemovePageDisconnectsHandlerForRemovedRootPage(true/false)` - Root page removal +- `RemovePageDisconnectsHandlerInShell` - Shell navigation + +
+ +
+๐Ÿ”ง Fix Candidates + +**Status**: โœ… COMPLETE + +| # | Source | Approach | Test Result | Files Changed | Notes | +|---|--------|----------|-------------|---------------|-------| +| 1 | try-fix | Add DisconnectHandlers() inside SendHandlerUpdateAsync callback | โŒ FAIL | `NavigationPage.cs` (+3) | **Why failed:** Timing issue - SendHandlerUpdateAsync uses FireAndForget(), so the callback with DisconnectHandlers() runs asynchronously. The test checks Handler immediately after RemovePage() returns, before the async callback executes. | +| 2 | try-fix | Add DisconnectHandlers() synchronously after SendHandlerUpdateAsync in MauiNavigationImpl | โŒ FAIL | `NavigationPage.cs` (+3) | **Why failed:** iOS uses `UseMauiHandler = false`, meaning it uses NavigationImpl (Legacy) NOT MauiNavigationImpl. My fix was in the wrong code path - iOS doesn't execute MauiNavigationImpl at all. | +| 3 | try-fix | Add DisconnectHandlers() at end of Legacy RemovePage method | โœ… PASS | `NavigationPage.Legacy.cs` (+3) | Works! iOS/MacCatalyst use Legacy path. Simpler fix - only 1 file needed for iOS. | +| 4 | try-fix | Approach 2+3 combined (both Maui and Legacy paths) | โœ… PASS (iOS) | `NavigationPage.cs`, `NavigationPage.Legacy.cs` (+6 total) | Works for NavigationPage on all platforms, BUT **misses Shell navigation** which has its own code path. | +| PR | PR #32289 | Add `DisconnectHandlers()` call in RemovePage for non-visible pages | โœ… PASS (Gate) | NavigationPage.cs, NavigationPage.Legacy.cs, NavigationProxy.cs, ShellSection.cs | Original PR - validated by Gate | + +**Note:** try-fix candidates (1, 2, 3...) are added during Phase 4. PR's fix is reference only. + +**Exhausted:** No (stopped after finding working alternative) +**Selected Fix:** PR's fix + +**Deep Analysis (Git History Research):** + +**Historical Timeline:** +1. **PR #24887** (Feb 2025): Fixed Android flickering by avoiding handler removal during PopAsync - inadvertently broke RemovePage scenarios +2. **PR #30049** (June 2025): Attempted fix by adding `page?.DisconnectHandlers()` to `NavigationProxy.OnRemovePage()` - **BUT THIS FIX WAS FUNDAMENTALLY FLAWED** +3. **PR #32289** (Current): Correctly fixes by adding DisconnectHandlers to the NavigationPage implementations + +**Why PR #30049's fix didn't work:** +- `MauiNavigationImpl` and `NavigationImpl` **override** `OnRemovePage()` +- The overrides do NOT call `base.OnRemovePage()` +- Therefore `NavigationProxy.OnRemovePage()` is **NEVER executed** for NavigationPage! +- ContentPage works because it doesn't override - uses the base NavigationProxy directly + +**Why calling base.OnRemovePage() won't work:** +- `MauiNavigationImpl.OnRemovePage()` is a **complete replacement** with its own validation, async flow, etc. +- Calling base would cause double removal and ordering issues + +**Conclusion:** The fix MUST be in the NavigationPage implementations themselves, not in NavigationProxy. PR #32289's approach is architecturally correct. + +**Comparison:** +- **My fix #3** works for iOS/MacCatalyst (Legacy path) - 1 file, 3 lines +- **PR's fix** works for ALL platforms (Legacy + Maui paths) - 3 files, ~10 lines +- **PR #30049's approach** โŒ Doesn't work - fix in NavigationProxy is bypassed by overrides + +**Rationale for selecting PR's fix:** +1. PR covers ALL platforms (iOS, MacCatalyst, Android, Windows) while my fix only covers iOS/MacCatalyst +2. PR also fixes ShellSection for Shell navigation scenarios +3. PR uses null-safety (`page?`) which is more defensive +4. PR correctly removes the ineffective DisconnectHandlers from NavigationProxy (cleanup) +5. My successful fix #3 is essentially a subset of the PR's approach + +**Independent validation:** My fix #3 independently arrived at the same solution as the PR for the Legacy path, which validates the PR's approach is correct. + +
+ +--- + +## โœ… Final Recommendation: APPROVE + +**Summary:** PR #32289 correctly fixes the handler disconnection issue when removing non-visible pages using `RemovePage()`. + +**Key Findings:** +1. โœ… **Root cause correctly identified** - NavigationPage overrides bypass NavigationProxy, requiring fix in implementations +2. โœ… **All code paths covered** - NavigationPage (Maui + Legacy) and ShellSection +3. โœ… **Unit tests adequate** - Cover both `useMaui: true/false` and Shell navigation +4. โœ… **Two maintainer approvals** - StephaneDelcroix and rmarinho +5. โœ… **Independent validation** - My try-fix #3 independently arrived at same solution for Legacy path + +**Alternative approaches tested:** +- Approach 2+3 (Maui + Legacy paths only) works but misses Shell navigation +- PR's fix is more complete and architecturally correct + +**No concerns identified.** diff --git a/src/Controls/src/Core/NavigationPage/NavigationPage.Legacy.cs b/src/Controls/src/Core/NavigationPage/NavigationPage.Legacy.cs index 3a9f24e6c610..a6c0528a4264 100644 --- a/src/Controls/src/Core/NavigationPage/NavigationPage.Legacy.cs +++ b/src/Controls/src/Core/NavigationPage/NavigationPage.Legacy.cs @@ -252,6 +252,10 @@ void RemovePage(Page page) _removePageRequested?.Invoke(this, new NavigationRequestedEventArgs(page, true)); RemoveFromInnerChildren(page); + // Disconnect handlers for the removed non-visible page. + // Note: When the current page is removed, PopAsync() is called instead. + page?.DisconnectHandlers(); + if (RootPage == page) RootPage = (Page)InternalChildren.First(); } diff --git a/src/Controls/src/Core/NavigationPage/NavigationPage.cs b/src/Controls/src/Core/NavigationPage/NavigationPage.cs index e4d7b68b1f8c..7f8f1047fa5a 100644 --- a/src/Controls/src/Core/NavigationPage/NavigationPage.cs +++ b/src/Controls/src/Core/NavigationPage/NavigationPage.cs @@ -909,6 +909,10 @@ protected override void OnRemovePage(Page page) { Owner.RemoveFromInnerChildren(page); + // Disconnect handlers for the removed non-visible page. + // Note: When the current page is removed, PopAsync() is called instead. + page?.DisconnectHandlers(); + if (Owner.RootPage == page) Owner.RootPage = (Page)Owner.InternalChildren[0]; }, diff --git a/src/Controls/src/Core/NavigationProxy.cs b/src/Controls/src/Core/NavigationProxy.cs index 8e374446304d..5d2f4806d17c 100644 --- a/src/Controls/src/Core/NavigationProxy.cs +++ b/src/Controls/src/Core/NavigationProxy.cs @@ -252,7 +252,6 @@ protected virtual void OnRemovePage(Page page) { currentInner.RemovePage(page); } - page?.DisconnectHandlers(); } Page Pop() diff --git a/src/Controls/src/Core/Shell/ShellSection.cs b/src/Controls/src/Core/Shell/ShellSection.cs index dcf7b73dc85a..1311e6085e8e 100644 --- a/src/Controls/src/Core/Shell/ShellSection.cs +++ b/src/Controls/src/Core/Shell/ShellSection.cs @@ -984,6 +984,7 @@ protected virtual void OnRemovePage(Page page) PresentedPageAppearing(); RemovePage(page); + page?.DisconnectHandlers(); var args = new NavigationRequestedEventArgs(page, false) { RequestType = NavigationRequestType.Remove diff --git a/src/Controls/tests/Core.UnitTests/NavigationUnitTest.cs b/src/Controls/tests/Core.UnitTests/NavigationUnitTest.cs index 787bc5abfb3b..745ea0c8a3da 100644 --- a/src/Controls/tests/Core.UnitTests/NavigationUnitTest.cs +++ b/src/Controls/tests/Core.UnitTests/NavigationUnitTest.cs @@ -679,6 +679,69 @@ public async Task TestRemovePage(bool useMaui) }); } + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task RemovePageDisconnectsHandlerForNonVisiblePage(bool useMaui) + { + // Arrange: Create a navigation stack with 3 pages + var root = new ContentPage { Title = "Root" }; + var middlePage = new ContentPage { Title = "Middle" }; + var topPage = new ContentPage { Title = "Top" }; + var navPage = new TestNavigationPage(useMaui, root); + _ = new TestWindow(navPage); + + await navPage.PushAsync(middlePage); + await navPage.PushAsync(topPage); + + // Assign a handler to the middle page to verify it gets disconnected + var mockHandler = new HandlerStub(); + middlePage.Handler = mockHandler; + + // Act: Remove the middle (non-visible) page from the stack + navPage.Navigation.RemovePage(middlePage); + await navPage.NavigatingTask; + + // Assert: Verify the page was removed from the navigation stack + Assert.Equal(2, navPage.Navigation.NavigationStack.Count); + Assert.Same(root, navPage.RootPage); + Assert.Same(topPage, navPage.CurrentPage); + Assert.DoesNotContain(middlePage, navPage.Navigation.NavigationStack); + + // Verify the page is no longer a logical child of the NavigationPage + Assert.DoesNotContain(middlePage, navPage.InternalChildren.Cast()); + + // Verify the handler was disconnected + Assert.Null(middlePage.Handler); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task RemovePageDisconnectsHandlerForRemovedRootPage(bool useMaui) + { + // Arrange: Create a navigation stack with 2 pages where the root page has a handler + var root = new ContentPage { Title = "Root" }; + var topPage = new ContentPage { Title = "Top" }; + var navPage = new TestNavigationPage(useMaui, root); + var window = new TestWindow(navPage); + + // Manually assign a mock handler to track disconnection + var mockHandler = new HandlerStub(); + root.Handler = mockHandler; + + await navPage.PushAsync(topPage); + + // Act: Remove the root page (non-visible) from the stack + navPage.Navigation.RemovePage(root); + await navPage.NavigatingTask; + + // Assert: Handler should be disconnected (set to null on the page) + Assert.Null(root.Handler); + Assert.Same(topPage, navPage.RootPage); + Assert.Same(topPage, navPage.CurrentPage); + } + [Fact] public async Task CurrentPageUpdatesOnPopBeforeAsyncCompletes() { diff --git a/src/Controls/tests/Core.UnitTests/ShellNavigatingTests.cs b/src/Controls/tests/Core.UnitTests/ShellNavigatingTests.cs index 1451bf322a6b..3042ef90f915 100644 --- a/src/Controls/tests/Core.UnitTests/ShellNavigatingTests.cs +++ b/src/Controls/tests/Core.UnitTests/ShellNavigatingTests.cs @@ -994,6 +994,31 @@ protected override Task OnPushModal(Page modal, bool animated) } } + [Fact] + public async Task RemovePageDisconnectsHandlerInShell() + { + Routing.RegisterRoute("page1", typeof(TestPage1)); + Routing.RegisterRoute("page2", typeof(TestPage2)); + var shell = new TestShell( + CreateShellItem(shellContentRoute: "root", shellItemRoute: "main") + ); + + await shell.GoToAsync("//main/root/page1"); + await shell.GoToAsync("page2"); + + // Get the middle page and assign a handler + var middlePage = shell.Navigation.NavigationStack[1]; + var mockHandler = new HandlerStub(); + middlePage.Handler = mockHandler; + + // Act: Remove the middle page + shell.Navigation.RemovePage(middlePage); + + // Assert: Handler should be disconnected + Assert.Null(middlePage.Handler); + Assert.Equal(2, shell.Navigation.NavigationStack.Count); + } + ShellNavigatingEventArgs CreateShellNavigatedEventArgs() => new ShellNavigatingEventArgs("..", "../newstate", ShellNavigationSource.Push, true); }