Make sure background color is opaque when attrs are reversed #5509
Conversation
| // If the bg isn't the default bg color, then make it fully opaque. | ||
| if (!attr.BackgroundIsDefault()) | ||
| // If the bg isn't the default bg color, or reverse video is enabled, make it fully opaque. | ||
| if (!attr.BackgroundIsDefault() || WI_IsFlagSet(attr.GetMetaAttributes(), COMMON_LVB_REVERSE_VIDEO)) |
There was a problem hiding this comment.
I may file a followup TODO: the foreground should become transparent in this case, which would just be wild. Also, it simply wouldn't work, because we would need to push it on as a clipping layer when we draw the background ;P
There was a problem hiding this comment.
On a related note, we may also want to consider whether a background color that simply matches the default background should also be made transparent. Right now, if I set my background to \e[40m or \e[48;5;0m those colors will also be treated as transparent. Once #2661 is fixed, though, that wouldn't be the case - only an explicit default background would be transparent. Maybe that's the way it should be - I don't what most people expect from this feature. We just need to be aware that the current behaviour is going to change after #2661 if we leave things as they are.
There was a problem hiding this comment.
I think it's the right thing to do (separate 40/48 and 37/38), but we may need to make it opt-in or opt-out for the ConPTY consumer. Per #293 (comment), this is one of those sad compatibility hacks.
Because legacy console applications could never emit colors that weren't in the default 16, we chose to make their explicit requests for backgrounds that matched the "default" buffer background (in the legacy 4-bit sense) and foreground into requests for the "default" when they came out of the PTY. 😕
There was a problem hiding this comment.
If there weren't so many apps with a dependency on the layout of a console text attribute or the SetConsoleTextAttributes API, I'd be gung-ho for it. After all, conhost itself has a mode that lets us differentiate them. It does look rather garish, however:
(from Rich's blog post about that)
DHowett-MSFT
changed the title
In order to support a transparent background for the acrylic effect, the renderer sets the alpha value to zero for the default background color. However, when the _reversed video_ attribute is set, the background is actually filled with the foreground color, and will not be displayed correctly if it is made transparent. This PR addresses that issue by making sure the rendered background color is opaque if the reversed video attribute is set. ## References * This is not a major issue at the moment, since the _reverse video_ attribute is not typically forwarded though conpty, but that will change once #2661 is fixed. ## PR Checklist * [x] Closes #5498 * [x] CLA signed. * [ ] Tests added/passed * [ ] Requires documentation to be updated * [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #5498 ## Detailed Description of the Pull Request / Additional comments This simply adds an additional check in `Terminal::GetBackgroundColor` to make sure the returned color is opaque if the _reverse video_ attribute is set. At some point in the future this check may need to be extended to support the `DECSCNM` reverse screen mode, but for now that's not an issue. ## Validation Steps Performed I've run the test case from issue #5498, and confirmed that it now works as expected. I've also got an experimental fix for #2661 that I've tested with this patch, and that now displays _reverse video_ attributes correctly too. Closes #5498
Summary of the Pull Request
In order to support a transparent background for the acrylic effect, the renderer sets the alpha value to zero for the default background color. However, when the reversed video attribute is set, the background is actually filled with the foreground color, and will not be displayed correctly if it is made transparent. This PR addresses that issue by making sure the rendered background color is opaque if the reversed video attribute is set.
References
PR Checklist
Detailed Description of the Pull Request / Additional comments
This simply adds an additional check in
Terminal::GetBackgroundColorto make sure the returned color is opaque if the reverse video attribute is set. At some point in the future this check may need to be extended to support theDECSCNMreverse screen mode, but for now that's not an issue.Validation Steps Performed
I've run the test case from issue #5498, and confirmed that it now works as expected. I've also got an experimental fix for #2661 that I've tested with this patch, and that now displays reverse video attributes correctly too.