Skip to content
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

[Android] Fix: Modal Animation Repeats When Returning from Background #28538

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bhavanesh2001
Copy link
Contributor

Description of Change

This PR provides a fix for #28492 and is an alternative to #28522. It ensures that modal animations only play once when the modal is first displayed and do not replay when the app returns from the background.

Rather than starting the animation in OnStart(),this fix attaches a GlobalLayoutListener to the view inside OnCreateView(). When the view's layout is complete for the first time, the listener invokes the animation and then removes itself to prevent any further invocations. Additionally, the OnAnimationEnded method has been moved out of the lambda for improved readability.

After Fix:

fix.mov

Issues Fixed

Fixes #28492

Related PR #28522

@bhavanesh2001 bhavanesh2001 requested a review from a team as a code owner March 20, 2025 13:45
Copy link
Contributor

Hey there @bhavanesh2001! 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-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Mar 20, 2025
@jsuarezruiz jsuarezruiz added area-core-lifecycle XPlat and Native UIApplicationDelegate/Activity/Window lifecycle events platform/android 🤖 area-controls-modal labels Mar 20, 2025
Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

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

Could include an UITest like
https://github.com/user-attachments/assets/bf28a45f-dcde-4b26-bdb3-1342dc9d4a3c
?

In the test, can use:

App.BackgroundApp();
App.ForegroundApp();

To move the App to background etc.

@bhavanesh2001
Copy link
Contributor Author

@jsuarezruiz Would it be possible to include a UI test for this? Since the issue is that the modal animation replays when returning from the background, how can we verify in an automated test that the animation has not been triggered again?

@@ -322,10 +322,28 @@ public override AView OnCreateView(LayoutInflater inflater, ViewGroup? container
_navigationRootManager.Connect(_modal, modalContext);

UpdateBackgroundColor();
if (IsAnimated && _navigationRootManager is not null && _navigationRootManager.RootView is not null)
{
_ = new GenericGlobalLayoutListener((listner,view) =>
Copy link
Contributor

@pictos pictos Mar 20, 2025

Choose a reason for hiding this comment

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

is it fine to not dispose this GenericGlobalLayoutListener? would be good to add double check with a memory leak test, if the existing ones don't test for that

Copy link
Contributor Author

@bhavanesh2001 bhavanesh2001 Mar 20, 2025

Choose a reason for hiding this comment

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

@pictos I added a commit to explicitly dispose the listener once it's no longer needed. Does this look like the correct approach for cleanup?

Copy link
Contributor Author

@bhavanesh2001 bhavanesh2001 Mar 20, 2025

Choose a reason for hiding this comment

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

On the other hand, I don't think it's a good idea to call Dispose() manually, so I reverted the commit. Since we are already removing it from ViewTreeObserver, it should be cleaned up automatically.

Copy link
Member

Choose a reason for hiding this comment

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

GenericGlobalLayoutListener has an Invalidate method you can call which won't dispose of it but still cleans it up

I think we should use that vs the calling Remove on line 329

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PureWeen Thanks for the suggestion

@bhavanesh2001 bhavanesh2001 requested a review from pictos March 20, 2025 15:06
@kubaflo
Copy link
Contributor

kubaflo commented Mar 20, 2025

I think GenericGlobalLayoutListener will cause memory leaks

@kubaflo
Copy link
Contributor

kubaflo commented Mar 20, 2025

IMO the animation should start in the OnStart() method like @pictos initially implemented.

GlobalLayoutListener is typically used to detect changes in the view's layout. Using it solely to start an animation adds unnecessary complexity.

@bhavanesh2001
Copy link
Contributor Author

@kubaflo I get that GlobalLayoutListener might not seem like the perfect fit, but using OnStart() for this isn’t a good alternative either. Relying on flags isn't a clean solution as it introduces unnecessary state tracking instead of addressing the root cause directly, which is why I explored a different approach in this PR.

@mattleibow mattleibow added this to the .NET 9 SR6 milestone Mar 21, 2025
@bhavanesh2001
Copy link
Contributor Author

I think GenericGlobalLayoutListener will cause memory leaks

@kubaflo I’ve added a memory leak test for when a modal is opened with animation, and it passes with the GenericGlobalLayoutListener implementation.

device_test.mov

@jsuarezruiz could you run pipelines on this?

@bhavanesh2001 bhavanesh2001 requested a review from PureWeen March 21, 2025 16:16
@PureWeen PureWeen modified the milestones: .NET 9 SR6, .NET 9 SR7 Mar 24, 2025
@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

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

The test Issue31366PushingAndPoppingModallyCausesArgumentOutOfRangeException is failing with a crash:

image

The app was expected to be running still, investigate as possible crash

Could you review if is related with the changes?

@bhavanesh2001
Copy link
Contributor Author

bhavanesh2001 commented Mar 24, 2025

@jsuarezruiz fixed failing tests.
Screenshot 2025-03-24 at 7 58 25 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-modal area-core-lifecycle XPlat and Native UIApplicationDelegate/Activity/Window lifecycle events community ✨ Community Contribution platform/android 🤖
Projects
Status: Changes Requested
Development

Successfully merging this pull request may close these issues.

[Android] Modal Animation Repeats When Returning from Background
6 participants