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

Incorrect target type for RelativeAncestor attached properties #106

Closed
Sergio0694 opened this issue Oct 21, 2021 · 3 comments · Fixed by #105
Closed

Incorrect target type for RelativeAncestor attached properties #106

Sergio0694 opened this issue Oct 21, 2021 · 3 comments · Fixed by #105

Comments

@Sergio0694
Copy link
Member

Describe the bug

The attached properties for the "relative ancestor" feature are currently targeting the wrong type:

https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/44ad0cc6806c99f1a693c923cb94ea7551718c13/Microsoft.Toolkit.Uwp.UI/Extensions/FrameworkElement/FrameworkElementExtensions.RelativeAncestor.cs#L18-L21

These are defined in FrameworkElementExtensions but target a DependencyObject. This is not correct on its own, but in particular it also makes the APIs technically more error prone, as there is no type guarantee at build-time, and we're doing additional checks at runtime to ensure the input elements are in fact FrameworkElement-s, otherwise we do nothing:

https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/44ad0cc6806c99f1a693c923cb94ea7551718c13/Microsoft.Toolkit.Uwp.UI/Extensions/FrameworkElement/FrameworkElementExtensions.RelativeAncestor.cs#L62-L65

Given that these APIs only make sense for FrameworkElement instances, and that they're defined within FrameworkElementExtensions too, we should update the signature and have them only target FrameworkElement objects. I should note: this would not be a source breaking change when the API was being used correctly, but it would be a binary breaking change.

@oscarjaergren
Copy link

Does this cause relativeAncestor not to work?

"DataContext="{Binding DataContext.(mainDetails:MasterDetailViewModel.CasaDetailsViewModel), RelativeSource={RelativeSource Mode=FindAncestor,AncestorType={x:Type mainDetails:MasterDetailView}}}""
and this version without being qualifed

DataContext="{Binding DataContext.CasaDetailsViewModel, RelativeSource={RelativeSource Mode=FindAncestor,AncestorType={x:Type mainDetails:MasterDetailView}}}"

Previously worked, but when I tried upgrading to source generator it no longer worked, as it just couldn't find the viewmodel, no error messages or anything, just a crash whenever I opened it due to property being null

@michael-hawker
Copy link
Member

@oscarjaergren your example appears to be WPF based, this issue is about the UWP polyfill we have for RelativeSource Mode=FindAncestor in the Windows Community Toolkit and is unrelated to whatever issue you may have had.

If there's some difference between code you had before with the MVVM Toolkit and code after switching to the Source Generator version, you should open a discussion/issue in the .NET Community Toolkit repo here: https://aka.ms/toolkit/dotnet

@michael-hawker michael-hawker transferred this issue from CommunityToolkit/WindowsCommunityToolkit Jun 27, 2023
@michael-hawker
Copy link
Member

Thanks @Sergio0694, brought this over to the new repo and will fix in #105.

Note, Ancestor can be a DependencyObject still as it's the output of the call to FindAscendant which can return a DependencyObject.

However, you're correct, since we generally want to wait until the element is loaded, we should make AncestorType dependent on FrameworkElement. Related to our discussion in Labs too: CommunityToolkit/Labs-Windows#449

Though technically, we could still call the visual tree helpers on a DependencyObject, I don't think there's going to be many of those cases though, so think fine to exclude?

michael-hawker added a commit that referenced this issue Jun 28, 2023
…Extensions.RelativeAncestor to FrameworkElement over DependencyObject

Code scoped to FrameworkElement in callback, so doesn't work with just DependencyObject as-is.
@michael-hawker michael-hawker moved this to 🏗 In progress in Toolkit 8.x Jul 6, 2023
@michael-hawker michael-hawker moved this from 🏗 In progress to 👀 In review in Toolkit 8.x Jul 6, 2023
Arlodotexe pushed a commit that referenced this issue Jul 11, 2023
…Extensions.RelativeAncestor to FrameworkElement over DependencyObject

Code scoped to FrameworkElement in callback, so doesn't work with just DependencyObject as-is.
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Toolkit 8.x Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants