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

TAB character shouldn't move past the end of the line #3168

Closed
j4james opened this issue Oct 12, 2019 · 7 comments · Fixed by #3197
Closed

TAB character shouldn't move past the end of the line #3168

j4james opened this issue Oct 12, 2019 · 7 comments · Fixed by #3197
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements 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 Oct 12, 2019

Environment

Windows build number: Version 10.0.18362.295
Also tested in a recent conhost build: commit 82dd0b9

Steps to reproduce

Open a bash shell and execute:

printf "A\e[999C\tB"

This outputs an A, then a CUF sequence to move the cursor position to the end of the line, then a TAB control character, and finally a B.

Expected behavior

When in VT mode, a TAB character should not move the cursor position past the end of the line, so the B should be output on the same line as the A, in the last column.

Here's what the above test case looks like in XTerm:

image

Actual behavior

The TAB moves the cursor position onto the start of the next line, so the B is output below the A.

This is what the test case looks like in the Windows console:

image

Proposed fix

In the SCREEN_INFORMATION::GetForwardTab method, simply get rid of the first condition that checks for the cursor being in the last column. See here:

if (cCurrCursorPos.X == sWidth)
{
cNewCursorPos.X = 0;
cNewCursorPos.Y += 1;
}
else if (_tabStops.empty() || cCurrCursorPos.X >= _tabStops.back())
{
cNewCursorPos.X = sWidth;
}

That case should be handled by the second condition (any X pos greater than or equal to the last tab stop will be set to the last column).

The change shouldn't have any effect on legacy code, because the GetForwardTab method is only used in VT mode, as far as I can tell - the legacy tab handling is a completely separate implementation.

I'd be happy to provide a PR for this if nobody disagrees with the suggested fix.

@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 12, 2019
@egmontkob
Copy link

Let's also note (and verify) that

  • it's irrelevant whether there's any tab stop nearby, i.e. what's the window width modulo 8,
  • printing tabs after B should also be swallowed (keeps the "pending wrap" state in xterm, or the cursor remains just after the last column as VTE and apparently WT thinks about this).

So with this command:

printf "A\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\tB\t\t\t\tC"

assuming the window is not too wide, tons of tab both before and after B should be ignored. B should be at the end of the line, C at the begining of the next line (under A).

@egmontkob
Copy link

Off topic: VTE and iTerm2 bugs about how the desired behavior sucks.

Maybe Windows Terminal intentionally wants to make it better, and see if anything breaks? :) @j4james Do you have an actual app that fails here?

@j4james
Copy link
Collaborator Author

j4james commented Oct 13, 2019

B should be at the end of the line, C at the begining of the next line (under A).

I know that's what XTerm does, but I'm not sure that's correct. If instead you had this:

printf "A\e[999CB\e[999CC"

The result would be the C overwriting the B, and that's essentially the same kind of operation. In both cases I would expect the Last Column Flag (what we call DelayedEolWrap) to be reset by the control following the B.

And if you look at page D-13 of the DEC STD 070 manual, this is what it says:

The Last Column Flag should also be reset by executing any control which potentially changes the Active Position from the end-of-line position. These controls include the following:

And then goes on to list a bunch of controls, including HORIZONTAL TAB.

Then again it also says that "existing products vary widely in their handling of the end-of-line condition in regards to resetting the Last Column Flag", so I don't suppose it matters that much. If we really think that the XTerm behaviour is preferable, though, that's going to be a more complicated change, so I think it's best addressed as a separate issue.

Do you have an actual app that fails here?

Vttest actually. I don't really think there's a lot of practical value in passing half of those tests, but that's how many people will judge the quality of our terminal emulation, so I'd like to try and get some of those outstanding issues fixed.

@egmontkob
Copy link

page D-13 of the DEC STD 070 manual [...] The Last Column Flag should also be reset by executing any control which potentially changes the Active Position from the end-of-line position. These controls include [...] HORIZONTAL TAB

Indeed, this seems to say a TAB should move "back" in some sense (clear pending wrap). (Note: I'm not checking the spec, only the bits you quoted.)

printf "A\e[999CB\e[999CC" [...] and that's essentially the same kind of operation.

I'd argue that it's not essentially the same. It could be if TAB was strictly considered a cursor positioning control escape sequence. Instead, TAB is commonly used in plain text files of all kinds, where "real" escapes equences (e.g. ones beginning with the ESC char, as well as other controls like SI SO) don't occur. TAB is frequently placed in text files even in environments that have nothing to do with terminal emulation (e.g. graphical text editors). We migh argue whether it's good or not, and whether it was intended so in the early days, but anyway, that's how it ended up.

It is already bad that simply printing such a file to the terminal doesn't preserve the entire amount of requested indentation (including when the terminal is subsequently resized to be wide enough). It is even worse that near the margin it might completely drop TABs, i.e. two words that are supposed to be separated become logically joined across a linewrap (unless you implement some real magic remembering this state and somehow taking into account for rewrap, word double click selection etc. – a new state that differs both from an explicit newline and from a long word simply wrapping). But dropping a character from the output would IMO be unacceptable. IMO it's a case where common sense needs to override the spec :)

Vttest actually. [...] that's how many people will judge the quality of our terminal emulation

Fair enough. (Note: Don't blindly aim for 100% pass rate. E.g. bullet point 2, screen 2 "Test of TAB setting/resetting" used to pass in VTE but fails nowadays after a change in VTE. Examining the situation reveals that VTE implements the standards correctly, vttest is faulty here.)

@j4james
Copy link
Collaborator Author

j4james commented Oct 14, 2019

"Test of TAB setting/resetting" used to pass in VTE but fails nowadays after a change in VTE. Examining the situation reveals that VTE implements the standards correctly, vttest is faulty here.)

I don't want to sound like I'm defending vttest, because I think that particular test is silly, but I wouldn't necessarily call it faulty. It's just that you're implementing a part of the ANSI standard which I don't think the VT terminals ever supported (at least as far I know), so for a VT test there is some justification in expecting that parameter to be ignored.

Also, I personally don't see the point in implementing only one bit of the spec without the rest of the line tabulation support (unless you know of some app that depends on just that one sequence). So for now at least, I would definitely expect us to be passing that vttest in its current form.

@DHowett-MSFT DHowett-MSFT added 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 and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Oct 14, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 14, 2019
@DHowett-MSFT DHowett-MSFT added this to the Console Backlog milestone Oct 14, 2019
@ghost ghost added the In-PR This issue has a related PR label Oct 15, 2019
@egmontkob
Copy link

It's just that you're implementing a part of the ANSI standard which I don't think the VT terminals ever supported

Could be. I can't recall of the top of my head what this issue was exactly about, and I'm lazy to look it up now if you don't mind :)

So for now at least, I would definitely expect us to be passing that vttest in its current form.

Fair enough. I am by no means suggesting that you should fail it because it's buggy :-) I think it's perfectly fine if you pass it, and a great improvement from the current state. Just keep in mind that vttest isn't necessarily fully authoritative or correct. Maybe you'll find another case that you do right in WT whereas vttest is faulty, who knows.

@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 Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR labels Dec 4, 2019
@ghost
Copy link

ghost commented Jan 14, 2020

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements 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
Development

Successfully merging a pull request may close this issue.

3 participants