microsoft-ui-xaml icon indicating copy to clipboard operation
microsoft-ui-xaml copied to clipboard

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

Open legistek opened this issue 2 years ago • 13 comments

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 avatar Jun 12 '23 18:06 legistek

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.

legistek avatar Apr 29 '24 14:04 legistek

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.

kmgallahan avatar Apr 30 '24 01:04 kmgallahan

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 avatar Apr 30 '24 10:04 legistek

Because that was so hard:

https://stackoverflow.com/questions/72899903/binding-in-a-style-setter-in-winui-3/78408646#78408646

legistek avatar Apr 30 '24 13:04 legistek

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

kmgallahan avatar Apr 30 '24 16:04 kmgallahan

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.

legistek avatar Apr 30 '24 16:04 legistek

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.

kmgallahan avatar Apr 30 '24 17:04 kmgallahan

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?

legistek avatar Apr 30 '24 17:04 legistek

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)

kmgallahan avatar Apr 30 '24 21:04 kmgallahan

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?

legistek avatar Apr 30 '24 22:04 legistek

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:

https://github.com/microsoft/microsoft-ui-xaml/blob/b91b3ce6f25c587a9e18c4e122f348f51331f18b/dxaml/xcp/components/style/OptimizedStyle.cpp#L189

// 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.

kmgallahan avatar May 01 '24 04:05 kmgallahan

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.

legistek avatar May 01 '24 12:05 legistek

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

https://github.com/microsoft/microsoft-ui-xaml/issues/9646 https://github.com/microsoft/microsoft-ui-xaml/issues/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.

GochenRyan avatar Jun 24 '24 08:06 GochenRyan