Fix handler not disconnected when removing non visible pages using RemovePage() by Vignesh-SF3580 · Pull Request #32289 · dotnet/maui

Vignesh-SF3580 · 2025-10-30T11:01:03Z

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 a non-visible page is removed from the navigation stack using RemovePage(), its handler is not disconnected.

Root Cause

The RemovePage() method removes the page from the navigation stack but does not explicitly disconnect its handler.

Description of Change

The handler is now properly disconnected when a page is removed using RemovePage().

Regression PR: #24887

Tested the behavior in the following platforms

Android
Windows
iOS
Mac

Issues Fixed

Fixes #32239

Screenshots

Android:

Before Issue Fix	After Issue Fix
32269BeforeFix.mov	32269AfterFix.mov

iOS:

Before Issue Fix	After Issue Fix
32269iOSBeforeFix.mov	32269iOSAfterFix.mov

Copilot

Pull Request Overview

This PR fixes a bug where RemovePage fails to disconnect handlers when the removed page is not currently visible in the navigation stack. The fix ensures proper cleanup of page handlers to prevent memory leaks and resource issues.

Key changes:

Added DisconnectHandlers() call in both NavigationPage.cs and NavigationPage.Legacy.cs when a non-visible page is removed from the navigation stack
Added comprehensive UI test to verify handlers are properly disconnected when removing pages

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
`src/Controls/src/Core/NavigationPage/NavigationPage.cs`	Added handler disconnection in `OnRemovePage` method for non-visible pages
`src/Controls/src/Core/NavigationPage/NavigationPage.Legacy.cs`	Added handler disconnection in `RemovePage` method for non-visible pages (legacy implementation)
`src/Controls/tests/TestCases.HostApp/Issues/Issue32239.cs`	Created UI test page with navigation stack setup to reproduce the issue
`src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue32239.cs`	Created NUnit test to verify handler disconnection when removing non-visible pages

Copilot · 2025-11-03T11:48:31Z

src/Controls/src/Core/NavigationPage/NavigationPage.cs

+						// Only disconnect handlers if the removed page is not visible.
+						// When the current page is removed, the handler is disconnected through PopAsync().


The comment is misleading. According to the code logic above (lines 897-902), when the current page is removed, PopAsync() is called and the method returns early. This means the DisconnectHandlers() call on line 914 is ONLY executed for non-visible pages. The comment should be revised to: 'Disconnect handlers for the removed non-visible page. Note: When the current page is removed, the handler disconnection is handled through PopAsync() (see lines 897-902).' Alternatively, remove the second sentence entirely since it doesn't accurately describe what happens in this code path.

Suggested change

// Only disconnect handlers if the removed page is not visible.

// When the current page is removed, the handler is disconnected through PopAsync().

// Disconnect handlers for the removed non-visible page.

// Note: When the current page is removed, the handler disconnection is handled through PopAsync() (see lines 897-902).

Copilot · 2025-11-03T11:48:31Z

src/Controls/src/Core/NavigationPage/NavigationPage.Legacy.cs

+			// Only disconnect handlers if the removed page is not visible.
+			// When the current page is removed, the handler is disconnected through PopAsync().


The comment is misleading. According to the code logic above (lines 242-247), when the current page is removed, PopAsync() is called and the method returns early. This means the DisconnectHandlers() call on line 257 is ONLY executed for non-visible pages. The comment should be revised to: 'Disconnect handlers for the removed non-visible page. Note: When the current page is removed, the handler disconnection is handled through PopAsync() (see lines 242-247).' Alternatively, remove the second sentence entirely since it doesn't accurately describe what happens in this code path.

Suggested change

// Only disconnect handlers if the removed page is not visible.

// When the current page is removed, the handler is disconnected through PopAsync().

// Disconnect handlers for the removed non-visible page.

// Note: When the current page is removed, the handler disconnection is handled through PopAsync() (see lines 242-247).

src/Controls/src/Core/NavigationPage/NavigationPage.cs

Copilot · 2025-11-03T11:48:31Z

src/Controls/src/Core/NavigationPage/NavigationPage.Legacy.cs

+			// Only disconnect handlers if the removed page is not visible.
+			// When the current page is removed, the handler is disconnected through PopAsync().
+			page?.DisconnectHandlers();


This creates duplicate handler disconnection logic. The NavigationProxy.OnRemovePage method (line 255 in NavigationProxy.cs) already calls page?.DisconnectHandlers() after calling currentInner.RemovePage(page). This means when RemovePage is invoked through the navigation proxy, handlers will be disconnected twice - once in this implementation and once in the proxy. Consider whether this duplication is intentional or if the NavigationProxy call should be removed to avoid redundant disconnection attempts.

Suggested change

// Only disconnect handlers if the removed page is not visible.

// When the current page is removed, the handler is disconnected through PopAsync().

page?.DisconnectHandlers();

// Handler disconnection is now handled by the NavigationProxy.

I’ve removed the redundant disconnection logic from NavigationProxy and updated the implementation in ShellSection. This ensures that DisconnectHandler is called only once and avoids any duplicate disconnection.

sheiksyedm · 2025-11-06T06:30:20Z

/azp run MAUI-UITests-public

azure-pipelines · 2025-11-06T06:30:33Z

Azure Pipelines successfully started running 1 pipeline(s).

- Updated comments in NavigationPage.cs and NavigationPage.Legacy.cs to accurately describe the code flow - Added missing trailing newlines to test files

StephaneDelcroix

✅ Approved

This PR correctly fixes the handler disconnection issue when removing non-visible pages using RemovePage().

Summary of Changes

File	Change
`NavigationPage.cs`	Added `page?.DisconnectHandlers()` in `OnRemovePage`
`NavigationPage.Legacy.cs`	Added `page?.DisconnectHandlers()` in `RemovePage`
`ShellSection.cs`	Added `page?.DisconnectHandlers()` in `OnRemovePage`
`NavigationProxy.cs`	Removed duplicate `DisconnectHandlers()` call

Improvements Made

I've pushed a commit that:

Fixed misleading comments - Updated comments to accurately describe the code flow (this code path is only reached for non-visible pages)
Added missing trailing newlines - Fixed cosmetic issue in test files

Verification

✅ Fix addresses the root cause from regression PR #24887
✅ Proper test coverage with UI automation
✅ All four platforms tested by author
✅ Null-safe implementation
✅ No duplicate disconnection (NavigationProxy call removed)

Great work! 🚀

StephaneDelcroix

Requesting changes: Adding a unit test in Controls.Core.UnitTests to complement the UI test.

Added two unit tests to verify handler disconnection behavior: - RemovePageDisconnectsHandlerForNonVisiblePage: Verifies the page is properly removed from navigation stack - RemovePageCallsDisconnectHandlersOnRemovedPage: Verifies handler is disconnected when page with handler is removed

StephaneDelcroix

✅ Approved

This PR correctly fixes the handler disconnection issue when removing non-visible pages using RemovePage().

Summary of Changes

File	Change
`NavigationPage.cs`	Added `page?.DisconnectHandlers()` in `OnRemovePage`
`NavigationPage.Legacy.cs`	Added `page?.DisconnectHandlers()` in `RemovePage`
`ShellSection.cs`	Added `page?.DisconnectHandlers()` in `OnRemovePage`
`NavigationProxy.cs`	Removed duplicate `DisconnectHandlers()` call

Improvements Made

I've pushed commits that:

Fixed misleading comments - Updated comments to accurately describe the code flow
Added missing trailing newlines - Fixed cosmetic issue in test files
Added unit tests - New tests in NavigationUnitTest.cs:
- RemovePageDisconnectsHandlerForNonVisiblePage - Verifies page is properly removed from navigation stack
- RemovePageCallsDisconnectHandlersOnRemovedPage - Verifies handler is disconnected when page with handler is removed

Test Coverage

Test Type	Location	Purpose
Unit Test	`NavigationUnitTest.cs`	Verifies handler disconnection logic
UI Test	`Issue32239.cs`	End-to-end validation on device

Verification

✅ Fix addresses the root cause from regression PR #24887
✅ Unit tests pass locally
✅ All four platforms tested by author
✅ Null-safe implementation
✅ No duplicate disconnection (NavigationProxy call removed)

Great work! 🚀

The unit tests in NavigationUnitTest.cs adequately verify the handler disconnection behavior. The UI test adds maintenance burden without additional value.

StephaneDelcroix

✅ Approved

Clean, minimal fix for handler disconnection when removing non-visible pages.

Changes (5 files, +65/-1)

File	Change
`NavigationPage.cs`	Added `page?.DisconnectHandlers()` in `OnRemovePage`
`NavigationPage.Legacy.cs`	Added `page?.DisconnectHandlers()` in `RemovePage`
`ShellSection.cs`	Added `page?.DisconnectHandlers()` in `OnRemovePage`
`NavigationProxy.cs`	Removed duplicate call (moved to implementations)
`NavigationUnitTest.cs`	Added 2 unit tests

Unit Tests

RemovePageDisconnectsHandlerForNonVisiblePage - Verifies page removal from stack
RemovePageCallsDisconnectHandlersOnRemovedPage - Verifies handler disconnection

All tests pass. LGTM! 🚀

- Remove line number references from comments (prone to becoming stale) - Add handler disconnection verification to existing test - Rename test for clarity (RemovePageCallsDisconnectHandlersOnRemovedPage -> RemovePageDisconnectsHandlerForRemovedRootPage) - Add Shell-specific test for RemovePage handler disconnection

rmarinho

✅ Approved

This PR properly fixes the handler disconnection issue when using RemovePage() on non-visible pages.

Summary of Changes

Root cause fix: Moves DisconnectHandlers() from NavigationProxy to the concrete implementations (NavigationPage, NavigationPage.Legacy, ShellSection)
No duplicate disconnection: By removing from the proxy and adding to implementations, handlers are disconnected exactly once
Clean comments: Comments explain the behavior without brittle line number references

Test Coverage

✅ RemovePageDisconnectsHandlerForNonVisiblePage - Tests removing middle page from 3-page stack
✅ RemovePageDisconnectsHandlerForRemovedRootPage - Tests removing root page when another page is on top
✅ RemovePageDisconnectsHandlerInShell - Tests Shell navigation scenario

All tests pass for both useMaui: true and useMaui: false scenarios, covering both the modern and legacy navigation implementations.

LGTM! 🚀

PureWeen · 2026-01-07T14:32:51Z

/azp run

azure-pipelines · 2026-01-07T14:33:15Z

Azure Pipelines successfully started running 3 pipeline(s).

…movePage() (#32289)  > [!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! ### Issue Details When a non-visible page is removed from the navigation stack using RemovePage(), its handler is not disconnected. ### Root Cause The RemovePage() method removes the page from the navigation stack but does not explicitly disconnect its handler. ### Description of Change The handler is now properly disconnected when a page is removed using RemovePage(). Regression PR: #24887 ### Tested the behavior in the following platforms - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Issues Fixed Fixes #32239 ### Screenshots **Android:** | Before Issue Fix | After Issue Fix | |----------|----------| | <video width="300" height="600" src="https://github.com/user-attachments/assets/890c3e4b-f520-4826-b942-6ec9e04233d1"> | <video width="300" height="600" src="https://github.com/user-attachments/assets/40ebc025-7298-4b43-8816-4932a01fb563">) | **iOS:** | Before Issue Fix | After Issue Fix | |----------|----------| | <video width="300" height="600" src="https://github.com/user-attachments/assets/484e17cd-bd83-415e-af27-99acf893b203"> | <video width="300" height="600" src="https://github.com/user-attachments/assets/062a1c6d-047c-4bde-add2-d2d261763c87">) | --------- Co-authored-by: Stephane Delcroix <stephane@delcroix.org> Co-authored-by: Rui Marinho <me@ruimarinho.net> Co-authored-by: Shane Neuville <shneuvil@microsoft.com>

@HarishwaranVijayakumar

## What's Coming .NET MAUI inflight/candidate introduces significant improvements across all platforms with focus on quality, performance, and developer experience. This release includes 16 commits with various improvements, bug fixes, and enhancements. ## Checkbox - [Android] Implement material3 support for CheckBox by @HarishwaranVijayakumar in #33339 <details> <summary>🔧 Fixes</summary> - [Implement Material3 Support for CheckBox](#33338) </details> ## CollectionView - [Android] Fixed EmptyView doesn’t display when CollectionView is placed inside a VerticalStackLayout by @NanthiniMahalingam in #33134 <details> <summary>🔧 Fixes</summary> - [CollectionView does not show an EmptyView template with an empty collection](#32932) </details> ## Essentials - [Windows]Fix NullReferenceException in OpenReadAsync for FileResult created with full path by @devanathan-vaithiyanathan in #28238 <details> <summary>🔧 Fixes</summary> - [[Windows] FileResult(string fullPath) not initialized properly](#26858) </details> ## Image - Fix Glide IllegalArgumentException in MauiCustomTarget.clear() for destroyed activities by @jfversluis via @Copilot in #29780 <details> <summary>🔧 Fixes</summary> - [java.lang.IllegalArgumentException: You cannot start a load for a destroyed activity - glide](#29699) </details> ## Label - [Android] Fix for Label WordWrap width issue causing HorizontalOptions misalignment by @praveenkumarkarunanithi in #33281 <details> <summary>🔧 Fixes</summary> - [[Android] Unexpected Line Breaks in Android, Label with WordWrap Mode Due to Trailing Space.](#31782) - [Label not sized correctly on Android](#27614) </details> - Fix to Improve Flyout Accessibility by Adjusting UITableViewController Labels by @SuthiYuvaraj in #31619 <details> <summary>🔧 Fixes</summary> - [Navigation section present under hamburger are programmatically define as table :A11y_.NET maui_User can get all the insights of Dashboard_Devtools](#30894) </details> ## Mediapicker - [Regression][iOS] Fix MediaPicker PickPhotosAsync getting file name in contentType property by @devanathan-vaithiyanathan in #33390 <details> <summary>🔧 Fixes</summary> - [[iOS] MediaPicker PickPhotosAsync getting file name in contentType property](#33348) </details> ## Navigation - Fix handler not disconnected when removing non visible pages using RemovePage() by @Vignesh-SF3580 in #32289 <details> <summary>🔧 Fixes</summary> - [NavigationPage.Navigation.RemovePage() fails to disconnect handlers when removing pages, unlike ContentPage.Navigation.RemovePage()](#32239) </details> ## Picker - [Android] Fix Picker IsOpen not reset when picker is dismissed by @devanathan-vaithiyanathan in #33332 <details> <summary>🔧 Fixes</summary> - [[Android] Picker IsOpen not reset when picker is dismissed](#33331) </details> ## Shell - [iOS & Catalyst ] Fixed IsEnabled property should work on Tabs by @SubhikshaSf4851 in #33369 <details> <summary>🔧 Fixes</summary> - [[Catalyst] TabBarBackgroundColor, TabBarUnselectedColor, and IsEnabled Not Working as Expected in Shell](#33158) </details> - [iOS,Windows] Fix navigation bar colors not resetting when switching ShellContent by @Vignesh-SF3580 in #33228 <details> <summary>🔧 Fixes</summary> - [[iOS, Windows] Shell Navigation bar colors are not updated correctly when switching ShellContent](#33227) </details> - [iOS] Fixed Shell navigation on search handler suggestion selection by @SubhikshaSf4851 in #33406 <details> <summary>🔧 Fixes</summary> - [[iOS] Clicking on search suggestions fails to navigate to detail page correctly](#33356) </details> ## Templates - Fix VoiceOver doesnot announces the State of the ComboBox by @SuthiYuvaraj in #32286 ## Xaml - [XSG][BindingSourceGen] Add support for CommunityToolkit.Mvvm ObservablePropertyAttribute by @simonrozsival via @Copilot in #33028 <details> <summary>🔧 Fixes</summary> - [[XSG] Add heuristic to support bindable properties generated by other source generators](#32597) </details> <details> <summary>📦 Other (2)</summary> - [XSG] Improve diagnostic reporting during binding compilation by @simonrozsival via @Copilot in #32905 - [Testing] Fixed Test case failure in PR 33574 - [01/19/2026] Candidate - 1 by @TamilarasanSF4853 in #33602 </details> **Full Changelog**: main...inflight/candidate

Vignesh-SF3580 added 5 commits October 29, 2025 17:34

fixed - disconnect handlers when removing pages using RemovePage

48fda91

updated the changes

be86aa9

added comments.

c480ac0

added test.

7b88676

Update Issue32239.cs

fc5303d

dotnet-policy-service bot added the partner/syncfusion Issues / PR's with Syncfusion collaboration label Oct 30, 2025

updated the fix.

89c1062

Vignesh-SF3580 marked this pull request as ready for review November 3, 2025 11:38

Copilot AI review requested due to automatic review settings November 3, 2025 11:38

Tamilarasan-Paranthaman added area-navigation NavigationPage platform/macos macOS / Mac Catalyst platform/windows platform/android platform/ios labels Nov 3, 2025

Copilot AI reviewed Nov 3, 2025

View reviewed changes

updated

df33c57

Vignesh-SF3580 added the community ✨ Community Contribution label Dec 3, 2025

Improve comments and fix missing newlines

d932fde

- Updated comments in NavigationPage.cs and NavigationPage.Legacy.cs to accurately describe the code flow - Added missing trailing newlines to test files

StephaneDelcroix approved these changes Jan 7, 2026

View reviewed changes

StephaneDelcroix requested changes Jan 7, 2026

View reviewed changes

StephaneDelcroix previously approved these changes Jan 7, 2026

View reviewed changes

Remove redundant UI test

5471df1

The unit tests in NavigationUnitTest.cs adequately verify the handler disconnection behavior. The UI test adds maintenance burden without additional value.

StephaneDelcroix dismissed their stale review via 5471df1 January 7, 2026 09:09

StephaneDelcroix previously approved these changes Jan 7, 2026

View reviewed changes

rmarinho dismissed StephaneDelcroix’s stale review via 1bee97c January 7, 2026 09:38

rmarinho previously approved these changes Jan 7, 2026

View reviewed changes

Add agent review session for PR dotnet#32289

d3b1db1

PureWeen dismissed rmarinho’s stale review via d3b1db1 January 7, 2026 16:52

PureWeen approved these changes Jan 7, 2026

View reviewed changes

PureWeen changed the base branch from main to inflight/current January 7, 2026 20:24

PureWeen merged commit f37b9d3 into dotnet:inflight/current Jan 7, 2026
11 of 23 checks passed

PureWeen mentioned this pull request Jan 23, 2026

January 19th, Candidate #33574

Merged

github-actions bot locked and limited conversation to collaborators Feb 7, 2026

		// Only disconnect handlers if the removed page is not visible.
		// When the current page is removed, the handler is disconnected through PopAsync().

Conversation

Vignesh-SF3580 commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Details

Root Cause

Description of Change

Tested the behavior in the following platforms

Issues Fixed

Screenshots

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Vignesh-SF3580 Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

sheiksyedm commented Nov 6, 2025

Uh oh!

azure-pipelines bot commented Nov 6, 2025

Uh oh!

StephaneDelcroix left a comment

Choose a reason for hiding this comment

✅ Approved

Summary of Changes

Improvements Made

Verification

Uh oh!

StephaneDelcroix left a comment

Choose a reason for hiding this comment

Uh oh!

StephaneDelcroix left a comment

Choose a reason for hiding this comment

✅ Approved

Summary of Changes

Improvements Made

Test Coverage

Verification

Uh oh!

StephaneDelcroix left a comment

Choose a reason for hiding this comment

✅ Approved

Changes (5 files, +65/-1)

Unit Tests

Uh oh!

rmarinho left a comment

Choose a reason for hiding this comment

✅ Approved

Summary of Changes

Test Coverage

Uh oh!

PureWeen commented Jan 7, 2026

Uh oh!

azure-pipelines bot commented Jan 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments

Vignesh-SF3580 commented Oct 30, 2025 •

edited

Loading