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

NumberBox should respect Background/Foreground properties #2998

Closed
Felix-Dev opened this issue Jul 27, 2020 · 12 comments
Closed

NumberBox should respect Background/Foreground properties #2998

Felix-Dev opened this issue Jul 27, 2020 · 12 comments
Labels
area-NumberBox NumberBox Control no-issue-activity team-Controls Issue for the Controls team

Comments

@Felix-Dev
Copy link
Contributor

Felix-Dev commented Jul 27, 2020

Describe the bug
Currently, the NumberBox control does not respect its Background and Foreground APIs like the TextBox control does it. See the following XAML and the resulting UI:

<muxc:NumberBox MinWidth="200" 
        Foreground="Green" 
        Background="#FF404040"
        Header="NumberBox"
        Value="100"/>
<TextBox MinWidth="200" 
     Margin="0,10,0,0"
     Foreground="Green"
     Background="#FF404040"
     Header="TextBox"
     Text="Some text"/>

image

Expected behavior
image

Open questions
How should these two APIs affect the NumberBox control when it is in SpinButtonPlacementMode.Inline? Developers might want to dynamically change the foreground color of the NumberBox's input field depending on the actual value of the NumberBox. For example, color green for positive values and color red for negative values. As such, the NumberBox.Foreground API should only affect the foreground of the input box and not the foreground of the spin buttons as well. Consequently, the NumberBox.Background property should just affect the background of the input field too:
image

If developers also need to modify the foreground and/or background color of the spin buttons in inline mode, lightweight styling can be used. I have opened proposal #2844 which suggests adding a whole new set of NumberBox specific theme resources to give developers plenty of flexibility in styling their NumberBox controls.

Version Info
Newest WinUI master builds.

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Jul 27, 2020
@StephenLPeters StephenLPeters added area-NumberBox NumberBox Control team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Jul 27, 2020
@StephenLPeters
Copy link
Contributor

Seems reasonable to me, do you have thoughts on the open question?

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Jul 28, 2020

@StephenLPeters As outlined above I think the Foreground API should just affect the input field of the NumberBox (its internal TextBox). Consequently, the Background API should do the same - only affect the background of the input field.

There is, however, issue #1925 which currently suggests that the BorderBrush API (and related APIs like BorderThickness) would affect the complete NumberBox control in inline mode:
image

As such, in my opinion it really comes down to how developers would expect the Background API to behave here - just affecting the input field or also the spin buttons of the NumberBox in inline mode since it is named NumberBox.Background and not NumberBox.TextBackground.

image
(NumberBox where the Background API affects its input field and spin buttons in inline mode)

If developers expect it to affect the background of both the input field and the spin buttons, then that raises the question how the NumberBox.Foreground API should behave as a consequence. Would it affect both the foreground of the input field and the foreground of the inline spin buttons? Or would we limit it to only affect the foreground of the input field? I believe that since developers can then change the background of the spin buttons dynamically at runtime, they should also be able to change the foreground of the spin buttons dynamically at runtime. This would effectively rule out the latter case in favor of the former (affecting both input field foreground and spin buttons foreground). In this case, we need to introduce a new API which only affects the foreground of the input field. This one could be named NumberBox.TextForeground (and perhaps add a NumberBox.TextBackground API too) similar to how the NumberBox will get a TextAlignment API.

NumberBox.TextForeground, if set, would take precedence over NumberBox.Foreground for the foreground color of the input field (Numberbox.TextBackground and NumberBox.Background would act analog to this).

If, however, developers would be fine if the NumberBox.Background API only allows configuring the background of the input field, then we can use the NumberBox.Foreground API here without having to introduce a new API like NumberBox.TextForeground.

What do you think?

@robloo
Copy link
Contributor

robloo commented Jul 28, 2020

I think Background, BorderThickness and BorderBrush should apply to the entire NumberBox -- spin buttons included. That is most intuitive from my viewpoint. From a styling standpoint, if the background needs to change it's likely the spin buttons need to changes as well -- except in data validation scenarios.

So right now I would hook everything up to use background. In the future it might make sense to have separate SpinButtonBackground/BorderBrush/BorderThickness properties.

@Felix-Dev
Copy link
Contributor Author

@robloo Yes, I agree. Applying the Background only to the input field background as shown in the opening post always looked kinda strange to me. And I think we will be fine for now if we only introduce a new TextForeground and possibly TextBackground API here as developers can use lightweight styling to individually style parts of the NumberBox. Put together, these customization options should be enough for now.

How do yow view the suggestion of TextForeground and TextBackground taking precendece, if set, over the respective Foreground and Background APIs?

Here are two examples:

<muxc:NumberBox MinWidth="200" x:Name="TestNumberBox"
        Foreground="DodgerBlue" 
        Background="#FF404040"
        Header="NumberBox"
        SpinButtonPlacementMode="Inline"
        Value="100"/>

image
and

<muxc:NumberBox MinWidth="200" x:Name="TestNumberBox"
        Foreground="DodgerBlue" 
        Background="#FF404040"
        TextForeground="Green"
        TextBackground="Black"
        Header="NumberBox"
        SpinButtonPlacementMode="Inline"
        Value="100"/>

image

@robloo
Copy link
Contributor

robloo commented Jul 28, 2020

So right now I would hook everything up to use background. In the future it might make sense to have separate SpinButtonBackground/BorderBrush/BorderThickness properties.

I realize we should do something like AutoSuggestBox.TextBoxStyle. There should be style properties for both the increase/decrease spin buttons (they need to be separate because of corner radius handling). Exposing two new properties for this would allow whatever customization is needed as the style would be applied directly to the repeat buttons.

This same concept should then be applied to the input TextBox itself. We should have a TextBox style that can be used to customize the foreground and background that you are trying to do above. That will allow a lot more styling as well. We shouldn't be adding new TextForeground and TextBackground properties for special cases like this in my opinion.

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Jul 28, 2020

We should have a TextBox style that can be used to customize the foreground and background that you are trying to do above. That will allow a lot more styling as well. We shouldn't be adding new TextForeground and TextBackground properties for special cases like this in my opinion.

@robloo The issue I'm seeing here is that some developers want to to easily change the foreground color of the input field of the NumberBox depending on the actual value. This doesn't strike me as a corner case so it should be something which can be easily achievable: I would consider having to create custom TextBox styles just for changing the text foreground color from red to green (and vice-versa) as not being developer-friendly. Just as developers today can use the TextBox.Foreground API to easily customize the foreground color of the TextBox text at runtime, so should it be possible to easily set the foreground of the NumberBox text (without affecting the spin buttons).

That said, depending on how proposal #784 will be handled, your suggested NumberBox.TextBoxStyle API might still be useful and needed in the future.

There should be style properties for both the increase/decrease spin buttons (they need to be separate because of corner radius handling). Exposing two new properties for this would allow whatever customization is needed as the style would be applied directly to the repeat buttons.

There is proposal #2789 which asks for the icons of the spin buttons to be customizable. I am mentioning this because the discussion there showed that there is some concern that providing a completely free styling experience could end up harmful to the customer experience and thus might not be something the team will greenlight. Your idea would give developers even more freedom than my initial suggestion in that proposal so I'm not sure how that would be looked upon by the WinUI team.

@robloo
Copy link
Contributor

robloo commented Jul 28, 2020

@robloo The issue I'm seeing here is that some developers want to to easily change the foreground color of the input field of the NumberBox depending on the actual value. This doesn't strike me as a corner case so it should be something which can be easily achievable:

Yes, you are certainly correct and there is always a trade-off with ease-of-use and functionality. Are there any other controls that use TextForeground? I can't recall any. Creating a new property like this is a bit counter-intuitive considering all existing controls. We should really follow convention and I can't help but think a general-purpose solution would be better for all the cases we aren't considering as well. It also beautifully solves the spin button styling topic. Overall, it prevents developers from having to re-template the entire control which is the main concern and also doesn't require custom resource definitions (often poorly documented). A simple style isn't so hard to write and is very concise in markup. This has been the standard convention since the beginning. XAML-related tech has always had a steeper learning curve as it's necessary for it to be as powerful as it is.

<NumberBox.TextBoxStyle>
   <Style TargetType="TextBox">
    <Setter Property="Foreground"
            Value="Green" />
  </Style>
</NumberBox.TextBoxStyle>

I am mentioning this because the discussion there showed that there is some concern that providing a completely free styling experience could end up harmful to the customer experience and thus might not be something the team will greenlight. Your idea would give developers even more freedom than my initial suggestion in that proposal so I'm not sure how that would be looked upon by the WinUI team.

This is counter to the look-less control principles that WPF started with and we are still working with today. I realize I'm getting more-and-more alone in that voice though.

  1. This is no different than other controls like the AutoSuggestBox example and matches with existing design patterns.
  2. Fear of giving developers too much customization and hurting the user experience is a monumental shift in thinking. XAML is customization -- it is the power to have look-less controls that can be styled however needed. That said, there are natural constraints that limit things:
    • Any basic and simple app will hardly ever re-style the in-box controls
    • Any app that becomes mainstream and affects a lot of users will follow platform conventions or their own consistent styling. If it becomes a user concern they, not Microsoft, would have to address it. It doesn't make sense for Microsoft to artificially hold the developer's hand here when they aren't responsible for the end result. It could actually harm the developer experience if we have to end up re-templating because things were over-constrained and our designers required something different.
    • Developers don't change things unless they have to. The vast majority of developers wouldn't change the styling unless they felt there was a good reason to. Who are we to say what is and isn't a good reason?

@Felix-Dev
Copy link
Contributor Author

@robloo

Are there any other controls that use TextForeground? I can't recall any. Creating a new property like this is a bit counter-intuitive considering all existing controls.

The issue here really is only the special NumberBox spin button inline state and how we interpret Foreground and Background in that case. If we say Foreground should just affect the input field foreground, then there is no need for a new API to be added. Otherwise, we have a situation special to the NumberBox (afaik there is no equivalent TextBox, PasswordBox, AutoSuggestBox or ComboBox situation similar to the NumberBox in rest mode with SpinButtons in inline mode). That, together with the impression that this does not sound like a rare scenario, would warrant to me the creation of NumberBox specific APIs here.

A simple style isn't so hard to write and is very concise in markup.

That is the true. Given the current NumberBosx setup a developer would first create a red and green foreground style of a TextBox. Then, they would have to create red and green foreground styles of the TextBox style used by the NumberBox when it is in compact mode. Finally, they could create a function binding to the suggested NumberBox.TextBoxStyle property where they will pick one of these four styles depending on the actual NumberBox value.

The question for me then is, and I won't be able to answer that, how common the scenario of changing the NumberBox's input field foreground color dynamically at runtime would be. If it turns out to be quite a common scenario, then I believe we should add dedicated TextForeground and TextBackground APIs (if actually required) to make this as simple as possible. Otherwise, the custom Style solution seems like the way to go.

As for your suggestion adding Style properties for the spin buttons: You could add that to proposal #2789 as yet another idea what WinUI should offer developers to style the spin buttons. For example, there is no need to create APIs like NumberBox.SpinButtonUpGlyph when we could also provide an API like NumberBox.SpinButtonUpStyle.

@robloo
Copy link
Contributor

robloo commented Jul 28, 2020

@Felix-Dev I certainly understand your points. If it was up to me, I would go the styling route first (it's more powerful and fits convention). If after some time a lot of people are complaining then I would consider TextForeground-type properties. But before adding them to NumberBox I would consider other similar scenarios in other controls and how they are handled.

Anyway, that's my 2 cents. We probably need more feedback from others or for Microsoft to weigh in directly at this point. It's great you take up working on these types of things!

@ldeffenb
Copy link

ldeffenb commented Apr 1, 2021

Is this issue just withering on the vine? Or has something that accomplishes this been implemented, but not linked to this issue?

@marcelwgn
Copy link
Collaborator

I think this was fixed with #4469

@github-actions
Copy link

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 5 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NumberBox NumberBox Control no-issue-activity team-Controls Issue for the Controls team
Projects
None yet
Development

No branches or pull requests

6 participants