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

Command Palette key bindings are unreadable in Dark theme with light accent color #15228

Closed
Molkree opened this issue Apr 23, 2023 · 12 comments · Fixed by #15677
Closed

Command Palette key bindings are unreadable in Dark theme with light accent color #15228

Molkree opened this issue Apr 23, 2023 · 12 comments · Fixed by #15677
Labels
Area-Accessibility Issues related to accessibility good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Milestone

Comments

@Molkree
Copy link

Molkree commented Apr 23, 2023

Windows Terminal version

1.16.10261.0

Windows build number

10.0.19045.0

Other Software

No response

Steps to reproduce

  1. Set the Terminal theme to dark.
  2. Enable "Automatically pick an accent colour from my background" in Windows settings. This image should do the trick. Or set it manually to Turquoise (41, 190, 204; #29BECC).
  3. Open Command Palette.
  4. Navigate to some entry with key binding.
    image

Key binding colours don't change in the Light theme too so maybe someone can find such accent colour so that it also becomes unreadable, not sure:
image

This is a bit similar to #6892 (my accent colour is almost the same! 😄)

Expected Behavior

Key bindings' colour gets inverted if needed as action text colour does.

Actual Behavior

Key bindings' colour doesn't change depending on the selection background colour.

@Molkree Molkree added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Apr 23, 2023
@237dmitry
Copy link

Key bindings' colour doesn't change depending on the selection background colour.

Yes, but keybindings are visible:

Screenshot 2023-04-26 171806

@zadjii-msft
Copy link
Member

Is there a secret step 0 here that's "set the OS to High Contrast Mode"?

This might be tricky. I wonder if there's a brush that's better for "text on top of the accent color", that we could set with like, a VisualState or something, only in HC mode...

@Molkree
Copy link
Author

Molkree commented Apr 26, 2023

keybindings are visible

@237dmitry, hm, my UI looks different, as you can see in the screenshots above (you only have accent colour as a short strip on the left and for me the whole background of selection changes). Is there a setting that controls this? I can't find anything obvious on the Appearance page of the settings. Are you on the same version of Terminal? Windows 10?

Is there a secret step 0 here that's "set the OS to High Contrast Mode"?

@zadjii-msft, I don't have High Contrast enabled in Windows (I have just checked it).

@zadjii-msft
Copy link
Member

INTERESTING.

Looks like ListView itself changed in Windows 11:

image

Left is a Win10 VM, and right is on Win11.

I would have assumed that just saying we were using Controlsv2 would have applied there as well (even on Windows 10), but perhaps there's something about the new style that isn't supported on windows 10.

I dunno what to do about this. I don't really want to try and have a conditional hack for "Only on Windows 10, do we also want to apply this other VisualStateGroup to the Command Palette items". That sounds tricky to do right in XAML, or annoying in codebehind. But I also don't know how to get the Win11 styles to work on Win10. Hmmm

@carlos-zamora carlos-zamora added Area-Accessibility Issues related to accessibility Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Apr 26, 2023
@carlos-zamora carlos-zamora added this to the Terminal v1.18 milestone Apr 26, 2023
@zadjii-msft
Copy link
Member

zadjii-msft commented Apr 26, 2023

Note

Walkthrough

Probably the easiest way to do this would be to just add a .Background() to this border:

<Border Grid.Column="2"
Padding="2,0,2,0"
HorizontalAlignment="Right"
VerticalAlignment="Center"
AutomationProperties.AccessibilityView="Raw"
Style="{ThemeResource KeyChordBorderStyle}"
Visibility="{x:Bind Item.KeyChordText, Mode=OneWay, Converter={StaticResource CommandKeyChordVisibilityConverter}}">

and use like, the CommandPalette background there, so that it appears transparent, but actually isn't.

(this was an off the cuff suggestion during triage, and we all thought it was a good idea)

@zadjii-msft zadjii-msft added Help Wanted We encourage anyone to jump in on these. good first issue This is a fix that might be easier for someone to do as a first contribution labels Apr 26, 2023
@zadjii-msft zadjii-msft moved this to Walkthrough in issue in Terminal Walkthroughs May 2, 2023
@zadjii-msft zadjii-msft modified the milestones: Terminal v1.18, Backlog May 5, 2023
@Random9ine
Copy link

@zadjii-msft Can I try my hand with this issue?

@zadjii-msft
Copy link
Member

@Random9ine always!

@Random9ine
Copy link

@zadjii-msft Unfortunately away from my desktop for the summer and didn't realize there's no way to run Win10 VMs on M1 MBP anymore. Don't think I'll be able to get to this for awhile

@RickleAndMortimer
Copy link
Contributor

Hey everyone, is it alright if I try taking a look at this issue?

@DHowett
Copy link
Member

DHowett commented Jun 29, 2023

Absolutely.

microsoft-github-policy-service bot pushed a commit that referenced this issue Jul 17, 2023
## Summary of the Pull Request

Adds a background to key chord border in the CommandPalette Screen. This
prevents certain accent colors from rendering the KeyChords unreadable.

Before (where the text is unreadble);

![image](https://github.com/microsoft/terminal/assets/33658638/370fa7c7-f42e-48b3-af54-6fe7d5f89c73)

After (from this PR):

![image](https://github.com/microsoft/terminal/assets/33658638/5ce8601a-80f2-4efe-9270-9dd7209cdfff)

See #15228 for more details
DHowett pushed a commit that referenced this issue Aug 2, 2023
## Summary of the Pull Request

Adds a background to key chord border in the CommandPalette Screen. This
prevents certain accent colors from rendering the KeyChords unreadable.

Before (where the text is unreadble);

![image](https://github.com/microsoft/terminal/assets/33658638/370fa7c7-f42e-48b3-af54-6fe7d5f89c73)

After (from this PR):

![image](https://github.com/microsoft/terminal/assets/33658638/5ce8601a-80f2-4efe-9270-9dd7209cdfff)

See #15228 for more details

(cherry picked from commit 91012a4)
Service-Card-Id: 90068187
Service-Version: 1.17
@RanjanTheDoer
Copy link

Hey is this issue fixed? If not I would like to work on it. please assign me

@zadjii-msft
Copy link
Member

Oh, huh. It looks like this was merged after all! Looks like this merged in #15677, but we never looped back and linked this PR to it! Sorry about that 😕

@github-project-automation github-project-automation bot moved this from Walkthrough in issue to Done in Terminal Walkthroughs Aug 9, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Aug 9, 2023
DHowett pushed a commit that referenced this issue Aug 11, 2023
## Summary of the Pull Request

Adds a background to key chord border in the CommandPalette Screen. This
prevents certain accent colors from rendering the KeyChords unreadable.

Before (where the text is unreadble);

![image](https://github.com/microsoft/terminal/assets/33658638/370fa7c7-f42e-48b3-af54-6fe7d5f89c73)

After (from this PR):

![image](https://github.com/microsoft/terminal/assets/33658638/5ce8601a-80f2-4efe-9270-9dd7209cdfff)

See #15228 for more details

(cherry picked from commit 91012a4)
Service-Card-Id: 90068188
Service-Version: 1.18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Accessibility Issues related to accessibility good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

8 participants