Fix Animation.FillMode when cue isn't 0% or 100% (#13775)

* Added failing Animation.FillMode tests

* Fix Animation.FillMode when cue isn't 0% or 100%

* Ensure keyframes are ordered in animator

* Fix Animation.Clock nullability

* Fix some inverted Assert.Equal parameters

src/Avalonia.Base/Animation/Animatable.cs

@@ -17,8 +17,8 @@ namespace Avalonia.Animation
         /// <summary>
         /// Defines the <see cref="Clock"/> property.
         /// </summary>
-        internal static readonly StyledProperty<IClock> ClockProperty =
-            AvaloniaProperty.Register<Animatable, IClock>(nameof(Clock), inherits: true);
+        internal static readonly StyledProperty<IClock?> ClockProperty =
+            AvaloniaProperty.Register<Animatable, IClock?>(nameof(Clock), inherits: true);
 
         /// <summary>
         /// Defines the <see cref="Transitions"/> property.
@@ -36,7 +36,7 @@ namespace Avalonia.Animation
         /// <summary>
         /// Gets or sets the clock which controls the animations on the control.
         /// </summary>
-        internal IClock Clock
+        internal IClock? Clock
         {
             get => GetValue(ClockProperty);
             set => SetValue(ClockProperty, value);

src/Avalonia.Base/Animation/Animation.cs

@@ -234,6 +234,8 @@ namespace Avalonia.Animation
                 }
             }
 
+            animatorKeyFrames.Sort(static (x, y) => x.Cue.CueValue.CompareTo(y.Cue.CueValue));
+
             var newAnimatorInstances = new List<IAnimator>();
 
             foreach (var handler in handlerList)
@@ -247,9 +249,22 @@ namespace Avalonia.Animation
             {
                 var animator = newAnimatorInstances.First(a => a.GetType() == keyframe.AnimatorType &&
                                                              a.Property == keyframe.Property);
+
+                if (animator.Count == 0 && FillMode is FillMode.Backward or FillMode.Both)
+                    keyframe.FillBefore = true;
+
                 animator.Add(keyframe);
             }
 
+            if (FillMode is FillMode.Forward or FillMode.Both)
+            {
+                foreach (var newAnimatorInstance in newAnimatorInstances)
+                {
+                    if (newAnimatorInstance.Count > 0)
+                        newAnimatorInstance[newAnimatorInstance.Count - 1].FillAfter = true;
+                }
+            }
+
             return (newAnimatorInstances, subscriptions);
         }
 

src/Avalonia.Base/Animation/AnimatorKeyFrame.cs

@@ -16,19 +16,6 @@ namespace Avalonia.Animation
         public static readonly DirectProperty<AnimatorKeyFrame, object?> ValueProperty =
             AvaloniaProperty.RegisterDirect<AnimatorKeyFrame, object?>(nameof(Value), k => k.Value, (k, v) => k.Value = v);
 
-        public AnimatorKeyFrame()
-        {
-
-        }
-
-        public AnimatorKeyFrame(Type? animatorType, Func<IAnimator>? animatorFactory, Cue cue)
-        {
-            AnimatorType = animatorType;
-            AnimatorFactory = animatorFactory;
-            Cue = cue;
-            KeySpline = null;
-        }
-
         public AnimatorKeyFrame(Type? animatorType, Func<IAnimator>? animatorFactory, Cue cue, KeySpline? keySpline)
         {
             AnimatorType = animatorType;
@@ -37,11 +24,12 @@ namespace Avalonia.Animation
             KeySpline = keySpline;
         }
 
-        internal bool isNeutral;
         public Type? AnimatorType { get; }
         public Func<IAnimator>? AnimatorFactory { get; }
         public Cue Cue { get; }
         public KeySpline? KeySpline { get; }
+        public bool FillBefore { get; set; }
+        public bool FillAfter { get; set; }
         public AvaloniaProperty? Property { get; private set; }
 
         private object? _value;

src/Avalonia.Base/Animation/Animators/Animator`1.cs

@@ -1,7 +1,5 @@
 using System;
-using System.Collections.Generic;
-using System.Linq;
-using Avalonia.Animation.Utils;
+using System.Diagnostics;
 using Avalonia.Collections;
 using Avalonia.Data;
 using Avalonia.Reactive;
@@ -13,94 +11,72 @@ namespace Avalonia.Animation.Animators
     /// </summary>
     internal abstract class Animator<T> : AvaloniaList<AnimatorKeyFrame>, IAnimator
     {
-        /// <summary>
-        /// List of type-converted keyframes.
-        /// </summary>
-        private readonly List<AnimatorKeyFrame> _convertedKeyframes = new List<AnimatorKeyFrame>();
-
-        private bool _isVerifiedAndConverted;
-
         /// <summary>
         /// Gets or sets the target property for the keyframe.
         /// </summary>
         public AvaloniaProperty? Property { get; set; }
 
-        public Animator()
-        {
-            // Invalidate keyframes when changed.
-            this.CollectionChanged += delegate { _isVerifiedAndConverted = false; };
-        }
-
         /// <inheritdoc/>
         public virtual IDisposable? Apply(Animation animation, Animatable control, IClock? clock, IObservable<bool> match, Action? onComplete)
         {
-            if (!_isVerifiedAndConverted)
-                VerifyConvertKeyFrames();
-
             var subject = new DisposeAnimationInstanceSubject<T>(this, animation, control, clock, onComplete);
             return new CompositeDisposable(match.Subscribe(subject), subject);
         }
 
         protected T InterpolationHandler(double animationTime, T neutralValue)
         {
-            AnimatorKeyFrame firstKeyframe, lastKeyframe;
+            if (Count == 0)
+                return neutralValue;
+
+            var (beforeKeyFrame, afterKeyFrame) = FindKeyFrames(animationTime);
 
-            int kvCount = _convertedKeyframes.Count;
-            if (kvCount > 2)
+            double beforeTime, afterTime;
+            T beforeValue, afterValue;
+
+            if (beforeKeyFrame is null)
             {
-                if (animationTime <= 0.0)
-                {
-                    firstKeyframe = _convertedKeyframes[0];
-                    lastKeyframe = _convertedKeyframes[1];
-                }
-                else if (animationTime >= 1.0)
-                {
-                    firstKeyframe = _convertedKeyframes[_convertedKeyframes.Count - 2];
-                    lastKeyframe = _convertedKeyframes[_convertedKeyframes.Count - 1];
-                }
-                else
-                {
-                    int index = FindClosestBeforeKeyFrame(animationTime);
-                    firstKeyframe = _convertedKeyframes[index];
-                    lastKeyframe = _convertedKeyframes[index + 1];
-                }
+                beforeTime = 0.0;
+                beforeValue = afterKeyFrame is { FillBefore: true, Value: T fillValue } ? fillValue : neutralValue;
             }
             else
             {
-                firstKeyframe = _convertedKeyframes[0];
-                lastKeyframe = _convertedKeyframes[1];
+                beforeTime = beforeKeyFrame.Cue.CueValue;
+                beforeValue = beforeKeyFrame.Value is T value ? value : neutralValue;
             }
 
-            double t0 = firstKeyframe.Cue.CueValue;
-            double t1 = lastKeyframe.Cue.CueValue;
-
-            double progress = (animationTime - t0) / (t1 - t0);
-
-            T oldValue, newValue;
-
-            if (!firstKeyframe.isNeutral && firstKeyframe.Value is T firstKeyframeValue)
-                oldValue = firstKeyframeValue;
+            if (afterKeyFrame is null)
+            {
+                afterTime = 1.0;
+                afterValue = beforeKeyFrame is { FillAfter: true, Value: T fillValue } ? fillValue : neutralValue;
+            }
             else
-                oldValue = neutralValue;
+            {
+                afterTime = afterKeyFrame.Cue.CueValue;
+                afterValue = afterKeyFrame.Value is T value ? value : neutralValue;
+            }
 
-            if (!lastKeyframe.isNeutral && lastKeyframe.Value is T lastKeyframeValue)
-                newValue = lastKeyframeValue;
-            else
-                newValue = neutralValue;
+            var progress = (animationTime - beforeTime) / (afterTime - beforeTime);
 
-            if (lastKeyframe.KeySpline != null)
-                progress = lastKeyframe.KeySpline.GetSplineProgress(progress);
+            if (afterKeyFrame?.KeySpline is { } keySpline)
+                progress = keySpline.GetSplineProgress(progress);
 
-            return Interpolate(progress, oldValue, newValue);
+            return Interpolate(progress, beforeValue, afterValue);
         }
 
-        private int FindClosestBeforeKeyFrame(double time)
+        private (AnimatorKeyFrame? Before, AnimatorKeyFrame? After) FindKeyFrames(double time)
         {
-            for (int i = 0; i < _convertedKeyframes.Count; i++)
-                if (_convertedKeyframes[i].Cue.CueValue > time)
-                    return i - 1;
+            Debug.Assert(Count >= 1);
 
-            throw new Exception("Index time is out of keyframe time range.");
+            for (var i = 0; i < Count; i++)
+            {
+                var keyFrame = this[i];
+                var keyFrameTime = keyFrame.Cue.CueValue;
+
+                if (time < keyFrameTime || keyFrameTime == 1.0)
+                    return (i > 0 ? this[i - 1] : null, keyFrame);
+            }
+
+            return (this[Count - 1], null);
         }
 
         public virtual IDisposable BindAnimation(Animatable control, IObservable<T> instance)
@@ -123,7 +99,7 @@ namespace Avalonia.Animation.Animators
                 clock ?? control.Clock ?? Clock.GlobalClock,
                 onComplete,
                 InterpolationHandler);
-            
+
             return BindAnimation(control, instance);
         }
 
@@ -131,52 +107,5 @@ namespace Avalonia.Animation.Animators
         /// Interpolates in-between two key values given the desired progress time.
         /// </summary>
         public abstract T Interpolate(double progress, T oldValue, T newValue);
-
-        private void VerifyConvertKeyFrames()
-        {
-            foreach (AnimatorKeyFrame keyframe in this)
-            {
-                _convertedKeyframes.Add(keyframe);
-            }
-
-            AddNeutralKeyFramesIfNeeded();
-
-            _isVerifiedAndConverted = true;
-        }
-
-        private void AddNeutralKeyFramesIfNeeded()
-        {
-            bool hasStartKey, hasEndKey;
-            hasStartKey = hasEndKey = false;
-
-            // Check if there's start and end keyframes.
-            foreach (var frame in _convertedKeyframes)
-            {
-                if (frame.Cue.CueValue == 0.0d)
-                {
-                    hasStartKey = true;
-                }
-                else if (frame.Cue.CueValue == 1.0d)
-                {
-                    hasEndKey = true;
-                }
-            }
-
-            if (!hasStartKey || !hasEndKey)
-                AddNeutralKeyFrames(hasStartKey, hasEndKey);
-        }
-
-        private void AddNeutralKeyFrames(bool hasStartKey, bool hasEndKey)
-        {
-            if (!hasStartKey)
-            {
-                _convertedKeyframes.Insert(0, new AnimatorKeyFrame(null, null, new Cue(0.0d)) { Value = default(T), isNeutral = true });
-            }
-
-            if (!hasEndKey)
-            {
-                _convertedKeyframes.Add(new AnimatorKeyFrame(null, null, new Cue(1.0d)) { Value = default(T), isNeutral = true });
-            }
-        }
     }
 }

src/Avalonia.Base/Animation/Animators/BaseBrushAnimator.cs

@@ -86,14 +86,18 @@ namespace Avalonia.Animation.Animators
                 {
                     gradientAnimator.Add(new AnimatorKeyFrame(typeof(GradientBrushAnimator), () => new GradientBrushAnimator(), keyframe.Cue, keyframe.KeySpline)
                     {
-                        Value = GradientBrushAnimator.ConvertSolidColorBrushToGradient(firstGradient, solidColorBrush)
+                        Value = GradientBrushAnimator.ConvertSolidColorBrushToGradient(firstGradient, solidColorBrush),
+                        FillBefore = keyframe.FillBefore,
+                        FillAfter = keyframe.FillAfter
                     });
                 }
                 else if (keyframe.Value is IGradientBrush)
                 {
                     gradientAnimator.Add(new AnimatorKeyFrame(typeof(GradientBrushAnimator), () => new GradientBrushAnimator(), keyframe.Cue, keyframe.KeySpline)
                     {
-                        Value = keyframe.Value
+                        Value = keyframe.Value,
+                        FillBefore = keyframe.FillBefore,
+                        FillAfter = keyframe.FillAfter
                     });
                 }
                 else
@@ -118,7 +122,9 @@ namespace Avalonia.Animation.Animators
                 {
                     solidColorBrushAnimator.Add(new AnimatorKeyFrame(typeof(ISolidColorBrushAnimator), () => new ISolidColorBrushAnimator(), keyframe.Cue, keyframe.KeySpline)
                     {
-                        Value = keyframe.Value
+                        Value = keyframe.Value,
+                        FillBefore = keyframe.FillBefore,
+                        FillAfter = keyframe.FillAfter
                     });
                 }
                 else
@@ -149,7 +155,9 @@ namespace Avalonia.Animation.Animators
                         {
                             animator.Add(new AnimatorKeyFrame(animatorType, animatorFactory, keyframe.Cue, keyframe.KeySpline)
                             {
-                                Value = keyframe.Value
+                                Value = keyframe.Value,
+                                FillBefore = keyframe.FillBefore,
+                                FillAfter = keyframe.FillAfter
                             });
                         }
 

src/Avalonia.Base/Media/Effects/EffectAnimator.cs

@@ -38,7 +38,9 @@ internal class EffectAnimator : Animator<IEffect?>
                 createdAnimator.Add(new AnimatorKeyFrame(typeof(TAnimator), () => new TAnimator(), keyFrame.Cue,
                     keyFrame.KeySpline)
                 {
-                    Value = keyFrame.Value
+                    Value = keyFrame.Value,
+                    FillBefore = keyFrame.FillBefore,
+                    FillAfter = keyFrame.FillAfter
                 });
             }
             else

tests/Avalonia.Base.UnitTests/Animation/AnimatableTests.cs

@@ -126,18 +126,16 @@ namespace Avalonia.Base.UnitTests.Animation
 
             var rect = new Rectangle() { Width = 11, };
 
-            var originalValue = rect.Width;
-
             var clock = new TestClock();
             animation.RunAsync(rect, clock);
 
             clock.Step(TimeSpan.Zero);
-            Assert.Equal(rect.Width, 1);
+            Assert.Equal(1, rect.Width);
             clock.Step(TimeSpan.FromSeconds(2));
-            Assert.Equal(rect.Width, 2);
+            Assert.Equal(2, rect.Width);
             clock.Step(TimeSpan.FromSeconds(3));
             //here we have invalid value so value should be expected and set to initial original value
-            Assert.Equal(rect.Width, originalValue);
+            Assert.Equal(11, rect.Width);
         }
 
         [Fact]

tests/Avalonia.Base.UnitTests/Animation/AnimationIterationTests.cs

@@ -131,17 +131,23 @@ namespace Avalonia.Base.UnitTests.Animation
         }
         
         [Theory]
-        [InlineData(FillMode.Backward, 0, 0d, 0.7d)]
-        [InlineData(FillMode.Both, 0, 0d, 0.7d)]
-        [InlineData(FillMode.Forward, 100, 0d, 0.7d)]
-        [InlineData(FillMode.Backward, 0, 0.3d, 0.7d)]
-        [InlineData(FillMode.Both, 0, 0.3d, 0.7d)]
-        [InlineData(FillMode.Forward, 100, 0.3d, 0.7d)]
-        public void Check_FillMode_Start_Value(FillMode fillMode, double target, double startCue, double endCue)
+        [InlineData(FillMode.Backward, 50.0, 0.0, 0.7, false)]
+        [InlineData(FillMode.Backward, 50.0, 0.0, 0.7, true )]
+        [InlineData(FillMode.Both,     50.0, 0.0, 0.7, false)]
+        [InlineData(FillMode.Both,     50.0, 0.0, 0.7, true )]
+        [InlineData(FillMode.Forward,  50.0, 0.0, 0.7, false)] // no delay but cue 0.0: the animation has started normally, explaining the 50.0 target without fill
+        [InlineData(FillMode.Forward, 100.0, 0.0, 0.7, true )]
+        [InlineData(FillMode.Backward, 50.0, 0.3, 0.7, false)]
+        [InlineData(FillMode.Backward, 50.0, 0.3, 0.7, true )]
+        [InlineData(FillMode.Both,     50.0, 0.3, 0.7, false)]
+        [InlineData(FillMode.Both,     50.0, 0.3, 0.7, true )]
+        [InlineData(FillMode.Forward, 100.0, 0.3, 0.7, false)]
+        [InlineData(FillMode.Forward, 100.0, 0.3, 0.7, true )]
+        public void Check_FillMode_Start_Value(FillMode fillMode, double target, double startCue, double endCue, bool delay)
         {
             var keyframe1 = new KeyFrame()
             {
-                Setters = { new Setter(Layoutable.WidthProperty, 0d), }, Cue = new Cue(startCue)
+                Setters = { new Setter(Layoutable.WidthProperty, 50d), }, Cue = new Cue(startCue)
             };
 
             var keyframe2 = new KeyFrame()
@@ -152,7 +158,7 @@ namespace Avalonia.Base.UnitTests.Animation
             var animation = new Animation()
             {
                 Duration = TimeSpan.FromSeconds(10d),
-                Delay = TimeSpan.FromSeconds(5d),
+                Delay = delay ? TimeSpan.FromSeconds(5d) : TimeSpan.Zero,
                 FillMode = fillMode,
                 Children = { keyframe1, keyframe2 }
             };
@@ -169,28 +175,34 @@ namespace Avalonia.Base.UnitTests.Animation
         }
         
         [Theory]
-        [InlineData(FillMode.Backward, 100, 0.3d, 1d)]
-        [InlineData(FillMode.Both, 300, 0.3d, 1d)]
-        [InlineData(FillMode.Forward, 300, 0.3d, 1d)]
-        [InlineData(FillMode.Backward, 100, 0.3d, 0.7d)]
-        [InlineData(FillMode.Both, 300, 0.3d, 0.7d)]
-        [InlineData(FillMode.Forward, 300, 0.3d, 0.7d)]
-        public void Check_FillMode_End_Value(FillMode fillMode, double target, double startCue, double endCue)
+        [InlineData(FillMode.Backward, 100.0, 0.3, 1.0, false)]
+        [InlineData(FillMode.Backward, 100.0, 0.3, 1.0, true )]
+        [InlineData(FillMode.Both,     300.0, 0.3, 1.0, false)]
+        [InlineData(FillMode.Both,     300.0, 0.3, 1.0, true )]
+        [InlineData(FillMode.Forward,  300.0, 0.3, 1.0, false)]
+        [InlineData(FillMode.Forward,  300.0, 0.3, 1.0, true )]
+        [InlineData(FillMode.Backward, 100.0, 0.3, 0.7, false)]
+        [InlineData(FillMode.Backward, 100.0, 0.3, 0.7, true )]
+        [InlineData(FillMode.Both,     300.0, 0.3, 0.7, false)]
+        [InlineData(FillMode.Both,     300.0, 0.3, 0.7, true )]
+        [InlineData(FillMode.Forward,  300.0, 0.3, 0.7, false)]
+        [InlineData(FillMode.Forward,  300.0, 0.3, 0.7, true )]
+        public void Check_FillMode_End_Value(FillMode fillMode, double target, double startCue, double endCue, bool delay)
         {
             var keyframe1 = new KeyFrame()
             {
-                Setters = { new Setter(Layoutable.WidthProperty, 0d), }, Cue = new Cue(0.7d)
+                Setters = { new Setter(Layoutable.WidthProperty, 0d), }, Cue = new Cue(startCue)
             };
 
             var keyframe2 = new KeyFrame()
             {
-                Setters = { new Setter(Layoutable.WidthProperty, 300d), }, Cue = new Cue(1d)
+                Setters = { new Setter(Layoutable.WidthProperty, 300d), }, Cue = new Cue(endCue)
             };
 
             var animation = new Animation()
             {
                 Duration = TimeSpan.FromSeconds(10d),
-                Delay = TimeSpan.FromSeconds(5d),
+                Delay = delay ? TimeSpan.FromSeconds(5d) : TimeSpan.Zero,
                 FillMode = fillMode,
                 Children = { keyframe1, keyframe2 }
             };
@@ -502,6 +514,96 @@ namespace Avalonia.Base.UnitTests.Animation
             animationRun.Wait();
         }
 
+        [Theory]
+        [InlineData(0, 1, 2)]
+        [InlineData(0, 2, 1)]
+        [InlineData(1, 0, 2)]
+        [InlineData(1, 2, 0)]
+        [InlineData(2, 0, 1)]
+        [InlineData(2, 1, 0)]
+        public void KeyFrames_Order_Does_Not_Matter(int index0, int index1, int index2)
+        {
+            static KeyFrame CreateKeyFrame(double width, double cue)
+                => new()
+                {
+                    Setters = { new Setter(Layoutable.WidthProperty, width) },
+                    Cue = new Cue(cue)
+                };
+
+            var keyFrames = new[]
+            {
+                CreateKeyFrame(100.0, 0.0),
+                CreateKeyFrame(200.0, 0.5),
+                CreateKeyFrame(300.0, 1.0)
+            };
+
+            var animation = new Animation
+            {
+                Duration = TimeSpan.FromSeconds(1.0),
+                IterationCount = new IterationCount(1),
+                Easing = new LinearEasing(),
+                FillMode = FillMode.Forward
+            };
+
+            animation.Children.Add(keyFrames[index0]);
+            animation.Children.Add(keyFrames[index1]);
+            animation.Children.Add(keyFrames[index2]);
+
+            var border = new Border
+            {
+                Height = 100.0,
+                Width = 50.0
+            };
+
+            var clock = new TestClock();
+            animation.RunAsync(border, clock);
+
+            clock.Step(TimeSpan.Zero);
+            Assert.Equal(100.0, border.Width);
+
+            clock.Step(TimeSpan.FromSeconds(0.5));
+            Assert.Equal(200.0, border.Width);
+
+            clock.Step(TimeSpan.FromSeconds(1.0));
+            Assert.Equal(300.0, border.Width);
+        }
+
+        [Theory]
+        [InlineData(0.0)]
+        [InlineData(0.5)]
+        [InlineData(1.0)]
+        public void Single_KeyFrame_Works(double cue)
+        {
+            var animation = new Animation
+            {
+                Duration = TimeSpan.FromSeconds(1.0),
+                IterationCount = new IterationCount(1),
+                Easing = new LinearEasing(),
+                FillMode = FillMode.Forward,
+                Children =
+                {
+                    new KeyFrame
+                    {
+                        Setters = { new Setter(Layoutable.WidthProperty, 100.0) },
+                        Cue = new Cue(cue)
+                    }
+                }
+            };
+
+            var border = new Border
+            {
+                Height = 100.0,
+                Width = 50.0
+            };
+
+            var clock = new TestClock();
+            animation.RunAsync(border, clock);
+
+            clock.Step(TimeSpan.Zero);
+            clock.Step(TimeSpan.FromSeconds(cue));
+            Assert.Equal(100.0, border.Width);
+        }
+
         private sealed class FakeAnimator : InterpolatingAnimator<double>
         {
             public double LastProgress { get; set; } = double.NaN;

tests/Avalonia.Base.UnitTests/Animation/SpringTests.cs

@@ -80,9 +80,9 @@ public class SpringTests
         animation.RunAsync(rect, clock);
 
         clock.Step(TimeSpan.Zero);
-        Assert.Equal(rotateTransform.Angle, -2.5);
+        Assert.Equal(-2.5, rotateTransform.Angle);
         clock.Step(TimeSpan.FromSeconds(5));
-        Assert.Equal(rotateTransform.Angle, 5.522828945000075);
+        Assert.Equal(5.522828945000075, rotateTransform.Angle);
 
         var tolerance = 0.01;
         clock.Step(TimeSpan.Parse("00:00:10.0153932"));