NumberBox: Fix header foreground color not being properly updated when NumberBox is disabled.

[Felix-Dev]

Description

This PR fixes the NumberBox header foreground not being updated correctly when the NumberBox is set to disabled.

Motivation and Context

Fixes #3003.

How Has This Been Tested?

Verified visually and added API test. The API test follows the NavigationView.VerifySingleSelection API test in the sense that we verify that modifying NumberBox.IsEnabled sets the correct visual state (just as the NavigationView test above verifies if modifying the selection state of a NavViewItem sets the correct visual state). While this does not directly check the new foreground used for the NumberBox header when disabled, I chose this method because:

• If the header foreground changes when set to disabled (and to what new value) might be changed over time and this test shouldn't simply fail because the used header colors are updated.
• In the future, we might have to set more values in the Disabled visual state so this test checks that the newly added states as part of this PR are applied correctly by the NumberBox.

Screenshots:

[robloo]

Convention is to name this group <VisualStateGroup x:Name="CommonStates">

[Felix-Dev]

I followed the NavigationViewItemPresenter style here:

microsoft-ui-xaml/dev/NavigationView/NavigationView_rs1_themeresources.xaml

Line 605 in 7acb9a7

<VisualStateGroup x:Name="DisabledStates">

The reason I did not went with "Common" here is that all the other visual states which are typically part of the "Common" state group like PointerOver and Pressed are not relevant here (they are already handled by the TextBox control, etc...). As I only have two states I'm interested here - the Enabled and Disabled states - I felt it would be more suitable to name their parent visual state group accordingly.

As for the naming convention you are mentioning: Observe that the NavigationViewItemPresenter styles use visual state groups like PointerStates and DisabledStates instead of Common.

It is true that many default control templates in UWP overwhelmingly have the Disabled visual state as part of the Common visual state group and I'm open to change this accordingly. Let's also see what the team thinks.

[robloo]

Makes sense. I would have used CommonStates for both controls if it was me but that's just preference. Will agree with the team either way.

[StephenLPeters]

The big thing to think about with different visual state groups is that states that are not in the same group cannot change the same properties. For instance if the disabled state changed the background color and the pointer over state also changed the background color, this would result in broken state manager. This is because there is no mechanism to merge two states (disabled pointerover) and the result is whichever state is set last wins. I think this is generally the reason the disabled state is included in the common states.

[Felix-Dev]

So we would be fine here leaving this as it is right now. If changes are made to the default NumberBox control template in the future which will create the above described situation, we can always make the necessary adjustments.

How does that sound? @StephenLPeters

[StephenLPeters]

The big thing to think about with different visual state groups is that states that are not in the same group cannot change the same properties. For instance if the disabled state changed the background color and the pointer over state also changed the background color, this would result in broken state manager. This is because there is no mechanism to merge two states (disabled pointerover) and the result is whichever state is set last wins.

That's interesting and something I never considered before. However, I'm not sure its really as bad as a broken stake manager sounds. Applying the visual state (GoToState) is analogous to just running that state's code and setting the properties as defined. It's perfectly fine if you then apply another state that sets the same properties. After all the states, state groups and even the state manager itself has no higher-level understanding of what it's doing. The logic in ensuring that what states are applied makes sense is handled by the control itself. It's perfectly fine to collapse the same control in two different state/stategroups. That can make sense in some situations and runs perfectly fine in my experience -- nothing breaks.

The problem is more pronounced when both states are setting color. Consider if you had selection state seperate from pointer states. The selected state might set the background color to the accent color, while the pointer over state might set it to a light grey. The result is that the background color ends up being whichever state is entered last. In general we like to limit the amount that order of operations matters.

[ranjeshj]

The IsEnabled property is on the Control type, so I think the Control type should already be listening to property changed on that type and sending the control to the CommonStates.Disabled visual state. I think it makes sense to use the CommonStates group and populate Normal and Disabled states there. (and we don't need to duplicate the disabled state here as well as listen to the property change and send to that state). I am not seeing a downside to doing that.

NavigationViewItemPresenter should probably have followed the same pattern, that is probably an oversight - or perhaps there were other motivations for that case.

[Felix-Dev]

@ranjeshj While that sounds reasonable I tested removing the IsEnabled property change listener and just use the CommonStates VSG here but then the NumberBox header is not updated at all 🤔I need to explicitly go to the CommonStates.Disabled state here to update the header properly.

[ranjeshj]

Interesting. I just looked through the control code and it does not send it to the Disabled state based on IsEnabled. @MikeHillberg Is the expectation that each deriving control do this ?

[MikeHillberg]

Yes, the visual state names are defined by each control, so the Control class can't know what state names to go to.

[StephenLPeters]

                                        <DiscreteObjectKeyFrame KeyTime="0" Value="{ThemeResource TextControlHeaderForegroundDisabled}" />

How does this change interact with this state, which seems to be setting the same property you are?

---

Refers to: dev/NumberBox/NumberBox.xaml:296 in f9af65e. [](commit_id = f9af65e, deletion_comment = False)

[Felix-Dev]

@StephenLPeters That visual state is irrelevant here as it sets the foreground of the header element of the TextBox and not the foreground of the actually used NumberBox header UI element. The NumberBox is using its own HeaderContentPresenter to display the header instead of template binding to its internal TextBox.Header API:

microsoft-ui-xaml/dev/NumberBox/NumberBox.xaml

Line 95 in 65e3204

<ContentPresenter x:Name="HeaderContentPresenter"

This is also the reason why disabling the NumberBox did not change the foreground color of the header since the NumberBox has to take care of it itself.

If we were to use the TextBox.Header API instead, the spin buttons in inline mode would vertically fill the entire space claimed by the header and the input field, instead of neatly aligning with just the input field. To prevent this, the NumberBox is using its own HeaderContentPresenter.

[StephenLPeters]

@StephenLPeters That visual state is irrelevant here as it sets the foreground of the header element of the TextBox and not the foreground of the actually used NumberBox header UI element. The NumberBox is using its own HeaderContentPresenter to display the header instead of template binding to its internal TextBox.Header API:

microsoft-ui-xaml/dev/NumberBox/NumberBox.xaml

Line 95 in 65e3204

<ContentPresenter x:Name="HeaderContentPresenter"

This is also the reason why disabling the NumberBox did not change the foreground color of the header since the NumberBox has to take care of it itself.

If we were to use the TextBox.Header API instead, the spin buttons in inline mode would vertically fill the entire space claimed by the header and the input field, instead of neatly aligning with just the input field. To prevent this, the NumberBox is using its own HeaderContentPresenter.

Make sense, but does that mean we could add your setter to this VisualState instead of making a new one?

[Felix-Dev]

Make sense, but does that mean we could add your setter to this VisualState instead of making a new one?

@StephenLPeters I am not sure I understand. With "this VisualState" do you mean the TextBox's Disabled visual state? If so, as I said above, that won't help us here because it is affecting a different UI element and the TextBox control template knows nothing about the ContentPresenter used by the NumberBox.

If you mean a different visual state here, then I'm not sure which one. Could you link to it?

[StephenLPeters]

Make sense, but does that mean we could add your setter to this VisualState instead of making a new one?

@StephenLPeters I am not sure I understand. With "this VisualState" do you mean the TextBox's Disabled visual state? If so, as I said above, that won't help us here because it is affecting a different UI element and the TextBox control template knows nothing about the ContentPresenter used by the NumberBox.

If you mean a different visual state here, then I'm not sure which one. Could you link to it?

My mistake, I should have looked closer at the VSM I was referring to, I didn't realize it was in the textbox style. Your solutions looks right

[StephenLPeters]

@Felix-Dev There are a few open comments here, any updates, are you blocked?

[Felix-Dev]

@StephenLPeters Will push a commit later today which will switch over to using a "Common" VSG with "Normal" and "Disabled" as its two visual states.

[StephenLPeters]

@StephenLPeters Will push a commit later today which will switch over to using a "Common" VSG with "Normal" and "Disabled" as its two visual states.

Perfect :)

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]