Skip to content

Conversation

@jsuarezruiz
Copy link
Contributor

@jsuarezruiz jsuarezruiz commented Mar 13, 2025

Description of Change

Changes in navigation events parameters. This PR apply the following changes:

NavigatedToEventArgs

  • Make PreviousPage property public.
  • Added NavigationType property.

NavigatingFromEventArgs

  • Added DestinationPage property.
  • Added NavigationType property.

image

	internal enum NavigationType
	{
		Push,
		Pop,
		PopToRoot,

		Insert, // remove this? 
		Remove, // remove this?

		Replace, // TabbedPage, FlyoutPage, Swapping out the Window.Page

		Initial  // remove this?
	}
  • Review what fires on TabbedPage and children pages when they are initially set

Issues Fixed

Fixes #21814

@jsuarezruiz jsuarezruiz added t/enhancement ☀️ New feature or request area-navigation NavigationPage labels Mar 13, 2025
Copilot AI review requested due to automatic review settings March 13, 2025 14:23
@jsuarezruiz jsuarezruiz requested a review from a team as a code owner March 13, 2025 14:23

[Test]
[Category(UITestCategories.Navigation)]
public void VerifyNavigationEventArgs()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test running:
navigation-args-parameters

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the navigation event argument classes and their corresponding usages by adding a new NavigationType parameter and exposing additional properties.

  • Updated NavigatedToEventArgs and NavigatingFromEventArgs with a NavigationType parameter and made PreviousPage public.
  • Propagated the changes across NavigationPage, NavigationPage.Legacy, ModalNavigationManager, FlyoutPage, NavigationModel, and MultiPage to pass appropriate NavigationType values during navigation events.

Reviewed Changes

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

Show a summary per file
File Description
src/Controls/src/Core/Page/NavigatingFromEventArgs.cs Added NavigationType property and updated constructor to accept destination page and navigation type
src/Controls/src/Core/Page/NavigatedToEventArgs.cs Updated constructor signature to include NavigationType and made PreviousPage public
src/Controls/src/Core/NavigationPage/NavigationPage.cs Updated calls to SendNavigated and SendNavigating with NavigationType parameters
src/Controls/src/Core/NavigationPage/NavigationPage.Legacy.cs Updated legacy navigation methods to use the new NavigationType parameters
src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.cs Updated modal navigation lifecycle calls with NavigationType parameters
src/Controls/src/Core/Page/NavigatedFromEventArgs.cs Changed Visibility of NavigationType enum and related properties from internal to public
src/Controls/src/Core/FlyoutPage/FlyoutPage.cs Updated flyout navigation event calls with a NavigationType to indicate PageSwap events
src/Controls/src/Core/NavigationModel.cs Updated modal navigation events with explicit NavigationType parameters
src/Controls/src/Core/MultiPage.cs Amended current page change events to pass NavigationType for PageSwap events

Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should discuss this more as a team before adding these APIs

},
() =>
{
// TODO this is the wrong navigation type
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was passing the wrong navigation type parameter. If we not decide to update the navigation EventArgs with the properties included in this PR, the changes to fix this should be included in another one.

@PureWeen
Copy link
Member

One area I'm curious about with this one is obsoleting and making sure we aren't ending up with duplicate enums

For example, we already haveNavigationRequestType which is hidden with EditorBrowsable but we might want to obsolete that as well?

@rmarinho
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen moved this from Todo to Ready To Review in MAUI SDK Ongoing May 20, 2025
rmarinho
rmarinho previously approved these changes May 27, 2025
@mattleibow mattleibow added the do-not-merge Don't merge this PR label May 30, 2025
@mattleibow
Copy link
Member

I am adding a do not merge label as @PureWeen said things about discussions. Not sure if that happened, but just in case...

@jsuarezruiz
Copy link
Contributor Author

One area I'm curious about with this one is obsoleting and making sure we aren't ending up with duplicate enums

For example, we already haveNavigationRequestType which is hidden with EditorBrowsable but we might want to obsolete that as well?

In NavigatedFromEventArgs, we have been using the NavigationType enum for defining navigation types. (Reference).

This PR introduces two key improvements:

  • Making NavigationType public.
  • Expanding its scope to use it across all the navigation events.

Compared to using NavigationRequestType everywhere (Reference), this approach results in less disruptive changes, maintaining compatibility while refining consistency.

However, both enums are nearly identical, except for certain differences like PageSwap value. Since we are already introducing breaking changes in .NET 10, this is an opportunity to standardize the navigation enum by maintaining a single, unified type for all cases where navigation type needs to be indicated.

@jsuarezruiz
Copy link
Contributor Author

I am adding a do not merge label as @PureWeen said things about discussions. Not sure if that happened, but just in case...

Not yet, we have to review it.

@PureWeen
Copy link
Member

PureWeen commented Jun 4, 2025

Discuss naming "PageSwap" and the existence of other types like ShellNavigationSource

@jsuarezruiz jsuarezruiz marked this pull request as draft June 5, 2025 10:34
@jsuarezruiz
Copy link
Contributor Author

jsuarezruiz commented Jun 17, 2025

Based on recently added tests, I've identified a platform discrepancy when using FlyoutPage. Specifically:

  • On Android, the initial NavigationType is set to Push, triggered by: Owner.SendNavigated(previousPage, NavigationType.Push); from the protected override Task OnPushAsync(Page root, bool animated) NavigationPage method.
  • On iOS, it's set to Replace, using: _detail?.SendNavigatedTo(new NavigatedToEventArgs(previousDetail, NavigationType.Replace)); within the public Page Detail property of Flyout.

This inconsistency leads to different navigation behavior across platforms for the same layout construct, and failing tests.

We must align this behavior so that navigation triggered by changing the FlyoutItem consistently uses NavigationType.Replace, just like it does when switching tabs with TabbedPage. Is that the expected behavior @PureWeen? That would better reflect the nature of layout-level navigation and ensure reliable cross-platform behavior.

@PureWeen
Copy link
Member

PureWeen commented Aug 1, 2025

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@github-project-automation github-project-automation bot moved this from Changes Requested to Approved in MAUI SDK Ongoing Aug 1, 2025
@PureWeen PureWeen merged commit 42f1f20 into net10.0 Aug 1, 2025
129 checks passed
@PureWeen PureWeen deleted the fix-21814 branch August 1, 2025 19:06
@github-project-automation github-project-automation bot moved this from Approved to Done in MAUI SDK Ongoing Aug 1, 2025
_detail?.SendNavigatingFrom(new NavigatingFromEventArgs());

// Get the actual pages for navigation events (unwrap NavigationPages)
var destinationPage =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ederbond
Copy link
Contributor

ederbond commented Aug 1, 2025

Finally, can't wait to get it on net 10 GA so I can use Shell without breaking MVVM nor causing memory leak because of extensive usage of Rx.NET 🙏🏼

@github-actions github-actions bot locked and limited conversation to collaborators Sep 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-navigation NavigationPage p/0 Work that we can't release without proposal/open t/enhancement ☀️ New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants