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

Fix transparent foreground color #4667

Merged
merged 3 commits into from
Aug 14, 2023

Conversation

Dennnnny
Copy link
Contributor

fix: #4408

Description:

I found this transparent foreground color will affect some text when the foreground is background
And inside the TextureAtlas, there are already exist some check for background transparent
So I put another check foreground transparent

Hope this make scenes

Any feedbacks is welcome, thanks


Screenshot:

For local test, I change the xtermjsTheme foreground color with transparent

// demo/client.ts
const xtermjsTheme = {
  foreground: '#F8F8F8CC'
}

And the when no doing anything, it looks like:
截圖 2023-08-12 上午12 25 04

After change:
截圖 2023-08-12 上午12 25 59

@jerch
Copy link
Member

jerch commented Aug 12, 2023

Have not looked at the PR code, just FYI:
We have no good idea, what transparency on FG should mean or be used for in a terminal. I tried to address this several times with other terminal devs, esp. because SGR has a dedicated transparent flag (which we dont support yet), but the response on that was more like "Idk, have not yet dealt in depth with it".

So until we resolve the issue, what transparency in FG can be used for and find a real good use case, I like @Tyriar 's idea to completely strip any transparency from FG (e.g. introduced by FG/BG colors swaps), maybe with an additional lumi correction to stay on sane ground.

src/browser/services/ThemeService.ts Outdated Show resolved Hide resolved
@Dennnnny Dennnnny requested a review from Tyriar August 14, 2023 16:13
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for investigating this 👍

@Tyriar Tyriar added this to the 5.3.0 milestone Aug 14, 2023
@Tyriar Tyriar self-assigned this Aug 14, 2023
@Tyriar Tyriar merged commit c908eee into xtermjs:master Aug 14, 2023
8 checks passed
@Dennnnny
Copy link
Contributor Author

Dennnnny commented Aug 15, 2023

thanks for all the help and suggestions.
it's my honor to contribute ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn or support transparent foreground colors
3 participants