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

fix(nativeframepresenter): animate pages depending on NavigationMode #19587

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ramezgerges
Copy link
Contributor

GitHub Issue (If applicable): closes #19574

PR Type

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@github-actions github-actions bot added the platform/android 🤖 Categorizes an issue or PR as relevant to the Android platform label Feb 24, 2025
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-19587/index.html


if (oldPage is not null)
{
if (GetIsAnimated(oldTransitionInfo))
if (args.NavigationMode is NavigationMode.Back && GetIsAnimated(oldTransitionInfo))
{
await oldPage.AnimateAsync(GetExitAnimation());
oldPage.ClearAnimation();
Copy link

@KikkoMax KikkoMax Feb 24, 2025

Choose a reason for hiding this comment

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

Just below here you still hide or remove the old page before the new page has a chance to appear. You will still get a moment without anything shown on screen. I reckon the new page visibility and optional animation should happen before old page handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The behaviour in the PR is the same as the old behaviour. I thought about this "moment without anything shown on screen" but I suspected that the alternative would be that both pages will be on top of one another and it will look weird. I tested now and indeed it looks really bad. I (subjectively) didn't find the split second between fading the old page out and adding the new page to be disturbing at all.

2025-02-24.22-37-31.mp4

Choose a reason for hiding this comment

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

Looking at the code prior to #19094 it does look to me that both pages are visible prior to animation and old page is hidden or removed after completion of either new page animation when navigating forward or exit animation when going back.

I'll try to get you videos to showcase the difference.

            switch (e.NavigationMode)
			{
				case NavigationMode.Forward:
				case NavigationMode.New:
				case NavigationMode.Refresh:
					_pageStack.Children.Add(newPage);
					if (GetIsAnimated(newEntry))
					{
						await newPage.AnimateAsync(GetEnterAnimation());
						newPage.ClearAnimation();
					}
					if (oldPage is not null)
					{
						if (FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages)
						{
							_pageStack.Children.Remove(oldPage);
						}
						else
						{
							oldPage.Visibility = Visibility.Collapsed;
						}
					}
					break;
				case NavigationMode.Back:
					if (FeatureConfiguration.NativeFramePresenter.AndroidUnloadInactivePages)
					{
						_pageStack.Children.Insert(0, newPage);
					}
					else
					{
						newPage.Visibility = Visibility.Visible;
					}
					if (GetIsAnimated(oldEntry))
					{
						await oldPage.AnimateAsync(GetExitAnimation());
						oldPage.ClearAnimation();
					}

					if (oldPage != null)
					{
						_pageStack.Children.Remove(oldPage);
					}
[...]

					break;
			}

Choose a reason for hiding this comment

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

Here is behavior on 5.6.70

XRecorder_20250224_01.mp4

And then previous behavior in 5.6.33

XRecorder_20250224_03.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the videos. I missed that the old behaviour when going back was inserting the new page below the old one. I still think it's problematic with pages that don't have a background (i.e. mostly transparent) because you'll see the intersection of both pages but this scenario is more important so fair enough.

Choose a reason for hiding this comment

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

Thank you, that should fix it :)

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-19587/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-19587/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-19587/index.html

@unodevops
Copy link
Contributor

⚠️⚠️ The build 156131 has failed on Uno.UI - CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform/android 🤖 Categorizes an issue or PR as relevant to the Android platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android] Native frame presenter regression
3 participants