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

CSI 58 (undercurl color) sequence misbehaves when in "legacy ANSI" format #17426

Open
tranzystorekk opened this issue Jun 12, 2024 · 11 comments
Open
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Attention The core contributors need to come back around and look at this ASAP. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting

Comments

@tranzystorekk
Copy link

Windows Terminal version

N/A

Windows build number

10.0.22631.0

Other Software

neovim 0.10.0
zellij 0.40.1
wezterm 20240520-135708-b8f94c47

Steps to reproduce

More details are described in wez/wezterm#5450

A short overview

In the wezterm issue mentioned above, I came across conpty handling the CS 58 (undercurl color) ANSI sequence in a way that causes visual bugs for zellij users on modern windows terminals.

In short windows implementation seems to only accept the format:

ESC[58:2::234:105:98m (colons, iterm-compatible)

While if a proxy like zellij emits a more common ANSI form:

ESC[58;2;234;105;98m (semicolons)

it leads to the terminal instead changing text background color.

Expected Behavior

I wanted to discuss a good way forward to fix this between the zellij project and the terminal implementations used on Windows. Some thoughts and options:

  • Resolve by simply accepting CSI 58 in the more "ANSI-compatible" semicolon form. Reportedly, wezterm on linux doesn't experience this, so it seems reasonable to sync behaviors between platforms.
  • If it's infeasible to fix this in WT code, zellij behavior can be fixed instead.

Actual Behavior

N/A

@tranzystorekk tranzystorekk 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 Jun 12, 2024
@DHowett
Copy link
Member

DHowett commented Jun 12, 2024

@tusharsnx had some good rationale for only supporting the ITU T.418 format for underline colors - Tushar, do you remember what that was?

@DHowett
Copy link
Member

DHowett commented Jun 12, 2024

Notes from #15795

This is a requirement for the implementation to avoid problems with VT clients that don't support sub parameters.

To my understanding, we explicitly do not support or emit ; because of the potential for confusing parsers who miss handling the leading 58.

Zellij should probably do the same for the same reason.

@DHowett DHowett added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 12, 2024
@tusharsnx
Copy link
Contributor

tusharsnx commented Jun 13, 2024

@tusharsnx had some good rationale for only supporting the ITU T.418 format for underline colors

I remember blocking the sending of CSI-58 with ; because it can have unexpected outcomes for unsupported terminals, but I'm not 100% sure if parsing of such sequences is something we want to block 🤔

If a console application sends us CSI-58 that's a strong signal that the intention is to color the underlines regardless of if it's followed by ; or :, given that there is no secondary meaning to CSI-58 other than underline colors. I think I need @j4james help to confirm this.

If we like to enable parsing of CSI-58 (like we do for CSI-38 and CSI-48), then there's a possibility, but only if it doesn't affect unsupported terminals. We will only emit CSI-58 with : to the terminal (Conpty client) though. This will eliminate the unexpected outcomes in case an application sends CSI-58 with ; since that will be translated to CSI-58 with : and unsupported terminals should just eat it without doing anything 🙂

@tranzystorekk
Copy link
Author

If we like to enable parsing of CSI-58 (like we do for CSI-38 and CSI-48), then there's a possibility, but only if it doesn't affect unsupported terminals. We will only emit CSI-58 with : to the terminal (Conpty client) though. This will eliminate the unexpected outcomes in case an application sends CSI-58 with ; since that will be translated to CSI-58 with : and unsupported terminals should just eat it without doing anything 🙂

I like this solution because as @imsnif mentioned in the wezterm PR, zellij would like to stay uniform and backward-compatible in its implementation, and as a terminal multiplexer it probably shouldn't be its responsibility to handle diverging parser behaviors between platforms.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jun 13, 2024
@j4james
Copy link
Collaborator

j4james commented Jun 13, 2024

For app devs (which includes zellij), I would recommend against using ITU sequences in any format unless you've somehow verified that the terminal actually supports those sequence. Both variants have the potential to break terminals that don't understand them. And if you use a DECRQSS query to check for compatibility, that should also tell you which variant the terminal supports.

That said, on our side I think it would be preferable if we supported both semicolons and colons for SGR 58, the same way we do for SGR 38 and 48. Although when it comes to passing the sequence through to conpty, I think we decided that semicolons are best for 38 and 48, since they're more widely supported, while colons were best for 58, since all terminals that support underline colors also support colons, but not all of them support semicolon (we obviously don't).

In an ideal world we should have first been testing the conpty client in the same way an app or multiplexer would, but if we have conpty passthrough soon that won't be necessary.

@tusharsnx
Copy link
Contributor

For app devs (which includes zellij), I would recommend against using ITU sequences in any format unless you've somehow verified that the terminal actually supports those sequence.

I agree with this, since even if we enable "SGR 58 with semicolon" parsing, most terminals on Windows won't support that out-of-box (for like.. another few years). AFAIK, only Windows Terminal and WezTerm would get the support because they consume the most up-to-date Conpty implementation (from this repo), while others rely on inbox conhost.exe (that sits inside system32 dir) 🙁. So, zellij should by default emit SGR 58 with colons, and use semicolons on a case-to-case basis.

@imsnif
Copy link

imsnif commented Jun 14, 2024

@tusharsnx - My reading of what @j4james wrote is that Zellij should interpret both colons and semicolons and only emit semicolons (which is what we do). And that Windows Terminal (apologies if I'm not using the right terminology, I do not know this ecosystem well) should interpret both semicolons and colons as well.

Did I misread or misunderstand you @j4james ?

@j4james
Copy link
Collaborator

j4james commented Jun 14, 2024

My reading of what @j4james wrote is that Zellij should [...] only emit semicolons

No. I was saying Zellij should check the capabilities of the terminal before emitting anything. But if that is too much effort, then it should emit semicolons for 38 and 48, and colons for 58.

@imsnif
Copy link

imsnif commented Jun 14, 2024

Alright. I'm willing to issue the fix on our side. Namely as you said because terminals that support underline color necessarily support the colon format, but it would be nice if the fix would also be issued here (I guess Windows Terminal has more working hands and the issue really, as far as I know at least, doesn't happen anywhere else).

@j4james
Copy link
Collaborator

j4james commented Jun 14, 2024

the issue really, as far as I know at least, doesn't happen anywhere else

@imsnif Just FYI, Mintty behaves the same way as Windows Terminal, i.e. SGR 38 ad 48 work with both colons and semicolons, but SGR 58 only works with colons. You can see the docs here:

https://github.com/mintty/mintty/blob/a79e7dd5acf30bf6fd7790990cbf5ffa535553ca/wiki/Tips.md#text-attributes-and-rendering

@imsnif
Copy link

imsnif commented Jun 15, 2024

Thanks for the correction @j4james. My expectation expressed above still stands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Attention The core contributors need to come back around and look at this ASAP. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting
Projects
None yet
Development

No branches or pull requests

5 participants