[Android,Windows] Fix NavigatingFrom event order inconsistency with PushAsync #31536
Conversation
|
Hey there @@Vignesh-SF3580! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
dotnet-policy-service
bot
added
the
partner/syncfusion
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an inconsistency in the NavigatingFrom event timing when using PushAsync navigation on Android and Windows platforms. The NavigatingFrom event was being triggered after the page was pushed to the navigation stack, but it should be triggered before the push operation to maintain consistency with other platforms and navigation methods.
- Moved the NavigatingFrom event invocation to occur before the page is pushed to the navigation stack
- Added comprehensive UI tests to validate the correct event ordering
- Fixed the timing inconsistency across Android and Windows platforms
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Controls/src/Core/NavigationPage/NavigationPage.cs | Moved SendNavigating call to occur before PushPage operation in the navigation stack update |
| src/Controls/tests/TestCases.HostApp/Issues/Issue31520.cs | Added UI test page that demonstrates and validates NavigatingFrom event timing relative to Disappearing event |
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue31520.cs | Added NUnit test that verifies NavigatingFrom triggers before Disappearing event during navigation |
| [Issue(IssueTracker.Github, 31520, "NavigatingFrom is triggered first when using PushAsync", PlatformAffected.Android)] | ||
| public class Issue31520 : NavigationPage | ||
| { | ||
| public static int count = 0; |
There was a problem hiding this comment.
Using a static counter variable can lead to issues if tests run in parallel or if state persists between test runs. Consider using instance variables or resetting the counter in test setup to ensure test isolation.
| return Owner.SendHandlerUpdateAsync(animated, | ||
| () => | ||
| { | ||
| // Move the SendNavigating here so that it's fired prior to the stack being modified |
There was a problem hiding this comment.
Can include in the comment something like:
"This ensures consistent event ordering across all platforms (iOS, Catalyst, Android, Windows)"
There was a problem hiding this comment.
@jsuarezruiz I’ve added the comment as suggested.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/rebase |
github-actions
bot
force-pushed
the
fix-navigatingPushAsync
branch
from
f86c66e to
b86ec9d
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
…ushAsync (#31536) * Update NavigationPage.cs * Added test * Update NavigationPage.cs * Update NavigationPage.cs
…ushAsync (#31536) * Update NavigationPage.cs * Added test * Update NavigationPage.cs * Update NavigationPage.cs
…ushAsync (#31536) * Update NavigationPage.cs * Added test * Update NavigationPage.cs * Update NavigationPage.cs
3 participants
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 Detail
NavigatingFrom event order is inconsistent across platforms when using PushAsync.
Root Cause
The NavigatingFrom event is currently invoked after the page is pushed.
Description of Change
Moved the NavigatingFrom event to be invoked before pushing the page.
Comment: #28998 (comment)
Reference:
iOS: https://github.com/dotnet/maui/blob/e838b500c3f62c579d97bcbaeacf321dc1632d2c/src/Controls/src/Core/NavigationPage/NavigationPage.Legacy.cs#L186
Modal Navigation: https://github.com/dotnet/maui/blob/main/src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.cs#L268
Tested the behavior in the following platforms
Issues Fixed
Fixes #31520
Screenshots
31520BeforeFix.mov
31520AfterFix.mov