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

Scrolling in the Alternate Screen Buffer should send up/down keystrokes? #3321

Closed
yossizahn opened this issue Oct 25, 2019 · 9 comments · Fixed by #12569
Closed

Scrolling in the Alternate Screen Buffer should send up/down keystrokes? #3321

yossizahn opened this issue Oct 25, 2019 · 9 comments · Fixed by #12569
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@yossizahn
Copy link

yossizahn commented Oct 25, 2019

Description of the new feature/enhancement

Many terminals will set different scrolling behavior within the Alternate Screen Buffer such that scrolling will send up/down key strokes. This seems to make more sense than scrolling through the main buffer's history.
xterm has this as an option (set by the 'alternatescroll' Xresource), VTE based terminals have this set as default behavior.
Please consider adding this capability to Windows Terminal.

Depends on #381.

@yossizahn yossizahn added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Oct 25, 2019
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Oct 25, 2019
@egmontkob
Copy link

FYI: Konsole supports this too. In all three of these emulators, you can toggle this behavior runtime using DECSET 1007.

@zadjii-msft zadjii-msft added Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase labels Oct 25, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 25, 2019
@zadjii-msft
Copy link
Member

I'd just like to add that the vintage console already does support this mode :)

I'd imagine that our mouse support will done in pieces, with basic mouse support coming first, and this being a follow-up. Once #376 and #545 are done, this would be the next logical follow-up task.

@zadjii-msft zadjii-msft added this to the Terminal v1.0 milestone Oct 25, 2019
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 29, 2019
@zadjii-msft zadjii-msft added the Area-VT Virtual Terminal sequence support label Jan 9, 2020
@j4james
Copy link
Collaborator

j4james commented Mar 22, 2020

I'd just like to add that the vintage console already does support this mode :)

Note that there are issues with the current conhost implementation though. I just did a quick test in VIM (version 8.0.1453), and mouse wheel scrolling had no effect. There are two reasons for this:

  1. VIM doesn't actually enable alternate scroll mode (private mode 1007), and for us it's disabled by default. I'm not sure whether we'd consider this a bug or not, since Xterm has the same default. However, a lot of other emulators do have it enabled by default, (or don't even have the option of turning it off), so that's maybe something worth considering.
  2. Even if you enable the alternate scroll mode manually, it still doesn't work in VIM, because VIM turns on Cursor Keys Mode (DECCKM), and our alternate scroll implementation doesn't take that into account, so we are sending the wrong escape sequences. This is definitely a bug which we need to fix.

@j4james
Copy link
Collaborator

j4james commented Mar 23, 2020

FYI, I've submitted a PR for the DECCKM problem mentioned in point 2 above. As for point 1, I think we were probably correct in leaving the scroll mode disabled by default as Xterm has done. I just discovered there is an option you can enable in VIM which will make it handle all the mouse scrolling itself. And if we were to enable the alternate scroll mode by default, we'd just end up breaking that functionality.

ghost pushed a commit that referenced this issue Mar 23, 2020
## Summary of the Pull Request

If the _Alternate Scroll Mode_ is enabled, the terminal generates up/down keystrokes when the mouse wheel is scrolled. However, the expected escape sequences for those keys are dependent on the state of the _Cursor Keys Mode_ ( `DECCKM`), but we haven't taken that into account. This PR updates the alternate scroll implementation to make sure the appropriate sequences are sent for both `DECCKM` modes.

## References

#3321

## PR Checklist
* [ ] Closes #xxx
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

## Detailed Description of the Pull Request / Additional comments

I've simply added a condition in the `TerminalInput::_SendAlternateScroll` method to send a different pair of sequences dependent on the state of `_cursorApplicationMode`  flag.

## Validation Steps Performed

Manually tested in VIM (although that required me enabling the _Alternate Scroll Mode_ myself first). Also added a new unit test in `MouseInputTest` to confirm the correct sequences were generated for both `DECCKM` modes.
@yossizahn
Copy link
Author

FYI, I've submitted a PR for the DECCKM problem mentioned in point 2 above

Er... I'm no console guru, so I may have not totally understood the previous discussion...
@j4james, will your fix allow mouse scrolling within less?

As for point 1, I think we were probably correct in leaving the scroll mode disabled by default as Xterm has done. I just discovered there is an option you can enable in VIM which will make it handle all the mouse scrolling itself. And if we were to enable the alternate scroll mode by default, we'd just end up breaking that functionality.

Are you sure about that? I just checked with Gnome Terminal (VTE based, I believe) and mintty. And they both behave correctly whether set mouse= or set mouse=a. i.e. when set mouse= they seem to send up/down key strokes, and when set mouse=a they seem to leave scroll handling to vim.

@j4james
Copy link
Collaborator

j4james commented Mar 26, 2020

Er... I'm no console guru, so I may have not totally understood the previous discussion...
@j4james, will your fix allow mouse scrolling within less?

Yes, but only if you enable the alternate scrolling mode first (i.e. something like printf "\e[?1007h"). And note that this still only applies to conhost - it's not supported by Windows Terminal yet (that's why this issue is still open).

Are you sure about that? I just checked with Gnome Terminal (VTE based, I believe) and mintty. And they both behave correctly whether set mouse= or set mouse=a. i.e. when set mouse= they seem to send up/down key strokes, and when set mouse=a they seem to leave scroll handling to vim.

I haven't tried Gnome Terminal, but Mintty definitely behaves weirdly for me with set mouse=a. It seems like it's generating up/down key strokes as well as passing the mouse events through, so some of the time the cursor moves and some of the time the screen scrolls.

That said, I don't feel strongly about what this defaults too, and maybe there is a way to have it enabled by default and still work well with applications that handle the scrolling themselves. But for now at least we're compatible with XTerm, and anyone that prefers it on by default can still trigger that manually.

@zadjii-msft
Copy link
Member

zadjii-msft commented Feb 24, 2022

Ruh roh, did we regress this for conhost? I'm going to do this for the Terminal branched off of #12561 and it's not even working in conhost anymore 😨

Sorry for the panic. The correct thing to do here is

printf "\e[?1007h" ; man ps

That enables alternate scroll mode, then runs man. That works fine in conhost. It's been a while since I worked in this area, that's my b.

zadjii-msft added a commit that referenced this issue Feb 24, 2022
_⚠️ targets #12561 ⚠️_

"Alternate scroll mode" is a neat little mode where the app wants mouse wheel events to come through as arrow keypresses instead, when in the alternate buffer. Now that we've got support for the alt buffer in the Terminal, we can support this as well.

* [x] Closes #3321
* [x] I work here
* [ ] Tests would be nice

Tested manually with

```bash
printf "\e[?1007h" ; man ps
```
@ghost ghost added the In-PR This issue has a related PR label Feb 24, 2022
DHowett pushed a commit that referenced this issue Apr 12, 2022
This PR allows the Terminal to actually use the alt buffer
appropriately. Currently, we just render the alt buffer state into the
main buffer and that is wild. It means things like `vim` will let the
user scroll up to see the previous history (which it shouldn't).

Very first thing this PR does: updates the
`{Trigger|Invalidate}Circling` methods to instead be
`{Trigger|Invalidate}Flush(bool circling)`. We need this so that when an
app requests the alt buffer in conpty, we can immediately flush the
frame before asking the Terminal side to switch to the other buffer. The
`Circling` methods was a great place to do this, but we don't actually
want to set the circled flag in VtRenderer when that happens just for a
flush. 

The Terminal's implementation is a little different than conhost's.
Conhost's implementation grew organically, so I had it straight up
create an entire new screen buffer for the alt buffer. The Terminal
doesn't need all that! All we need to do is have a separate `TextBuffer`
for the alt buffer contents. This makes other parts easier as well - we
don't really need to do anything with the `_mutableViewport` in the alt
buffer, because it's always in the same place. So, we can just leave it
alone and when we come back to the main buffer, there it is. Helper
methods have been updated to account for this. 

* [x] Closes #381
* [x] Closes #3492
* #3686, #3082, #3321, #3493 are all good follow-ups here.
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Apr 12, 2022
ghost pushed a commit that referenced this issue Apr 12, 2022
"Alternate scroll mode" is a neat little mode where the app wants mouse wheel events to come through as arrow keypresses instead, when in the alternate buffer. Now that we've got support for the alt buffer in the Terminal, we can support this as well.

* [x] Closes #3321
* [x] I work here
* [ ] Tests would be nice

Tested manually with

```bash
printf "\e[?1007h" ; man ps
```
@zadjii-msft
Copy link
Member

This was merged in #12657, I didn't notice the two didn't get linked up.

@ghost
Copy link

ghost commented May 24, 2022

🎉This issue was addressed in #12569, which has now been successfully released as Windows Terminal Preview v1.14.143.:tada:

Handy links:

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-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants