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

Style Setters Apply Bindings to Themselves Rather than the Element Being Styled #8547

Open
legistek opened this issue Jun 12, 2023 · 13 comments
Labels
bug Something isn't working feature proposal New feature proposal team-Controls Issue for the Controls team

Comments

@legistek
Copy link

Describe the bug

When using a Binding for Setter.Value in a Style, I would expect, as in WPF, that the Setter instance wouldn't actually be the binding target, but, rather, once the Style is applied, the Binding would be applied to each element receiving the Style.

Instead, it seems the Setter instance becomes the binding target literally, which is quite useless. Therefore something like this:

<Setter Property="Label" Value="{Binding RelativeSource={RelativeSource Mode=Self}, Path=Command.Label}"/>

doesn't work, because Self evaluates to the Setter, rather than the element the Style is being applied to.

Steps to reproduce the bug

Here is a trivial example that, while rather silly, does exemplify the problem:

  1. Create a style for a button like so:
   <Style x:Key="BindingExampleStyle"
          TargetType="Button">
       <Setter Property="Background"
               Value="{Binding RelativeSource={RelativeSource Mode=Self}, Path=BorderBrush}" />
   </Style>  
  1. Example usage:
     <Button BorderBrush="Green"
             Style="{StaticResource BindingExampleStyle}">
         Button
     </Button>

Expected behavior

The button background should be green by virtue of setting the BorderBrush property.

Instead this error is logged in the console:

Error: BindingExpression path error: 'BorderBrush' property not found on 'Microsoft.UI.Xaml.Setter'. BindingExpression: Path='BorderBrush' DataItem='Microsoft.UI.Xaml.Setter'; target element is 'Microsoft.UI.Xaml.Setter' (Name='null'); target property is 'Value' (type 'Object')

In other words, as noted, the binding is being applied immediately with the style setter as the target, rather than the recipient of the style.

Screenshots

No response

NuGet package version

WinUI 3 - Windows App SDK 1.3.1: 1.3.230502000

Windows version

Windows 11 (22H2): Build 22621, Windows 11 (21H2): Build 22000

Additional context

No response

@legistek legistek added the bug Something isn't working label Jun 12, 2023
@bpulliam bpulliam added the team-Markup Issue for the Markup team label Aug 22, 2023
@JesseCol JesseCol added team-Controls Issue for the Controls team and removed team-Markup Issue for the Markup team labels Jan 19, 2024
@DmitriyKomin DmitriyKomin added the feature proposal New feature proposal label Jan 23, 2024
@legistek
Copy link
Author

Just wondering if this is being given any consideration? It's still present in 1.5.

Is there a workaround I'm not aware of? I would have thought the inability to pre-set bindings via style setters would impact a lot of control designers.

I see the documentation Migration Notes discusses this so I realize it's a known issue, but it seems more like a bug than a missing feature. Either way I don't think it would be a breaking change because there's no conceivable reason you'd want the Setter instance to be the binding source.

@kmgallahan
Copy link
Contributor

As your link states:

The Windows Runtime doesn't support a Binding usage for Setter.Value

Sounds like it isn't a "missing feature", "known issue", or a bug, you just can't use a Binding there by design.

@legistek
Copy link
Author

As your link states:

The Windows Runtime doesn't support a Binding usage for Setter.Value

Sounds like it isn't a "missing feature", "known issue", or a bug, you just can't use a Binding there by design.

Pretty poor design to let you assign a binding as a setter value that then does nothing. I'd like to think the MS team is a bit smarter than to do that "by design".

And fortunately WinUI3 doesn't use the Windows Runtime, otherwise I wouldn't be using it.

@legistek
Copy link
Author

@kmgallahan
Copy link
Contributor

And fortunately WinUI3 doesn't use the Windows Runtime, otherwise I wouldn't be using it.

WinUI 3 is built on top of WinRT.

https://learn.microsoft.com/en-us/windows/windows-app-sdk/api/winrt/?view=windows-app-sdk-1.5

@legistek
Copy link
Author

No, it's a refactor of WinRT out of the OS and into the application layer. Meaning Microsoft can finally start fixing some of the 10-year-old techncial debt - like what we're talking about - leftover from the "Metro" days without having to update the OS.

@kmgallahan
Copy link
Contributor

No, it's a refactor of WinRT out of the OS and into the application layer.

Moving some of the code out of the OS doesn't change the fact that it is still a part of WinRT, it just makes deploying changes easier.

Pretty poor design to let you assign a binding as a setter value that then does nothing.

I agree that silent failures are bad.

Otherwise, considering this is how it works and it's documented, I'd say this decision was a part of the design process relating to increasing performance by discouraging the use of slow reflection-based binding that your workaround relies on.

@legistek
Copy link
Author

I'd say this decision was a part of the design process

Oh were you part of the design process? Otherwise why are you opining?

@kmgallahan
Copy link
Contributor

I don't have to opine that it isn't supported for performance reasons. I can just read the Setter.cpp, Style.cpp, and OptimizedStyle.cpp source code. It shows all of the logic regarding how Style Setters are optimized and mutable setters are handled.

They strictly rely on subscribing to dependency property notifications and not having special handling for the Binding markup extension (among other things) like WPF.

https://github.com/microsoft/microsoft-ui-xaml/blob/winui3/release/1.5.2/dxaml/xcp/core/core/elements/Setter.cpp

https://github.com/microsoft/microsoft-ui-xaml/blob/winui3/release/1.5.2/dxaml/xcp/core/core/elements/Style.cpp

https://github.com/microsoft/microsoft-ui-xaml/blob/winui3/release/1.5.2/dxaml/xcp/components/style/OptimizedStyle.cpp

https://referencesource.microsoft.com/#PresentationFramework/src/Framework/System/Windows/Setter.cs

https://referencesource.microsoft.com/#PresentationFramework/src/Framework/System/Windows/Style.cs

https://referencesource.microsoft.com/#PresentationFramework/src/Framework/System/Windows/StyleHelper.cs (main binding handling logic)

@legistek
Copy link
Author

legistek commented Apr 30, 2024

No one is talking about mutable setters. I want to assign a BindingBase instance to an immutable Setter Value and have that binding be applied into a BindingExpression and the binding source resolved with respect to the target FE at the time the Style is applied, like every other XAML framework does it, and NOT with respect to the Style/Setter instance itself which is 100% objectively useless. I'm assuming what you're talking about has to do with ThemeResource as a replacement for DynamicResource, but there is no reason Setter.Value can't accept a dumb and unevaluated Binding instance directly assigned to it just like it accepts any other simple value and just not do anything with it until the appropriate time.

The fact that I could do what I did in my workaround without sacrificing whatever "performance" optimizations are the supposed reason this critical feature isn't possible proves that it in fact is possible. The only reason I can't make my solution more elegant (e.g. no I indeed shouldn't have to use reflection to get the target DP) is because 90% of everything in this maddeningly inflexible framework is sealed or otherwise not extensible.

Anyway I don't see anyone from MS closing the issue as "as designed" so rather than argue with me pointlessly why don't we see what TPTB actually have to say shall we?

@kmgallahan
Copy link
Contributor

without sacrificing whatever "performance" optimizations

Per the WPF documentation on Binding performance, every 1000 Binding setups takes ~100ms. If Bindings were supported in styles and setup at runtime, then each time new XAML elements were created there could be noticeable delays. The more controls with Style Setters using Bindings, the worse the perf. 50 controls with 5 Binding Setters = ~25ms wait time.

https://learn.microsoft.com/en-us/dotnet/desktop/wpf/advanced/optimizing-performance-data-binding


and NOT with respect to the Style/Setter instance itself which is 100% objectively useless

Per the documentation you provided and the source code itself, full Binding support in Style Setters is not supported, only limited, one-time bindings:

else if (depObj && depObj->GetTypeIndex() == KnownTypeIndex::Binding)

// We support only limited, one-time bindings on style setters.
// To get the resolved value, create a temp setter, set the binding on
// its Value property, then get the resolved value from Setter.Value.
// This is equivalent to how the final resolved value is set on a
// non-optimized style setter, where it's set while the setter is
// unparented, and the setter is sealed when added to a collection.

The source indicates Setter Bindings are setup and resolved once per style in a limited manner by design. A BindingBase is not stored in Setter.Value for later use like WPF.

@legistek
Copy link
Author

legistek commented May 1, 2024

If Bindings were supported in styles and setup at runtime, then each time new XAML elements were created there could be noticeable delays.

Then don't use style bindings and you won't be impacted by any fix to this bug/design flaw. For everyone else it'll be no worse than declaring the bindings individually for each control like we have to do now. I'll roll the dice that my users aren't on Surface RT tablets anymore.

Per the documentation you provided and the source code itself, full Binding support in Style Setters is not supported, only limited, one-time bindings:

Which don't actually do anything useful because at the time they're being evaluated there's no data context or other way to intelligently resolve a binding source. Hence, bug/design flaw.

The source indicates Setter Bindings are setup and resolved once per style in a limited manner

Awesome, thanks for pinpointing the source of the bug/design flaw.

// This is equivalent to how the final resolved value is set on a
// non-optimized style setter,

Except it's not equivalent, because at the time it tries to resolve the binding here, it uses the Setter instance for binding source resolution rather than the FE being styled. So someone may have incorrectly thought they were doing something efficient/useful or maybe some subsequent change broke the original goal and rendered it useless. Either way, bug/design flaw.

A BindingBase is not stored in Setter.Value for later use like WPF.

Exactly. Hence bug/design flaw.

I authored the original Relativesource and Multibinding implementations for Xamarin Forms and MAUI, and made other contributions to their XAML compiler, ok? I know how this stuff works and how to do it efficiently and the Microsoft devs are a heck of a lot better at this than me. So please stop clogging this issue with irrelevant arguments over design intent and let Microsoft decide whether they want to address this limitation or not. Thanks.

@GochenRyan
Copy link

It seems that my two issues are related to this topic:

#9646
#9716

I used Binding in Style Setters, and when the control is created at runtime, there will be binding problems. So now I have to use TemplatePart and use DataContextChanged to assign values ​​in code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature proposal New feature proposal team-Controls Issue for the Controls team
Projects
None yet
Development

No branches or pull requests

6 participants