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

CUU and CUD should not move "across" margins #2929

Closed
j4james opened this issue Sep 27, 2019 · 6 comments · Fixed by #2996
Closed

CUU and CUD should not move "across" margins #2929

j4james opened this issue Sep 27, 2019 · 6 comments · Fixed by #2996
Assignees
@zadjii-msft
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) 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.
Milestone
20H1

Comments

@j4james
Copy link
Collaborator

j4james commented Sep 27, 2019

Environment

Windows build number: Version 10.0.18362.295
Also tested with a recent commit 7faf334

Steps to reproduce

In a conhost WSL shell, execute the following command:

echo -e "\e[6;19r\e[24H\e[99ACUU\e[1H\e[99BCUD\e[r"

This does the following:

  • sets the top and bottom DECSTBM margins to 6 and 19
  • moves to line 24 (i.e. below the bottom margin)
  • executes the CUU sequence with a count of 99, to move up 99 lines
  • writes out CUU
  • moves to line 1 (i.e. above the top margin)
  • executes the CUD sequence with a count of 99, to move down 99 lines
  • writes out CUD

Expected behavior

Quoting from the DEC STD 070 manual:

If the Active Position is at or below the Top Margin when the CUU control is executed, and an attempt is made to move the Active Position above the Top Margin, the control will be executed until the Active Position reaches the Top Margin. The Active Position will not move beyond the Top Margin.

Similarly for the CUD command:

If the Active Position is at or above the Bottom Margin when the CUD control is executed, and an attempt is made to move the Active Position below the Bottom Margin, the control will be executed until the Active Position reaches the Bottom Margin. The Active position will not move beyond the Bottom Margin.

So I would expect the CUU sequence to stop on line 6, the top margin, and the CUB sequence to stop on line 19, the bottom margin.

This is what the output looks like in XTerm:

image

Actual behavior

In the Windows console, the movement isn't constrained by the margins. As a result, the CUU sequences ends up at the top of the viewport, and the CUD sequences ends up at the bottom.

image

Note that this isn't a regression of #170. We do get it right if the initial cursor position was inside the margins. It's just when it starts off outside the margins that it's not being constrained.

The complication is that you sometimes don't want the cursor position constrained. For example, if you're moving up (with CUU), and you started off above the top margin, then there's no need to constrain the position below that margin.
@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 27, 2019
@DHowett-MSFT DHowett-MSFT 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-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 27, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 27, 2019
@DHowett-MSFT
Copy link
Contributor

Are CNL and CPL analogous to CUD and CUU? Should they share an implementation? If not, how do they differ? It seems like this and bug #2926 are asking for the same resolution.

@j4james
Copy link
Collaborator Author

j4james commented Sep 27, 2019

They are quite similar - I think the only difference is that CNL and CPL also perform a carriage return - so there's definitely room for code sharing between the two groups. The current implementations are completely separate, though. And while CUD and CUU gets the margins right some of the time, CNL and CPL perform no margin checking at all.

@egmontkob
Copy link

Just a side note:

Origin Mode (DECOM) (CSI ? 6 high/low) is also relevant. When enabled, as far as I know the cursor is always guaranteed to stay within the scroll region. (Also, as its name implies, the origin for absolute cursor placement is different.)

The original report here showcases what's supposed to happen when DECOM is disabled (the default).

While fixing it, could you guys please also double check the behavior with DECOM enabled?

@zadjii-msft zadjii-msft added this to the 20H1 milestone Sep 27, 2019
@zadjii-msft
Copy link
Member

This seems like a pretty well-scoped, manageable fix. I'd even consider it for 20H1, considering it's just loosening a margins.InBounds() to a > margins.Top() / < margins.Bottom() that's pseudocode check.

@zadjii-msft zadjii-msft added the Priority-2 A description (P2) label Sep 27, 2019
@zadjii-msft zadjii-msft self-assigned this Sep 30, 2019
@zadjii-msft zadjii-msft mentioned this issue Sep 30, 2019
Merged
3 tasks
@ghost ghost added the In-PR This issue has a related PR label Sep 30, 2019
DHowett-MSFT pushed a commit that referenced this issue Oct 1, 2019
@zadjii-msft @DHowett
do not allow CUU and CUD to move "across" margins. (#2996)
0ce08af 
If we're moving the cursor up, its vertical movement should be stopped
at the top margin. It should not magically jump up to the bottom margin.
Similarly, this applies to moving down and the bottom margin.
Furthermore, this constraint should always apply, not just when the
start position is within BOTH margins

Fixes #2929.
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements 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 Oct 1, 2019
DHowett-MSFT pushed a commit that referenced this issue Oct 3, 2019
@zadjii-msft @DHowett
do not allow CUU and CUD to move "across" margins. (#2996)
c0ee499 
If we're moving the cursor up, its vertical movement should be stopped
at the top margin. It should not magically jump up to the bottom margin.
Similarly, this applies to moving down and the bottom margin.
Furthermore, this constraint should always apply, not just when the
start position is within BOTH margins

Fixes #2929.
@ghost
Copy link

ghost commented Oct 4, 2019

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

Handy links:

@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 4, 2019
DHowett-MSFT pushed a commit that referenced this issue Oct 17, 2019
@zadjii-msft @DHowett
do not allow CUU and CUD to move "across" margins. (#2996)
28115f7 
If we're moving the cursor up, its vertical movement should be stopped
at the top margin. It should not magically jump up to the bottom margin.
Similarly, this applies to moving down and the bottom margin.
Furthermore, this constraint should always apply, not just when the
start position is within BOTH margins

Fixes #2929.

(cherry picked from commit 0ce08af)
DHowett-MSFT pushed a commit that referenced this issue Oct 17, 2019
@DHowett
Merged PR 3896152: Merge selected github changes up to dd2fbef
62f5299 
- Fix snapping to the cursor in "Terminal Scrolling" mode (GH-2705)

fixes GH-1222

  PSReadline calls SetConsoleCursorPosition on each character. We try to snap.

  However, we'd only ever do this with the visible viewport, the viewport
  defined by `SCREEN_INFORMATION::_viewport`. When there's a virtual viewport in
  Terminal Scrolling mode, we actually need to snap the virtual viewport, so
  that this behavior looks more regular.

(cherry picked from commit 6f4b98a)
- Passthrough BEL in conpty (GH-2990)

fixes GH-2952.

(cherry picked from commit 6831120)
- make filling chars (and, thus, erase line/char) unset wrap (GH-2831)

EraseInLine calls `FillConsoleOutputCharacterW()`. In filling the row with
chars, we were setting the wrap flag. We need to specifically not do this on
ANY _FILL_ operation. Now a fill operation UNSETS the wrap flag if we fill to
the end of the line.

Originally, we had a boolean `setWrap` that would mean...
- **true**: if writing to the end of the row, SET the wrap value to true
- **false**: if writing to the end of the row, DON'T CHANGE the wrap value

,- current wrap value
| ,- fill last cell?
| | ,- new wrap value
| | | ,- comments
|-|-|-|
|1|0|0| THIS CASE WAS UNHANDLED

To handle that special case (1-0-0), we need to UNSET the wrap. So now, we have
~setWrap~ `wrap` mean the following:
- **true**: if writing to the end of the row, SET the wrap value to TRUE
- **false**: if writing to the end of the row, SET the wrap value to FALSE
- **nullopt**: leave the wrap value as it is

Closes GH-1126

(cherry picked from commit 4dd9f9c)
- When reverse indexing, preserve RGB/256 attributes (GH-2987)

(cherry picked from commit 847d6b5)
- do not allow CUU and CUD to move "across" margins. (GH-2996)

If we're moving the cursor up, its vertical movement should be stopped
at the top margin.
Similarly, this applies to moving down and the bottom margin.

Fixes GH-2929.

(cherry picked from commit 0ce08af)
- Prevent the v1 propsheet from zeroing colors, causing black text on black background. (GH-2651)

  fixes GH-2319

(cherry picked from commit b97db63)
- Render the cursor state to VT (GH-2829)

(cherry picked from commit a9f3849)
- Don't put NUL in the buffer in VT mode (GH-3015)

(cherry picked from commit a82d6b8)
- Return to ground when we flush the last char (GH-2823)

The InputStateMachineEngine was incorrectly not returning to the ground
state after flushing the last sequence. That means that something like
alt+backspace would leave us in the Escape state, not the ground state.
This makes sure we return to ground.

Additionally removes the "Parser.UnitTests-common.vcxproj" file, which
was originally used for a theoretical time when we only open-sourced the
parser. It's unnecessary now, and we can get rid of it.

Also includes a small patch to bcz.cmd, to make sure bx works with
projects with a space in their name.

(cherry picked from commit 53c81a0)
- Add support for passing through extended text attributes, like… (GH-2917)
 ...

Related work items: #23678919, #23678920, #23731910, #23731911, #23731912, #23731913, #23731914, #23731915, #23731916, #23731917, #23731918
@DHowett-MSFT
Copy link
Contributor

Hey! The fix for this went out in Windows in the Insider channel's Fast Ring with build 19013.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zadjii-msft zadjii-msft

Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) 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
20H1
Development

Successfully merging a pull request may close this issue.

CUU and CUD should not move "across" margins. microsoft/terminal
4 participants
@j4james @egmontkob @DHowett-MSFT @zadjii-msft