[Android] Overriding back button functionality with OnBackButtonPressed returning false in a modally pushed page causes stack overflow - fix#28812
Conversation
|
Hey there @@kubaflo! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
| if (!preventBackPropagation) | ||
| { | ||
| customComponentDialog.OnBackPressedDispatcher.OnBackPressed(); | ||
| _ = _modalNavigationManager?.PopModalAsync(true); |
There was a problem hiding this comment.
Ensure that _modalNavigationManager is always non-null when invoked in HandleOnBackPressed. If there's any possibility for a null value, consider adding appropriate fallback handling or logging to maintain robust modal navigation behavior.
| WeakReference<CustomComponentDialog> _customComponentDialog; | ||
|
|
||
| public CallBack(bool enabled, CustomComponentDialog customComponentDialog) : base(enabled) | ||
| private ModalNavigationManager? _modalNavigationManager; |
There was a problem hiding this comment.
just because this CallBack is a Java.Lang.Object, can we store the ModalNavigationManager as a WeakReference?
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| App.WaitForElement("NavigateToDetailPage"); | ||
| App.Click("NavigateToDetailPage"); | ||
| App.WaitForElement("Issue28811DetailPage"); | ||
| App.Back(); |
There was a problem hiding this comment.
Yea probably, thanks!
rmarinho
left a comment
There was a problem hiding this comment.
OverridingBackButtonShouldNotCauseStackOverflow failing on iOS and Android, also please rebase :)
|
Can you please /azp again? |
|
/rebase |
|
/azp run |
12c50ef to
a195d3e
Compare
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/rebase |
a195d3e to
f848c82
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| { | ||
| customComponentDialog.OnBackPressedDispatcher.OnBackPressed(); | ||
| { | ||
| _ = mnm.PopModalAsync(true); |
There was a problem hiding this comment.
@kubaflo this one is a little interesting because the original author is returning false but they are opting out of all of our navigation logic
The fix here also feels incorrect because we are only running this test on android.
My first thought with how this would work is that if the user is overriding OnBackButotnPResed on page and they don't call the base class and then they return false
That by default we don't do anything at that point.
If they are opting out of calling our base class and returning false we can't really assume what to do for them at this point.
@pictos do you remember why we opted to call the OnBackPresed from the Dispatcher here?
There was a problem hiding this comment.
The Dispatcher is from AndroidX, just for reference. I think this was implemented to enable modal pages to listen for and handle back button presses. We register the ComponentDialog here, and when the callback is triggered, we forward that event into Maui's structure.
There was a problem hiding this comment.
There was a problem hiding this comment.
@kubaflo I think the fix here is to just delete this code completely
if (!preventBackPropagation && _modalNavigationManager.TryGetTarget(out var mnm))
{
customComponentDialog.OnBackPressedDispatcher.OnBackPressed();
}This whole situation is a bit of a mess :-/
I don't think the "OnBackButtonPressed" was fully thought through with Modal or Android really as we moved to MAUI
like, currently with a navigation page if you return false it'll cause a double back navigation and exit the app.
Technically from the API description I think the way you'd use this API would be more like this
protected override bool OnBackButtonPressed()
{
if (mycustomLogic)
{
return true;
}
else
{
return base.OnBackButtonPressed();
}
}I also worry about this change because if users call "base.OnBackButtonPressed" I think that's going to cause a double navigation back since the base class will also navigate back
[Android] ModalNavigationManager
Eliminated the preventBackPropagation variable and related event handler code from ModalNavigationManager.Android.cs, simplifying the OnBackPressed lifecycle event handling.
…ed returning false in a modally pushed page causes stack overflow - fix (#28812) * Update ModalNavigationManager.Android.cs [Android] ModalNavigationManager * Update ModalNavigationManager.Android.cs * Remove unused back propagation logic in Android modal manager Eliminated the preventBackPropagation variable and related event handler code from ModalNavigationManager.Android.cs, simplifying the OnBackPressed lifecycle event handling.
…ed returning false in a modally pushed page causes stack overflow - fix (#28812) * Update ModalNavigationManager.Android.cs [Android] ModalNavigationManager * Update ModalNavigationManager.Android.cs * Remove unused back propagation logic in Android modal manager Eliminated the preventBackPropagation variable and related event handler code from ModalNavigationManager.Android.cs, simplifying the OnBackPressed lifecycle event handling.
…ed returning false in a modally pushed page causes stack overflow - fix (#28812) * Update ModalNavigationManager.Android.cs [Android] ModalNavigationManager * Update ModalNavigationManager.Android.cs * Remove unused back propagation logic in Android modal manager Eliminated the preventBackPropagation variable and related event handler code from ModalNavigationManager.Android.cs, simplifying the OnBackPressed lifecycle event handling.
…ed returning false in a modally pushed page causes stack overflow - fix (#28812) * Update ModalNavigationManager.Android.cs [Android] ModalNavigationManager * Update ModalNavigationManager.Android.cs * Remove unused back propagation logic in Android modal manager Eliminated the preventBackPropagation variable and related event handler code from ModalNavigationManager.Android.cs, simplifying the OnBackPressed lifecycle event handling.


Issues Fixed
Fixes #28811
Screen.Recording.2025-04-05.at.02.58.12.mov
Screen.Recording.2025-04-05.at.03.00.04.mov
CC @pictos