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.

Sign up for GitHub

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

Jump to bottom

LF shouldn't scroll the viewport when below the bottom margin #2657

Closed
j4james opened this issue Sep 4, 2019 · 1 comment · Fixed by #3704
Closed

LF shouldn't scroll the viewport when below the bottom margin #2657

j4james opened this issue Sep 4, 2019 · 1 comment · Fixed by #3704
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@j4james
Copy link
Collaborator

j4james commented Sep 4, 2019

Environment

Windows build number: Version 10.0.18362.239

Steps to reproduce

Open a WSL conhost shell, and execute the following command:

printf "\e[1;2r\e[999HBottom line?\n\n\n\e[r"

This sets the DECSTBM margins to the top two lines, moves the cursor position to the very bottom of the screen (i.e. below the bottom margin), and then writes out "Bottom line?" followed by 3 line feeds, and finally resets the margins.

Expected behavior

The line feeds should not cause the viewport to scroll when below the bottom margin, so the "Bottom line?" text should remain on the last line of the screen.

This is what the above test case looks like in XTerm:

image

And here's an extract from the DEC STD 070 documentation for the LF control, explaining that behaviour:

If the Active Position is below the Bottom Margin when this control is executed (as the result of absolute cursor positioning) it will still move downward by one line and no scrolling will occur. In this case, the Active Position will not move beyond the bottom line of the display.

Actual behavior

In the Windows console, the line feeds cause the viewport to scroll up, just as they would if there were no margins set, so the "Bottom line?" actually ends up 3 lines from the bottom, and the top line has scrolled out of view.

image
@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 Sep 4, 2019
@bitcrazed bitcrazed added Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase and removed Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 9, 2019
@j4james j4james mentioned this issue Nov 26, 2019
Merged
3 tasks
@ghost ghost added the In-PR This issue has a related PR label Nov 26, 2019
@ghost ghost closed this as completed in #3704 Dec 11, 2019
ghost pushed a commit that referenced this issue Dec 11, 2019
@j4james
Don't allow a linefeed to scroll when outside DECSTBM margins (#3704)
6f21f30 
## Summary of the Pull Request

When the `DECSTBM` margins are set, scrolling should only allowed within
those margins. So if the cursor is below the bottom margin, it should
just be clamped when it tries to move down from the bottom line of the
viewport, instead of scrolling the screen up. This PR implements that
restriction, i.e. it prevents scrolling taking place outside the
`DECSTBM` margins, which was previously allowed.

## PR Checklist
* [x] Closes #2657
* [x] CLA signed.
* [x] Tests added/passed

## Detailed Description of the Pull Request / Additional comments

This simply adds a condition in the `AdjustCursorPosition` function to
check if the cursor is moving below the bottom of the viewport when the
margins are set. If so, it clamps the y coordinate to the bottom line of
the viewport.

The only time it's acceptable for scrolling to happen when margins are
set, is if the scrolling is taking place within those margins. But those
cases would already have been handled earlier in the function (in the
`fScrollDown` or `scrollDownAtTop` conditions), and thus the y
coordinate would have already been prevented from moving out of the
viewport.

## Validation Steps Performed

I've added some screen buffer tests to confirm this new behaviour, and
I've also checked the test case from the initial bug report (#2657) and
made sure it now matches the results in XTerm.
@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 Dec 11, 2019
@ghost
Copy link

ghost commented Jan 14, 2020

🎉This issue was addressed in #3704, which has now been successfully released as Windows Terminal Preview v0.8.10091.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Don't allow a linefeed to scroll when outside DECSTBM margins j4james/terminal
2 participants
@bitcrazed @j4james