-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[Android] Fix Flickering issue when calling Navigation.PopAsync #24887
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.
Seems is making the Windows Device tests fail , I run few times and got same results. Maybe this is causing some side effects on Windows. But maybe also rebase just to make sure branch is updated with latest .
/rebase |
16c0e0d
to
22d7398
Compare
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
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.
This is the failing test NavigatedFiresAfterSwitchingFlyoutItems.
|
will this ptach be merged into dotnet 8 maui? |
@ygl-rg I think at this point with .NET 9 being close to being released this will be .NET 9 only |
/rebase |
/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.
Azure Pipelines successfully started running 3 pipeline(s). |
/rebase |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Changes Applied
@@ -402,9 +402,6 @@ void FireAppearing(Page page) | |||
void RemoveFromInnerChildren(Element page) | |||
{ | |||
InternalChildren.Remove(page); | |||
|
|||
// TODO For NET9 we should remove this because the DisconnectHandlers will take care of it | |||
page.Handler = null; |
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.
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.
Hi! @devanathan-vaithiyanathan can you please fix it? It is an annoying bug that would be great to have fixed as soon as possible. I can help you if you want :)
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.
@PureWeen I have updated the code to include null checks for MauiContext to address the WinUI device test crashes. Could you please review the changes and let me know if you have any further concerns?
…sLayout is set for Tablet but not on mobile devices (dotnet#26152)" This reverts commit 0ddc794.
…msLayout is set for Tablet but not on mobile devices (dotnet#26152)" This reverts commit 4b9074c.
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
@@ -19,7 +19,7 @@ public partial class TabbedPage | |||
WFrame? _navigationFrame; | |||
bool _connectedToHandler; | |||
WFrame NavigationFrame => _navigationFrame ?? throw new ArgumentNullException(nameof(NavigationFrame)); | |||
IMauiContext MauiContext => this.Handler?.MauiContext ?? throw new InvalidOperationException("MauiContext cannot be null here"); | |||
IMauiContext? MauiContext => this.Handler?.MauiContext; |
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.
I think we want to leave the exception
Can you just add a method to check nullability?
bool HasHandler => this.Handler?.MauiContext is not null
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.
@PureWeen MauiContext reference is used in multiple places in this file, and I have ensured that null checks are added to prevent exceptions. Do we need to assign this.Handler?.MauiContext based on HasHandler in each place?
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.
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.
@PureWeen Upon checking further, the SelectionChanged event was unhooked properly during navigation, but the _navigationFrame.Navigated event was not. Since I am unable to reproduce the exact crash, based on the call stack from the dump files, I suspect that the issue occurs because the Navigated event was not unhooked properly. I have now unhooked the Navigated event correctly. If the Navigated event is properly unhooked during the disconnect handling, it will prevent the UpdateCurrentPageContent method from being called, even when triggered from the selection changing event. Could you please review and share your concerns?
/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.
Love it!!! Thank you
Root Cause
The flickering during navigation occurred because, when performing PopAsync, the removed page’s handlers were set to null. This caused flickering as the navigation was processed.
Description of Change
The fix involves avoiding the removal of page handlers during PopAsync, which eliminates the flickering issue during navigation.
Issues Fixed
Fixes #13810
Validated the behaviour in the following platforms
Output Screenshot
Before
366413807-f0592d6d-6b89-48b0-bab8-eed46d20336c.mp4
After
366413828-b47e24f4-68f0-44a0-b1bf-ed8dd24d1dcf.mp4