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

mouse events are sent incorrectly in win32-input-mode #15083

Open
unxed opened this issue Apr 2, 2023 · 21 comments
Open

mouse events are sent incorrectly in win32-input-mode #15083

unxed opened this issue Apr 2, 2023 · 21 comments
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase Product-Conpty For console issues specifically related to conpty
Milestone

Comments

@unxed
Copy link

unxed commented Apr 2, 2023

Steps to reproduce

  1. Compile this test app using gcc under WSL:
#include <termios.h> 
#include <assert.h>
#include <errno.h>
#include <signal.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <stdlib.h> 
#include <stdio.h> 
#include <unistd.h> 


int main()
{
    struct termios _ts = {};
	int _ts_r = tcgetattr(1, &_ts);
	if (_ts_r == 0) {
		struct termios ts_ne = _ts;
		//ts_ne.c_lflag &= ~(ECHO | ECHONL);
		cfmakeraw(&ts_ne);
		if (tcsetattr(1, TCSADRAIN, &ts_ne ) != 0) {
			perror("TTYBackend: tcsetattr");
		}
	}

	fprintf(stderr, "\x1b[?1049h");
	fprintf(stderr, "\x1b[?1000h");
	fprintf(stderr, "\x1b[?1001h");
	fprintf(stderr, "\x1b[?1002h");

	fprintf(stderr, "\x1b[?9001h");

	for (;;) {
		char ch = 0;
		if (read(0, &ch, 1) <= 0) break;
		if (ch <= 0x20 || ch == 0x7f) {
			if (ch == 0x1b) fprintf(stderr, " ");
			fprintf(stderr, "\\x%02x", (unsigned int)(unsigned char)ch);
		} else {
			fprintf(stderr, "%c", ch);
		}
		if (ch == 0x0d) break;
    	if (ch == 0x0a) break;
	}
	fprintf(stderr, "\x1b[?1049l");
	fprintf(stderr, "\x1b[?1000l");
	fprintf(stderr, "\x1b[?1001l");
	fprintf(stderr, "\x1b[?1002l");

	fprintf(stderr, "\x1b[?9001l");

	if (tcsetattr(1, TCSADRAIN, &_ts ) != 0) {
		perror("TTYBackend: tcsetattr");
	}
}
  1. Run it under Windows Terminal

  2. Move mouse over the terminal window.

Expected Behavior

We should see mouse movement escape sequences (\x1b[M... format)

Actual Behavior

We see win32-input-mode escape sequences (\x1b[A;B;C;D;E;F_ format)

изображение

@unxed unxed 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 Apr 2, 2023
@zadjii-msft zadjii-msft added Product-Conhost For issues in the Console codebase Area-Input Related to input processing (key presses, mouse, etc.) Area-VT Virtual Terminal sequence support labels Apr 4, 2023
@zadjii-msft zadjii-msft added this to the Terminal v1.18 milestone Apr 4, 2023
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Apr 4, 2023
@DHowett
Copy link
Member

DHowett commented Apr 5, 2023

This is very much probably a corner case we never expected. We didn't actually vet win32 input mode for compatibility with applications (!) as it was originally a stopgap for getting our two terminal emulators to agree on how to handle key events.

This explanation is going to use some terms specific to this codebase.

When we generate a mouse event, we serialize it into a set of key events in InputStateMachineEngine for maximal compatibility with Win32 console applications (which are expecting a Windows API called ReadConsoleInput to return events rather than a string).

It seems we're taking those key events re-encoding them as win32 input events again. From one capture, I see this:

\x1b[0;0;27;1;0;1_   -> \x1b  (pressed)
\x1b[0;0;91;1;0;1_   -> [     (pressed)
\x1b[0;0;77;1;0;1_   -> M     (pressed)
\x1b[56;9;56;1;0;1_  -> 8     (pressed)
\x1b[56;9;56;0;0;1_  -> 8     (released)
\x1b[0;0;27;1;0;1_   -> \x1b  (pressed)
\x1b[0;0;91;1;0;1_   -> [     (pressed)
\x1b[0;0;77;1;0;1_   -> M     (pressed)
\x1b[16;42;0;1;16;1_ -> ???   (pressed)
\x1b[51;4;35;1;16;1_ -> #     (pressed)
\x1b[51;4;35;0;16;1_ -> #     (released)
\x1b[16;42;0;0;0;1_  -> ???   (released)

Which is, of course, entirely bogus.

You're also going to see this for any DSR-CPR, any X11 focus mode, and any other types of answerback.

Excellent find!

@DHowett DHowett added the Product-Conpty For console issues specifically related to conpty label Apr 5, 2023
@unxed
Copy link
Author

unxed commented Apr 6, 2023

Guessed the reason is something like it. Also was very surprised to see other escape sequences packed in the format of win32-input-mode :) I'm glad you're ready to solve it!

As for the use of this protocol in apps, there is a very interesting situation. To date, there are several protocols for extended keyboard support, and each has its own imperfections. kovidgoyal's kitty protocol looks overcomplicated. iTerm2 protocol is strongly tied to MacOS. far2l terminal extensions protocol is not yet very popular outside of far2l (although three applications already support it; I would be glad if you find an opportunity to implement it: it is nice and beautiful, and its keyboard part is based on the same KEY_EVENT_RECORD structure as win32-input-mode).

As a result, I fully admit that it is the win32-input-mode protocol that can become the de facto standard or one of standards.

Yes, I know that there are limitations associated with the fact that virtual scan codes are tied to keyboard layouts that the application on the other side of the terminal does not know anything about. Not a big problem — they are anyway not needed for anything other than distinguishing between the right and left Shift keys as there is a separate field for Unicode characters, and virtual key codes are suitable for implementing hotkeys that work independently of keyboard layout selected.

@unxed
Copy link
Author

unxed commented Apr 6, 2023

Actually, far2l terminal extensions were designed to solve exactly the same problem as win32-input-mode: how to put all the power of the Windows console API (working with the clipboard, full support for keyboard input, etc) into the terminal escape sequences.

@j4james
Copy link
Collaborator

j4james commented Apr 6, 2023

Actually, far2l terminal extensions were designed to solve exactly the same problem as win32-input-mode.

@unxed Note that the file you linked above is GPL-licensed source code, which I think can be legally problematic for some people to view.

@unxed
Copy link
Author

unxed commented Apr 6, 2023

Oh, I certainly missed that, thanks!

@elfmz can you please as the only author change the license of FarTTY.h to 3-terms BSD? (looked into the commit list, I am also there, and I agree to relicensing under 3-terms BSD)

@unxed
Copy link
Author

unxed commented Apr 6, 2023

Also some (still incomplete, but better then nothing) far2l terminal extensions documentation written by me is available here:
cyd01/KiTTY#74 (comment)

Also you may take a look at the code of pull request implementing most important parts of far2l extensions in cyd01's kitty terminal and ssh client. It's a fork of putty, and putty is MIT licensed, so I guess it is totally legal to view such code even if you are developing some proprietary software.

Implementation itself:
cyd01/KiTTY#357
Some important fixes:
https://github.com/cyd01/KiTTY/pulls?q=is%3Apr+author%3Aunxed

You can also search for "far2l" in commit log of Turbo Vision by @magiblot, it has implementation of same important parts of far2l tty extensions (clipboard, advanced keyboard support) on the side of TUI console app framework.
https://github.com/magiblot/tvision
far2l-releated code is under MIT:
https://github.com/magiblot/tvision/blob/master/COPYRIGHT

@magiblot
Copy link

magiblot commented Apr 6, 2023

Thanks for the mention, @unxed. I didn't know about win32-input-mode and it would be very interesting to support it in Turbo Vision.

Note that the FarTTY.h file you were talking about is already distributed under Public Domain terms since elfmz/far2l@70472d3.

@j4james
Copy link
Collaborator

j4james commented Apr 6, 2023

Note that the FarTTY.h file you were talking about is already distributed under Public Domain terms

Ah sorry. My bad. I just looked at the project home page and when I saw GPL I didn't look any further.

@unxed
Copy link
Author

unxed commented Apr 8, 2023

kovidgoyal's kitty protocol looks overcomplicated

In fact, I slightly exaggerated the complexity of kovidgoyal's kitty keyboard protocol. Converting its escape sequences into a KEY_EVENT_RECORD is not so difficult. I proposed such code as PR to far2l, but as it is GPL licensed, I am publishing a parsing function alone (s is a C string with source kitty's esc sequence, l is a number of chars to parse) under CC0 license just for your interest:
https://gist.github.com/unxed/8649a6223e28312eb0df9ac11660b1d8

magiblot added a commit to magiblot/tvision that referenced this issue Sep 11, 2023
@zadjii-msft zadjii-msft added the Help Wanted We encourage anyone to jump in on these. label Oct 4, 2023
@zadjii-msft zadjii-msft modified the milestones: Terminal v1.19, Backlog Oct 4, 2023
@magiblot
Copy link

Given the advantages win32-input-mode provides, I chose to add support for it in Turbo Vision, including a work-around for the re-encoded mouse events this issue is about.

This is especially useful for non-Windows applications running in WSL or via SSH. However, it is likely not the ultimate solution to the problem of extended keyboard support, given that if an application which enabled win32-input-mode gets unexpectedly killed or the connection breaks, then the current shell may become permanently unusable (it has happened a few times to me as I was testing). As I understand, this is not a problem win32-input-mode is intended to address. Although I also wonder if this can be solved at the protocol level.

@unxed
Copy link
Author

unxed commented Mar 14, 2024

I chose to add support for it in Turbo Vision, including a work-around for the re-encoded mouse events this issue is about

Exact commit:
magiblot/tvision@ae1a685

@atolismesh
Copy link

atolismesh commented Mar 17, 2024

I looked in the WinDbg debugger to see what was going on. Spoils mouse messages OpenConsole.exe from Windows Terminal, not conhost.exe as I thought before. Something like the following happens: One part of the program reads events from the keyboard, mouse, etc., then puts them in an internal event queue marked with the event type. Then this queue is processed and, depending on the selected terminal emulation, it is converted into a CSI etc sequence. Mouse sequences are correctly converted for win32input to the ESC[Mbxy format we need. The event type marks are already lost in this case. All of this is queued up for output.
But then, in a different thread (second handler) VtInputThread::DoReadInput(), the "write queue" starts reading from this queue, but there's no info about the event type anymore. The handler just sees ESC and converts it to CSI again, thinking it's keyboard input. So the results get sent to our terminal and far2l.Yeah, if we'd totally disabled the second one, it would've helped us, since the first one already had everything encoded for us.

@unxed
Copy link
Author

unxed commented Mar 19, 2024

Fixed on far2l's side:
elfmz/far2l#2085

Thanks @magiblot for inspiration!

@BDisp
Copy link

BDisp commented Aug 19, 2024

Sorry for using this issue to relate that the mouse events are not sent when the mouse is outside the top of the WT, no mater I'm using win32-input-mode or using virtual terminal sequences. It only send events inside and outside left, bottom and right, but not when it's outside of the top window.
With the conhost this doesn't happens, so I imagine that it's only a WT issue. Thanks for your attention.

@DHowett
Copy link
Member

DHowett commented Aug 19, 2024

Sorry for using this issue to relate that the mouse events are not sent when the mouse is outside the top of the WT, no mater I'm using win32-input-mode or using virtual terminal sequences. It only send events inside and outside left, bottom and right, but not when it's outside of the top window. With the conhost this doesn't happens, so I imagine that it's only a WT issue. Thanks for your attention.

@BDisp Please just file a new issue. It is just as much work as adding a comment to an existing unrelated issue.

@BDisp
Copy link

BDisp commented Aug 19, 2024

@BDisp Please just file a new issue. It is just as much work as adding a comment to an existing unrelated issue.

I opened this issue #17744. It gave me much more work but worth it. Thanks.

@unxed
Copy link
Author

unxed commented Oct 5, 2024

Should be fixed in latest Pre-release (v1.22.2702.0), see
https://github.com/microsoft/terminal/blob/v1.22.2702.0/src/terminal/adapter/IInteractDispatch.hpp

In latest Release (v1.21.2701.0) still don't,
https://github.com/microsoft/terminal/blob/v1.21.2701.0/src/terminal/adapter/IInteractDispatch.hpp

WriteStringRaw presence is marker of fix applied.

@DHowett
Copy link
Member

DHowett commented Oct 5, 2024

WriteStringRaw presence is marker of fix applied.

WriteStringRaw is also an indicator of the completely rewritten input engine in 1.22, so it’s not a straightforward backport. We aren’t intending to fix this for 1.21.

@unxed
Copy link
Author

unxed commented Oct 5, 2024

Thanks for fixing this anyway!

@unxed
Copy link
Author

unxed commented Oct 8, 2024

Clipboard paste problem in win32-input-mode is caused by the same input events double encoding issue:
#17656

@unxed
Copy link
Author

unxed commented Oct 12, 2024

Unfortunately, the mouse and double encoding bug in WT-1.22.2702 is still present, although it manifests differently and can be worked around.

I performed three tests. In all screenshots, the embedded Far2l GUI terminal at the top serves as a reference implementation of win32-input-mode, which doesn't suffer from this bug. The Windows Terminal is at the bottom.

The first test uses the X10 mouse protocol. Here, WT doesn't wrap the \x1b[M marker in win32-input-mode, but for some reason encodes the command parameters.

The second test enables SGR extended mouse reporting. And what do we see? The commands are transmitted exactly as they should be, without any extra transformations.

The third test uses bracketed paste. Predictably, the start and end paste markers are not encoded, but the content is.

Apparently, WT mistakenly interprets \x1b[M as some other code at some stage, and treats its parameters as regular character input. Since everyone uses SGR for mouse input now, the bug is insignificant, and if console program has a problem with this, it's easier for it to implement SGR than to create workarounds for double decoding.

For the tests, the source code from the ticket's initial message was used, with the addition of necessary instructions to support mouse move, mouse drag, SGR extended mouse reporting, and bracketed paste.

fprintf(stderr, "\x1b[?1002h");
fprintf(stderr, "\x1b[?1003h");
fprintf(stderr, "\x1b[?1006h");
fprintf(stderr, "\x1b[?2004h");

Image
Image
Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase Product-Conpty For console issues specifically related to conpty
Projects
None yet
Development

No branches or pull requests

7 participants