Skip to content

Fix OnNavigatedTo not called when navigating back to a specific page#25552

Merged
PureWeen merged 11 commits intodotnet:mainfrom
devanathan-vaithiyanathan:fix-25371
Jan 15, 2025
Merged

Fix OnNavigatedTo not called when navigating back to a specific page#25552
PureWeen merged 11 commits intodotnet:mainfrom
devanathan-vaithiyanathan:fix-25371

Conversation

@devanathan-vaithiyanathan
Copy link
Contributor

@devanathan-vaithiyanathan devanathan-vaithiyanathan commented Oct 28, 2024

Root Cause

When the back button is pressed and hold, and then released, the navigation pop action is not recognized as a user-initiated action.

Description of Change

To address an issue where the navigation pop action (when the back button is pressed and held, then released) was not being recognized as initiated by the user. Because the _uiRequestedPop field was not being set to true. The _uiRequestedPop field is used to recognize user tap actions on the back button. By setting _uiRequestedPop to true, the inner navigation actions are performed correctly, ensuring that the navigation pop action is recognized

Issues Fixed

Fixes #25371

Tested the behaviour in the following platforms

  • Android
  • Windows
  • iOS
  • Mac

Output Screenshot

Before After
BeforeIssue.mp4
AfterIssue.mp4

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Oct 28, 2024
@dotnet-policy-service
Copy link
Contributor

Hey there @devanathan-vaithiyanathan! 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 dotnet deleted a comment from azure-pipelines bot Oct 29, 2024
@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Shalini-Ashokan
Copy link
Contributor

/rebase

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines

This comment was marked as outdated.

@Shalini-Ashokan
Copy link
Contributor

/rebase

@jsuarezruiz
Copy link
Contributor

jsuarezruiz commented Nov 20, 2024

/azp run

@azure-pipelines

This comment was marked as off-topic.

@sheiksyedm sheiksyedm added the partner/syncfusion Issues / PR's with Syncfusion collaboration label Dec 6, 2024
@jfversluis jfversluis added the area-navigation NavigationPage label Dec 9, 2024
@jfversluis jfversluis requested a review from PureWeen January 8, 2025 13:07
@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

if (_navigation.TryGetTarget(out navRenderer))
{
if (!navRenderer._uiRequestedPop)
navRenderer._uiRequestedPop = true;
Copy link
Member

Choose a reason for hiding this comment

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

One thing I noticed is that this code is called if you just pop the page off the stack vs click the back button.

So, on the second page of this app if you add a button that calls "Navigation.popasync" this gets transitioned to true but then never transitions back to false.

if we change to instead use DidPopItem vs ShouldPopItem does that make this all work a bit better?

[Export("navigationBar:didPopItem:")]
		[Internals.Preserve(Conditional = true)]
		internal bool DidPopItem(UINavigationBar _, UINavigationItem __)
		{
            if (_ignorePopCall) return;
            _uiRequestedPop = true;
			return true;
		}

Copy link
Contributor

Choose a reason for hiding this comment

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

@PureWeen, In the current source, the issue is not reproducible. The issue was fixed by the changes in this  PR, so can we close this PR or consider test case alone?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, I like the idea of just taking the test case.

Let's just get this PR merged with the test case since the other PR didn't have one

Copy link
Contributor

Choose a reason for hiding this comment

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

@PureWeen, I have removed the fix. The PR now contains only the test case. Could you please review it?

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen added this to the .NET 9 SR4 milestone Jan 14, 2025
@PureWeen
Copy link
Member

  • failing tests unrelated. This PR only added a test that runs on iOS

@PureWeen PureWeen merged commit 0c59016 into dotnet:main Jan 15, 2025
101 of 105 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 15, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-navigation NavigationPage community ✨ Community Contribution partner/syncfusion Issues / PR's with Syncfusion collaboration platform/ios

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

OnNavigatedTo not called when navigating back to a specific page

7 participants