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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+30 −19
Draft

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f99ea1e
fix(nativeframepresenter): animate pages depending on NavigationMode
ramezgerges Feb 24, 2025
5a937be
chore: fix order of operations when updating frame stack
ramezgerges Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/Uno.UI/Controls/NativeFramePresenter.Android.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,13 +110,13 @@ private async Task InvalidateStack()
while (_stackUpdates.Any())
{
var navigation = _stackUpdates.Dequeue();
await UpdateStack(navigation.page, navigation.args.NavigationTransitionInfo);
await UpdateStack(navigation.page, navigation.args);
}

_isUpdatingStack = false;
}

private async Task UpdateStack(Page newPage, NavigationTransitionInfo transitionInfo)
private async Task UpdateStack(Page newPage, NavigationEventArgs args)
{
// When AndroidUnloadInactivePages is false, we keep the pages that are still a part of the navigation history
// (i.e. in BackStack or ForwardStack) as children and make then invisible instead of removing them. The order
Expand All @@ -133,11 +133,11 @@ private async Task UpdateStack(Page newPage, NavigationTransitionInfo transition

var oldPage = _currentPage.page;
var oldTransitionInfo = _currentPage.transitionInfo;
_currentPage = (newPage, transitionInfo);
_currentPage = (newPage, args.NavigationTransitionInfo);

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 •
edited
Loading

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

Copy link

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;
			}

Copy link

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.

Copy link

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 :)

Expand All @@ -162,7 +162,7 @@ private async Task UpdateStack(Page newPage, NavigationTransitionInfo transition
{
Children.Add(newPage);
}
if (GetIsAnimated(transitionInfo))
if (args.NavigationMode is not NavigationMode.Back && GetIsAnimated(args.NavigationTransitionInfo))
{
await newPage.AnimateAsync(GetEnterAnimation());
newPage.ClearAnimation();
Expand Down
Loading