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

Don't change background color of selected terminal text #200428

Closed
akbyrd opened this issue Dec 9, 2023 · 11 comments · Fixed by #201227 or #203831
Closed

Don't change background color of selected terminal text #200428

akbyrd opened this issue Dec 9, 2023 · 11 comments · Fixed by #201227 or #203831
Assignees
Labels
author-verification-requested Issues potentially verifiable by issue author feature-request Request for new features or functionality info-needed Issue requires more information from poster insiders-released Patch has been released in VS Code Insiders terminal-rendering verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@akbyrd
Copy link
Contributor

akbyrd commented Dec 9, 2023

Type: Bug

Text selection in the terminal seems to render by changing the background color of text. While that seems reasonable on the surface, it does weird things with powerline type features and other highlighting that might be built into the shell. Notice in the second screenshot below that different parts of the powerline are no longer visible nor are the highlights on directories.

image
image

Contrast this with Windows Terminal

image
image

Perhaps vscode could blend the selection background and foreground colors with the existing colors instead of overwriting them. Or render selection as an overlay.

VS Code version: Code - Insiders 1.86.0-insider (02b9071, 2023-12-08T11:53:10.286Z)
OS version: Windows_NT x64 10.0.19045
Modes:

System Info
Item Value
CPUs AMD Ryzen 9 5950X 16-Core Processor (32 x 3400)
GPU Status 2d_canvas: enabled
canvas_oop_rasterization: enabled_on
direct_rendering_display_compositor: disabled_off_ok
gpu_compositing: enabled
multiple_raster_threads: enabled_on
opengl: enabled_on
rasterization: enabled
raw_draw: disabled_off_ok
skia_graphite: disabled_off
video_decode: enabled
video_encode: enabled
vulkan: disabled_off
webgl: enabled
webgl2: enabled
webgpu: enabled
Load (avg) undefined
Memory (System) 31.92GB (20.17GB free)
Process Argv --crash-reporter-id 22cf4e37-a96e-4ad9-82f0-8715f8e2bf6e
Screen Reader no
VM 0%
Extensions: none
A/B Experiments
vsliv695:30137379
vsins829:30139715
vsliv368cf:30146710
vsreu685:30147344
python383:30185418
vspor879:30202332
vspor708:30202333
vspor363:30204092
vslsvsres303:30308271
pythontb:30258533
pythonptprofiler:30281269
vsdfh931cf:30280410
vshan820:30294714
vscod805cf:30301675
bridge0708:30335490
bridge0723:30353136
vsaa593cf:30376535
pythonvs932:30404738
py29gd2263:30784851
vsclangdf:30492506
c4g48928:30535728
dsvsc012cf:30540253
2i9eh265:30646982
showlangstatbar:30737417
fixshowwlkth:30771523
showindicator:30805243
pythongtdpath:30726887
i26e3531:30792625
welcomedialog:30812478
pythonnosmt12:30779711
pythonidxpt:30768918
pythonnoceb:30776497
asynctok:30898717
dsvsc013:30777762
dsvsc014:30777825
pythonmpsinfo:30901776
dsvsc015:30821418
pythontestfixt:30866404
pythonregdiag2:30902439
pyreplss1:30879911
pythonmypyd1:30859725
pythoncet0:30859736
pythontbext0:30879054
accentitlementst:30870582
dsvsc016:30879898
dsvsc017:30880771
dsvsc018:30880772
aa_t_chat:30882232

@Tyriar
Copy link
Member

Tyriar commented Dec 18, 2023

It was actually a pretty big feature to get the behavior working that you're saying you want changed 😅. The overlay option was how it worked before (and actually how it still worked with "terminal.integrated.gpuAcceleration": "canvas") and it was trivial to add as it's simply rectangles drawn on top of the content. Unfortunately it has big accessibility issues as it does nothing to ensure contrast is sufficient, making the text quite unreadable.

The powerline specific "problem" is happening due to the fact that powerline is essentially a hack where it alternates between the background color (which changes with selection) and special characters (which changes to ensure minimum contrast ratio). I have considered making it a little nicer, perhaps by just hiding the powerline symbols completely, but its not clear what the right behavior is here.

image

We could quite easily make the above do this:

image

But anything more clever than that may be a little too complex, I'm hesitant to try to track "powerline" state in the cells that simply change their background color.

FWIW I also hit this problem, this is what mine looks like:

image

@Tyriar Tyriar added under-discussion Issue is under discussion for relevance, priority, approach terminal-rendering labels Dec 18, 2023
@Tyriar Tyriar added this to the Backlog milestone Dec 18, 2023
@akbyrd
Copy link
Contributor Author

akbyrd commented Dec 19, 2023

I would make the case that if background colors in terminal output are supported then having the terminal ever overwrite those background colors is necessarily a problem.

I wouldn't call powerline a hack any more than I would call highlighting directories using a background color a hack. Color conveys information in the same way glyphs do. If I choose to output something in a particular way I don't want that information arbitrarily erased by the terminal.

Either the background color is reserved for selection highlighting or it's not. If it's not reserved and users are allowed to choose background colors to convey information, then the terminal does not have the authority to erase that entirely. Munge based on user settings yes, erase no.

As far as solutions, I can think of a few possibilities:

  1. Modify existing background colors instead of overwriting them. This is what other terminals seem to do and seems like a happy middle ground.

  2. Blend the selection background color and the existing background color. It may not meet the desired contrast though.

  3. Allow the overlay approach to be used. Between my vscode theme and my terminal configuration I have enough control to maintain the level of contrast I need.

  4. Treat "terminal.integrated.minimumContrastRatio": 1 as an opt-out and don't overwrite background colors.

image

image

@Tyriar
Copy link
Member

Tyriar commented Dec 19, 2023

Thanks for taking the time to put forward the proposals. I'm leaning towards 2 and blending with the selection color @ 50% opacity. Contrast is less of a problem as the foreground changes dynamically due to minimumContrastRatio.

Plus we can have some special logic on the powerline glyphs to also be included in the blending, they already do something similar in how they ignore the minimumContrastRatio requirements as they are seen as background by the user.

@Tyriar Tyriar added feature-request Request for new features or functionality and removed under-discussion Issue is under discussion for relevance, priority, approach labels Dec 19, 2023
@Tyriar Tyriar modified the milestones: Backlog, December / January 2024 Dec 19, 2023
@Tyriar
Copy link
Member

Tyriar commented Dec 19, 2023

WIP:

image

Tyriar added a commit that referenced this issue Dec 19, 2023
@Tyriar Tyriar mentioned this issue Dec 19, 2023
@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Dec 19, 2023
@aiday-mar aiday-mar added the verification-needed Verification of issue is requested label Jan 24, 2024
@andreamah andreamah added the verified Verification succeeded label Jan 24, 2024
@andreamah
Copy link
Contributor

andreamah commented Jan 24, 2024

image
image

Tried to verify using the quick-term ohmyposh theme. Not sure if this is the expected result.

@andreamah andreamah removed the verified Verification succeeded label Jan 24, 2024
@rzhao271 rzhao271 added verified Verification succeeded verification-steps-needed Steps to verify are needed for verification and removed verified Verification succeeded labels Jan 24, 2024
@meganrogge meganrogge added author-verification-requested Issues potentially verifiable by issue author and removed verification-steps-needed Steps to verify are needed for verification labels Jan 25, 2024
@VSCodeTriageBot
Copy link
Collaborator

This bug has been fixed in the latest release of VS Code Insiders!

@akbyrd, you can help us out by commenting /verified if things are now working as expected.

If things still don't seem right, please ensure you're on version 3dea5cb of Insiders (today's or later - you can use Help: About in the command palette to check), and leave a comment letting us know what isn't working as expected.

Happy Coding!

@akbyrd
Copy link
Contributor Author

akbyrd commented Jan 25, 2024

The behavior on insiders doesn't quite match @Tyriar's WIP screenshot, which matches my desired behavior. In practice powerline background colors are still being overwritten.

image

@meganrogge meganrogge reopened this Jan 25, 2024
@meganrogge meganrogge modified the milestones: December / January 2024, February 2024 Jan 25, 2024
@VSCodeTriageBot VSCodeTriageBot removed the insiders-released Patch has been released in VS Code Insiders label Jan 25, 2024
@Tyriar
Copy link
Member

Tyriar commented Jan 29, 2024

@akbyrd @andreamah can you share your settings file? Still seems to work as expected for me 🤔

image

@Tyriar Tyriar added the info-needed Issue requires more information from poster label Jan 29, 2024
@akbyrd
Copy link
Contributor Author

akbyrd commented Jan 29, 2024

I'm able to repro in latest insiders (5d34400) with no VS Settings, no pwsh config (except loading oh-my-posh), and default oh-my-posh config. Maybe related to oh-my-posh if you're using a different powerline implementation?

pwsh config

oh-my-posh init pwsh | Invoke-Expression

image

@Tyriar
Copy link
Member

Tyriar commented Jan 30, 2024

Can repro with oh-my-posh, looking into it

image

Tyriar added a commit to Tyriar/xterm.js that referenced this issue Jan 30, 2024
@Tyriar Tyriar mentioned this issue Jan 30, 2024
@Tyriar Tyriar closed this as completed in e130132 Jan 30, 2024
@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Jan 30, 2024
@andreamah andreamah added the verified Verification succeeded label Feb 20, 2024
@andreamah
Copy link
Contributor

image Looks like it works 👍

@microsoft microsoft locked and limited conversation to collaborators Jun 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
author-verification-requested Issues potentially verifiable by issue author feature-request Request for new features or functionality info-needed Issue requires more information from poster insiders-released Patch has been released in VS Code Insiders terminal-rendering verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants