-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[iOS] Fixed NavigationStack not updating when OnAppearing is invoked #28666
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this 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 fixes an issue where the NavigationStack was not correctly updated on iOS when the OnAppearing event was triggered. The changes include:
- Reordering the FireAppearing call in NavigationPage.Legacy.cs to occur after the page is removed from the stack.
- Adding corresponding tests in both TestCases.Shared.Tests and TestCases.HostApp to verify the updated navigation behavior.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue28414.cs | Adds a test verifying the NavigationStack count after a page pop. |
| src/Controls/tests/TestCases.HostApp/Issues/Issue28414.cs | Sets up pages and navigation steps to validate the OnAppearing behavior. |
| src/Controls/src/Core/NavigationPage/NavigationPage.Legacy.cs | Adjusts the logic for firing the OnAppearing event after removing a page. |
bhavanesh2001
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bhavanesh2001 I have updated the code changes to resolve the test case failures. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
build seems a bit stuck? |
|
this needs re-running as the pipeline agents got disconnected |
|
nobody? @jsuarezruiz |
|
@MitchBomcanhao I think the pipelines will be run if someone reviewing this feels it’s necessary. It's possible the team is currently busy with other PRs. |
|
they tried to run it once and it hasn't worked. they've been ignoring it since... the build agent failed, the build needs to be triggered again. it isn't much effort for someone to stop this and trigger it again. nobody is that busy. this PR possibly fixes an important issue for us and to see it be ignored is quite disconcerting. |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
bump |
|
nobody? |
|
bump |
|
hello? |
|
/rebase |
…hen the OnAppearing method is called
6fd89dd to
d78db48
Compare
PureWeen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to use a unit test vs a UITest? Since this is all xplat code we should be able to reproduce inside a test
…28666) * Fixed - 28414 : iOS] Popping a page includes in the NavigationStack when the OnAppearing method is called * updated NavigationPage.Legacy.cs * Updated NavigationPage.Legacy.cs
…28666) * Fixed - 28414 : iOS] Popping a page includes in the NavigationStack when the OnAppearing method is called * updated NavigationPage.Legacy.cs * Updated NavigationPage.Legacy.cs
<!-- Please let the below note in for people that find this PR --> > [!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 In the failed test `AppDoesntCrashWhenSettingNewTitleViewOnExistingPage`, the TitleView was updated in the OnAppearing method. When popping the page using PopAsync, the OnAppearing event is triggered twice with my fix, whereas previously it was triggered only once: once from [FireAppearing](https://github.com/dotnet/maui/blob/b2c3452f68298f407ca18f8fe9e0f8841e7c9cad/src/Controls/src/Core/NavigationPage/NavigationPage.Legacy.cs#L67) and once from [CurrentPageChanged](https://github.com/dotnet/maui/blob/b2c3452f68298f407ca18f8fe9e0f8841e7c9cad/src/Controls/src/Core/NavigationPage/NavigationPage.Legacy.cs#L70). This additional trigger causes the title to mismatch the expected value, resulting in the test failure. ### Root Cause The fix moves FireAppearing to after RemoveFromInnerChildren(page), but at that point InternalChildren.Last() has already changed. Currently, StackDepth - 2 is calculated based on the original stack depth. Once the popped page is removed from stack, accessing InternalChildren[StackDepth - 2] is not valid. ### Description of Change Remove the page first, then use InternalChildren.Last(); directly to get the correct page that should appear. Regressed PR: #28666 Fixes AppDoesntCrashWhenSettingNewTitleViewOnExistingPage test failure in PR: #31327 ### Screenshots <img width="540" height="300" alt="image" src="https://github.com/user-attachments/assets/9dfaed05-0b66-489b-9b81-9a285088cc96" /> <img width="540" height="300" alt="image" src="https://github.com/user-attachments/assets/81626605-6567-4a30-b183-d26513d5f60c" /> <img width="540" height="300" alt="image" src="https://github.com/user-attachments/assets/260ca192-4baa-45e2-9991-8abcdadb2961" />
…28666) * Fixed - 28414 : iOS] Popping a page includes in the NavigationStack when the OnAppearing method is called * updated NavigationPage.Legacy.cs * Updated NavigationPage.Legacy.cs
…28666) * Fixed - 28414 : iOS] Popping a page includes in the NavigationStack when the OnAppearing method is called * updated NavigationPage.Legacy.cs * Updated NavigationPage.Legacy.cs


Issue Detail
The NavigationStack was not updated when OnAppearing was invoked.
Root Cause
The OnAppearing event was triggered before the popped page was removed from the stack in the RemoveAsyncInner method. This method is triggered on iOS when popping pages.
Description of Change
Triggering the OnAppearing event after updating the stack resolves the issue.
Tested the behavior in the following platforms
Issues Fixed
Fixes #28414
Screenshots
28414Before.mov
28414After.mov