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 3 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
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" };
};
25 changes: 23 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,22 @@
<VisualState x:Name="Normal" />
<VisualState x:Name="Determinate" />
<VisualState x:Name="Updating" />
<VisualState x:Name="UpdatingError">
<Storyboard>
<Storyboard>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have two storyboards?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed now

<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"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a setter more appropriate than a 0 duration story board?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Quite frankly, I wasn't able to get a setter to that property working. I've tried different variations of brackets and they always crashed.

</Storyboard>
</Storyboard>
</VisualState>
<VisualState x:Name="Indeterminate">
<Storyboard RepeatBehavior="Forever">
<DoubleAnimationUsingKeyFrames
Expand Down Expand Up @@ -119,7 +140,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 +156,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