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

Fixed incorrect default easing for XAML animation types #4311

Merged

Conversation

Sergio0694
Copy link
Member

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

There are a few callsites in the animation APIs in the Microsoft.Toolkit.Uwp.UI.Animations package where a value of default is accidentally being passed to the easing mode and easing type when inserting keyframes into animations built using the XAML mapping types. This causes the easing mode being selected to be "ease in" (as it's the first in its declaring type) and not "ease in out", which is special cased to map to the default easing function for the composition layer when combined with the default easing type. This makes it so that in some cases, inserted keyframes end up using a custom beizer curve easing function instead of just null, which would let the framework automatically use whatever the default easing mode is on that current OS version. This is currently harder to spot as the custom animation approximates what the default one looks like, but it's technically not correct and might become more apparent in the future in case an OS update tweaked the way the default easing function works.

What is the new behavior?

XAML animation types where no custom easing type/mode are set just pass null as the composition easing function.
This lets the framework just apply whatever default easing it wants to use.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • New component
    • Pull Request has been submitted to the documentation repository instructions. Link:
    • Added description of major feature to project description for NuGet package (4000 total character limit, so don't push entire description over that)
    • If control, added to Visual Studio Design project
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

@Sergio0694 Sergio0694 added bug 🐛 An unexpected issue that highlights incorrect behavior animations 🏮 hotfix 🌶 labels Oct 12, 2021
@Sergio0694 Sergio0694 added this to the 7.1.1 milestone Oct 12, 2021
@ghost
Copy link

ghost commented Oct 12, 2021

Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@@ -74,7 +74,7 @@ public CompositionAnimation GetAnimation(UIElement element, out string? target)

if (from is not null)
{
builder.KeyFrame(0.0, from.Value, default, default);
builder.KeyFrame(0.0, from.Value, DefaultEasingType, DefaultEasingMode);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we pass the easing modes here but not in the other case?

(Also, why do we use the defaults here vs. checking for the overrides in the other cases?) If these distinctions are important should we add comments about it somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's becase the other case with no easing type/mode is calling INormalizedKeyFrameAnimationBuilder<TKeyFrame>.KeyFrame, which indicates the default easing values in its public API in the interface, so we can just omit those parameters and the compiler will insert them for us like it does normally for optional parameters with default values. Here instead we're calling NormalizedKeyFrameAnimationBuilder<T>.KeyFrame (on the concrete type, not the interface), which doesn't define the default values for those parameters (as it's an internal API only exposed publicly through that interface method, which inherits those from the interface itself). At the end of the day it's the same, really, it's just slightly different due to the language rules here. Does that make sense? 😄

As in:

IFoo i = new Foo();

i.Bar(); // Ok, passes 42

Foo c = new Foo();

c.Bar(); // Fails to build
c.Bar(42); // This is fine

interface IFoo
{
    void Bar(int x = 42);
}

class Foo : IFoo
{
    public void Bar(int x) { }
}

@Sergio0694 Sergio0694 merged commit 83bcf26 into CommunityToolkit:main Oct 13, 2021
@Sergio0694 Sergio0694 deleted the bugfix/xaml-animation-glitches branch October 13, 2021 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
animations 🏮 bug 🐛 An unexpected issue that highlights incorrect behavior hotfix 🌶 priority 🚩
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants