Maui icon indicating copy to clipboard operation
Maui copied to clipboard

[BUG] TouchBehavior cancel animation even though it is just short tap

Open albilaga opened this issue 1 year ago • 15 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

Did you read the "Reporting a bug" section on Contributing file?

  • [X] I have read the "Reporting a bug" section on Contributing file: https://github.com/CommunityToolkit/Maui/blob/main/CONTRIBUTING.md#reporting-a-bug

Current Behavior

Touch behavior when set DefaultAnimationDuration and user just tap the element, it will just abort the animation and throw TaskCanceledException even though the animation is not finished. I am not sure if this is expected behavior. Because from user perspective if I just tap it, it should show and finish the animation within the duration of DefaultAnimationDuration. Here it is in current implementation even though I increase the DefaultAnimationDuration to be 500

https://github.com/user-attachments/assets/cd4f37a4-55b4-4515-a46b-4866f9de4077

And here is the stacktrace

System.Threading.Tasks.TaskCanceledException: A task was canceled.
   at CommunityToolkit.Maui.Behaviors.GestureManager.SetScale(TouchBehavior sender, TouchState touchState, HoverState hoverState, TimeSpan duration, Easing easing, CancellationToken token) in /Users/albilaga/Documents/Personal/CommunityToolkit.Maui/src/CommunityToolkit.Maui/Behaviors/PlatformBehaviors/Touch/GestureManager.shared.cs:line 475
   at CommunityToolkit.Maui.Behaviors.GestureManager.RunAnimationTask(TouchBehavior sender, TouchState touchState, HoverState hoverState, CancellationToken token, Nullable`1 durationMultiplier) in /Users/albilaga/Documents/Personal/CommunityToolkit.Maui/src/CommunityToolkit.Maui/Behaviors/PlatformBehaviors/Touch/GestureManager.shared.cs:line 799
   at CommunityToolkit.Maui.Behaviors.GestureManager.ChangeStateAsync(TouchBehavior sender, Boolean animated, CancellationToken token) in /Users/albilaga/Documents/Personal/CommunityToolkit.Maui/src/CommunityToolkit.Maui/Behaviors/PlatformBehaviors/Touch/GestureManager.shared.cs:line 118
   at CommunityToolkit.Maui.Behaviors.TouchBehavior.ForceUpdateState(CancellationToken token, Boolean animated) in /Users/albilaga/Documents/Personal/CommunityToolkit.Maui/src/CommunityToolkit.Maui/Behaviors/PlatformBehaviors/Touch/TouchBehavior.methods.shared.cs:line 65
System.Threading.Tasks.TaskCanceledException: A task was canceled.

Expected Behavior

TouchBehavior should finish the animation in DefaultAnimationDuration and not just directly abort the animation. It should be doing something like this.

https://github.com/user-attachments/assets/3553c111-770c-4681-8499-5e154c51a5ce

Steps To Reproduce

  1. Open and run the project in here https://github.com/albilaga/CommunityToolkit.Maui/tree/bugfix/wait_animation
  2. Open the side menu -> Behaviors -> TouchBehavior
  3. Scroll to the most bottom of view and tap the Lorem Ipsum view. It will just directly cancel the animation after we tap it

Link to public reproduction project repository

https://github.com/albilaga/CommunityToolkit.Maui/tree/bugfix/wait_animation

Environment

- .NET MAUI CommunityToolkit: 9.0.2
- OS: MacOS Sonoma 14.5
- .NET MAUI: 8.0.60

Anything else?

For now the workaround I have as seen in this commit https://github.com/CommunityToolkit/Maui/commit/033416b67383e13c8854499105bffc78fa310c62 is I just waiting with Task.Delay(DefaultAnimationDuration if touch status is completed. This is still not handling if user tap the view more than once in duration of DefaultAnimationDuration so that's why I am not publishing the PR in here. And I am also not sure if this bug is intended or not. Help is appreciated also. Thank you

albilaga avatar Jul 26 '24 09:07 albilaga

Hi, thanks for the report. To confirm the issue is that when you tap on the control the completion state change is somehow cancelling the animation before it has finished?

I understand your change doesn't solve all use cases but would you be willing to open it as a PR to make it easy to see what has changed and then we can work from there to see how we might solve it?

bijington avatar Aug 05 '24 08:08 bijington

Hi @bijington I saw the PR template and the bug should be verified to create PR https://github.com/CommunityToolkit/Maui/blob/main/.github/PULL_REQUEST_TEMPLATE.md ?

For the issue. Yes so basically when we tap, the animation will be cancelled directly and it will throw TaskCanceledException in debug console. This makes the animation not finished. This is fine if user long press this, as the state not changed until user release the gesture

albilaga avatar Aug 05 '24 15:08 albilaga

@bijington just created PR in here https://github.com/CommunityToolkit/Maui/pull/2101

albilaga avatar Aug 08 '24 15:08 albilaga

I am no expert in the TouchBehavior or the old TouchEffect so I would like to call on the opinions of @Axemasta, @jfversluis and anyone else that may have an opinion. Currently the animation shows for the duration of the touch, if you watch my attached video below there is a very short touch followed by a longer touch.

https://github.com/user-attachments/assets/8bce2fd6-7274-4932-8422-2cafd2c77f36

Is this the expected behavior or would we expect the full animation to complete? I am wondering whether the current behavior is expected and if a developer wanted to invoke the full animation we either make that an option somehow (if it isn't already) or guide them to using the AnimationBehavior.

What does everyone think?

bijington avatar Aug 09 '24 11:08 bijington

I'm also facing issues with Touch Effect on Mac Catalyst, once I have TaskCanceledException thrown, rest of UI becomes non-interactive, Buttons, Sliders stops working.

osaleem303 avatar Aug 09 '24 21:08 osaleem303

@bijington I think this is something that might have been broken during the pr review of the effect. It used to play the full animation even after lifting from a click (something I'd expect personally).

The task cancelled exception is really just noise, but it does happen a lot especially if you are relying on this control so I probably need to go back to a commit that was working on my end and see what's changed, then pr it back in.

I'm not aware of any crashes or issues with non responsiveness on mac catalyst, @osaleem303 can you provide a reproduction?

Axemasta avatar Aug 14 '24 19:08 Axemasta

@bijington I think this is something that might have been broken during the pr review of the effect. It used to play the full animation even after lifting from a click (something I'd expect personally).

The task cancelled exception is really just noise, but it does happen a lot especially if you are relying on this control so I probably need to go back to a commit that was working on my end and see what's changed, then pr it back in.

I'm not aware of any crashes or issues with non responsiveness on mac catalyst, @osaleem303 can you provide a reproduction?

Hi @Axemasta I've reproduced issue on following repo https://github.com/osaleem303/TouchEffectDemo

and also attached a screen recording on reproducing of that issue. Let me know if you need any further info on that.

osaleem303 avatar Aug 14 '24 22:08 osaleem303

FWIW I am getting an unhandled exception with the exact same stack trace shown in the original report - while resizing the window frame running on Windows. Not sure how the touch behavior is getting triggered by the resize but that's the stack trace I'm seeing in the exception. And even though the unhandled exception is "handled" the app becomes useless at that point.

DennisWelu avatar Aug 19 '24 15:08 DennisWelu

My logs are littered with these.

System.Threading.Tasks.TaskCanceledException: A task was canceled.
   at CommunityToolkit.Maui.Behaviors.GestureManager.SetScale(TouchBehavior sender, TouchState touchState, HoverState hoverState, TimeSpan duration, Easing easing, CancellationToken token)
   at CommunityToolkit.Maui.Behaviors.GestureManager.RunAnimationTask(TouchBehavior sender, TouchState touchState, HoverState hoverState, CancellationToken token, Nullable`1 durationMultiplier)
   at CommunityToolkit.Maui.Behaviors.GestureManager.ChangeStateAsync(TouchBehavior sender, Boolean animated, CancellationToken token)
   at CommunityToolkit.Maui.Behaviors.TouchBehavior.ForceUpdateState(CancellationToken token, Boolean animated)

Hackmodford avatar Aug 21 '24 16:08 Hackmodford

I've found the commit that introduced this issue, I'll try reverting the changes and see if they stop this issue. It looks like the animation also started messing up around this commit, the earlier commits dont throw this exception and always play the full animation (which is to be expected imo).

Axemasta avatar Aug 21 '24 23:08 Axemasta

I've found the commit that introduced this issue, I'll try reverting the changes and see if they stop this issue. It looks like the animation also started messing up around this commit, the earlier commits dont throw this exception and always play the full animation (which is to be expected imo).

I don’t know if I agree. I think it makes sense for the animation to be cancelled prematurely if the user is quick. We just don’t need the error in the logs.

Hackmodford avatar Aug 22 '24 11:08 Hackmodford

I don’t know if I agree. I think it makes sense for the animation to be cancelled prematurely if the user is quick. We just don’t need the error in the logs.

I probably worded this badly. The animations in the shipped version are what I would consider broken, there is some glitches and pop in especially of colors. I believe the reason why is that the animations are actually getting cancelled whereas before they were fire and forget.

Before beforeanim-ezgif com-video-to-gif-converter

After (Live) afteranim-ezgif com-video-to-gif-converter

You can see in the 2nd gif there is some strange black pop in, its caused by the animation cancelling. I'll be submitting a PR reverting those changes and we can see what the maintainers think. It wasn't a feature of the Xamarin version and it would squash this issue which is annoying for many people (including myself)

Axemasta avatar Aug 22 '24 14:08 Axemasta

DennisWelu

Can you provide a repro or some example xaml demoing the unresponsiveness? I am able to see the messages when resizing, but I haven't been able to get the app to go unresponive on windows using the mct sample app. If you can provide a repro I can push a fix 😊

Axemasta avatar Aug 25 '24 15:08 Axemasta

@Axemasta [One week later] I "think" I remember where in the app was current when I was triggering that problem with a resize....but today I'm not seeing it happen. :-) BUT...2 days after that post I updated from 9.0.1 to 9.0.3 and so maybe that's the difference....

DennisWelu avatar Aug 28 '24 13:08 DennisWelu

I'm also facing this issue

makazeu avatar Oct 08 '24 05:10 makazeu

Hey Gang! I've added the needs discussion label to chat more about this with the maintainers in our January standup.

This issue is documenting the expected behavior of TouchBehahvior: The animation ends when the touch ends and CommunityToolkit.Maui provides a TaskCanceledException allowing the developer to properly handle the canceled animation.

If we add work-arounds to continue playing your animation once the touch has ended, we risk introducing new bugs into existing apps.

My vote is to keep the current implementation, update the TouchBehavior documentation to note this as the expected behavior, and close https://github.com/CommunityToolkit/Maui/pull/2101. We will discuss it as a group in our next standup.

TheCodeTraveler avatar Dec 24 '24 19:12 TheCodeTraveler

Thanks @albilaga! We reviewed this in our January Standup and we decided that the best way forward is to make this behavior configurable.

@jfversluis Will be opening a New Feature Proposal to include the details discussed in the standup.

TheCodeTraveler avatar Jan 09 '25 20:01 TheCodeTraveler

Opened new proposal here: https://github.com/CommunityToolkit/Maui/issues/2433

jfversluis avatar Jan 10 '25 08:01 jfversluis