Skip to content

Conversation

@Sergio0694
Copy link
Member

@Sergio0694 Sergio0694 commented Apr 21, 2021

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

The VisualExtensions class lacks an attached property to set the "Translation" property of a UI element.

What is the new behavior?

This PR adds a VisualExtensions.Translation attached property.
The behavior and implementation mirrors that of the other extensions.

Here's a GIF that shows both XAML hot-reload and this extension being combined with the animation system 🚀 🚀 🚀

HWNH1d4bur

Additional info

This extension now works just fine, and can be combined with the existing animation APIs to start a translation animation with an implicit starting point just fine. The reason why it wasn't originally working was because I incorrectly assumed that the "Translation" property in question was Visual.TransformMatrix.Translation, but turns out that wasn't the case 😄
When setting ElementCompositionPreview.SetIsTranslationEnabled, the Composition layer adds a new "Translation" property to CompositionObject.Properties (Visual is a CompositionObject too), and that's the property that is actually being animated in this case. In fact, you can set/animate both this property and Visual.TransformMatrix at the same time, and the final translation position for the object will just be the combination of the transform matrix + the additional explicit translation from this "hidden" property. That also explains why the animation didn't seem to work before: in fact it was working, but it was animating a different target ("Translation" instead of TransformMatrix.Translation) which was already at 0 (as it had never been set before), so it seemed as though the animation was just not doing anything. This also explains the final value for that transform matrix being unchanged 🙂

Marking this PR as ready for review, as this functionality is now working correctly! 🎉

Original (outdated) notes (click to expand):

This attached property comes with a caveat, which is that if it's used to set thee "Translation" property of a Visual before an animation, and then the animation is started without an initial value, the animation will just not run. In fact, the "Translation" property won't even be changed at all to the final value of the animation once it completes (though it still takes the expected duration for the animation to complete, it just... Does nothing). This only happens for "Translation", whereas doing exactly the same with eg. Visual.Offset works just fine. For instance, consider this repro code:

<Grid>
    <Button
        VerticalAlignment="Top"
        HorizontalAlignment="Left"
        Height="120"
        Width="360"
        Background="Green"
        Content="CLICK ME"
        FontSize="32"
        Click="Button_Click"
        ui:VisualExtensions.Translation="160,80,0">
    </Button>
</Grid>
private async void Button_Click(object sender, RoutedEventArgs e)
{
    var before = ((Button)sender).GetVisual().TransformMatrix.Translation;

    await AnimationBuilder.Create()
        .Translation(to: Vector3.Zero, duration: TimeSpan.FromSeconds(2))
        .StartAsync((Button)sender);

    var after = ((Button)sender).GetVisual().TransformMatrix.Translation;
}

Running this and clicking on the button will have the button remain in the same initial position, and those before and after values will still be exactly the same (so 160,80,0 in this case). If you try to change the target property to Offset in both XAML and C# instead, the animation will run as expected, and the after value will in fact change to 0.

This seems an issue with the framework, or at the very least a very specific behavior for "Translation" that's not consistent with the other Visual properties, and that I don't see documented on the MS docs for either Visual.TransformMatrix or ElementCompositionPreview.SetIsTranslationEnabled. I might open an issue in the WinUI repo about this, at least to try to get more information on this, but opening this PR in the meantime to gather feedbacks and because the actual VisualExtensions.Translation property itself does work, so we might also just want to merge it in the meantime 🙂

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • 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

@ghost
Copy link

ghost commented Apr 21, 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 🙌

@ghost ghost requested review from Kyaa-dost, Rosuavio, azchohfi and michael-hawker April 21, 2021 21:23
@michael-hawker
Copy link
Member

@Sergio0694 these are effectively wrappers like I called out in this WinUI issue, eh? microsoft/microsoft-ui-xaml#2130

I don't think I realized we had these in the Toolkit already... 🤦‍♂️ This is basically just adding one of the ones we missed? Did we get the whole list from the WinUI issue?

@Sergio0694 Sergio0694 marked this pull request as ready for review April 22, 2021 11:47
@michael-hawker
Copy link
Member

@Sergio0694 do we have all the cases covered now? Is there UWP documentation we can improve on this, or should we call these cases out somehow more explicitly in our own docs?

@michael-hawker michael-hawker added this to the 7.1 milestone Apr 22, 2021
@Sergio0694
Copy link
Member Author

"do we have all the cases covered now?"

The unit tests I've added for this extension should cover the expected use case scenario, yes 🤔
I'm testing setting and getting values for the property, and also animating it and checking that it's reflected there as well.

"Is there UWP documentation we can improve on this, or should we call these cases out somehow more explicitly in our own docs?"

I think these details should be covered in the docs for UWP itself, since these are all standard APIs from there.
I have found no proper explanations for this in any of the docs for the APIs being involved, which is why it took me a minute to actually figure out how this needed to be setup in order to work correctly. I could write down a list of the specific APIs I tried to check in the docs and the info that I could've used if they had been there, would that help? 🙂

@michael-hawker
Copy link
Member

Thanks @Sergio0694, yeah, if you could at least find the most relevant existing docs and check if there are any issues or create a new one to call out these important details that'd be a good starting point then. At least we'll have good helpers and behavior in the Toolkit now on top of it.

Was there any other properties we're missing exposed on the VisualExtensions beyond this one?

@ghost
Copy link

ghost commented Apr 23, 2021

Hello @michael-hawker!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@michael-hawker
Copy link
Member

Also, did you want to add your test image code you did above as a sample in the Sample App? 😉

@Sergio0694
Copy link
Member Author

Sergio0694 commented Apr 23, 2021

"these are effectively wrappers like I called out in this WinUI issue, eh? microsoft/microsoft-ui-xaml#2130"

Correction, this is not the case. UIElement.Transition is yet another translation property that is indipendent from all the others. Basically what I gathered from investigating this is that we have (at least) 3 different ways to change the translation of things:

  • Setting Visual.TransformMatrix.Translation
  • Setting Visual.Properties["Translation"]
  • Setting UIElement.Translation
  • (Bonus) Using UIElement.RenderTransform with either TranslateTransform or CompositeTransform

All these translations can be stacked together and set/animated indipendently. I'm not entirely sure why that is the case, and in particular why "Translation" is (1) so different from the others (ie. it's not just a Visual property like eg. Offset or Scale) and (2) why it needs to be enabled separately (I think it's due to some back-compat thing when the API was introduced a few years back, but not entirely sure). In either case, the docs for these properties should be improved to clearly specify what is being animated though, yeah. I'd make a PR myself but unfortunately the MS Docs for these APIs are not open source so I can't propose edits directly. I can definitely open an isssue in the WinUI repo to ask for more info and propose some better documentaation (or at least get some links in case it's there already and I just somehow missed it), sure 🙂

And to answer your question and clarify, the extension in this PR works on Visual.Properties["Translation"], and it's not a wrapper for that lightweight UIElement.Translation property mentioned in microsoft/microsoft-ui-xaml#2130. That can already be animated by using UIElement.TranslationTransition from XAML, which is combined with the other existing translations if any 👍

"Also, did you want to add your test image code you did above as a sample in the Sample App?"

Sure, I can put together a small sample page to showcase the extension 🙂

EDIT: done in ca7baea, added the samples to the "VisualExtensions" page.

@michael-hawker
Copy link
Member

Thanks for the detailed details @Sergio0694! Please open an issue on the WinUI repo about this confusion with your details here (feel free to point to this PR too), if you send the link I can add our wct label on it.

They'll at least know where to reach out to get docs updated, they'd probably have to be contacted anyway to help clarify/update the content anyway for remarks.

@michael-hawker
Copy link
Member

@RosarioPulella @Kyaa-dost want to test out the sample on Monday? Then @Sergio0694 can just link to the newly opened WinUI issue for this, and I think we'll be good to go!

@ghost ghost removed the auto merge ⚡ label Apr 24, 2021
@Sergio0694 Sergio0694 force-pushed the feature/translation-attached-property branch from e81983e to 129ab97 Compare April 24, 2021 13:41
Copy link
Contributor

@Kyaa-dost Kyaa-dost left a comment

Choose a reason for hiding this comment

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

Nice Work @Sergio0694! Looks great 🚀

@michael-hawker michael-hawker merged commit 0bb6eb8 into CommunityToolkit:master Apr 28, 2021
@Sergio0694 Sergio0694 deleted the feature/translation-attached-property branch April 28, 2021 15:45
@michael-hawker michael-hawker modified the milestones: 7.1, 7.0.2 May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants