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

[API Proposal]: Win 11 Theming of Controls - Changes in Controls #8645

Open
singhashish-wpf opened this issue Jan 8, 2024 · 27 comments
Open
Labels
API suggestion Early API idea and discussion, it is NOT ready for implementation Win 11 Theming

Comments

@singhashish-wpf
Copy link
Member

singhashish-wpf commented Jan 8, 2024

Title

Win11 Theming of Controls - Changes in Controls

Background and motivation

The current WPF controls appear outdated and are not visually consistent with the newly rejuvenated Windows UI. This issue has been identified as the most frequently requested feature [276 Upvotes] (#7555) by the development community. The lack of visually appealing WPF controls makes the platform look outdated. A solution is required to update WPF controls to match the modern Windows UI and improve the aesthetic appeal of WPF applications. The proposal adds relevant properties and methods to the controls classes for using them inside the xaml.

Part of effort - #8655

API Pages

TextBox class

Represents a control that can be used to display or edit unformatted text.
Adding new property PlaceholderText to TextBox

TextBox.Placeholder

This property get or set the placeholder text in the TextBox.
Placeholder text is usually displayed when there is no text input in the TextBox.

Usage

To add a text ("Enter some text") to a TextBox when there is no input.

    <TextBox  Placeholder="Enter some Text" />

    <TextBox>
        <TextBox.Placeholder>
            <StackPanel Orientation="Horizontal">
                <TextBlock Text="&#xE894;" Margin="2" FontFamily="Segoe Fluent Icons" />
                <TextBlock Text="Search ..." Margin="2"/>
            </StackPanel>
        </TextBox.Placeholder>
    </TextBox>

API Details

 public partial class TextBox : System.Windows.Controls.Primitives.TextBoxBase, System.Windows.Markup.IAddChild 
     {
+        public static readonly System.Windows.DependencyProperty PlaceholderProperty;
+        public object Placeholder { get { throw null; } set { } }
          
          //Protected properties
+        protected override void OnGotFocus(System.Windows.RoutedEventArgs e) { }
+        protected override void OnLostFocus(System.Windows.RoutedEventArgs e) { }
+        protected override void OnTextChanged(System.Windows.Controls.TextChangedEventArgs e) { }
     }

PasswordBox class

Represents a control designed for entering and handling passwords.
Adding properties for Placeholder ( other properties will be introduced iteratively )

PasswordBox.Placeholder

This property get or set the placeholder text in the PasswordBox.
Placeholder text is usually displayed when there is no password input in the PasswordBox.

Usage

To add a text ("Enter your password") to a PasswordBox when there is no input password.

    <PasswordBox  PlaceholderText="Enter your password" />

API Details

    public sealed partial class PasswordBox : System.Windows.Controls.Control 
     {
+        public static readonly System.Windows.DependencyProperty PlaceholderProperty;
+        public object Placeholder { get { throw null; } set { } }
     }

ComboBox class

Represents a selection control with a drop-down list that can be shown or hidden by clicking the arrow on the control.
Adding properties for Placeholder ( other properties will be introduced iteratively )

ComboBox.Placeholder

This property get or set the placeholder text in the ComboBox.
Placeholder text is usually displayed when there is no password input in the PasswordBox.

Usage

To add a text ("Enter your password") to a PasswordBox when there is no input password.

    <ComboBox  PlaceholderText="Enter your password" />
    <ComboBox ItemsSource="{StaticResource ComboBoxItemsList}"
            PlaceholderText="Select City..."
            IsEditable="true"
            IsReadOnly="true"/>

API Details

    public sealed partial class PasswordBox : System.Windows.Controls.Control 
     {
+        public static readonly System.Windows.DependencyProperty PlaceholderProperty;
+        public object Placeholder { get { throw null; } set { } }
     }

Risks

No response

cc: @dotnet/dotnet-wpf-triage @pomianowski

@singhashish-wpf singhashish-wpf added API suggestion Early API idea and discussion, it is NOT ready for implementation Win 11 Theming and removed draft labels Jan 9, 2024
@miloush
Copy link
Contributor

miloush commented Jan 9, 2024

There is two different types of additions. One are appearance based (corner radius, backgrounds etc.), one are function based (placeholder text, editing style etc.). Perhaps these could be two different proposals. The functional ones seem reasonable in principle, I am bit uneasy with the appearance ones. Perhaps they might be better off in separate classes, like ThemeButton or IconButton or something.

IRelayCommand is missing.

What is the reason for ClientAreaBorder?

As for PassiveScrollViewer, can the property just not be added to normal ScrollViewer? What are the "certain events" to be bubbled and does "inactive" mean "not focused"?

Win11 is probably not an appropriate way to describe the theme and namespace, I am sure there is a name for it akin to Aero.

@batzen
Copy link
Contributor

batzen commented Jan 9, 2024

Just adding my opinion for the changes to Button, TextBox and DataGrid as of yet.

Button:
We already have styles, i don't see any reason to add an Appearance property.
Why not just provide different styles for different buttons?
99% (guessed) of buttons are regular buttons, why should every button instance loaded a plethora of triggers for the 1% case?
The same applies to the proposed Icon property.

DataGrid:
I don't think that column specific stuff belongs in DataGrid.
What if i have different checkbox columns that should behave differently?

TextBox:
Whats the purpose of IsTextSelectionEnabled? We already can make it read-only.
Same as for button and Icon.
Whats the purpose of TemplateButtonCommand?

General:
The general direction of changes seems to be to extend controls with niche properties that blow up the complexity of the templates/styles.
Has anyone considered memory and performance for these changes?

@singhashish-wpf
Copy link
Member Author

@miloush @batzen
Thanks a lot for your feedback and responses.
As of now we have a choice of adding more properties and functionality to the existing controls or to provide new controls in a different namespace or controls with different names (Specialized controls eg: ThemedButton or namespaced button eg: win11: button).

I am organizing my thoughts like below, please feel free to add any criteria which is missed and should be accounted for.

S.No Criteria Create new controls Modify existing controls Explanation
1 Compatibility Existing functionality and behavior shouldn’t be modified. Existing apps should keep on working with the same behavior, functionality and performance metrics.
2 Performance Introducing additional dependency properties entails increased memory consumption and subsequent property change notifications, thereby amplifying processing overhead. This escalation in memory usage may impact users who opt not to transition to new themes while upgrading to .NET 9. Moreover, both UI responsiveness and rendering can be adversely affected, attributed to the incorporation of new templates and XAML processing, even for individuals not utilizing the updated themes. Notably, the majority of the added properties pertain to appearance and fulfill specific purposes; however, these properties lack a generic use case, potentially imposing overhead on existing controls when modifications are made to them.
3 Ease of use/upgrade/Adoption The amount of effort app developers would have to invest in changing the theme of their apps using .NET9 would mainly involve some xaml changes and using proper classes wherever required in case of new controls approach. In case of modification to existing controls, they won’t have to make any change, their apps would be themed by default, just when they upgrade.
4 Testing With new controls, test cases need to be written specifically to test the newer controls and the newer properties and the new functionality being added.
5 Maintainability Any problems occurring due to theming will be restricted to the newer classes introduced and the impact radius will be limited to the people using new controls in their xaml

@dipeshmsft
Copy link
Member

I have updated the proposal above. The initial proposal was generated after the integration of the styles and had a lot of properties that were not needed.

@batzen

Button:
We already have styles, i don't see any reason to add an Appearance property.
Why not just provide different styles for different buttons?
99% (guessed) of buttons are regular buttons, why should every button instance loaded a plethora of triggers for the 1% case?
The same applies to the proposed Icon property.

Agreed. Hence, we decided to remove these from button and have two separate styles, one for standard and other for accent button, similar to WinUI.

DataGrid:
I don't think that column specific stuff belongs in DataGrid.
What if i have different checkbox columns that should behave differently?

As of now, we have removed the properties from DataGrid, but can you provide/point to a code which showcases this.

TextBox:
Whats the purpose of IsTextSelectionEnabled? We already can make it read-only.
Same as for button and Icon.
Whats the purpose of TemplateButtonCommand?

IsTextSelectionEnabled was not needed. Similarly, we removed Icon property.
TemplateButtonCommand, was there to allow customization when clicking the clear button. However, we have made it internal for now, and provide the default functionality to the clear button.

@miloush

As for PassiveScrollViewer, can the property just not be added to normal ScrollViewer? What are the "certain events" to be bubbled and does "inactive" mean "not focused"?

For now, this is not included. Probably it can be added to ScrollViewer, but this will be considered later.

What is the reason for ClientAreaBorder?

This manages the padding for the window. When using WindowChrome to extend into non-client area, this can be used to automatically fill the client area. I do think, we can provide this functionality by default, but I need to do some more experiments for this.

@MikeHillberg
Copy link

There's a PlaceholderText property here now on TextBox and PasswordBox. I think ComboBox is an important one to have as well.

@batzen
Copy link
Contributor

batzen commented Feb 27, 2024

@dipeshmsft

As of now, we have removed the properties from DataGrid, but can you provide/point to a code which showcases this.

Different CheckBox-Columns might:

  • have different bindings for their IsEnabled
  • have different bindings for their IsVisible

For example in our codebase we have a DataGrid that's capable of displaying hierarchical data. As our solution is solely based on the data source (ICollectionView) and there are zero modifications to the DataGrid itself, we have to show/hide content in cells based on their data. Parent rows might not have a visible checkbox (or vice versa).

How should that even work technically as CheckBoxColumn already has those properties?
Which style has precedence?
If i want to have the same styles for all CheckBoxColumns i can simply inherit from it and set the desired defaults there.
If the desired outcome is to simplify the sharing of defaults/settings for DataGridColumns (instead of inheritance) then the right place would be DataGridColumn and not DataGrid with various, but limited, amounts of defaults for selected column types.

@dipeshmsft
Copy link
Member

Got it, that is a legitimate use case, that's why we have removed the extra properties on DataGrid.

@miloush
Copy link
Contributor

miloush commented Feb 27, 2024

Wouldn't PlaceholderText be better on TextBoxBase?

@rchauhan18
Copy link
Contributor

Wouldn't PlaceholderText be better on TextBoxBase?

Thanks for the suggestion. @miloush
Initially we intended to add PlaceholderText property to TextBoxBase directly, however we didn't do that because it is also inherited by RichTextBox, and it is not as trivial to trigger this as it is in TextBox.
We have our plan to move placeholder property to TextBoxBase in the upcoming preview releases and investigate on adding Placeholder property to RichTextBox.

@miloush
Copy link
Contributor

miloush commented Feb 27, 2024

While I appreciate that depending on the implementation it might be more challenging to get it working for RichTextBox, I wonder whether putting it on TextBoxBase and implementing it later for RTB might be a better course of actions than trying to move the property later and figuring out doing so breaks existing code. For example, the owner of the property would change.

I also want to double check that people are happy with the property being a string. I would expect, like it is the case of various Header properties in the framework, that they could be object and any element could be used as a placeholder. How is one going to be able to format the placeholder text if it's just a string?

@kmahone
Copy link

kmahone commented Feb 27, 2024

This proposal looks good to me.

@dipeshmsft
Copy link
Member

I wonder whether putting it on TextBoxBase and implementing it later...

TextBox and RichTextBox both will have different implementations for handling the PlaceholderText property. Moreover, adding it in TextBoxBase, will make it constrained for developers deriving from TextBoxBase to have this property and use it in a particular way.

that they could be object and any element could be used as a placeholder.

Setting it to string will support majority of cases and it can anyways be overlayed for specific scenarios.
Moreover, from the A11y perspective it will be ambiguous, whose responsibility will it be to make the placeholder content grey when you set it up as an object and then it will require template changes and complex scenarios.

@dipeshmsft dipeshmsft added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Feb 27, 2024
@miloush
Copy link
Contributor

miloush commented Feb 27, 2024

@dipeshmsft do you have an implementation in mind that you could point to or describe?

If it was an object, you can just have a ContentPresenter in the template which will turn a string into a TextBlock. You can set TextBlock.Foreground on the ContentPresenter and it will get grey by default and people can still have full control over the representation.

Even if it is string only it should possibly have an associated style property (such as PlaceholderStyle).

However, if the implementation is actually setting the TextBox text to the placeholder text or if you are drawing the string on render, then this obviously does not apply (but also would be problematic).

@dipeshmsft
Copy link
Member

Okay, for implementation you can refer to feature/win11theming/api/implementation branch. It is still in a rough form.
As of now, in implementation we have a TextBlock that is responsible for showing the Placeholder text.

With ContentPresenter, developers will be able to set an image or other controls as placeholder, in which case the A11y point comes up.

@miloush
Copy link
Contributor

miloush commented Feb 27, 2024

What is the A11y point coming up? Does it apply now when we don't support any placeholder, or when developer does not set any? Developers can break A11y in any way they wish, I don't see why trying to block it in this specific case helps it.

Even with your solution I assume putting a TextBlock style in the TextBox resources would affect the placeholder...

@batzen
Copy link
Contributor

batzen commented Feb 27, 2024

@dipeshmsft Please don't use the A11y argument to force a specific implementation on users of WPF.
Don't get me wrong. A11y is important and should always be considered.
But removing flexibility in the name of A11y is absolutely going to backfire.
If you force Placeholder to be string people will have to re-implement everything just to add, for example, an icon to their placeholder.
I totally agree with @miloush that it should be implemented the WPF way and that means using a Placeholder, PlaceholderTemplate etc. Just like it's done in almost all places in WPF.
If someone wants to use an image as a placeholder that's totally fine and it would also be totally fine to violate/ignore A11y requirements if the software is not built for people with disabilities.

If WPF would have been built based on the A11y and the spirit that it's more important than everything else and that people won't get it right in their software things like ToolTip would have been force to be a string.

@dipeshmsft
Copy link
Member

@miloush @batzen, after taking in consideration your points, I reviewed the API again and modified the Placeholder property to be of type object.

Thoughts on the new API surface ?

@batzen
Copy link
Contributor

batzen commented Feb 29, 2024

That looks better.

When does the placeholder become visible?
I am asking because currently it looks like it's always visible when there is no input.
In my application the placeholder also gets hidden when the control has focus.
But that might not be the desired behavior for everyone.
So i guess we need a way to define when the placeholder is visible.

DatePicker seems to be missing from the list of controls where a placeholder might be desired.
Not sure if there are other controls that might benefit from a placeholder.

@dipeshmsft
Copy link
Member

Currently the check is that when the textbox or password box has some non zero content, the placeholder becomes invisible.

@miloush
Copy link
Contributor

miloush commented Feb 29, 2024

By the way the example with stackpanel and textblocks can be done using textblock and runs.

I was going to put in some more scenarios in case that helps convincing people:

  • Stricter accessibility standards requiring higher contrast where gray text won't make it
  • Using different colors to distinguish e.g. mandatory and optional fields
  • Having to use different fonts in the text when font fallback does not handle the languages or other requirements, such as the emoji/icon example above.
  • Different formatting from the main content, like an italic text for example.
  • Having the placeholder appear and disappear differently - on focus, on hover etc.
  • Allowing the placeholder to recognize accelerators and focus the textbox.
  • Supporting animations
  • Having a button or buttons to set or generate particular content. Copilot icon?

(I am sticking to the reasonable ones)

@batzen from the proposal it was unclear whether the visibility is part of the TextBlock, in which case you could restyle/retemplate it. From the code it seems that it is based on content, but in a scary way, see here (used in template here). I can see now how this was getting tricky for RTB. It should probably done using triggers in the template instead. I mean currently if I read the code correctly, the implementation removes any binding applied to the PlaceholderText property.

Since we have it an object now, the TextBlock will be presumably automatically created. I don't think we need a template property, but to achieve some of the points above including @batzen's scenario, I think we need a style property applied to the container (easier than to the content), like PlaceholderStyle or PlaceholderContainerStyle which is also in charge of the visibility.

(Unrelated to the issue: I do want to point people to the "clear button" implementation which I also find problematic, currently it is such that it doesn't need to go through API approval)

@batzen
Copy link
Contributor

batzen commented Feb 29, 2024

@miloush Indeed.
The code for placeholder and clear is extremely problematic.
Changes like setting the caret position on focus are concerning too.

@dipeshmsft
Can you clarify why placeholder and clear are solved in code behind?
For example:

  • Changing the PlaceholderText just to trigger something in the style/template doesn't seem right.
  • Why do we need a separate internal property for the enabled state of the button? Why isn't it using a user settable ICommand for that?
  • Why does the clear button get invisible when the control looses focus?

Is there a different issue for the clear functionality? Then i could add my feedback there.

Currently the check is that when the textbox or password box has some non zero content, the placeholder becomes invisible.

I see. What about my other concerns?

@miloush
Copy link
Contributor

miloush commented Feb 29, 2024

Perhaps we should keep this issue for the public API changes approval and open issues for the implementations?

@dipeshmsft
Copy link
Member

@miloush @batzen, with Win11 theming the approach that we are taking is to achieve the correct Win11 styles with minimal API changes and have parity with WinUI in terms of style and behaviors.

We want to work iteratively to allow more customizations for developers.

Why isn't it using a user settable ICommand for that?
(Unrelated to the issue: I do want to point people to the "clear button" implementation which I also find problematic, currently it is such that it doesn't need to go through API approval)
Placeholder for datepicker
I think we need a style property applied to the container (easier than to the content), like PlaceholderStyle or PlaceholderContainerStyle which is also in charge of the visibility.

We would like to take these issues in next iterations of API proposals.

Why does the clear button get invisible when the control looses focus?

I think WinUI has this behavior, so we kept it the same way.

Regarding the implementations, thanks for both your inputs. We will look into how to handle Placeholder and Clear in a better way, but some of issues will be resolved ( some properties and methods in the branch are present as a result on integration of themes, we will remove things that are not necessary ).

Why do we need a separate internal property for the enabled state of the button?

@batzen can you clarify the following,

I see. What about my other concerns?
If I am not wrong, the gist is that with the current implementation you will lose the customization for when to hide the placeholder.

I think we can address this in the next iterations, right ?

Also, I don't understand what you mean by this -

Changes like setting the caret position on focus are concerning too.

@batzen
Copy link
Contributor

batzen commented Feb 29, 2024

@dipeshmsft

If I am not wrong, the gist is that with the current implementation you will lose the customization for when to hide the placeholder.

I wouldn't loose the customization as i am using my own implementation, but it would be nice if i could just switch to the built in implementation from WPF once it's ready.
If the default implementation in WPF isn't flexible enough to incorporate such customizations it won't be adopted.
We should have a look at existing implementations for everything that should be added to WPF to make those implementations obsolete/no longer needed.
I think it's a good idea to get ideas from WinUI, but we should also take into account solutions from existing WPF libraries.
In case of placeholder and clear button you can have a look at https://github.com/MahApps/MahApps.Metro/blob/develop/src/MahApps.Metro/Controls/Helper/TextBoxHelper.cs
It's not perfect there, but it's way more flexible than the current proposed implementation.

I think WinUI has this behavior, so we kept it the same way.

Same as above. I think it's a good idea to have a look at WinUI, but at the same time that shouldn't be the only source.
WPF is used more widely than WinUI and after trying WinUI a few times already i still very much prefer WPF. Especially because it's way more flexible/customizable, easier to debug, easier to diagnose etc..

Also, I don't understand what you mean by this -

Changes like setting the caret position on focus are concerning too.

In the current implementation branch inside OnGotFocus of TextBox the CaretIndex is changed to be at the end of the text.
I don't know why this was added, but it might break existing apps. The behavior was always that the CaretIndex did not change when loosing/getting focus.
It's just unexpected to see such changes being made.

@dipeshmsft
Copy link
Member

@miloush @batzen , I think that adding a PlaceholderVisible will give us enough flexibility to handle the custom behvaiours of Placeholder property.

Customizing style of Placeholder ( like colour, italics, etc. ), providing buttons, etc. can be done using the Placeholder object.

What are your thoughts on this ? Will this cover the scenarios ?

@miloush
Copy link
Contributor

miloush commented Mar 1, 2024

PlaceholderVisible is unclear. Either it is read-only in which case it should be IsPlaceholderVisible or it is settable, in which case PlaceholderVisibility of type Visibility would be better (and easier for implementation and faster).

Yes once you have an object, you can do the customization in there.

I agree that visibility is probably the only important thing behavior-wise. Associated Style or Template property could take care of that, but I admit we do have a precedence here too, like DataGrid.GridLinesVisibility/HeadersVisibility/PlaceholderVisibility, DataGridRow.DetailsVisibility etc.

@Neme12
Copy link

Neme12 commented Apr 10, 2024

I would just a global setting somewhere to set the default control styles (classic, win11, maybe win10 or other windows versions, etc). Or maybe per window. And add the styles to the default resource dictionary so that even if people don't use the global setting, they can use the appropriate ThemeResource on individual buttons and other controls to change their styles anyway. But there's no need to add anything like a new Appearance property IMO - WPF already has styling.

@dipeshmsft dipeshmsft removed the api-ready-for-review API is ready for review, it is NOT ready for implementation label May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API suggestion Early API idea and discussion, it is NOT ready for implementation Win 11 Theming
Projects
None yet
Development

No branches or pull requests

8 participants