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 based on MaterialDesignOutlinedTextBox can't set Padding #2811

Open
Dan127001 opened this issue Aug 9, 2022 · 6 comments
Open

Style based on MaterialDesignOutlinedTextBox can't set Padding #2811

Dan127001 opened this issue Aug 9, 2022 · 6 comments
Labels

Comments

@Dan127001
Copy link

Bug explanation

I want to use a Style for TextBox based on MaterialDesignOutlinedTextBox and set the Padding.
This doesnt work.

What I did:

  • create a TextBox Style (implicit or with a x:Key) based on MaterialDesignOutlinedTextBox
  • have a Setter for Padding
  • create a TextBox deriving from it
<!--in the Resources -->
<Style x:Key="SampleStyle" TargetType="{x:Type TextBox}" BasedOn="{StaticResource MaterialDesignOutlinedTextBox}">
     <Setter Property="Padding" Value="5"></Setter>
</Style>


<!--in Body of the UserControl / Page / Window-->
<TextBox Style="{StaticResource SampleStyle}" Text="Sample"/>


Result:
No Padding changes

Expected:
Padding changes

When I change the Padding at the TextBox directly


<TextBox Padding="5" Style="{StaticResource SampleStyle}" Text="Sample"/>

The Padding changes

The Project is in WPF with .net6.0-windows

Version

4.5.0

@Dan127001 Dan127001 added bug evaluation required Items is pending review or evaluation by the team labels Aug 9, 2022
@Dan127001
Copy link
Author

I think the reason is right here:

When HasOutlinedTextField is true
<Trigger Property="wpf:TextFieldAssist.HasOutlinedTextField" Value="True">
the Padding gets overridden
<Setter Property="Padding" Value="16,16,12,16" />

@Keboo Keboo removed the evaluation required Items is pending review or evaluation by the team label Aug 12, 2022
@Keboo
Copy link
Member

Keboo commented Aug 12, 2022

Yep your analysis is spot on. It also means similar issues will be seen with the other properties that are set as part of the triggers.
I am not entirely sure the appropriate fix for this, happy to accept suggestions.

I suspect the setters within the control triggers that do not specify a TargetName (the ones that target root level properties) should be pulled out of the control triggers and instead set as style trigger. This would at least then make them overridable (though would still require you creating a similar trigger in your derived style).

Another possibility would be to split the style to not use triggers in that way, but that would be a larger refactor and would also mean lots of duplicated XAML.

@Dan127001
Copy link
Author

Dan127001 commented Aug 12, 2022

I agree with your suggestion of pulling certain properties out of the ControlTemplate.Triggers.

However, having them in the Style.Triggers would result in a similar behaviour. Right, now when creating a style based on it, you can already override it when using the right trigger condition in a derived Style.

~ After my findings I tested this as a workaround and so far, I use it this way.
(I create a new Style based on MaterialDesignOutlinedTextBox and then in Style.Triggers add a Trigger with the condition TextFieldAssist.HasOutlinedTextField being true -> in there I can in my case change the Padding)

The best way to address this in my opinion would be to not set the property in a Trigger, but instead a dedicated Style.

So instead of:
Having the TextFieldAssist.HasOutlinedTextField Property and it being set to true in MaterialDesignOutlinedTextBox, only for the MaterialDesignTextBoxBase to check if the property is a certain value

I suggest to change it to:
Have the MaterialDesignTextBoxBase implement the Basic ControlTemplate and then have different Styles implementing the specific looks one wants to achieve.

For example:
MaterialDesignOutlinedTextBox would then implement the Outline/Border Thickness, Brushes, Padding and so on itself.

Another thought:
Then "triggered-behaviours" like IsMouseOver/IsFloating can be implemented easier/cleaner
-> on the Base Style for a default behaviour
-> and then have a custom implementation in the more specific derived styles.
//instead of having multiple MultiTrigger in one style only for IsMouse over

Those are my thoughts on this.
Yes, this would require refactoring, but it would help the project & its users in my opinion.

Looking forward to your response!

@Keboo
Copy link
Member

Keboo commented Aug 17, 2022

Sorry for the delay (I have been sick and not spending as much time online). I think I agree with you. I have often struggled between having lots of derived styles vs attached properties to turn features on and off (which some have suggested are more discoverable).

With that said, I have been leaning more and more away from the attached properties and instead favoring doing things in derived styles. So, I think a design similar to your first suggestion would be what I would want to move toward.

@Dan127001
Copy link
Author

Yes I get where you are coming from.

In the end - what is more discoverable might be open for discussion, even though I would argue for using the Style hierarchy and placing / implementing the properties and behaviours right at the layer, where they "get activated" rather than making another (deeper) layer do the actual changing. This way one can discover which layer changes the TextBox in what way and what to expect when using a sibling (hierarchy wise).

Still, the biggest upside is removing side-effects, such as the one why I created this issue in the first place.

I see, that you are pretty active in this project overall -> What would be the next / first steps to discuss / apply these changes?

@Keboo
Copy link
Member

Keboo commented Aug 22, 2022

Sorry for the long delay. I am perfectly happy to accept a PR that starts breaking up the styles to eliminate some of these problems. If you want to take that on feel free to open up a PR for it.

Be aware, that I do try to avoid breaking backwards compatibility until a major version change. I keep a running list of breaking changes that I want to do in the next major release (5.0.0) in issue #2255. So any changes right now might have to leave the old styles (or at least the style keys in place) until that point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants