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.

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

Proposal: Foreground for ComboBox.PlaceholderText should be same as TextBox.PlaceholderText #2774

Closed
harvinders opened this issue Jun 29, 2020 · 7 comments · Fixed by #2798
Closed
Labels
area-ComboBox area-Styling feature proposal New feature proposal help wanted Issue ideal for external contributors team-Controls Issue for the Controls team

Comments

@harvinders
Copy link

Proposal: Foreground for ComboBox.PlaceholderText should be same as TextBox.PlaceholderText

Summary

The foreground colour for ComboBox's PlaceholderText should be same as TextBox's PlaceholderText, as it would allow it to be differentiated from the selected value in the combobox.

Rationale

  • Help align PlaceholderText foreground between controls
  • Help differentiate the PlaceholderText from the selected value

Scope

Capability Priority
Match the foreground colour to TextBox's PlaceholderText foreground Must
Allow lightweight style Should

Important Notes

Open Questions

@harvinders harvinders added the feature proposal New feature proposal label Jun 29, 2020
@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Jun 29, 2020
@Felix-Dev
Copy link
Contributor

Felix-Dev commented Jun 29, 2020

Makes sense to me. Below you will find a table showcasing the current placeholder foreground color of a ComboBox in uneditable mode while in rest state, focused state and focused pressed state (there is no separate FocusedPointerOver visual state). The proposed solution updates the placeholder foreground color to match that of the TextBox and keep the placeholder foreground consistent between the visual states outlined above to separate it from non-placeholder text.

Theme Current Proposed
Light combobox-placeholder-foreground-focused combobox-placeholder-foreground-focused-popsolution
Dark combobox-placeholder-foreground-focused-dark combobox-placeholder-foreground-focused-dark-popsolution

The above proposed solution would add a new ComboBoxPlaceHolderForegroundFocused theme resource alongside the already existing ComboBoxPlaceHolderForeground and ComboBoxPlaceHolderForegroundFocusedPressed theme resources.

When the TextBox is in editable mode, only the placeholder foreground color needs to be updated as above. In the other Focused* visual states it already matches that of the TextBox's placeholder foreground:
combobox-placeholder-foreground-focused-editable

@StephenLPeters StephenLPeters added area-ComboBox area-Styling help wanted Issue ideal for external contributors team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Jun 29, 2020
@StephenLPeters
Copy link
Contributor

Should be pretty simple given that we have lifted the combo box template to WinUI 2 already. I agree the placeholder text foreground color should match textbox's. FYI @chigy

@Felix-Dev
Copy link
Contributor

I will create a PR tomorrow for the placeholder foreground.

@Felix-Dev
Copy link
Contributor

Felix-Dev commented Jun 30, 2020

About lightweight styling the placeholder foreground: This doesn't appear to work as intended and expected for now. Observe for example the following XAML:

<ComboBox PlaceholderText="Some placeholder">
    <ComboBox.Resources>
        <SolidColorBrush x:Key="ComboBoxPlaceHolderForeground" Color="Green" />
        <SolidColorBrush x:Key="ComboBoxPlaceHolderForegroundFocused" Color="Orange" />
        <SolidColorBrush x:Key="ComboBoxPlaceHolderForegroundFocusedPressed" Color="Red" />
    </ComboBox.Resources>
</ComboBox>

The expected result would be this:
combobox-placeholder-foreground-focused-themeresources-issue-expected

but the actual result is this:
combobox-placeholder-foreground-focused-themeresources-issue

These issues are located here, for example:

contract5Present:Foreground="{Binding PlaceholderForeground, RelativeSource={RelativeSource TemplatedParent}, TargetNullValue={ThemeResource ComboBoxPlaceHolderForeground}}" />

What we are saying here and in the other visual states of Focused and FocusedPressed here and here is that if the ComboBox.PlaceholderForeground API is set, it will take precendence over the specified theme resources. However, if the PlaceholderForeground API is not set, as in my example XAML above, these theme resources should be used. And in fact they are (TargetNullValue works correctly). Unfortunately for us though, the theme resources values applied here are the values defined in the ComboBox_themeresources.xml resource dictionary! The overriden resource values in my example XAML above are being ignored (probably not seen by the theme resource lookup algorithm in use here). @StephenLPeters I'm not sure how related this XAML resource lookup issue is with the issue we were seeing in #2674 (comment) but I'm still pinging you here as a FYI at least.

This issue also effects the TextBox and its placeholder theme resources like TextControlPlaceholderForeground and TextControlPlaceholderForegroundFocused and as such is also an issue for the ComboBox when it is in an editable state (ComboBox.IsEditable = true):

<ObjectAnimationUsingKeyFrames Storyboard.TargetName="PlaceholderTextContentPresenter" Storyboard.TargetProperty="Foreground">

These lightweight styling issues above should presumably be tracked in a separate issue about the XAML resource lookup algorithm. Your thoughts, @StephenLPeters?

@chigy
Copy link
Member

chigy commented Jun 30, 2020

@kikisaints FYI

@StephenLPeters
Copy link
Contributor

@Felix-Dev making sure I understand, this issue is still present after your PR correct? If you put the theme resource in a parent resource dictionary does it work? what about the app.xaml resources? I wonder if the lookup initiated by the binding functions differently than a normal resource lookup, similar to how resource lookups from code behind don't match xaml resource lookups..

@MikeHillberg or @jevansaks maybe you already know about any intricacies to lookups from binding?

@Felix-Dev
Copy link
Contributor

Felix-Dev commented Jun 30, 2020

@StephenLPeters Yes, the issue would still be present even with my PR merged. In other words, using lightweight styling to set the Placeholder foreground for the TextBox/ComboBox/... would still not work (see my example XAML markup in the post above).

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.

And yes, "the lookup initiated by the binding functions" seems like a good candidate for being the culprit here. Observe that this only happens for the Placeholder which is the only one using Binding in ComboBox visual states. If I change the following XAML code in the visual state from

<ObjectAnimationUsingKeyFrames Storyboard.TargetName="PlaceholderTextBlock" Storyboard.TargetProperty="Foreground">
    <DiscreteObjectKeyFrame KeyTime="0" Value="{Binding PlaceholderForeground, RelativeSource={RelativeSource TemplatedParent}, TargetNullValue={ThemeResource ComboBoxPlaceHolderForegroundFocused}}" />
</ObjectAnimationUsingKeyFrames>

to:

<ObjectAnimationUsingKeyFrames Storyboard.TargetName="PlaceholderTextBlock" Storyboard.TargetProperty="Foreground">
    <DiscreteObjectKeyFrame KeyTime="0" Value="{ThemeResource ComboBoxPlaceHolderForegroundFocused}" />
</ObjectAnimationUsingKeyFrames>

my theme resource overrides for ComboBoxPlaceHolderForegroundFocused will be applied just as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-ComboBox area-Styling feature proposal New feature proposal help wanted Issue ideal for external contributors team-Controls Issue for the Controls team
Projects
None yet
5 participants