Fix handler not disconnected when removing non visible pages using RemovePage()

.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 |
+
+---
+
+<details>
+<summary><strong>📋 Issue Summary</strong></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
+
+</details>
+
+<details>
+<summary><strong>📁 Files Changed</strong></summary>
+
+| 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
+
+</details>
+
+<details>
+<summary><strong>💬 PR Discussion Summary</strong></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
+
+</details>
+
+<details>
+<summary><strong>🧪 Tests</strong></summary>
+
+**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.
+
+</details>
+
+<details>
+<summary><strong>🚦 Gate - Test Verification</strong></summary>
+
+**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
+
+</details>
+
+<details>
+<summary><strong>🔧 Fix Candidates</strong></summary>
+
+**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.
+
+</details>
+
+---
+
+## ✅ 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.**

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();
 		}

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];
 					},

src/Controls/src/Core/NavigationProxy.cs

@@ -252,7 +252,6 @@ protected virtual void OnRemovePage(Page page)
 			{
 				currentInner.RemovePage(page);
 			}
-			page?.DisconnectHandlers();
 		}
 
 		Page Pop()

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

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<Page>());
+
+			// 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()
 		{

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);
 	}