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

Shift modifier not working/recognised in conjunction with other modifier keys when inside Zellij session #3789

Open
blixenkrone opened this issue Nov 18, 2024 · 19 comments

Comments

@blixenkrone
Copy link

Basic information

zellij --version:
0.41.1

stty size:
78 150

Issue description

Whenever I am inside a zellij in any terminal (tried 3 different ones), the shift modifier key is not picked up as a modifier key, when used in combination with others.
This is mostly an issue when using terminal editors like vim, since I have a keybinding with shift+alt+s to do :w or write_file.

As the most minimal example, a use case flow would be:

  • I can write uppercase letters ie. ABCD with no issue
  • I can not write my files with the key combination of shift+alt+s. Only alt+s combination is picked up.

If I am not inside Zellij, I can do above use flow without issues. The bug seem to only exists when I am within a zellij session.

Minimal reproduction

Terminal: Ghostty
Editor: Helix
1 Enter a zellij session
2 Open a file with Helix (or vim)
3 Hit shift+alt+s
4 Only alt+s gets picked up.

Extra relevant information

1 No /tmp/zellij-log file was produced when running with $ zellij --debug for some reason.
2 I don't think it makes sense to show this in a video recording, but I am happy to record one if needed.

Thanks for a great tool!

@igor-ramazanov
Copy link

Same issue:
Terminal: Alacritty 0.14.0
Editor: Kakoune v2024.05.18
Zellij: 0.41.2

Fixed by downgrading 0.40.1

@blixenkrone
Copy link
Author

blixenkrone commented Nov 25, 2024

@imsnif Just FYI this thread was created from a Reddit thread upon your commenting - so another user having same issue.
I'm trying to setup a bug bounty for implementing the solution, but can't find anywhere to donate to whomever fixes this (if not you).
Thanks!

@imsnif
Copy link
Member

imsnif commented Nov 25, 2024

Hey @blixenkrone - this issue is on my to-fix (or rather, to investigate and then fix if it's a Zellij issue). My GitHub Sponsors is linked at the top of this repo and here for convenience: https://github.com/sponsors/imsnif - do with this info as you wish.

My personal opinion on bounty systems as a full-time OSS dev: please avoid bounty systems and middle-person OSS sponsoring platforms in general. Give directly to maintainers, everything else is noise at best or actively takes money out of our pockets at worst. Not going to expand on this further, just something to think about.

Thanks for your report and for caring about OSS sustainability.

@blixenkrone
Copy link
Author

@imsnif Good stuff! I'll have a look at sponsoring.

The bounty idea was mostly if we had some way to go 1:1 with user > zellij maintainer, I wasn't about to pay extra for using a middleman service. I'll be using the link you provided.

Thanks for the swift response 🙏

@Iorvethe
Copy link

Iorvethe commented Nov 30, 2024

FWIW, using the method listed here to read input events, it seems to me that the issue may lie in the fact that, for a generic key S with the aforementioned modifiers, Zellij sends Alt + Shift + s instead of Alt + S, which could be misinterpreted by applications such as Helix.

Also of note, with the procedure replicated outside Zellij (tested in both Alacritty and Kitty), the received event is Alt + S and the binding works in Helix, whereas it doesn’t inside a Zellij session.

It also seems that #3817 is a duplicate of this issue. Anyway, thanks for looking into it and working on Zellij!

Edit: the behaviour is different in Konsole where the key events sent do not change, whether in Zellij or not. Could it be linked to Kitty’s keyboard enhancement protocol, as it seems to not be supported by Konsole?

@blixenkrone
Copy link
Author

blixenkrone commented Dec 1, 2024

@Iorvethe Good thoughts, thanks for the write up.

for a generic key S with the aforementioned modifiers, Zellij sends Alt + Shift + s instead of Alt + S, which could be misinterpreted by applications such as Helix.

I think it's the inverse, but your point still stands. Different key interpretations between applications may be a reason.
I'll try to tinker a bit with the bindings to verify.

It's not a full solution though, some of vim/vimlike/helix builtin keybindings still rely on the shift modifier to work properly. We still need an implementation that ensures parity between applications keymaps.

@blixenkrone
Copy link
Author

blixenkrone commented Dec 3, 2024

In a duplicate thread #3817 a short term fix was provided by OP:
#3817 (comment)

TL;DR: Basically setting this option in config.kdl:
support_kitty_keyboard_protocol false

This seemingly does remove the keybinding issue, although I have to properly investigate.
Tested this in Ghostty and Alacritty.

While I don't believe this issue should be resolved from doing this config, I believe it can act as an intermediate solution until @imsnif or likeminded can implement a fix.

@Iorvethe
Copy link

Iorvethe commented Dec 3, 2024

Great fix, and easy to apply at that; more than enough for the time being. Thanks!

@giyany
Copy link
Member

giyany commented Dec 4, 2024

Thank you everyone for the helpful and civil discussion - this will surely help others with similar problems.
As a side note, please refrain from tagging the lead maintainers. In a project of this size, a maintainer can receive dozens of notifications daily and they make work harder, not easier. All the issues and discussions are read.

@imsnif
Copy link
Member

imsnif commented Dec 5, 2024

Hey, so I had a chance to look into this and I'm sorry to say but at least in my view this is an issue with Helix (or crossterm, depending on how you look at it).

Technical explanation

Zellij uses only the first progressive enhancement level of the Kitty Keyboard Protocol "Disambiguate escape codes" (https://sw.kovidgoyal.net/kitty/keyboard-protocol/#disambiguate). In this enhancement level, we report Alt Shift s as lowercase s with the Alt and Shift modifiers. This is what the protocol tells us to do, to quote:

"The unicode-key-code above is the Unicode codepoint representing the key, as a decimal number. For example, the A key is represented as 97 which is the unicode code for lowercase a. Note that the codepoint used is always the lower-case (or more technically, un-shifted) version of the key. If the user presses, for example, ctrl+shift+a the escape code would be CSI 97;modifiers u. It must not be CSI 65; modifiers u." (reference: https://sw.kovidgoyal.net/kitty/keyboard-protocol/#key-codes).

Helix, through crossterm, queries the terminal support for the Kitty Keyboard Protocol. Crossterm however, only returns a bool. Basically, supported/not-supported: https://github.com/crossterm-rs/crossterm/blob/fc8f9779382b7faa7f92bc40d72f8dcbd5849296/src/terminal/sys/unix.rs#L186

This means that there is no room left for the particularities of the protocol. Namely - allowing terminals to only support the first progressive enhancement as Zellij does. It's either on or off.

Helix then assumes that if the protocol is on, it can also accept additional progressive enhancement levels such as REPORT_ALTERNATE_KEYS, seen here: https://github.com/helix-editor/helix/blob/4c8175ca04dd18a74e8d1a5973042b89a381e3ce/helix-tui/src/backend/crossterm.rs#L184

In this progressive enhancement, Helix expects to receive an uppercase S in this example as explained in the original protocol: "If alternate key reporting is requested by the program running in the terminal, the terminal can send two additional Unicode codepoints, the shifted key and base layout key, separated by colons." (reference: https://sw.kovidgoyal.net/kitty/keyboard-protocol/#key-codes). But since Zellij does not support this enhancement (and indeed reports that it does not when queried), we keep sending the unshifted variant of the character with the specified Alt and Shift modifiers.

There is unfortunately not much I can do on the Zellij side here. The protocol is extremely explicit on the way the terminal (Zellij in this case) should behave, and we are complying with it.

So why does this work in...

The reason this works outside of Zellij in terminals that support the kitty keyboard protocol: likely these terminals support the relevant progressive enhancement that Helix is hard-codedly expecting.

The reason this works outside of Zellij in terminals that do not support the kitty keyboard protocol (or when disabling the protocol in Zellij): this then falls back to the classic STDIN parsing which happens to support this additional modifier.

So - what now?

I would recommend Helix (either through crossterm if possible or on its own) query the host terminal to make sure it supports both progressive enhancements it expects and only expect the behavior that the terminal advertizes it supports.

I would also recommend crossterm make the query command less ambiguous, supporting the particularities of the protocol's progressive enhancement. Either that or provide an additional query interface that queries the specific progressive enhancement levels.

I'm sorry to give such an answer - I'd rather have solved this on the Zellij-side of course rather than kick the ball down the road. But given that we're talking about a protocol that is extremely explicit about the way applications should behave, changing the Zellij behavior would be wrong here and likely cause unexpected side-effects.

@Iorvethe
Copy link

Iorvethe commented Dec 7, 2024

Thank you for your comprehensive and in-depth answer!

I agree that, as you suggested, the best way forward would be that applications have a more granular support for the protocol via crossterm (reported it here crossterm-rs/crossterm#952). They should be able to accommodate partial support by falling back to STDIN if their required subset isn’t supported, for example. Hacking around it in Helix could be a temporary solution.

Out of curiosity, are there specific reasons why the entire protocol wasn’t implemented in Zellij? Or is it because of time constraints and planned for the future?

@imsnif
Copy link
Member

imsnif commented Dec 9, 2024

They should be able to accommodate partial support by falling back to STDIN if their required subset isn’t supported, for example. Hacking around it in Helix could be a temporary solution.

Just to be clear, specifically Helix doesn't even need to do this in this case. Until crossterm implements a more granular function, they should be able to implement a similar function on their own (it's really a matter of parsing the response of the query - you can see it from any shell with echo -e "\033[?u" in supporting terminals), and then only inserting the relevant supported enhancements into the linked crossterm initialization code. Unless they implicitly rely on all kitty keyboard implementation supporting the 2nd enhancement level - they should "just work".

Out of curiosity, are there specific reasons why the entire protocol wasn’t implemented in Zellij? Or is it because of time constraints and planned for the future?

I chose to implement this protocol in order to solve the pain point of not being able to properly support multiple modifiers (in keybinds for Zellij itself and in applications running inside Zellij). This is provided by the first progressive enhancement. The rest are all nice-to-haves but they solve problems we (to the best of my knowledge) don't currently have or solve on the application level (eg. the different between Ctrl + + and Ctrl + =).

@Iorvethe
Copy link

Iorvethe commented Dec 9, 2024

Thanks again, this clears everything out. I will see to open an issue on the Helix repo as well, then. Have a nice day!

@the-mikedavis
Copy link

I wrote that function in crossterm (crossterm-rs/crossterm#732). I'm a bit confused by https://sw.kovidgoyal.net/kitty/keyboard-protocol/#detection-of-support-for-this-protocol now:

An application can query the terminal for support of this protocol by sending the escape code querying for the current progressive enhancement status followed by request for the primary device attributes. If an answer for the device attributes is received without getting back an answer for the progressive enhancement the terminal does not support this protocol.

I read this as saying that support for the protocol is binary on the terminal's side - it either fully supports the protocol or it doesn't, signaled by whether the terminal returns the current values of the flags or not. The explanation of progressive enhancement further confuses me since it seems to suggest that progressive enhancement is intended for the benefit of terminal programs (e.g. Helix) rather than a mechanism for the terminal to partially support the protocol:

... in reality there is a vast universe of existing terminal programs that expect legacy control codes for key events and that are not likely to ever be updated. To support these, in default mode, the terminal will emit legacy escape codes for compatibility. If a terminal program wants more robust key handling, it can request it from the terminal, via the mechanism described here

But you're saying that the terminal's response to CSI ? u query shows which flags it supports? I read this instead as a mechanism to query the current values of the flags and presumably these flags would all be zeroed before the client requests any enhancement.

I'm happy to make this change in crossterm and Helix but I'll need to test it out against a bunch of terminals to see if this is consistent. If that's how detection is meant to work we can send a patch to the Kitty docs as well to make this part of detection clearer.

@imsnif
Copy link
Member

imsnif commented Dec 10, 2024

Hey @the-mikedavis - thanks for popping in :)

I read this as saying that support for the protocol is binary on the terminal's side - it either fully supports the protocol or it doesn't, signaled by whether the terminal returns the current values of the flags or not.

I think this is more meant as an answer to the question "what happens if I don't get a response from the terminal to my support query" - as in, if the terminal I'm operating in doesn't know about this protocol at all - I can send this query followed by a device query and safely assume that if I got a response to the device query, I don't need to wait around for a response to the kitty keyboard query because it's not going to come.

But you're saying that the terminal's response to CSI ? u query shows which flags it supports? I read this instead as a mechanism to query the current values of the flags and presumably these flags would all be zeroed before the client requests any enhancement.

Yes, exactly. Just like terminal programs can request specific progressive enhancements, so can terminals elect to only support up to specific progressive enhancements. The query mechanism is there to communicate this difference to applications. You request the specific progressive enhancement you like with eg. echo -e "\033[>2u" and then query the terminal to see which level it's currently at with eg. echo -e "\033[?u". You'll get a response like [?0u with the digit indicating the level of support (0 being no support, 1 being the first progressive enhancement, etc.)

I'm happy to make this change in crossterm and Helix but I'll need to test it out against a bunch of terminals to see if this is consistent. If that's how detection is meant to work we can send a patch to the Kitty docs as well to make this part of detection clearer.

I'm a firm believer that protocol specifications should never change for any reason whatsoever except in a new version of the protocol, but who knows - maybe the maintainer of this protocol believes otherwise. :)

Let me know if I can be of any further assistance.

@the-mikedavis
Copy link

I misread the above post, thanks for clarifying! I thought you were saying that CSI ? u sent before any push requests would show the supported flags but I believe you're actually saying that we should be querying the currently enabled flags after pushing to see if the values we requested are now set?

@imsnif
Copy link
Member

imsnif commented Dec 10, 2024

That's what I would do, yes!

@the-mikedavis
Copy link

the-mikedavis commented Dec 16, 2024

Adding a function to crossterm to query the currently active flags should be a really small change, I don't mind sending a PR to crossterm for that. (Edit: crossterm-rs/crossterm#958)

After pushing the flags we want in Helix though and reading that "report alternate keys" is not enabled what I would prefer to do is then pop all the active flags. Without "report alternate keys" we get key events like AltShift9 for Alt( and we would need to translate that on our end. Would you be interested in adding "report alternate keys"? It should fix compatibility with Kakoune as well since AFAIK we request the same flags. I wouldn't be able to commit to a time table but I could take a look at sending a PR at some point - I bet Alacritty and WezTerm have good reference code for this feature.

The reason Helix handles this weirdly at the moment (specifically inputs like AltShifts) is that we "canonicalize" key events to always remove the shift modifier. Without the Kitty keyboard protocol enabled or with "report alternate keys" this canonicalization makes sense since the terminal sends uppercase characters but without "report alternate keys" we're dropping the shift modifier which is important. We can also make a small change on our end to make this canonicalization smarter and actually uppercase the character when dropping the shift modifier. It would be a naive uppercase though rather than what is needed to solve things like AltShift9 from above

@imsnif
Copy link
Member

imsnif commented Dec 17, 2024

Thanks for looking into this and for your work @the-mikedavis !

The issue with "report alternate keys" is that this problem is not exclusive to STDIN parsing. I guess it's the same for you. Users can bind these keys in configurations, we show them in the UI, etc. This means both of us absolutely have to solve this problem in the application layer regardless of whether we solve it on the protocol layer. I'm assuming this is the reason you need to normalize these keys. My thinking is: as long as we're already solving this on the application layer (each of us in their own way) we might as well use the same or similar normalization for whatever comes to us on the wire.

In general, integration and collaboration with the ecosystem is important to me and to Zellij, but I am personally not a fan of this protocol for reasons demonstrated very well in this thread. Adding an entire progressive enhancement to Zellij would be committing to a huge amount of complexity. Not just at the time of implementation but also for the future through continued maintenance. In hindsight, if I knew I had to implement this progressive enhancement (and who knows how many more due to similar problems in the protocol's design) I would have elected not to implement this protocol at all.

This is not a hard no on my part - I would agree to take on complexity in order to benefit our joint user-base, but unless I'm missing something major (you are of course the expert on your code-base), I think the road of least resistance (by orders of magnitude) is to normalize keypresses on the application layer on both sides rather than commit further to this protocol which will only partially solve our problem.

What do you think?

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

No branches or pull requests

6 participants