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

Fix #4214 - Update AttachedDropShadow visual on layout/visibility #4230

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

michael-hawker
Copy link
Member

@michael-hawker michael-hawker commented Sep 8, 2021

Fixes #4214

We weren't updating the shadow visual if the parent element's visibility changed or layout was effected by other elements.

PR Type

  • Bugfix

What is the current behavior?

In some scenarios the shadow wouldn't be updated by the changes to the parent element.

What is the new behavior?

Shadow is now updated in more scenarios (though maybe not all). We should properly handle Visibility, Scale, and some changes due to Layout updates.

Note: I tried with Canvas from the implicit animation sample and it seems Canvas moving an object doesn't call LayoutUpdated on that element... wonder if that's a framework bug (this was the closest open one I could find)? Otherwise, not sure how we detect that in the element...

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

Other information

@zateutsch you hadn't attached your sample project. Did you want to see if the Preview Package spit out by the CI works in your test project?

@michael-hawker michael-hawker added bug 🐛 An unexpected issue that highlights incorrect behavior extensions ⚡ labels Sep 8, 2021
@michael-hawker michael-hawker added this to the 7.1 milestone Sep 8, 2021
@ghost
Copy link

ghost commented Sep 8, 2021

Thanks michael-hawker 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 azchohfi and Rosuavio September 8, 2021 00:45
…ment layout changes and visibility changes
@zateutsch
Copy link
Contributor

@michael-hawker Update fixes all the scenarios I tested in my sample project. Thanks, Michael!

@michael-hawker michael-hawker marked this pull request as ready for review September 8, 2021 17:01
Copy link
Contributor

@zateutsch zateutsch left a comment

Choose a reason for hiding this comment

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

Updates fixes tested scenarios in my sample project. Thanks!

private static void BindSizeAndScale(CompositionObject source, UIElement target)
{
var visual = ElementCompositionPreview.GetElementVisual(target);
var bindSizeAnimation = source.Compositor.CreateExpressionAnimation($"{nameof(visual)}.Size * {nameof(visual)}.Scale.XY");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please tell me there was a document describing how to do this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Kind of? https://docs.microsoft.com/uwp/api/Windows.UI.Composition.ExpressionAnimation

I just asked @Sergio0694 😋. We have helpers in the Toolkit for this, but they're in the Animations package and we don't want to include references to that everywhere.

I also just copied this from the Media package from another internal method and tweaked it... But yeah it is a bit of black magic.

@@ -166,6 +167,16 @@ protected internal override void OnElementContextUninitialized(AttachedShadowEle
_container.Children.Remove(context.SpriteVisual);
}

context.SpriteVisual?.StopAnimation("Size");
Copy link
Member Author

Choose a reason for hiding this comment

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

@Sergio0694 any other clean-up I need to do beyond this to clean-up the composition animation?

Copy link
Member

Choose a reason for hiding this comment

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

Don't think so, the actual objects would just be disposed normally, we're not doing any specific cleanup in other places of the animation package either, it's mostly just fire and forget as usual. Stopping these animations is the only important bit because otherwise the composition layer will just keep them always active even if the XAML object is gone, but other than that we should be good 😄

@michael-hawker michael-hawker merged commit b6ec0bc into CommunityToolkit:main Sep 9, 2021
@ghost ghost added the bugbash 🏗️ label Sep 9, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior bugbash 🏗️ extensions ⚡
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AttachedDropShadow Doesn't Update on Position or Visibility changed
4 participants