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.

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

Progressbar: Fix issue with color changing when updating value while error state #3201

Merged
merged 11 commits into from
Dec 14, 2020
Merged

Progressbar: Fix issue with color changing when updating value while error state #3201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1209284
Fix issue with color changing when updating value while error state
marcelwgn Aug 26, 2020
b9fd0fe
Switch back to path binding
marcelwgn Aug 26, 2020
9f9449b
Cleanup
marcelwgn Aug 26, 2020
a4cc438
Remove duplicate storyboard
marcelwgn Aug 31, 2020
7b8b13c
Merge branch 'master' into user/chingucoding/progressbar-errorcolor-u…
marcelwgn Sep 3, 2020
9c8e5f2
Split into testsuites
marcelwgn Sep 10, 2020
3c92a95
Merge branch 'master' into user/chingucoding/progressbar-errorcolor-u…
marcelwgn Sep 10, 2020
a10278e
Add polyfill
marcelwgn Sep 10, 2020
410d297
Update theme resource file
marcelwgn Sep 11, 2020
909300a
Update vcx file
marcelwgn Sep 11, 2020
5ab2d71
CR feedback
marcelwgn Sep 18, 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
5 changes: 0 additions & 5 deletions dev/CommonStyles/CommonStyles.vcxitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@
<Version>RS1</Version>
<Type>ThemeResources</Type>
</Page>
<Page Include="$(MSBuildThisFileDirectory)ProgressBar_themeresources.xaml">
<Version>RS1</Version>
<Type>ThemeResources</Type>
<UseNonstandardCondtionalXAML>true</UseNonstandardCondtionalXAML>
</Page>
<Page Include="$(MSBuildThisFileDirectory)MediaTransportControls_themeresources.xaml">
<Version>RS1</Version>
<Type>ThemeResources</Type>
Expand Down
9 changes: 8 additions & 1 deletion dev/ProgressBar/InteractionTests/ProgressBarTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See LICENSE in the project root for license information.

using System;
Expand Down Expand Up @@ -42,6 +42,7 @@ public void TestCleanup()
}

[TestMethod]
[TestProperty("TestSuite", "A")]
public void ChangeValueTest()
{
using (var setup = new TestSetupHelper("ProgressBar Tests"))
Expand All @@ -67,6 +68,7 @@ public void ChangeValueTest()
}

[TestMethod]
[TestProperty("TestSuite", "A")]
public void UpdateIndicatorWidthTest()
{
using (var setup = new TestSetupHelper("ProgressBar Tests"))
Expand Down Expand Up @@ -128,6 +130,7 @@ public void UpdateIndicatorWidthTest()
}

[TestMethod]
[TestProperty("TestSuite", "A")]
public void UpdateMinMaxTest()
{
using (var setup = new TestSetupHelper("ProgressBar Tests"))
Expand Down Expand Up @@ -191,6 +194,7 @@ public void UpdateMinMaxTest()
}

[TestMethod]
[TestProperty("TestSuite", "B")]
public void ChangeStateTest()
{
using (var setup = new TestSetupHelper("ProgressBar Tests"))
Expand Down Expand Up @@ -253,6 +257,7 @@ public void ChangeStateTest()
}

[TestMethod]
[TestProperty("TestSuite", "B")]
public void PaddingOffsetTest()
{
using (var setup = new TestSetupHelper("ProgressBar Tests"))
Expand Down Expand Up @@ -297,6 +302,7 @@ public void PaddingOffsetTest()
}

[TestMethod]
[TestProperty("TestSuite", "C")]
public void RetemplateUpdateIndicatorWidthTest()
{
using (var setup = new TestSetupHelper("ProgressBar Tests"))
Expand Down Expand Up @@ -362,6 +368,7 @@ public void RetemplateUpdateIndicatorWidthTest()
}

[TestMethod]
[TestProperty("TestSuite", "C")]
public void ReTemplateChangeStateTest()
{
using (var setup = new TestSetupHelper("ProgressBar Tests"))
Expand Down
56 changes: 35 additions & 21 deletions dev/ProgressBar/ProgressBar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,31 +78,38 @@ void ProgressBar::OnShowErrorPropertyChanged(const winrt::DependencyPropertyChan

void ProgressBar::UpdateStates()
{
if (ShowError() && IsIndeterminate())
{
winrt::VisualStateManager::GoToState(*this, s_IndeterminateErrorStateName, true);
}
else if (ShowError())
{
winrt::VisualStateManager::GoToState(*this, s_ErrorStateName, true);
}
else if (ShowPaused() && IsIndeterminate())
{
winrt::VisualStateManager::GoToState(*this, s_IndeterminatePausedStateName, true);
}
else if (ShowPaused())
{
winrt::VisualStateManager::GoToState(*this, s_PausedStateName, true);
}
else if (IsIndeterminate())
if (IsIndeterminate())
{
if (ShowError())
{
winrt::VisualStateManager::GoToState(*this, s_IndeterminateErrorStateName, true);
}
else if (ShowPaused())
{
winrt::VisualStateManager::GoToState(*this, s_IndeterminatePausedStateName, true);
}
else
{
winrt::VisualStateManager::GoToState(*this, s_IndeterminateStateName, true);
}
UpdateWidthBasedTemplateSettings();
winrt::VisualStateManager::GoToState(*this, s_IndeterminateStateName, true);
}
else if (!IsIndeterminate())
else
{
winrt::VisualStateManager::GoToState(*this, s_DeterminateStateName, true);
if (ShowError())
{
winrt::VisualStateManager::GoToState(*this, s_ErrorStateName, true);
}
else if(ShowPaused())
{
winrt::VisualStateManager::GoToState(*this, s_PausedStateName, true);
}
else
{
winrt::VisualStateManager::GoToState(*this, s_DeterminateStateName, true);
}
}

}

void ProgressBar::SetProgressBarIndicatorWidth()
Expand All @@ -121,7 +128,14 @@ void ProgressBar::SetProgressBarIndicatorWidth()

// Adds "Updating" state in between to trigger RepositionThemeAnimation Visual Transition
// in ProgressBar.xaml when reverting back to previous state
winrt::VisualStateManager::GoToState(*this, s_UpdatingStateName, true);
if (ShowError())
{
winrt::VisualStateManager::GoToState(*this, s_UpdatingWithErrorStateName, true);
}
else
{
winrt::VisualStateManager::GoToState(*this, s_UpdatingStateName, true);
}

if (IsIndeterminate())
{
Expand Down
1 change: 1 addition & 0 deletions dev/ProgressBar/ProgressBar.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,5 @@ class ProgressBar :
static constexpr wstring_view s_IndeterminatePausedStateName{ L"IndeterminatePaused" };
static constexpr wstring_view s_DeterminateStateName{ L"Determinate" };
static constexpr wstring_view s_UpdatingStateName{ L"Updating" };
static constexpr wstring_view s_UpdatingWithErrorStateName{ L"UpdatingError" };
};
7 changes: 6 additions & 1 deletion dev/ProgressBar/ProgressBar.vcxitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,16 @@
<Version>RS1</Version>
<Type>DefaultStyle</Type>
</Page>
<Page Include="$(MSBuildThisFileDirectory)ProgressBar_themeresources.xaml">
<Version>RS1</Version>
<Type>ThemeResources</Type>
<UseNonstandardCondtionalXAML>true</UseNonstandardCondtionalXAML>
</Page>
</ItemGroup>
<ItemGroup>
<Midl Include="$(MSBuildThisFileDirectory)ProgressBar.idl" />
</ItemGroup>
<ItemGroup>
<PRIResource Include="$(MSBuildThisFileDirectory)Strings\en-us\Resources.resw" />
</ItemGroup>
</Project>
</Project>
23 changes: 21 additions & 2 deletions dev/ProgressBar/ProgressBar.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@
<RepositionThemeAnimation TargetName="DeterminateProgressBarIndicator" FromHorizontalOffset="{Binding RelativeSource={RelativeSource TemplatedParent}, Path=TemplateSettings.IndicatorLengthDelta}" />
</Storyboard>
</VisualTransition>
<VisualTransition From="UpdatingError" To="Error">
<Storyboard>
<RepositionThemeAnimation TargetName="DeterminateProgressBarIndicator" FromHorizontalOffset="{Binding RelativeSource={RelativeSource TemplatedParent}, Path=TemplateSettings.IndicatorLengthDelta}" />
</Storyboard>
</VisualTransition>
<VisualTransition From="Paused" To="Determinate">
<Storyboard>
<DoubleAnimation
Expand Down Expand Up @@ -65,6 +70,20 @@
<VisualState x:Name="Normal" />
<VisualState x:Name="Determinate" />
<VisualState x:Name="Updating" />
<VisualState x:Name="UpdatingError">
<Storyboard>
<ColorAnimation
Storyboard.TargetName="DeterminateProgressBarIndicator"
Storyboard.TargetProperty="(Shape.Fill).(SolidColorBrush.Color)"
To="{Binding Source={ThemeResource SystemControlErrorTextForegroundBrush},Path=Color}"
Duration="0:0:0.0"/>
<ColorAnimation
Storyboard.TargetName="ProgressBarRoot"
Storyboard.TargetProperty="(Border.Background).(SolidColorBrush.Color)"
To="{StaticResource SystemControlErrorBackgroundColor}"
Duration="0:0:0.0"/>
</Storyboard>
</VisualState>
<VisualState x:Name="Indeterminate">
<Storyboard RepeatBehavior="Forever">
<DoubleAnimationUsingKeyFrames
Expand Down Expand Up @@ -119,7 +138,7 @@
Storyboard.TargetName="IndeterminateProgressBarIndicator2"
Storyboard.TargetProperty="(Shape.Fill).(SolidColorBrush.Color)">
<LinearColorKeyFrame Value="{ThemeResource SystemAccentColor}" KeyTime="0:0:2.75" />
<LinearColorKeyFrame Value="{ThemeResource SystemErrorTextColor}" KeyTime="0:0:3" />
<LinearColorKeyFrame Value="{Binding Source={ThemeResource SystemControlErrorTextForegroundBrush},Path=Color}" KeyTime="0:0:3" />
</ColorAnimationUsingKeyFrames>
<ColorAnimationUsingKeyFrames
Storyboard.TargetName="ProgressBarRoot"
Expand All @@ -135,7 +154,7 @@
<ColorAnimation
Storyboard.TargetName="DeterminateProgressBarIndicator"
Storyboard.TargetProperty="(Shape.Fill).(SolidColorBrush.Color)"
To="{ThemeResource SystemErrorTextColor}"
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this change necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

According to #1766 we should use SystemControlErrorTextForegroundBrush which is a SolidColorbrush. However ColorAnimation does not support brushes, but expects us to pass a Color object, that's why we need to use the binding now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@YuliKl can you comment? This seems like it's going to create 2 extra allocations (at a minimum - one for the Binding and one for the Brush) when I don't think it's necessary. Seems like having a ThemeResource to SystemErrorTextColor would do the trick just fine?

Copy link

Choose a reason for hiding this comment

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

Sorry I missed this question earlier. A potential problem I foresee with using a Color resource is what happens with the control is running in High Contrast. SystemControlErrorTextForegroundBrush falls back to SystemColorWindowTextColor in HC (which can be modified by the user), but SystemErrorTextColor retains its static value of #FFF000. If we hardcode a color, I think we might get the appearance wrong in HC.

Copy link
Contributor

Choose a reason for hiding this comment

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

looking at generic.xaml the high contrast version of the brush should be

In reply to: 489609371 [](ancestors = 489609371)

Copy link
Contributor

Choose a reason for hiding this comment

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

So yes, there is a tangible difference between the brush and the color, and I think its worth keeping the binding.

In reply to: 491100783 [](ancestors = 491100783,489609371)

To="{Binding Source={ThemeResource SystemControlErrorTextForegroundBrush},Path=Color}"
Duration="0:0:0.25"/>
<ColorAnimation
Storyboard.TargetName="ProgressBarRoot"
Expand Down
4 changes: 4 additions & 0 deletions ...monStyles/ProgressBar_themeresources.xaml → ...ogressBar/ProgressBar_themeresources.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
<Thickness x:Key="ProgressBarBorderThemeThickness">0</Thickness>
<Color x:Key="SystemControlErrorBackgroundColor">#33FFFFFF</Color>
<contract7NotPresent:Color x:Key="SystemErrorTextColor">#FFF000</contract7NotPresent:Color>
<contract7NotPresent:SolidColorBrush x:Key="SystemControlErrorTextForegroundBrush" Color="{ThemeResource SystemErrorTextColor}"/>
<SolidColorBrush x:Key="ProgressBarBackgroundThemeBrush" Color="#59FFFFFF" />
<SolidColorBrush x:Key="ProgressBarBorderThemeBrush" Color="Transparent" />
<SolidColorBrush x:Key="ProgressBarForegroundThemeBrush" Color="#FF5B2EC5" />
Expand All @@ -20,6 +21,8 @@
<x:Double x:Key="ProgressBarIndicatorPauseOpacity">1</x:Double>
<x:Double x:Key="ProgressBarThemeMinHeight">4</x:Double>
<Thickness x:Key="ProgressBarBorderThemeThickness">1</Thickness>
<contract7NotPresent:Color x:Key="SystemErrorTextColor">#FFF000</contract7NotPresent:Color>
<contract7NotPresent:SolidColorBrush x:Key="SystemControlErrorTextForegroundBrush" Color="{ThemeResource SystemErrorTextColor}"/>
<SolidColorBrush x:Key="ProgressBarBackgroundThemeBrush" Color="{ThemeResource SystemColorButtonFaceColor}" />
<SolidColorBrush x:Key="ProgressBarBorderThemeBrush" Color="{ThemeResource SystemColorButtonTextColor}" />
<SolidColorBrush x:Key="ProgressBarForegroundThemeBrush" Color="{ThemeResource SystemColorHighlightColor}" />
Expand All @@ -32,6 +35,7 @@
<Color x:Key="SystemControlErrorBackgroundColor">#29C50500</Color>
<!-- Hex value for SystemErrorText at 0.16 opacity -->
<contract7NotPresent:Color x:Key="SystemErrorTextColor">#C50500</contract7NotPresent:Color>
<contract7NotPresent:SolidColorBrush x:Key="SystemControlErrorTextForegroundBrush" Color="{ThemeResource SystemErrorTextColor}"/>
<SolidColorBrush x:Key="ProgressBarBackgroundThemeBrush" Color="#30000000" />
<SolidColorBrush x:Key="ProgressBarBorderThemeBrush" Color="Transparent" />
<SolidColorBrush x:Key="ProgressBarForegroundThemeBrush" Color="#FF4617B4" />
Expand Down