Add missing templatebindings to NumberBox

[marcelwgn]

Description

This adds the TemplateBinding to NumberBox, that are necessary to customize FontSize, FontFamily and FontWeight.

Motivation and Context

Fixes #2603

How Has This Been Tested?

Tested manually:

Screenshots (if appropriate):

[mdtauk]

Things like this should have been caught before release.

I wonder if there shouldn't be some UI Test for customised control properties, to ensure the rendering matches the set properties?

[marcelwgn]

Things like this should have been caught before release.
I wonder if there shouldn't be some UI Test for customised control properties, to ensure the rendering matches the set properties?

The problem is how to determine what the correct rendering is in every case. In this case, it was forgotten to test FontSize, which is unfortunate. At the end it was a bug, which shouldn't have happened, but well all bugs shouldn't happen.

Having a checklist for all the cases and options to check would be great and would help finding those bugs before they make it into production. Maybe this is something we should think about, however that list could potentially contain dozens or hundreds of checks, after all there are a lot of options to customize and use controls. Another option would be to test that automatically, however how do you determine what the correct rendering for all configurations is without a person deciding that at some point?

[mdtauk]

Things like this should have been caught before release.
I wonder if there shouldn't be some UI Test for customised control properties, to ensure the rendering matches the set properties?

The problem is how to determine what the correct rendering is in every case. In this case, it was forgotten to test FontSize, which is unfortunate. At the end it was a bug, which shouldn't have happened, but well all bugs shouldn't happen.

Having a checklist for all the cases and options to check would be great and would help finding those bugs before they make it into production. Maybe this is something we should think about, however that list could potentially contain dozens or hundreds of checks, after all there are a lot of options to customize and use controls. Another option would be to test that automatically, however how do you determine what the correct rendering for all configurations is without a person deciding that at some point?

Maybe that kind of thing should be decided at the spec stage?

Generally there is some common sense at play here. If the control has a FontSize property, changing the value should be reflected.

The default values in the ControlTemplates should reflect the 100% scaling Fluent Designs, but changing properties should override - this is what is expected.

Numberbox having sub controls, would need Template Bindings to the parent control in most cases, but I think some kind of Unit Test could be setup to test overriding of values on the parent control, and compare it to the render result.

[marcelwgn]

Maybe that kind of thing should be decided at the spec stage?
Generally there is some common sense at play here. If the control has a FontSize property, changing the value should be reflected.

The problem is that a lot of properties are inherited from control, and are not part of the control itsself, that is that it was added specifically to the control. If you look at all the properties we inherit from the Control class, it might be very time expensive to think about all that are applicable for that control.

The default values in the ControlTemplates should reflect the 100% scaling Fluent Designs, but changing properties should override - this is what is expected.

Yes agreed, if there is any control that does not do that, I think that should change.

Numberbox having sub controls, would need Template Bindings to the parent control in most cases, but I think some kind of Unit Test could be setup to test overriding of values on the parent control, and compare it to the render result.

The problem is that, in order to verify all properties, we would need a lot of tests, and those tests can often break. We currently have VisualTreeVerification tests, however we do not have them for all controls (I think). I think this is an area we could improve on.

[mdtauk]

The problem is that a lot of properties are inherited from control, and are not part of the control itsself, that is that it was added specifically to the control. If you look at all the properties we inherit from the Control class, it might be very time expensive to think about all that are applicable for that control.

The problem is that, in order to verify all properties, we would need a lot of tests, and those tests can often break. We currently have VisualTreeVerification tests, however we do not have them for all controls (I think). I think this is an area we could improve on.

Not sure if this is a WinUI feature, or inherent with the language projections - but it would be a good idea to allow the hiding or removal of those Control properties that are not needed for the control inheriting from it.

Maybe there could be subsets of Control which can be categorised with certain properties?

I know its not a simple solution

[marcelwgn]

Not sure if this is a WinUI feature, or inherent with the language projections - but it would be a good idea to allow the hiding or removal of those Control properties that are not needed for the control inheriting from it.

The issue here is that, that's how C++ and C# behave with inheritance.

Maybe there could be subsets of Control which can be categorised with certain properties?

The problem is that, in order to not inherit those properties, you would go up the chain of types so much that you are left with DependencyObject, since even UIObject has a lot of properties. However a lot of collection controls such as ListView or ItemsRepeater need some of those properties in order to work. So trying to not inherit from UIObject renders the control unusable in scenarios where it is being accessed/used by other controls.

I know its not a simple solution

Yes unfortunately, it's a very difficult problem, and there only seem to be options that always have some issues associated with them :/

[StephenLPeters]

As @MikeHillberg said in the original issue, the text properties should flow down the visual tree to the children. Do we understand why these binding are needed here?

[StephenLPeters]

Why isn't FontWeight needed here?

[marcelwgn]

We already have added "FontWeight=Normal", which I assumed was added for a reason (same why we enforce certain font sizes on other controls and their subcontrols).

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[marcelwgn]

As @MikeHillberg said in the original issue, the text properties should flow down the visual tree to the children. Do we understand why these binding are needed here?

No, I have no idea why those properties did not propagate down the visual tree.

[robloo]

No, I have no idea why those properties did not propagate down the visual tree.

I wouldn't hide the bug without first understanding what's going on here. We need to understand why these properties aren't coming down the visual tree. Then it can be determined if this is the best fix or interum solution.

Things like this should have been caught before release.
I wonder if there shouldn't be some UI Test for customised control properties, to ensure the rendering matches the set properties?

I don't think there is a testing team anymore. This has been an ongoing problem for years now and is why software is getting buggier -- over-reliance on automated testing which assumes you understand all failure modes and have tests for them. In this case, I can't complain as it's now an open source project. Letting the community test and be involved for free is a benefit of open source.

I raised a lot of concerns about the way NumberBox v1 came to fruition. There were a great many bugs that shouldn't have been there: #1857 (comment) . We definitely need a test plan for controls so at least the original developer can run through it.

[marcelwgn]

It seems that the properties get passed down to TextBlock, but not to a TextBox. The AutoSuggestBox for example, also needs TemplateBinding for those properties.

[StephenLPeters]

It seems that the properties get passed down to TextBlock, but not to a TextBox. The AutoSuggestBox for example, also needs TemplateBinding for those properties.

@MikeHillberg is this expected?

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[robloo]

I bet this is another bug with the TextBox then. Perhaps a new issue should be filed after this to be fixed with WinUI 3.0 -- TextBox needs a refactoring in my opinion.

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[MikeHillberg]

It seems that the properties get passed down to TextBlock, but not to a TextBox. The AutoSuggestBox for example, also needs TemplateBinding for those properties.

The text properties inherit until they're overridden by an element. They're not override by TextBlock (or any Panels), but many of the controls, such as TextBox, have a default style that overrides them with default values. AutoSuggestBox doesn't set default values for them, but its TextBoxStyle property defaults to the AutoSuggestBoxTextBoxStyle, which sets some text properties.

[StephenLPeters]