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

CoreMouseService #2323

Merged
merged 29 commits into from
Sep 5, 2019
Merged

CoreMouseService #2323

merged 29 commits into from
Sep 5, 2019

Conversation

jerch
Copy link
Member

@jerch jerch commented Jul 18, 2019

Part one of the mouse rewrite. This PR introduces a CoreMouseService which deals with the following:

  • hold terminal mouse tracking state (active protocol and encoding)
  • proper state reset
  • interface to switch protocol/encoding
  • default protocols + encodings
  • interface to trigger/send a mouse event to the backend
  • event to announce protocol changes to frontend components

The service already simplifies alot of the code mess in Terminal.ts. The remaining mouse related parts mostly deal with event handler registration/dispatching on the DOM, which can be moved into the browser mouse service in a second PR.

Up for review.

jerch added 2 commits July 19, 2019 16:21
- use onProtocolChange
- remove mouse states from Terminal.ts
- remove selectionService from InputHandler
- fix up events in VT200 mode
- fix move event with no buttons in ANY mode
@Tyriar Tyriar added this to the 4.0.0 milestone Jul 19, 2019
@mofux
Copy link
Contributor

mofux commented Jul 19, 2019

Just tested, works fine and I haven't been able to break it. I like the way you are approaching the protocol / encoding thingy. Should we expose the triggerMouseEvent as part of the public terminal api?

@jerch
Copy link
Member Author

jerch commented Jul 20, 2019

@mofux Hmm I see two possible use cases here:

  • script automation with mouse support
    A pexpect like nodejs thingy might want to use our yet to come downstripped corelibs with mouse support. For this triggerMouseEvent would be useful. Still I never needed mouse support for this myself, the mouse is a very uncommon concept in the terminal app field (always seen as an addon only, everything should work with text input only).

  • local REPL with mouse support
    A REPL build in browser/electron might want to catch mouse events. For this the addProtocol/addEncoding might be useful, the REPL can inject custom handlers and grab the row/col translated events for its needs. Advantage here: the REPL doesnt have to deal with pixel translations and the event listener juggling. Still a very exotic use case imho. (Not sure how vscode implements the debug/task output, if its xterm.js based it could easily hook into mouse interactions and do folding etc.)

Maybe you have other use cases in mind that would justify adding mouse primitives to the public API, for those two above I dont see enough value currently.

@jerch
Copy link
Member Author

jerch commented Jul 21, 2019

Not sure yet how to deal with some of the default actions:

  • middle click - still inserts data from the clipboard (on linux)
    This should never be reported to an app with mouse events active, it might either screw the app's stdin read loop due to getting unexpected data or even leak sensitive data if the clipboard holds things like passwords.
  • right click - still opens context menu
    The default context menu does not make much sense for apps with mouse enabled. It should be bound to selection manager only (after a selection ppl might want access to the "copy" action), or if a custom context menu is in place - use some modifier escape right click to get it back.

Especially escaping mouse actions with some modifier to get the default actions back is tricky to get done right - currently the selection manager does that with Shift to get selection back. Imho the Alt modifier is meant to enable column selection (does not work for me since xfce binds window movement to it). Imho those escape modifiers need some attention/rework as well.

@Tyriar Thats the reason why I think we might need an event listener service, that does the juggling/routing of the events correctly. Currently it is hard to track the correct "listener states" since they are scattered over several places in Terminal.ts.

src/Terminal.ts Show resolved Hide resolved
src/common/Types.d.ts Outdated Show resolved Hide resolved
src/common/services/CoreMouseService.ts Outdated Show resolved Hide resolved
src/common/services/CoreMouseService.ts Show resolved Hide resolved
src/common/services/CoreMouseService.ts Show resolved Hide resolved
@textshell
Copy link

I'm not sure if the mouse move code on OS X will lead to mouse move events without correct button code, as that code seems have to workaround restrictions in the event interface.

@jerch jerch mentioned this pull request Jul 21, 2019
@jerch
Copy link
Member Author

jerch commented Jul 21, 2019

@textshell ❤️ Wow many thx for sharing your knowledge about emulators and interaction, much appreciated! Answered as good as I can, gonna have to review the button section again (for the wheel stuff and more than 3 buttons).

I'm not sure if the mouse move code on OS X will lead to mouse move events without correct button code, as that code seems have to workaround restrictions in the event interface.

Yes its stated in the JS API that it cannot report buttons pressed during move due to an OS restriction. The browsers have implemented own shims around that according to MDN and are likely to report the last one pressed. I have implemented a workaround to take that into account, still this needs serious testing under OSX with different browsers.

@textshell
Copy link

@textshell Wow many thx for sharing your knowledge about emulators and interaction, much appreciated! Answered as good as I can, gonna have to review the button section again (for the wheel stuff and more than 3 buttons).

Thanks. I've spend a lot of time staring at code of terminal implementations and we really need to share our knowledge in this space. Also i fundamentally believe it's best if terminals behave the same except if there is a good reason to differ.

Feel free to mention me when it's unclear what terminal sequences do exactly in other terminals. Although i have not yet covered enough terminals i try to document what i learn over at https://terminalguide.netlify.com/. For newly invented sequences of terminals i've not yet covered i try to at least have a short note pointing to external resources, so it would be nice to know when xterm.js invents new sequences as well (via mention or issue/pr in terminalguide's github repository). But of course new sequences are also a topic for terminal-wg.

@Tyriar
Copy link
Member

Tyriar commented Jul 22, 2019

so it would be nice to know when xterm.js invents new sequences as well

@textshell I don't think this would ever happen outside of a proposal on terminal-wg

@jerch
Copy link
Member Author

jerch commented Jul 23, 2019

Feel free to mention me when it's unclear what terminal sequences do exactly in other terminals.

Sure thing, we still have to catch up at several edges. (Just started to cleanup many of the partly faulty implemented VT commands.)

... so it would be nice to know when xterm.js invents new sequences as well ...

As @Tyriar already pointed out, we are not much interested in doing fancy private stuff on our own, still we leave the window open for integrators to do so (via customizable OSC and CSI entries, DCS and ESC yet to come #2332).

@jerch
Copy link
Member Author

jerch commented Jul 23, 2019

@textshell Added AUX button support to the core libs, but had to leave out support for those in the frontend part, since I cannot get reliable values from other terminals. Tested with a 5 button mouse, seems all emulators I have dont agree here on what to send:

  • xterm Ubuntu 18 stock package (v330): sends 8 resp. 9 for for button 4 and 5, thats pretty broken imho (clashes with modifier)
  • xterm recompiled (v346): sends nothing for those buttons (Missed a compile flag?)
  • gnome-terminal Ubuntu stock package: sends same codes as for button 1

Thats to much of cutting edge for me atm, thus I limit the button support to 3 buttons + wheel for now, which seems to be somewhat uniform across the emulators.

Also added wheelLeft and wheelRight support, if you have a mouse capable to do this, maybe you can test it? Otherwise I gonna disable this as well for now.

@jerch
Copy link
Member Author

jerch commented Jul 23, 2019

Tested mouse move with several buttons pressed under Mojave in Safari - works and reports the same button as under Linux (lowest button).

@textshell
Copy link

@textshell Added AUX button support to the core libs, but had to leave out support for those in the frontend part, since I cannot get reliable values from other terminals. Tested with a 5 button mouse, seems all emulators I have dont agree here on what to send:

  • xterm Ubuntu 18 stock package (v330): sends 8 resp. 9 for for button 4 and 5, thats pretty broken imho (clashes with modifier)

Yes that was fixed later.

  • xterm recompiled (v346): sends nothing for those buttons (Missed a compile flag?)

That's odd. I get ^[[<128;29;19M^[[<128;29;18m^[[<129;27;18M^[[<129;27;18m in xterm 346 for clicking and releasing first the left side button and then the right side button.

My debian inspired compile options are

./configure --enable-logging --enable-wide-chars --enable-luit --enable-256-color --disable-imake --enable-narrowproto --enable-exec-xterm --enable-dabbrev --enable-backarrow-is-erase --enable-sixel-graphics --with-utempter 'CPPFLAGS=-DDEF_ALLOW_FONT=False -DDEF_ALLOW_TCAP=False -DDEF_DISALLOWED_WINDOW="1,2,3,4,5,6,7,8,9,11,13,14,18,19,20,21,GetSelection,SetSelection,SetWinLines,SetXprop"' 'CPPFLAGS=-DDEF_ALLOW_FONT=False -DDEF_ALLOW_TCAP=False -DDEF_DISALLOWED_WINDOW=\"1,2,3,4,5,6,7,8,9,11,13,14,18,19,20,21,GetSelection,SetSelection,SetWinLines,SetXprop\"'
  • gnome-terminal Ubuntu stock package: sends same codes as for button 1

Thats to much of cutting edge for me atm, thus I limit the button support to 3 buttons + wheel for now, which seems to be somewhat uniform across the emulators.

Also added wheelLeft and wheelRight support, if you have a mouse capable to do this, maybe you can test it? Otherwise I gonna disable this as well for now.

I don't have a mouse with left/right scrolling handy and my usual remapping tricks don't seem to work in firefox. So i can't test this.

@mofux
Copy link
Contributor

mofux commented Jul 24, 2019

@jerch My use-case for making triggerMouseEvent public would be for my custom monaco renderer. I could easily capture and translate the mouse events inside the monaco-editor and then send them to xterm.js. At the moment I'm monkey-patching some xterm.js internals to get this working. I think your use-cases are also valid.

@jerch jerch mentioned this pull request Jul 24, 2019
@jerch
Copy link
Member Author

jerch commented Jul 24, 2019

@textshell Thx, your compile settings work for me too. Had DEC locator on before, maybe those dont work in combination, did not investigate. Now I can test at least button 4 and 5.

@mofux I am not against adding some mouse stuff to the API. Still my primary goal for this PR is to get the core stuff working as close as possible to the standard and to fix the most obvious errors. Maybe we could discuss that in a separate issue/PR? Same with the escape rules for the standard actions.

Created new issues for that:

@jerch
Copy link
Member Author

jerch commented Jul 24, 2019

@textshell Successfully tested button 4 and 5 thanks to your compile string. Also left/right wheel works (just replaced up/down with those to see if it gets reported correctly, so direction might be wrong).

Still I deactivated support for both for these reasons:

  • Button 4 and 5 are typically used for history back and forward navigation, there is no straight forward way to supress/intercept those standard actions, unless we want to mess with navigation entries (no we dont want to do that as it would screw up the browser state).
  • Support for higher buttons does not work in some platform/browser combinations. Seems Firefox cannot report button 4 and 5 at all for me (its stated at MDN that this is a limitation of Gtk platforms).
  • Left/right wheel was not really tested (beside my hack above).

I think we are on the safe side with the current 3 button + wheel restriction. It also shares the best conformance to other emulators.

Somewhat offtopic:
I think we have to come back to the mouse sooner or later in terminal-wg, once we managed to get some default image handling rolling, a better more capable mouse reporting sequence (different metrics?) might be an issue. Imho DEC locator should be buried with its really awkward interface.

Once again, up for review.

@jerch
Copy link
Member Author

jerch commented Jul 25, 2019

@Tyriar Btw before we can add this PR we should have #2321 merged, so I can cherrypick the tests and finish the missing bits that are not working in master (like DECSET 1003 and the modifiers).

@textshell
Copy link

Still I deactivated support for both for these reasons:

  • Button 4 and 5 are typically used for history back and forward navigation, there is no straight forward way to supress/intercept those standard actions, unless we want to mess with navigation entries (no we dont want to do that as it would screw up the browser state).

I'm not an expert on these things, but the randomly googled mouse event test page does not have any trouble handling them in chromium. I just tested in firefox and there it does not capture those events. So that seems to be browser dependent.

  • Support for higher buttons does not work in some platform/browser combinations. Seems Firefox cannot report button 4 and 5 at all for me (its stated at MDN that this is a limitation of Gtk platforms).
  • Left/right wheel was not really tested (beside my hack above).

If someone cares enough there will be someone to test this with real hardware.

I think we are on the safe side with the current 3 button + wheel restriction. It also shares the best conformance to other emulators.

The 3 buttons, wheel up and down plus a hopefully correct but disabled implementation for the rest should be pretty safe, yes.

Somewhat offtopic:
I think we have to come back to the mouse sooner or later in terminal-wg, once we managed to get some default image handling rolling, a better more capable mouse reporting sequence (different metrics?) might be an issue. Imho DEC locator should be buried with its really awkward interface.

Yes, i have some mouse wishes as well. But they are more in the direction of coexistance of the terminal functions users expect and having some kind of mouse support in the application at the same time. But that's for another time and for terminal-wg indeed.

Once again, up for review.

No problem.

@jerch
Copy link
Member Author

jerch commented Jul 26, 2019

@textshell Thx again for helping getting this done 👍

@jerch
Copy link
Member Author

jerch commented Aug 1, 2019

Merged in the mouse integration tests. Couple of changes applied:

  • had to revert the wheel event code to the previous puppeteer variants (due to modifiers not working correctly)
  • already removed most faulty tests and bug comments
  • beside Shift overwrites by selection manager modifiers work now and can be tested

Left to do:

  • DECSET 1003 with plain move event tests
  • deside how to deal with wanted/unwanted standard actions and possible modifier overwrites (issue mouse standard actions #2337)

I think I gonna skip UTF8 and URXVT tests for now to lower the runtime of the mouse tests. They are likely to be removed anyway (#2334).

@jerch
Copy link
Member Author

jerch commented Aug 1, 2019

@Tyriar Basically done with this PR. Imho we should address the remaining issues #2334 and #2337 separately. Also the coord cap at 95 cannot be resolved before we fixed our onData payload to support binary stuff (#2326).

Up for review.

@jerch jerch self-assigned this Aug 4, 2019
src/common/services/Services.ts Show resolved Hide resolved
src/InputHandler.ts Show resolved Hide resolved
src/Terminal.ts Outdated Show resolved Hide resolved
src/InputHandler.ts Show resolved Hide resolved
src/Terminal.ts Outdated Show resolved Hide resolved
src/Terminal.ts Show resolved Hide resolved
src/Terminal.ts Show resolved Hide resolved
src/Terminal.ts Outdated Show resolved Hide resolved
src/Terminal.ts Show resolved Hide resolved
src/Terminal.ts Show resolved Hide resolved
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't do much testing in this area but tmux mouse mode to move panes seems to work. LGTM 👍

@jerch jerch merged commit bc5e1e6 into xtermjs:master Sep 5, 2019
@jerch jerch deleted the mouse_services branch September 5, 2019 19:08
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

Successfully merging this pull request may close these issues.

4 participants