From 11b753afb10fa49d8b038168416ba2ed271fc084 Mon Sep 17 00:00:00 2001 From: xiaoy312 Date: Thu, 30 Mar 2023 12:07:18 -0400 Subject: [PATCH] fix(animation): setting Local value should clear Animation value in filling state --- ...ardExtensions.cs => TimelineExtensions.cs} | 15 +-- .../Given_DoubleAnimation.cs | 117 ++++++++++++++++-- src/Uno.UI/DataBinding/BindingPath.cs | 81 ++++++++++-- .../DataBinding/BindingPropertyHelper.cs | 6 +- .../UI/Xaml/DependencyPropertyDetails.cs | 29 ++++- .../DependencyPropertyValuePrecedences.cs | 6 + .../Media/Animation/Timeline.animation.cs | 44 +++---- .../UI/Xaml/Media/Animation/Timeline.cs | 10 ++ 8 files changed, 257 insertions(+), 51 deletions(-) rename src/Uno.UI.RuntimeTests/Extensions/{StoryboardExtensions.cs => TimelineExtensions.cs} (73%) diff --git a/src/Uno.UI.RuntimeTests/Extensions/StoryboardExtensions.cs b/src/Uno.UI.RuntimeTests/Extensions/TimelineExtensions.cs similarity index 73% rename from src/Uno.UI.RuntimeTests/Extensions/StoryboardExtensions.cs rename to src/Uno.UI.RuntimeTests/Extensions/TimelineExtensions.cs index bd35becf11cc..1bb1449b516e 100644 --- a/src/Uno.UI.RuntimeTests/Extensions/StoryboardExtensions.cs +++ b/src/Uno.UI.RuntimeTests/Extensions/TimelineExtensions.cs @@ -8,20 +8,11 @@ namespace Uno.UI.RuntimeTests.Extensions; -internal static class StoryboardExtensions +internal static class TimelineExtensions { - public static void Begin(this Timeline timeline) + public static Storyboard ToStoryboard(this Timeline timeline) { - var storyboard = new Storyboard { Children = { timeline } }; - - storyboard.Begin(); - } - - public static async Task RunAsync(this Timeline timeline, TimeSpan? timeout, bool throwsException = false) - { - var storyboard = new Storyboard { Children = { timeline } }; - - await storyboard.RunAsync(timeout, throwsException); + return new Storyboard { Children = { timeline } }; } public static async Task RunAsync(this Storyboard storyboard, TimeSpan? timeout = null, bool throwsException = false) diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Media_Animation/Given_DoubleAnimation.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Media_Animation/Given_DoubleAnimation.cs index 9a8a74af6e06..d8c3e5b48817 100644 --- a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Media_Animation/Given_DoubleAnimation.cs +++ b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Media_Animation/Given_DoubleAnimation.cs @@ -149,7 +149,7 @@ public async Task When_RepeatForever_ShouldLoop() RepeatBehavior = RepeatBehavior.Forever, Duration = TimeSpan.FromMilliseconds(500 * TimeResolutionScaling), }.BindTo(target, nameof(Rectangle.Width)); - animation.Begin(); + animation.ToStoryboard().Begin(); // In an ideal world, the measurements would be [0 or 50,10,20,30,40] repeated 10 times. var list = new List(); @@ -185,7 +185,7 @@ public async Task When_RepeatForever_ShouldLoop() [TestMethod] [RunsOnUIThread] - public async Task When_StartingFrom_AnimatedValue() + public async Task When_StartingFrom_AnimatedValue() // value from completed(filling) animation { var translate = new TranslateTransform(); var border = new Border() @@ -208,7 +208,7 @@ public async Task When_StartingFrom_AnimatedValue() To = 50, Duration = new Duration(TimeSpan.FromSeconds(2)), }.BindTo(translate, nameof(translate.Y)); - await animation0.RunAsync(timeout: animation0.Duration.TimeSpan + TimeSpan.FromSeconds(1)); + await animation0.ToStoryboard().RunAsync(timeout: animation0.Duration.TimeSpan + TimeSpan.FromSeconds(1)); await Task.Delay(1000); // Start an second animation which should pick up from current animated value. @@ -218,24 +218,127 @@ public async Task When_StartingFrom_AnimatedValue() To = -50, Duration = new Duration(TimeSpan.FromSeconds(5)), }.BindTo(translate, nameof(translate.Y)); - animation1.Begin(); + animation1.ToStoryboard().Begin(); await Task.Delay(125); // ~125ms into a 5s animation where the value is animating from 50 to -50, // the value should be still positive. - // note: On android, the value will be scaled by ViewHelper.Scale, but the assertion will still hold true. var y = GetTranslateY(translate, isStillAnimating: true); Assert.IsTrue(y > 0, $"Expecting Translate.Y to be still positive: {y}"); } - private double GetTranslateY(TranslateTransform translate, bool isStillAnimating = false) => + [TestMethod] + [RunsOnUIThread] + public async Task When_StartingFrom_AnimatingValue() // value from mid animation + { + var translate = new TranslateTransform(); + var border = new Border() + { + Background = new SolidColorBrush(Colors.Pink), + Margin = new Thickness(0, 50, 0, 0), + Width = 50, + Height = 50, + RenderTransform = translate, + }; + WindowHelper.WindowContent = border; + await WindowHelper.WaitForLoaded(border); + await WindowHelper.WaitForIdle(); + + translate.Y = 50; + await WindowHelper.WaitForIdle(); + await Task.Delay(1000); + var threshold = GetTranslateY(translate); // snapshot the value + + // Start an animation. Its animating value will serve as + // the inferred starting value for the next animation. + var animation0 = new DoubleAnimation + { + From = 100, + To = 105, + Duration = new Duration(TimeSpan.FromSeconds(5)), + }.BindTo(translate, nameof(translate.Y)); + animation0.ToStoryboard().Begin(); + await Task.Delay(125); + + // Start an second animation which should pick up from current animating value. + var animation1 = new DoubleAnimation + { + // From = should be around 100~105 from animation #0 + To = 50, + Duration = new Duration(TimeSpan.FromSeconds(5)), + }.BindTo(translate, nameof(translate.Y)); + animation1.ToStoryboard().Begin(); + await Task.Delay(125); + + var value = GetTranslateY(translate, isStillAnimating: true); + if (value is double y) + { + // Animation #1 should be animating from around[100~105] to 50, and not from 0 (unanimated Local value). + Assert.IsTrue(y > 50, $"Expecting Translate.Y to be still positive: {y}"); + } + else + { + Assert.Fail($"Translate.Y is not a double: value={value}"); + } + } + + [TestMethod] + [RunsOnUIThread] + public Task When_OverridingFillingValue_WithLocalValue() => + When_OverridingFillingValue_WithLocalValue_Impl(skipToFill: false); + + [TestMethod] + [RunsOnUIThread] + public Task When_OverridingFillingValue_WithLocalValue_Skipped() => + When_OverridingFillingValue_WithLocalValue_Impl(skipToFill: true); + + public async Task When_OverridingFillingValue_WithLocalValue_Impl(bool skipToFill) + { + var translate = new TranslateTransform(); + var border = new Border() + { + Background = new SolidColorBrush(Colors.Pink), + Margin = new Thickness(0, 50, 0, 0), + Width = 50, + Height = 50, + RenderTransform = translate, + }; + WindowHelper.WindowContent = border; + await WindowHelper.WaitForLoaded(border); + await WindowHelper.WaitForIdle(); + + // Animate the value to fill. + var animation0 = new DoubleAnimation + { + To = 100, + Duration = new Duration(TimeSpan.FromSeconds(1)), + }.BindTo(translate, nameof(translate.Y)); + if (skipToFill) + { + animation0.ToStoryboard().SkipToFill(); + } + else + { + await animation0.ToStoryboard().RunAsync(); + } + var beforeValue = translate.Y; + + // Set a new local value + translate.Y = 312.0; + var afterValue = translate.Y; + + Assert.AreEqual(beforeValue, 100.0, "before: Should be animated to 100"); + Assert.AreEqual(afterValue, 312.0, "after: Should be set to 312"); + } + + private static double GetTranslateY(TranslateTransform translate, bool isStillAnimating = false) => #if !__ANDROID__ translate.Y; #else isStillAnimating // On android, animation may target a native property implementing the behavior instead of the specified dependency property. // We need to retrieve the value of that native property, as reading the dp value will just give the final value. - ? (double)translate.View.TranslationY + ? ViewHelper.PhysicalToLogicalPixels((double)translate.View.TranslationY) // And, when the animation is completed, this native value is reset even for HoldEnd animation. : translate.Y; #endif diff --git a/src/Uno.UI/DataBinding/BindingPath.cs b/src/Uno.UI/DataBinding/BindingPath.cs index 27a5ca83e675..4a1b0ba37c36 100644 --- a/src/Uno.UI/DataBinding/BindingPath.cs +++ b/src/Uno.UI/DataBinding/BindingPath.cs @@ -16,6 +16,7 @@ using Windows.UI.Xaml; using Windows.UI.Xaml.Data; using Windows.UI.Xaml.Media; +using System.Diagnostics.CodeAnalysis; namespace Uno.UI.DataBinding { @@ -236,6 +237,14 @@ internal void SetLocalValue(object value) } } + internal void SetAnimationFillingValue(object value) + { + if (!_disposed) + { + _value?.SetAnimationFillingValue(value); + } + } + /// /// Clears the value of the current precedence. /// @@ -249,6 +258,13 @@ public void ClearValue() _value?.ClearValue(); } } + public void ClearAnimationFillingValue() + { + if (!_disposed) + { + _value?.ClearAnimationFillingValue(); + } + } public Type? ValueType => _value?.PropertyType; @@ -489,7 +505,9 @@ private sealed class BindingItem : IBindingItem, IDisposable private ValueGetterHandler? _substituteValueGetter; private ValueSetterHandler? _valueSetter; private ValueSetterHandler? _localValueSetter; + private ValueSetterHandler? _animationFillingValueSetter; private ValueUnsetterHandler? _valueUnsetter; + private ValueUnsetterHandler? _animationFillingValueUnsetter; private Type? _dataContextType; @@ -580,6 +598,12 @@ public void SetLocalValue(object value) SetSourceValue(_localValueSetter!, value); } + public void SetAnimationFillingValue(object value) + { + BuildAnimationFillingValueSetter(); + SetSourceValue(_animationFillingValueSetter!, value); + } + public Type? PropertyType { get @@ -696,14 +720,15 @@ private void BuildValueSetter() { if (_valueSetter == null && _dataContextType != null) { + _valueSetter = BindingPropertyHelper.GetValueSetter( + _dataContextType, + PropertyName, + convert: true, + precedence: _precedence ?? DependencyPropertyValuePrecedences.Local + ); if (_precedence == null) { - BuildLocalValueSetter(); - _valueSetter = _localValueSetter; - } - else - { - _valueSetter = BindingPropertyHelper.GetValueSetter(_dataContextType, PropertyName, convert: true, precedence: _precedence.Value); + _localValueSetter = _valueSetter; } } } @@ -712,7 +737,25 @@ private void BuildLocalValueSetter() { if (_localValueSetter == null && _dataContextType != null) { - _localValueSetter = BindingPropertyHelper.GetValueSetter(_dataContextType, PropertyName, convert: true); + _localValueSetter = BindingPropertyHelper.GetValueSetter( + _dataContextType, + PropertyName, + convert: true, + precedence: DependencyPropertyValuePrecedences.Local + ); + } + } + + private void BuildAnimationFillingValueSetter() + { + if (_animationFillingValueSetter == null && _dataContextType != null) + { + _animationFillingValueSetter = BindingPropertyHelper.GetValueSetter( + _dataContextType, + PropertyName, + convert: true, + precedence: DependencyPropertyValuePrecedences.FillingAnimations + ); } } @@ -838,6 +881,30 @@ internal void ClearValue() } } + private void BuildAnimationFillingValueUnsetter() + { + if (_animationFillingValueUnsetter == null && _dataContextType != null) + { + _animationFillingValueUnsetter = BindingPropertyHelper.GetValueUnsetter( + _dataContextType, + PropertyName, + precedence: DependencyPropertyValuePrecedences.FillingAnimations + ); + } + } + + internal void ClearAnimationFillingValue() + { + BuildAnimationFillingValueUnsetter(); + + // Capture the datacontext before the call to avoid a race condition with the GC. + var dataContext = DataContext; + if (dataContext != null) + { + _animationFillingValueUnsetter!(dataContext); + } + } + private void RaiseValueChanged(object? newValue) { ValueChangedListener?.OnValueChanged(newValue); diff --git a/src/Uno.UI/DataBinding/BindingPropertyHelper.cs b/src/Uno.UI/DataBinding/BindingPropertyHelper.cs index 895b1ae8af29..d8504858e928 100644 --- a/src/Uno.UI/DataBinding/BindingPropertyHelper.cs +++ b/src/Uno.UI/DataBinding/BindingPropertyHelper.cs @@ -49,7 +49,7 @@ internal static partial class BindingPropertyHelper // those. If this situation changes, we could remove the associated code and // revert to memoized Funcs. // - private static Dictionary, ValueGetterHandler> _getValueGetter = new Dictionary, ValueGetterHandler>(CachedTuple.Comparer); + private static Dictionary, ValueGetterHandler> _getValueGetter = new Dictionary, ValueGetterHandler>(CachedTuple.Comparer); private static Dictionary, ValueSetterHandler> _getValueSetter = new Dictionary, ValueSetterHandler>(CachedTuple.Comparer); private static Dictionary, ValueGetterHandler> _getPrecedenceSpecificValueGetter = new Dictionary, ValueGetterHandler>(CachedTuple.Comparer); private static Dictionary, ValueGetterHandler> _getSubstituteValueGetter = new Dictionary, ValueGetterHandler>(CachedTuple.Comparer); @@ -1132,7 +1132,7 @@ private static ValueUnsetterHandler InternalGetValueUnsetter(Type type, string p { if (type == typeof(UnsetValue)) { - return _ => { }; + return UnsetValueUnsetter; } property = SanitizePropertyName(type, property); @@ -1250,6 +1250,8 @@ private static object UnsetValueGetter(object unused) private static void UnsetValueSetter(object unused, object? unused2) { } + private static void UnsetValueUnsetter(object unused) { } + /// /// Determines if the type can be provided by the MetadataProvider /// diff --git a/src/Uno.UI/UI/Xaml/DependencyPropertyDetails.cs b/src/Uno.UI/UI/Xaml/DependencyPropertyDetails.cs index 366e59219b8d..1ed49874d5e0 100644 --- a/src/Uno.UI/UI/Xaml/DependencyPropertyDetails.cs +++ b/src/Uno.UI/UI/Xaml/DependencyPropertyDetails.cs @@ -130,8 +130,33 @@ private void SetValueFull(object? value, DependencyPropertyValuePrecedences prec return; } - // If we were unsetting the current highest precedence value, we need to find the next highest - if (valueIsUnsetValue && precedence == _highestPrecedence) + // On Windows, explicitly calling SetValue(dp, DP.UnsetValue) or ClearValue(dp) doesnt clear the animation value. + // (note: Both the methods target local value) + // This means that we should not be handling those special case in here. Instead, + // when Timeline clears the animation value, it should also clears the filling animation value at the same time. + // --- + // Clear the animated value, when we are setting a local value to a property + // with an animated value from the filling part of an HoldEnd animation. + // note: There is no equivalent block in SetValueFast, as its condition would never be satisfied: + // _stack would've been materialized if the property had been animated. + bool forceUpdatePrecedence = false; + if (!valueIsUnsetValue && + _highestPrecedence == DependencyPropertyValuePrecedences.FillingAnimations && + (precedence is DependencyPropertyValuePrecedences.Local or DependencyPropertyValuePrecedences.Animations)) + { + stackAlias[(int)DependencyPropertyValuePrecedences.FillingAnimations] = UnsetValue.Instance; + if (precedence is DependencyPropertyValuePrecedences.Local) + { + stackAlias[(int)DependencyPropertyValuePrecedences.Animations] = UnsetValue.Instance; + } + + forceUpdatePrecedence = true; + } + + // Update highest precedence, when the current highest value was unset or + // when animation value was overridden by local value. + if ((valueIsUnsetValue && precedence == _highestPrecedence) || + forceUpdatePrecedence) { // Start from current precedence and find next highest for (int i = (int)precedence; i < (int)DependencyPropertyValuePrecedences.DefaultValue; i++) diff --git a/src/Uno.UI/UI/Xaml/DependencyPropertyValuePrecedences.cs b/src/Uno.UI/UI/Xaml/DependencyPropertyValuePrecedences.cs index 4893205f7bd0..ee897c79ba4e 100644 --- a/src/Uno.UI/UI/Xaml/DependencyPropertyValuePrecedences.cs +++ b/src/Uno.UI/UI/Xaml/DependencyPropertyValuePrecedences.cs @@ -11,6 +11,12 @@ public enum DependencyPropertyValuePrecedences : int /// Coercion = 0, + /// + /// Defined when filling from a HoldEnd animation + /// + /// Set the animation or local value will clear this value. + FillingAnimations, + /// /// Defined by animation storyboards /// diff --git a/src/Uno.UI/UI/Xaml/Media/Animation/Timeline.animation.cs b/src/Uno.UI/UI/Xaml/Media/Animation/Timeline.animation.cs index 8d5176097afb..b7b76a085a8b 100644 --- a/src/Uno.UI/UI/Xaml/Media/Animation/Timeline.animation.cs +++ b/src/Uno.UI/UI/Xaml/Media/Animation/Timeline.animation.cs @@ -72,10 +72,11 @@ private TimelineState State private string[] GetTraceProperties() => _owner?.GetTraceProperties(); private void ClearValue() => _owner?.ClearValue(); + private void ClearAnimationFillingValue() => _owner?.ClearAnimationFillingValue(); private void SetValue(object value) => _owner?.SetValue(value); + private void SetAnimationFillingValue(object value) => _owner?.SetAnimationFillingValue(value); private object GetValue() => _owner?.GetValue(); private object GetNonAnimatedValue() => _owner?.GetNonAnimatedValue(); - private bool NeedsRepeat(Stopwatch activeDuration, int replayCount) => _owner?.NeedsRepeat(activeDuration, replayCount) ?? false; public void Begin() @@ -115,6 +116,7 @@ public void Stop() _animator?.Cancel(); // stop could be called before the initialization _startingValue = null; ClearValue(); + ClearAnimationFillingValue(); State = TimelineState.Stopped; } @@ -188,18 +190,21 @@ public void SkipToFill() { if (_animator is { IsRunning: true }) { - _animator.Cancel();//Stop the animator if it is running + _animator.Cancel(); // Stop the animator if it is running _startingValue = null; } - SetValue(ComputeToValue());//Set property to its final value + // Set property to its final value + var value = ComputeToValue(); + SetValue(value); + SetAnimationFillingValue(value); OnEnd(); } public void Deactivate() { - _animator?.Cancel();//Stop the animator if it is running + _animator?.Cancel(); // Stop the animator if it is running _startingValue = null; State = TimelineState.Stopped; @@ -296,7 +301,7 @@ private void Play() State = TimelineState.Active; #if XAMARIN_IOS - // On iOS, animations started while the app is in the background will lead to properties in an incoherent state (native static + // On iOS, animations started while the app is in the background will lead to properties in an incoherent state (native static // values out of syc with native presented values and Xaml values). As a workaround we fast-forward the animation to its final // state. (The ideal would probably be to restart the animation when the app resumes.) if (Application.Current?.IsSuspended ?? false) @@ -343,27 +348,23 @@ private void OnEnd() return; } - if (FillBehavior == FillBehavior.HoldEnd)//Two types of fill behaviors : HoldEnd - Keep displaying the last frame + // There are two types of fill behaviors: + if (FillBehavior == FillBehavior.HoldEnd) // HoldEnd: Keep displaying the last frame { -#if __IOS__ || __MACOS__ - // iOS && macOS: Here we make sure that the final frame is applied properly (it may have been skipped by animator) - // Note: The value is applied using the "Animations" precedence, which means that the user won't be able to alter - // it from application code. Instead we should set the value using a lower precedence - // (possibly "Local" with PropertyInfo.SetLocalValue(ComputeToValue())) but we must keep the - // original "Local" value, so will be able to rollback the animation when - // going to another VisualState (if the storyboard ran in that context). - // In that case we should also do "ClearValue();" to remove the "Animations" value, even if using "HoldEnd" - // cf. https://github.com/unoplatform/uno/issues/631 - PropertyInfo.Value = ComputeToValue(); -#endif - State = TimelineState.Filling; + + // The HoldEnd value can be overriden by setting a Local value while it is filling, but not while it is animating. + // Since in the dependency property, we doesnt have the context here as to wheter the "animation value" is animating or filling, + // we also need to set the AnimationFilling value to indicate the case. + SetValue(ComputeToValue()); + SetAnimationFillingValue(ComputeToValue()); } - else // Stop -Put back the initial state + else // Stop: Put back the initial state { State = TimelineState.Stopped; ClearValue(); + ClearAnimationFillingValue(); } _owner.OnCompleted(); @@ -379,6 +380,7 @@ private void OnAnimatorFailed(object sender, EventArgs e) State = TimelineState.Stopped; ClearValue(); + ClearAnimationFillingValue(); _owner.OnFailed(); } @@ -403,7 +405,7 @@ private void OnAnimatorCancelled(object sender, EventArgs e) #if XAMARIN_IOS || __MACOS__ _startingValue = null; - // On Android, AnimationEnd is always called after AnimationCancel. We don't unset _startingValue yet to be able to calculate + // On Android, AnimationEnd is always called after AnimationCancel. We don't unset _startingValue yet to be able to calculate // the final value correctly. #endif } @@ -438,7 +440,7 @@ private T ComputeFromValue() var value = FeatureConfiguration.Timeline.DefaultsStartingValueFromAnimatedValue ? GetValueCore() : GetNonAnimatedValue(); - if (value != null) + if (value != null && value != DependencyProperty.UnsetValue) { return AnimationOwner.Convert(value); } diff --git a/src/Uno.UI/UI/Xaml/Media/Animation/Timeline.cs b/src/Uno.UI/UI/Xaml/Media/Animation/Timeline.cs index d203b8f1b375..5ef0968eeecc 100644 --- a/src/Uno.UI/UI/Xaml/Media/Animation/Timeline.cs +++ b/src/Uno.UI/UI/Xaml/Media/Animation/Timeline.cs @@ -297,6 +297,11 @@ protected void SetValue(object value) PropertyInfo.Value = value; } + internal void SetAnimationFillingValue(object value) + { + PropertyInfo.SetAnimationFillingValue(value); + } + /// /// Clears the animated value of the dependency property value precedence system /// @@ -310,6 +315,11 @@ protected void ClearValue() PropertyInfo.ClearValue(); } + internal void ClearAnimationFillingValue() + { + PropertyInfo.ClearAnimationFillingValue(); + } + void ITimeline.Begin() { // Timeline should not be used directly. Please use derived class.