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

marcelwgn · 2020-08-26T16:01:45Z

Description

Fix issue with color changing when changing the value. That was caused as we moved to a visual state that didn't set the color to our error color resulting in that switch.

Also updated the template to use the SystemControlErrorTextForegroundBrush. For reasons unknown to me, lightweight styling of that resource does not really work. The themeresource lookup doesn't seem to pick up if that resource is specified on that page.

Another thing I noticed is that switching themes DOES NOT update the progressbar color since we set the color of a solidcolorbrush instead of referencing a themeresource. What should we do with this?
While those animations seem awesome, they are currently create a few issues that we wouldn't have with plain setters to SolidColorBrushes.

Motivation and Context

Fixes #1766

How Has This Been Tested?

Screenshots (if appropriate):

Felix-Dev · 2020-08-26T16:15:51Z

dev/ProgressBar/ProgressBar.xaml

+                                            <ColorAnimation
+                                            Storyboard.TargetName="DeterminateProgressBarIndicator"
+                                            Storyboard.TargetProperty="(Shape.Fill).(SolidColorBrush.Color)"
+                                            To="{Binding Source={ThemeResource SystemControlErrorTextForegroundBrush},Converter={StaticResource SolidColorBrushToColor}}"


Does
To="{Binding Source={ThemeResource SystemControlErrorTextForegroundBrush},Path=Color}"
not work?

It does to the same degree as converters: It doesn't pick up a resource override.

I've changed it to path as we should avoid those converters if they are not necessary, thanks for pointing that out.

That resource update failure seems familiar....and turns out in another issue, we already started speculating if the resource lookup initiated by the Binding function is currently broken:

See the two messages starting here.

Somehow this managed to slip off my mind when it came to creating a separate issue but this PR serves as a good reminder for that. I would create such an issue if you don't mind?

Feel free to create an issue for this. Another issue caused by this, which also slipped my mind, is #3083 which exhibits the same behavior.

I think the issue with themresource lookup inside binding is the following:
The binding is inside WinUI, so the resource probably starts there. It then goes up the chain, and since you load WinUI in your app.xaml, this is the next place where it will look for that resource. Overriding those resources in App.xaml works as expected, but overriding on pages does not work with the classical setup.

Overriding those resources in App.xaml works as expected, but overriding on pages does not work with the classical setup.

In my case linked above, not even the app level was working:

It also does not matter if I override these resources on the control level, the page level or the app level. Same effect - my overriden placeholder foreground theme resources will be ignored.

I will double-check that though before creating the issue.

Nevermind, with the progressbar, it worked once for some reason but now it doesn't anymore...

Really strange how resourcelookup behaves while inside binding.

Does the comment from the other PR about bindings in styles only being applied once explain something here?

Yes, that comment is also explaining why this is not working as expected. What do you think should we do with this given that we would need to manually do the binding for it to update correctly?

StephenLPeters · 2020-08-31T17:50:14Z

dev/ProgressBar/ProgressBar.xaml

+                                            Storyboard.TargetName="ProgressBarRoot"
+                                            Storyboard.TargetProperty="(Border.Background).(SolidColorBrush.Color)"
+                                            To="{StaticResource SystemControlErrorBackgroundColor}"
+                                            Duration="0:0:0.0"/>


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

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.

StephenLPeters · 2020-08-31T17:51:22Z

dev/ProgressBar/ProgressBar.xaml

@@ -65,6 +70,22 @@
                                <VisualState x:Name="Normal" />
                                <VisualState x:Name="Determinate" />
                                <VisualState x:Name="Updating" />
+                                <VisualState x:Name="UpdatingError">
+                                    <Storyboard>
+                                        <Storyboard>


Why have two storyboards?

StephenLPeters · 2020-09-02T21:40:38Z

/azp run

azure-pipelines · 2020-09-02T21:40:53Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters · 2020-09-03T21:09:38Z

@chingucoding The pipeline fix just went in, could you please merge master into this PR?

…pdates

StephenLPeters · 2020-09-03T22:04:21Z

/azp run

azure-pipelines · 2020-09-03T22:04:37Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters · 2020-09-08T21:29:57Z

/azp run

azure-pipelines · 2020-09-08T21:30:16Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters · 2020-09-09T17:58:28Z

@kmahone do you know what is going on with the test failures for this PR? (we had the same error twice in a row)

StephenLPeters · 2020-09-09T20:18:50Z

@chingucoding I think the Progressbar tests work item is failing because it is taking too long, probably because there are too many failing tests. You have confirmed that they pass locally right?

marcelwgn · 2020-09-09T22:07:10Z

Just ran the tests again on my machine, and all of them run fine:

StephenLPeters · 2020-09-10T17:21:56Z

Can you try putting the tests in different suites? hopefully with that we can get the pipeline to give us more diagnostics.

…pdates

marcelwgn · 2020-09-10T17:29:34Z

Can you try putting the tests in different suites? hopefully with that we can get the pipeline to give us more diagnostics.

Done hopefully that helps tracking the issue down.

StephenLPeters · 2020-09-10T17:37:21Z

/azp run

azure-pipelines · 2020-09-10T17:37:40Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters · 2020-09-10T20:10:29Z

Looking through the change history, I believe SystemControlErrorTextForegroundBrush was added in RS5, the majority of the downlevel failures are the binding to that resource failing. I think we need to do something like what we do for SystemErrorTextColor here

ranjeshj · 2020-09-15T20:14:34Z

Yeah, I'm also surprised... I'm hoping someone else from the team might have incite as to whats going on, looking through the code I still have no idea.

The only difference between those should be the resources right ? I'm curious if some resource in high contrast is incorrect causing the VSM state parsing to fail - although I would have expected it fail/throw right there instead of returning null. Could you try to see what happens if you replace the high contrast resources with the default resources and see if it still repros ? perhaps we can track this down that way. @stevenbrix @alwu-msft any ideas why we might be seeing this behavior ?

evelynwu-msft · 2020-09-15T20:30:32Z

The only thing that comes to mind (note that this is pure speculation) is that the template lives in a theme dictionary (so there's different templates for different themes) and the template contained in the high contrast theme dictionary doesn't have a VSM specification. My recommendation would be to debug through the call to retrieve the visual state groups... unfortunately the people who have the most knowledge of how VSM works have long since left the team so I don't have any better suggestions.

Also, set a breakpoint on Windows.UI.Xaml!OnFailureEncountered... it might be that the real root cause lies elsewhere and it's simply manifesting as this weird VSM crash.

stevenbrix · 2020-09-15T21:21:54Z

I was going to say something similar to Alan, although we don't generally put templates in theme dictionaries.

VSM silently fails, since @chingucoding doesn't have access to our source yet, maybe you can find something in the output spew?

stevenbrix · 2020-09-15T21:23:05Z

dev/ProgressBar/ProgressBar.xaml

@@ -135,7 +154,7 @@
                                        <ColorAnimation
                                            Storyboard.TargetName="DeterminateProgressBarIndicator"
                                            Storyboard.TargetProperty="(Shape.Fill).(SolidColorBrush.Color)"
-                                            To="{ThemeResource SystemErrorTextColor}"


why is this change necessary?

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.

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

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.

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

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

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)

stevenbrix · 2020-09-15T21:33:05Z

The only other thing that comes to mind is that something about the way vsm is deferred is causing hiccups.

@chingucoding can you set <DisableXbfGeneration>true</DisableXbfGeneration> in the project file and see if the issue still reproes?

evelynwu-msft · 2020-09-15T21:34:39Z

Oops, my bad, "debug the call" isn't going to be too useful then. :D

In that case, if nothing is obvious from the debug spew, then capturing a Time Travel Debugging trace and attaching it to a new issue would allow somebody from our team to step through the scenario and figure out what's going wrong.

marcelwgn · 2020-09-15T23:31:38Z

I filed #3289 for tracking, unfortunately TTD didn't work as expected. Hopefully the DLM file is enough to help debugging, otherwise, the repro steps are also mentioned in the issue.

Given that the CI is running now, is there any work left to do in this PR?

stevenbrix · 2020-09-16T01:02:57Z

Given that the CI is running now, is there any work left to do in this PR?

Is the crash we're seeing new behavior, or is this already existing?

marcelwgn · 2020-09-16T07:27:50Z

The crash occured before this PR but is gone now as the missing high contrast resource would be added with this PR.

My old comment was wrong, @StephenLPeters is totally right.

Edit: Edited my comment, sorry about the confusion haha

StephenLPeters · 2020-09-16T16:48:21Z

The crash occurs in the mux controls test app without this change.

stevenbrix · 2020-09-16T17:12:14Z

My old comment was wrong, @StephenLPeters is totally right.

@chingucoding I'm very confused, which comment? 😄

I'd still like @YuliKl to comment on whether using SystemControlErrorTextForegroundBrush is a necessity, or if we can use SystemErrorTextColor where we just need a Color

Edit: Ahh, I see you edited your original comment. I get it now. That's why I was so confused haha

StephenLPeters · 2020-09-18T17:43:24Z

dev/ProgressBar/ProgressBar_themeresources.xaml

@@ -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="#C50500"/>


C50500 [](start = 103, length = 6)

These brushes should have their color set to the SystemErrorTextColor, not its value.

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

The high contrast version in system xaml actually points to SystemColorWindowTextColor instead of SystemErrorTextColor.

When trying to reference that resource, I get the same visual state bugs we got earlier in this PR. Any ideas what could lead to that resource not being found? A few control in OS XAML reference that brush, so this shouldn't be an issue right?

Sorry, its been a while, can you remind me what the issue was?

@chingucoding ping

Oh I am very sorry, thought that I already replied. The issue I am encountering is that the visual states of the ProgressBar seem to be missing resulting in crashes when visiting the page. As mentioned in my comment earlier, it seems that it fails to find the themeresource "SystemColorWindowTextColor". Should we add the resource to the ProgressBar_themeresources.xaml file to mitigate this?

StephenLPeters · 2020-10-02T20:03:39Z

/azp run

azure-pipelines · 2020-10-02T20:03:55Z

Azure Pipelines successfully started running 1 pipeline(s).

marcelwgn · 2020-10-20T06:54:03Z

Is there anything missing in this PR or are there any changes that need to be made?

…error state (microsoft/microsoft-ui-xaml#3201)

Fix issue with color changing when updating value while error state

1209284

msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Aug 26, 2020

marcelwgn changed the title ~~Fix issue with color changing when updating value while error state~~ Progressbar: Fix issue with color changing when updating value while error state Aug 26, 2020

Felix-Dev reviewed Aug 26, 2020

View reviewed changes

marcelwgn added 2 commits August 26, 2020 18:21

Switch back to path binding

b9fd0fe

Cleanup

9f9449b

YuliKl added area-Progress ProgressBar, ProgressRing team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Aug 26, 2020

StephenLPeters reviewed Aug 31, 2020

View reviewed changes

Remove duplicate storyboard

a4cc438

Merge branch 'master' into user/chingucoding/progressbar-errorcolor-u…

7b8b13c

…pdates

marcelwgn added 2 commits September 10, 2020 19:28

Split into testsuites

9c8e5f2

Merge branch 'master' into user/chingucoding/progressbar-errorcolor-u…

3c92a95

…pdates

stevenbrix reviewed Sep 15, 2020

View reviewed changes

StephenLPeters reviewed Sep 18, 2020

View reviewed changes

CR feedback

5ab2d71

marcelwgn mentioned this pull request Dec 10, 2020

Visual state manager VisualStateGroups null instead of crashed visual state #3289

Open

StephenLPeters approved these changes Dec 14, 2020

View reviewed changes

StephenLPeters merged commit 3a176d2 into microsoft:master Dec 14, 2020

Kinnara added a commit to Kinnara/ModernWpf that referenced this pull request Dec 20, 2020

Progressbar: Fix issue with color changing when updating value while …

18eefa0

…error state (microsoft/microsoft-ui-xaml#3201)

dependabot bot mentioned this pull request Jan 17, 2021

Bump ModernWpfUI from 0.9.2 to 0.9.3 HMBSbige/BilibiliLiveRecordDownLoader#32

Closed

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

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

Conversation

marcelwgn commented Aug 26, 2020 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Felix-Dev Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Felix-Dev Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenLPeters commented Sep 2, 2020

azure-pipelines bot commented Sep 2, 2020

StephenLPeters commented Sep 3, 2020

StephenLPeters commented Sep 3, 2020

azure-pipelines bot commented Sep 3, 2020

StephenLPeters commented Sep 8, 2020

azure-pipelines bot commented Sep 8, 2020

StephenLPeters commented Sep 9, 2020

StephenLPeters commented Sep 9, 2020

marcelwgn commented Sep 9, 2020

StephenLPeters commented Sep 10, 2020

marcelwgn commented Sep 10, 2020

StephenLPeters commented Sep 10, 2020

azure-pipelines bot commented Sep 10, 2020

StephenLPeters commented Sep 10, 2020 • edited Loading

ranjeshj commented Sep 15, 2020

evelynwu-msft commented Sep 15, 2020 • edited Loading

stevenbrix commented Sep 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenbrix commented Sep 15, 2020

evelynwu-msft commented Sep 15, 2020

marcelwgn commented Sep 15, 2020

stevenbrix commented Sep 16, 2020

marcelwgn commented Sep 16, 2020 • edited Loading

StephenLPeters commented Sep 16, 2020

stevenbrix commented Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenLPeters commented Oct 2, 2020

azure-pipelines bot commented Oct 2, 2020

marcelwgn commented Oct 20, 2020

marcelwgn commented Aug 26, 2020 •

edited

Loading

Felix-Dev Aug 26, 2020 •

edited

Loading

Felix-Dev Aug 26, 2020 •

edited

Loading

StephenLPeters commented Sep 10, 2020 •

edited

Loading

evelynwu-msft commented Sep 15, 2020 •

edited

Loading

marcelwgn commented Sep 16, 2020 •

edited

Loading

stevenbrix commented Sep 16, 2020 •

edited

Loading