Skip to content

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

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.

Sign up for GitHub

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

Merged
StephenLPeters merged 7 commits into from
Aug 11, 2020
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
82fe9ca
Fix for NumberBox header foreground color not being properly updated …
Felix-Dev Jul 28, 2020
67edbd2
Small test code structure change.
Felix-Dev Jul 28, 2020
f9af65e
Small test improvement.
Felix-Dev Jul 29, 2020
1270c8b
Changed DisabledStates VSG to CommonStates VSG.
Felix-Dev Aug 5, 2020
04d1021
Merge branch 'master' into user/Felix-Dev/numberbox-disabled-headerco…
Felix-Dev Aug 6, 2020
3412d3c
Fix failing test (forgot to update in previous commit).
Felix-Dev Aug 6, 2020
20f286e
CR feedback.
Felix-Dev Aug 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions dev/NumberBox/APITests/NumberBoxTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,47 @@ public void VerifyNumberBoxCornerRadius()
});
}

[TestMethod]
public void VerifyIsEnabledChangeUpdatesVisualState()
{
var numberBox = SetupNumberBox();

VisualStateGroup disabledStatesGroup = null;
bool testCondition = false;
RunOnUIThread.Execute(() =>
{
// Check 1: Set IsEnabled to true.
numberBox.IsEnabled = true;
Content.UpdateLayout();

var numberBoxLayoutRoot = (FrameworkElement)VisualTreeHelper.GetChild(numberBox, 0);
disabledStatesGroup = VisualStateManager.GetVisualStateGroups(numberBoxLayoutRoot).First(vsg => vsg.Name.Equals("DisabledStates"));

testCondition = disabledStatesGroup.CurrentState.Name.Equals("Enabled");
Verify.IsTrue(testCondition);

// Check 2: Set IsEnabled to false.
numberBox.IsEnabled = false;
});
IdleSynchronizer.Wait();

RunOnUIThread.Execute(() =>
{
testCondition = disabledStatesGroup.CurrentState.Name.Equals("Disabled");
Verify.IsTrue(testCondition);

// Check 3: Set isEnabled back to true.
numberBox.IsEnabled = true;
});
IdleSynchronizer.Wait();

RunOnUIThread.Execute(() =>
{
testCondition = disabledStatesGroup.CurrentState.Name.Equals("Enabled");
Verify.IsTrue(testCondition);
});
}

private NumberBox SetupNumberBox()
{
NumberBox numberBox = null;
Expand Down
14 changes: 14 additions & 0 deletions dev/NumberBox/NumberBox.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,12 +198,16 @@ void NumberBox::OnApplyTemplate()
m_popupUpButtonClickRevoker = popupSpinUp.Click(winrt::auto_revoke, { this, &NumberBox::OnSpinUpClick });
}

m_isEnabledChangedRevoker = IsEnabledChanged(winrt::auto_revoke, { this, &NumberBox::OnIsEnabledChanged });

// .NET rounds to 12 significant digits when displaying doubles, so we will do the same.
m_displayRounder.SignificantDigits(12);

UpdateSpinButtonPlacement();
UpdateSpinButtonEnabled();

UpdateVisualStateForIsEnabledChange();

if (ReadLocalValue(s_ValueProperty) == winrt::DependencyProperty::UnsetValue()
&& ReadLocalValue(s_TextProperty) != winrt::DependencyProperty::UnsetValue())
{
Expand Down Expand Up @@ -338,6 +342,16 @@ void NumberBox::OnValidationModePropertyChanged(const winrt::DependencyPropertyC
UpdateSpinButtonEnabled();
}

void NumberBox::OnIsEnabledChanged(const winrt::IInspectable& /*sender*/, const winrt::DependencyPropertyChangedEventArgs& /*args*/)
{
UpdateVisualStateForIsEnabledChange();
}

void NumberBox::UpdateVisualStateForIsEnabledChange()
{
winrt::VisualStateManager::GoToState(*this, IsEnabled() ? L"Enabled" : L"Disabled", false);
}

void NumberBox::OnNumberBoxGotFocus(winrt::IInspectable const& sender, winrt::RoutedEventArgs const& args)
{
// When the control receives focus, select the text
Expand Down
5 changes: 5 additions & 0 deletions dev/NumberBox/NumberBox.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ class NumberBox :
void OnNumberBoxLostFocus(winrt::IInspectable const& sender, winrt::RoutedEventArgs const& args);
void OnNumberBoxScroll(winrt::IInspectable const& sender, winrt::PointerRoutedEventArgs const& args);
void OnCornerRadiusPropertyChanged(const winrt::DependencyObject& /*sender*/, const winrt::DependencyProperty& /*args*/);
void OnIsEnabledChanged(const winrt::IInspectable& /*sender*/, const winrt::DependencyPropertyChangedEventArgs& /*args*/);

void ValidateInput();
void CoerceMinimum();
Expand All @@ -83,6 +84,8 @@ class NumberBox :

void UpdateHeaderPresenterState();

void UpdateVisualStateForIsEnabledChange();

bool IsInBounds(double value);

void MoveCaretToTextEnd();
Expand All @@ -105,4 +108,6 @@ class NumberBox :
winrt::RepeatButton::Click_revoker m_popupDownButtonClickRevoker{};

PropertyChanged_revoker m_cornerRadiusChangedRevoker{};

winrt::Control::IsEnabledChanged_revoker m_isEnabledChangedRevoker{};
};
8 changes: 8 additions & 0 deletions dev/NumberBox/NumberBox.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@
<ControlTemplate TargetType="local:NumberBox">
<Grid>
<VisualStateManager.VisualStateGroups>
<VisualStateGroup x:Name="DisabledStates">
Copy link
Contributor

@robloo robloo Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Felix-Dev Felix-Dev Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@robloo robloo Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@StephenLPeters StephenLPeters Jul 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Felix-Dev Felix-Dev Jul 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@StephenLPeters StephenLPeters Aug 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ranjeshj ranjeshj Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Felix-Dev Felix-Dev Aug 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@ranjeshj ranjeshj Aug 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

@MikeHillberg MikeHillberg Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<VisualState x:Name="Enabled"/>
<VisualState x:Name="Disabled">
<VisualState.Setters>
<Setter Target="HeaderContentPresenter.Foreground" Value="{ThemeResource TextControlHeaderForegroundDisabled}"/>
</VisualState.Setters>
</VisualState>
</VisualStateGroup>
<VisualStateGroup x:Name="SpinButtonStates">
<VisualState x:Name="SpinButtonsCollapsed" />

Expand Down