ComboBox: Match Placeholder foreground color with TextBox's Placeholder foreground color and improve lightweight styling options

[Felix-Dev]

Description

This PR aims to to align the placeholder foreground color of the ComboBox with the placeholder foreground color of the TextBox. The following theme resources for the ComboBox's Placeholder have been changed to achieve this:

• ComboBoxPlaceHolderForeground
• ComboBoxPlaceHolderForegroundFocusedPressed

Additionally, this PR adds a bunch of new theme resources to provide better lightweight styling options for the placeholder foreground, the ComboBox foreground depending on the visual state of the ComboBox and the CombBox header:

• ComboBoxPlaceHolderForegroundPointerOver
• ComboBoxPlaceHolderForegroundPressed
• ComboBoxPlaceHolderForegroundFocused
• ComboBoxForegroundPointerOver
• ComboBoxForegroundPressed
• ComboBoxHeaderForeground
• ComboBoxHeaderForegroundDisabled

Adding these theme resources matches the theming resources available to customize the background the ComboBox (such as ComboBoxBackgroundPointerOver and ComboBoxBackgroundPressed). Furthermore, they also match the theme resources available for the TextBox (such as TextControlPlaceholderForegroundPointerOver and TextControlForegroundPointerOver) and this help provide a consistent lightweight styling experience across WinUI.

Relevant for the next paragraphs: When I'm speaking of a "UI breaking change" I mean a change which would cause the app UI to look different upon updating the app's consumed WinUI version without the developer changing anything else. It does not mean a breaking change in the sense of the app cannot be compiled/run successfully any longer without the developer having to modify their app code.

Four UI breaking changes are included here:

1. The theme resource ComboBoxForegroundDisabled has been replaced with the theme resource ComboBoxPlaceHolderForegroundDisabled to control the placeholder foreground of the ComboBox when disabled.
2. The theme resource ComboBoxForegroundFocused has been replaced with the theme resource ComboBoxPlaceHolderForegroundFocused to control the placeholder foreground of the ComboBox is focused while in non-editable mode.
3. The ComboBox header foreground appearance is now controlled by the ComboBoxHeaderForeground theme resource and no longer the ComboBoxForeground theme resource (which also controls the foreground of the ComboBox).
4. A disabled ComboBox's header foreground appearance is now controlled by the ComboBoxHeaderForegroundDisabled theme resource and no longer the ComboBoxForegroundDisabled theme resource (which also controls the foreground of the ComboBox when its disabled).

The UI breaking changes give developers much more fine-grained lightweight-styling customization options for the CombBox.

Motivation and Context

Fixes #2774.

How Has This Been Tested?

Tested visually (see below).

Screenshots and Gifs:

Theme	Enabled	Disabled
Light
Dark
HC Light
HC Dark

There are a couple of issues the the current theme resources in High-contrast mode:

• There is no visual distinction between the ComboBox Focused and FocusedPressed visual states as with the Light and Dark themes
• The placeholder foreground for the TextBox and the ComboBox looks the same as non-placeholder content. Below are two images showcasing this:

Theme	Placeholder	Content
HC Light
HC Dark

[StephenLPeters]

In your breaking change section you say ComboBoxForegroundDisabled was replaced, but, from the name alone, this still seems useful no?

[StephenLPeters]

I'm not sure I understand, why would introducing ComboBoxHeaderForeground be a breaking change?

[StephenLPeters]

Could you please include the screenshots of the HighContrast themes as well?

[Felix-Dev]

@StephenLPeters

In your breaking change section you say ComboBoxForegroundDisabled was replaced, but, from the name alone, this still seems useful no?

ComboBoxForegroundDisabled is currently used to set the foreground color of

• the ComboBox content
• the ComboBox's header
  when the ComboBox is disabled. My proposal is to use a new theme resource called ComboBoxHeaderForegroundDisabled to control the foreground color of the ComboBox Header when the Combobox is disabled. The resource ComboBoxForegroundDisabled would still exist and be used to set the foreground color of the ComboBox when disabled.

I'm not sure I understand, why would introducing ComboBoxHeaderForeground be a breaking change?
Currently, apps wanting to set the ComboBox Header foreground color via lightweight styling have to use the following two theme resources:

• ComboBoxForeground
• ComboBoxForegroundDisabled

Since these two theme resources, however, also control the foreground color of the ComboBox, currently a customer cannot easily set the Header foreground color and the ComboBox foreground separately using lightweight styling. To my understanding, adding new ComboBoxHeaderForeground and ComboBoxHeaderForegroundDisabled theme resources would be a breaking change, because it would break apps which are currently using the ComboBoxForeground/ComboBoxForegroundDisabled theme resources to set the Header foreground. If a customer upgrades to a WinUI version which contains this proposed change, they will suddenly notice that their custom CombBox header foreground resource is no longer applied. Instead, they will have to change their code and use the proposed ComboBoxHeaderForeground/ComboBoxHeaderForegroundDisabled theme resources instead.

[StephenLPeters]

I see, I think we have a slightly different definition for Breaking Change. This change wont cause someone who upgrades winui versions to have an app that does not compile or crashes at runtime (what I was thinking by breaking change) but could cause someones UI to change without and changes on their part. I worry a bit less about this. This could be perceived as broken, but we change this visuals of our controls with style updates.

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Felix-Dev]

@StephenLPeters

This change wont cause someone who upgrades winui versions to have an app that does not compile or crashes at runtime (what I was thinking by breaking change) but could cause someones UI to change without and changes on their part.

Exactly! It might have been my mistake here thinking about these as "breaking" changes. While these changes might "break" the UI of customers, their apps certainly won't break as they will still compile and run just fine. I will keep this in mind and be more precise next time - whether I mean app compilation/runtime break or something like an UI break.

Edit: Edited my PR summary post to to be clear what I meant here.

I worry a bit less about this. This could be perceived as broken, but we change this visuals of our controls with style updates.

That's perfect. I would view it a bit unfortunate if we would have to to hold back on improving the lightweight styling options we give developers because we weren't 100% precise in the past when introducing/consuming resources in control templates.

[Felix-Dev]

In addition to the proposed theme resources, I would also like to propose adding a ComboBox specific theme resource for the ComboBox.Description API. Right now, developers would have to override the SystemControlDescriptionTextForegroundBrush to be able to change the description foreground (as can be seen here). How about adding a

• ComboBoxDescriptionForeground

theme resource instead?

This would create a coherent theme resource offering for the ComboBox and makes it easier for developers to find the availble theme resources without having to check the actual control template and see what theme resources are used there.

One thing to note though is the following: The TextBox and NumberBox controls also have a Description API and both control templates use the SystemControlDescriptionTextForegroundBrush as well to set the description foreground. In other words, today you can can override SystemControlDescriptionTextForegroundBrush once and that changes the description foreground for all your TextBoxes, NumberBoxes and ComboBoxes, for example. We would lose this "functionality" when we start having each control consume a dedicated *DescriptionForeground theme resource instead (like TextBoxDescriptionForeground, NumberBoxDescriptionForeground, ComboBoxDescriptionForeground). We would, however, gain

• a more consistent theme resource offering,
• make lightweight-styling options more easily discoverable and
• provide a more fine-grained lightweight-styling experience

Thoughts?

[StephenLPeters]

In addition to the proposed theme resources, I would also like to propose adding a ComboBox specific theme resource for the ComboBox.Description API. Right now, developers would have to override the SystemControlDescriptionTextForegroundBrush to be able to change the description foreground (as can be seen here. How about adding a

• ComboBoxDescriptionForeground

theme resource instead?

This would create a coherent theme resource offering for the ComboBox and makes it easier for developers to find the availble theme resources without having to check the actual control template and see what theme resources are used there.

One thing to note though is the following: The TextBox and NumberBox controls also have a Description API and both control templates use the SystemControlDescriptionTextForegroundBrush as well to set the description foreground. In other words, today you can can override SystemControlDescriptionTextForegroundBrush once and that changes the description foreground for all your TextBoxes, NumberBoxes and ComboBoxes, for example. We would lose this "functionality" when we start having each control consume a dedicated *DescriptionForeground theme resource instead (like TextBoxDescriptionForeground, NumberBoxDescriptionForeground, ComboBoxDescriptionForeground). We would, however, gain

• a more consistent theme resource offering,
• make lightweight-styling options more easily discoverable and
• provide a more fine-grained lightweight-styling experience

Thoughts?

@kikisaints and @MikeHillberg for FYI. I get the feeling that this is a case where it would be great if we could add a level of indirection, by having these 3 controls have their own resource (ComboBoxDescriptionForeground etc.) which points to SystemControlDescriptionTextForegroundBrush. Our current resource system doesn't support this well, as the only place you could override SystemControlDescriptionTextForegroundBrush would be in App.xaml . Doing so in the page or a parent of the UI element would cause the override to get "missed" as the system walks up the tree looking for ComboBoxDescriptionForeground and missing the SystemControlDescriptionTextForegroundBrush definition that we would want it to resolve to.

@Felix-Dev In this case, I'm inclined to leave this as is, if you want to override the description foreground of just the combobox and not textboxes you can do so currently by providing the override in the dictionary of an exclusive parent. Thus, I think adding this would not enable a new customization. It would make the system more verbose, but its not clear to me that is really wanted for this. This is at the cost of potentially changing someones UI with an upgrade.

[Felix-Dev]

@StephenLPeters

In this case, I'm inclined to leave this as is,

Alright 🙂

Could you please include the screenshots of the HighContrast themes as well?

Edited my PR summary with examples in HC mode. Unfortunately, there are a couple of issues with HighContrast here currently.

[StephenLPeters]

tests look good except for the visual verification updates.

[Felix-Dev]

I think I will wait a bit before updating the visual verification files to see if more changes are requested here for existing theme values used in the ComboBox control template as part of this PR (like HighContrast).

[StephenLPeters]

@Felix-Dev I think its probably safe to update the visual state masters now.

[Felix-Dev]

@StephenLPeters Done.

Can I also add the proposed ComboBoxHeaderForeground and ComboBoxHeaderForegroundDisabled theme resources (listed in the [Additional Context] section in the opening post)?

[StephenLPeters]

@StephenLPeters Done.

Can I also add the proposed ComboBoxHeaderForeground and ComboBoxHeaderForegroundDisabled theme resources (listed in the [Additional Context] section in the opening post)?

I think that makes sense, I'll wait to start the gates until these are added.

[Felix-Dev]

Added them now.

[StephenLPeters]

/azp run

[Felix-Dev]

@StephenLPeters It appears to me I'm currently being "trickle fed" by Azure Pipelines here regarding visual verification files:

Now, I'm getting a single link to a ComboBox-8.xml file:

yet the failing tests also still include a failing visual verification file test for the ColorPicker (which was supposed to be fixed with the previous PR):

Which means I'm pretty sure once once I've added the ComboBox-8.xml file and we run the test pipeline again that we will be greeted with more test failures.

[StephenLPeters]

@StephenLPeters It appears to me I'm currently being "trickle fed" by Azure Pipelines here regarding visual verification files:

Now, I'm getting a single link to a ComboBox-8.xml file:

yet the failing tests also still include a failing visual verification file test for the ColorPicker (which was supposed to be fixed with the previous PR):

Which means I'm pretty sure once once I've added the ComboBox-8.xml file and we run the test pipeline again that we will be greeted with more test failures.

ugh thats very frustrating, maybe @kmahone can take a look?

[kmahone]

It looks like there was a typo in the processhelixfiles.ps1 script that was causing the copying process to fail for the VerifyVisualTree files. I have a PR out with a fix #2959.

[kmahone]

If you merge the latest master to pick up #2959 it should resolve the issue with the PR run.

[Felix-Dev]

@kmahone Thanks for your work 🙂 @StephenLPeters Merged with master.

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Felix-Dev]

@StephenLPeters It looks better now but I'm still not sure if the verification file output by Azure Pipelines works entirely as intended currently. Opening the downloaded drop archive, I see both an UpdatedVisualTreeVerificationFiles folder and a LinksToHelixTestFiles.html file. Both of these contain a different set of verification files:

And the LinksToHelixTestFiles file:

Both sets of verification files appear valid to me so I would upload all four new verification files in a commit. Why are they being separated though? Ideally, the updated verification files would all be in one place - either in the UpdatedVisualTreeVerificationFiles folder or the LinksToHelixTestFiles file. But surely not a situation where the folder contains one set and the file containing links to the other set, right?

I have not yet pushed the commit so the team can take a look at this with their own eyes instead of just looking at my images, if desired. Let me know when I should make that commit.

[StephenLPeters]

@StephenLPeters It looks better now but I'm still not sure if the verification file output by Azure Pipelines works entirely as intended currently. Opening the downloaded drop archive, I see both an UpdatedVisualTreeVerificationFiles folder and a LinksToHelixTestFiles.html file. Both of these contain a different set of verification files:

And the LinksToHelixTestFiles file:

Both sets of verification files appear valid to me so I would upload all four new verification files in a commit. Why are they being separated though? Ideally, the updated verifiction files would all be in one place - either in the UpdatedVisualTreeVerificationFiles folder or the LinksToHelixTestFiles file. But surely not a situtation where the folder contains one set and the file containing links to the other set, right?

I have not yet pushed the commit so the team can take a look at this with their own eyes instead of just looking at my images, if desired. Let me know when I should make that commit.

@kmahone who knows this better than i do. @chingucoding who made the name change

[marcelwgn]

It seems that the LinksToHelixFiles just takes all xml files that have been uploaded to the helix job, and adds them to the LinksToHelixFiles, while also adding them to the visualTreeVerifictionFiles folder.

The VisualTreeVerificationFiles folder consists of all files that are in that folder, and those that where placed there from the helix downloads. The files in that folder are merged, that is if there are multiple files with the same prefix, and merge them.

Where those numbers come from at the end of the file is a mystery to me though, I am not sure if it is enough to place the file from the VerificationFilesFolder in there or if you also need the one from the helix link.
The ColorPicker.xml and the ColorPicker-7.xml have the same content, though while the files in the master branch they do not have the same content 🤔

[StephenLPeters]

It seems that the LinksToHelixFiles just takes all xml files that have been uploaded to the helix job, and adds them to the LinksToHelixFiles, while also adding them to the visualTreeVerifictionFiles folder.

The VisualTreeVerificationFiles folder consists of all files that are in that folder, and those that where placed there from the helix downloads. The files in that folder are merged, that is if there are multiple files with the same prefix, and merge them.

Where those numbers come from at the end of the file is a mystery to me though, I am not sure if it is enough to place the file from the VerificationFilesFolder in there or if you also need the one from the helix link.
The ColorPicker.xml and the ColorPicker-7.xml have the same content, though while the files in the master branch they do not have the same content 🤔

The number at the end of the file refers to the OS version that the test was run on. The script renames the lowest numbered prefix-*.xml file to prefix.xml before copying it to the folder, so they should be the same. I think this rename is wrong though.

I dug around this for a while with the help of @kmahone and there is some faulty logic in the ProcessHelixFiles script which is trying to combine visualverification files which are identical. The root of the issue is that you cannot determine solely from the failed test output what the correct set of files that need to be checked in are.

Consider the case where there is a VisualVerification(VV) test which has 1 VV file checked in. lets say I make a change that is exclusive to rs4. This would produce a Foo-4.xml file which the script would copy to the helix links as foo-4.xml but to the VV folder as foo.xml. If I checked in the foo-4.xml then the system would use that for OS versions rs4 and up and would fail on rs5+. If i checked in the foo.xml overriding the previous version it would use that for all versions of the os and fail on everything but RS4. So neither option is correct. I think what we need is to publish all of the .xml files to the artifacts and then have a sperate script that you can run locally that compares the directory that you download to the files in your repository and determines the set of files you actually need. I'll look into this soon.

In the mean time... I think that the safest thing to do is look at which versions of the VV files are checked in to reason about which updates you need to get this right. you might get away with just doing the ones in UpdatedVisualTreeVerificationFiles folder, which i think is what people have been doing, but its not guaranteed to be right in its current state.

[StephenLPeters]

@Felix-Dev I have a script written that I beleive will generate the correct set of visual verification files for updating. I'm a bit confused by its output on your branch though, can you help me understand why the ColorPicker.VerifyVisualTree test only failed on RS5 and not on 19h1? I would have expected both.

[Felix-Dev]

@StephenLPeters The ColorPicker.VerifyVisualTree test did also fail on 19H1: #2798 (comment) I added the new ColorPicker-8.xml vv file in commit dd43763. I have just diff'ed that ColorPicker-8.xml file and the ColorPicker-7.xml file (which was listed in the LinksToHelixTestFiles) and VS tells me they are exactly the same.

Given your explanation above it is my understanding that - since both ColorPicker-7 and ColorPicker-8 files are identical - only the ColorPicker-7.xml file would be needed here, if at all. So ColorPicker-8.xml should be deleted again in a new commit.

[StephenLPeters]

@StephenLPeters The ColorPicker.VerifyVisualTree test did also fail on 19H1: #2798 (comment) I added the new ColorPicker-8.xml vv file in commit dd43763. I have just diff'ed that ColorPicker-8.xml file and the ColorPicker-7.xml file (which was listed in the LinksToHelixTestFiles) and VS tells me they are exactly the same.

Given your explanation above it is my understanding that - since both ColorPicker-7 and ColorPicker-8 files are identical - only the ColorPicker-7.xml file would be needed here, if at all. So ColorPicker-8.xml should be deleted again in a new commit.

Ah I see, that explains it and points out a bug in my new script, thanks. And yes, you should remove -8 and add -7

[Felix-Dev]

@StephenLPeters Updated the verification files.

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Felix-Dev]

Hopefully with this last commit, no more failing visual verification tests will occur.

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

🕐

[StephenLPeters]