-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
Incorrect Shift / Meta / Control bits set in xterm mouse mode #8291
Comments
Whoa. You're right. How did we miss this? |
terminal/src/terminal/input/mouseInput.cpp Lines 134 to 137 in 6115f8d
terminal/src/terminal/input/mouseInput.cpp Lines 191 to 196 in 6115f8d
That's not great. |
Yup. The control fix is easy enough. It's just wrong in those comments and code. Control should add 0x10 and meta should add 0x08. |
I'm alarmed that we missed this for every windows release after (and including) 14393! It was introduced broken back then 😄 and hasn't been caught, or fixed, since. I guess I have to recommend SGR encoding for now. EDIT: I rescind the frowning face above since SGR encoding is superior in every way. |
The ECMA-48 conformant format of DEC private mode 1006 is indeed the better format of mouse report, and isn't just a "for now" recommendation, really. It's a "from now on" recommendation. Yes, a terminal emulator should get the mouse report formats of DEC private modes 9, 1005, and 1015 right if it supports them at all, but they are part of an evolutionary process that the world of XTerm and RXVT went through back around 2010 to 2012, with DEC private mode 1006, and the conseqential private control sequence CSI < M , being the upshot. Applications that use terminfo will nowadays use DEC private mode 1006 on a wide variety of terminal types, from genuine XTerm through iTerm2 and Konsole to libVTE terminal emulators, and won't in fact use DEC private modes 1005 or 1015 on any terminal type, as no entry in a modern terminfo database employs them. Terminfo applications will furthermore only use the old X10/X11 report format with just three remaining terminal types: screen, terminology, and iTerm. Interestingly, the current terminfo database doesn't yet list Windows Terminal ( When that is updated, then |
Alas, that terminfo entry was built when Terminal 0.2 shipped without considering the things supported by the underlying console host. We’ve had XM (apparently with the caveats mentioned here) for three or so years 😄 |
Unfortunately, I couldn't get mouse reporting to work at all here with 1.4.3141.0, but I suspect that terminfo is a side issue here. So see #8303. Back to |
Ah yes, the SSH client shipped inbox (calls itself 7.7) synthesizes its own VT input from console event records. By 8.1+, we’d finally convinced them to let us handle it with |
Looking inside, I'm deeply horrified by what I see.
It's almost enough to suggest that perhaps no version of Terminal has ever really supported mouse mode, and conhost has only barely done so until you held down a modifier. |
This commit also fixes an observed issue in Windows Terminal where we were passing in a console-style modifiers enum when MouseInput is expecting MK_ constants. I decided to unify MouseInput around the console-style modifier constants because they have support for META (which MK_ does not) and can differentiate between left/right alt/ctrl. Our tests are fundamentally flawed here: they use a copy of the modifier key generating logic _themselves_, so we got a bit of "error carried forward." I did not fix the tests to use known-good control sequences, I simply replaced the character generator with another copy of the modifier code. I did, however, extend them to test ctrl|meta and left/right modifiers. Fixes #8291
We had the xterm and SGR codings for meta/ctrl backwards. Oops. This commit also fixes an observed issue in Windows Terminal where we were passing in a console-style modifiers enum when MouseInput is expecting MK_ constants. I decided to unify MouseInput around the console-style modifier constants because they have support for META (which MK_ does not) and can differentiate between left/right alt/ctrl. Our tests are fundamentally flawed here: they use a copy of the modifier key generating logic _themselves_, so we got a bit of "error carried forward." I did not fix the tests to use known-good control sequences, I simply replaced the character generator with another copy of the modifier code. I did, however, extend them to test ctrl|meta and left/right modifiers. Fixes #8291
We had the xterm and SGR codings for meta/ctrl backwards. Oops. This commit also fixes an observed issue in Windows Terminal where we were passing in a console-style modifiers enum when MouseInput is expecting MK_ constants. I decided to unify MouseInput around the console-style modifier constants because they have support for META (which MK_ does not) and can differentiate between left/right alt/ctrl. Our tests are fundamentally flawed here: they use a copy of the modifier key generating logic _themselves_, so we got a bit of "error carried forward." I did not fix the tests to use known-good control sequences, I simply replaced the character generator with another copy of the modifier code. I did, however, extend them to test ctrl|meta and left/right modifiers. Fixes #8291 (cherry picked from commit b1e1c7c)
🎉This issue was addressed in #8379, which has now been successfully released as Handy links: |
🎉This issue was addressed in #8379, which has now been successfully released as Handy links: |
Environment
Steps to reproduce
Expected behavior
Per https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h2-Mouse-Tracking, we should get back six characters on a mouse button down event followed by an additional six characters on a mouse button up event. The first three characters (
ESC
[
M
) will not be displayed, only the last three characters per event get displayed - this is not a bug, it is just important to understand.Space
!
!
0
!
!
(
!
!
$
!
!
8
!
!
Actual behavior
(
!
!
as if meta is held.Space
!
!
, as if nothing is held.(
!
!
, as if only meta is held.The text was updated successfully, but these errors were encountered: