Skip to content

Fix transparent foreground color#4667

Merged
Tyriar merged 3 commits intoxtermjs:masterfrom
Dennnnny:fix-transparent-foreground-color
Aug 14, 2023
Merged

Fix transparent foreground color#4667
Tyriar merged 3 commits intoxtermjs:masterfrom
Dennnnny:fix-transparent-foreground-color

Conversation

@Dennnnny
Copy link
Copy Markdown
Contributor

@Dennnnny Dennnnny commented Aug 12, 2023

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

Tyriar
Tyriar requested changes Aug 12, 2023
View reviewed changes
@jerch
Copy link
Copy Markdown
Member

jerch commented Aug 12, 2023 •
edited
Loading

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.

Tyriar
Tyriar requested changes Aug 14, 2023
View reviewed changes
@Dennnnny Dennnnny requested a review from Tyriar August 14, 2023 16:13
Tyriar
Tyriar approved these changes Aug 14, 2023
View reviewed changes
Copy link
Copy Markdown
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
@Dennnnny
Copy link
Copy Markdown
Contributor Author

Dennnnny commented Aug 15, 2023 •
edited
Loading

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

@jonas807900643 jonas807900643 mentioned this pull request Sep 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Tyriar Tyriar Tyriar approved these changes

Assignees

@Tyriar Tyriar

Labels

None yet

Projects

None yet

Milestone

5.3.0

Development

Successfully merging this pull request may close these issues.

Warn or support transparent foreground colors

3 participants

@Dennnnny @jerch @Tyriar