microsoft / microsoft-ui-xaml

Windows UI Library: the latest Windows 10 native controls and Fluent styles for your applications
MIT License
6.27k stars 674 forks source link

Template Bind all the Properties #6945

Open michael-hawker opened 2 years ago

michael-hawker commented 2 years ago

Template Bind all the Properties

Problem Statement

Developers want to easily change common properties on controls or item templates like Foreground, Background, BorderBrush, BorderThickness, Margin, Padding, CornerRadius, FontSize, FontFamily, Horizontal/Vertical[Content]Alignment etc...

The common XAML pattern is to do this with the property of the control and for XAML control templates to use template binding to enable that.

The Issue

It seems to be a common issue I'm seeing all the time now across how the current control templates have been redone in WinUI. There are many resources exposed from controls which is great, but instead of setting them in the Style for the component they're being used directly. Instead, they should be used in the Style as a Setter and then referenced using TemplateBinding within the control's template.

For instance, CornerRadius on GridViewItem isn't using TemplateBinding here (among other properties here which should be template bound): https://github.com/microsoft/microsoft-ui-xaml/blob/e59614ebda2fad608b5eed0967ffd890f6316385/dev/CommonStyles/GridViewItem_themeresources_21h1.xaml#L177

Without this, folks either have to re-template the entire control (if there is no resource available) or try and override the resource, though there is a common issue with StaticResource not respecting overrides compared to ThemeResource (don't know if there's a tracking issue, #5457 seemed the closest?), so that doesn't always work.

Solution

This issue is requesting an overall audit across the entire set of styles to fix these issues now before they get reported by customers. As well, guidance for new controls during creation and review should audit these best practices like template binding and template naming so they can maintain a high-standard for enabling the customization XAML provides as a system to developers.

Without controls maintaining these patterns, the benefits of the XAML Styling system are diminished.

Related existing Issues reporting this same issue

Other General Style Issues for Audit

michael-hawker commented 2 years ago

Another scenario which just came up in the UWP Discord

AppBarButton doesn't support centering its Icon content when made larger:

image

No template binding here in template:

https://github.com/microsoft/microsoft-ui-xaml/blob/318b036fe05a1d0ea57b6762a60b9f6877bde769/dev/CommonStyles/AppBarButton_themeresources.xaml#L408-L416

michael-hawker commented 1 year ago

Think I found another case to be audited with this, this time from ContentDialog and CornerRadius.

It properly sets up the template binding pattern here:

https://github.com/microsoft/microsoft-ui-xaml/blob/cb398934eb0d0a6812df0dca45dcab67fe466cb9/dev/CommonStyles/ContentDialog_themeresources.xaml#L80

But then fails to use it consistently in the template, it only does it in one spot instead of both:

https://github.com/microsoft/microsoft-ui-xaml/blob/cb398934eb0d0a6812df0dca45dcab67fe466cb9/dev/CommonStyles/ContentDialog_themeresources.xaml#L238-L256

L256 should be using the TemplateBinding so if the value is directly overridden by the developer that it'll get picked up instead of the resource/default value. (Compare to L245 where it's properly template bound.)

FYI @ranjeshj

michael-hawker commented 1 year ago

Bumping, based on https://github.com/microsoft/microsoft-ui-xaml/discussions/8638

This is something I know there's a lot of issues that get filed on.

mdtauk commented 1 year ago

If the Indentation of NavigationViewItems as they are nested within other items - could be taken out of the code, and made into a Resource value - this would also be useful.

michael-hawker commented 7 months ago

Another example is the HeaderStyle for Expander isn't exposed as a HeaderStyle type property on Expander and then template bound:

https://github.com/microsoft/microsoft-ui-xaml/blob/75f7666f5907aad29de1cb2e49405cc06d433fba/dev/Expander/Expander.xaml#L126-L137

This makes it hard to restyle the Header for an Expander without having to restyle the whole control itself, even if you want to leave the rest of the Expander template unchanged. (We had this in the Toolkit originally too.)

michael-hawker commented 7 months ago

Just found another case of this with InfoBar and Padding... 😢

https://github.com/microsoft/microsoft-ui-xaml/blob/75f7666f5907aad29de1cb2e49405cc06d433fba/dev/InfoBar/InfoBar.xaml#L115

This should be template bound to the padding property of the control (which does nothing currently) and then the resource set in the style bound to the Padding property as the default value.

Actually, this resource doesn't seem to control the whole padding of the control which is odd. It seems to be only the left-side padding and can't adjust the top/bottom padding between the background plate and the icon/title/message... not sure what controls that. That's odd... makes creating less space around the InfoBar content very difficult.