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

linewrap then backspace produces unexpected visuals, missing characters #1245

Closed
fredless opened this issue Jun 13, 2019 · 12 comments · Fixed by #4403
Closed

linewrap then backspace produces unexpected visuals, missing characters #1245

fredless opened this issue Jun 13, 2019 · 12 comments · Fixed by #4403
Assignees
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. 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.
Milestone

Comments

@fredless
Copy link

Environment

Windows build number: Microsoft Windows [Version 10.0.18362.175]
Windows Terminal version (if applicable): built using d82eab4

Steps to reproduce

  • open terminal with following profile:
{
    "globals" : 
    {
        "alwaysShowTabs" : false,
        "defaultProfile" : "{9d2406b9-7e40-4e2b-a20d-82dc61c4cc24}",
        "initialCols" : 80,
        "initialRows" : 60,
        "requestedTheme" : "dark",
        "showTabsInTitlebar" : true,
        "showTerminalTitleInTitlebar" : false
    },
    "profiles" : 
    [
        {
            "acrylicOpacity" : 0.80000001192092896,
            "closeOnExit" : true,
            "colorScheme" : "Solarized Dark",
            "commandline" : "cmd.exe",
            "cursorColor" : "#FFFFFF",
            "cursorShape" : "bar",
            "fontFace" : "Consolas",
            "fontSize" : 10,
            "guid" : "{9d2406b9-7e40-4e2b-a20d-82dc61c4cc24}",
            "historySize" : 9001,
            "name" : "cmd",
            "padding" : "0, 0, 0, 0",
            "snapOnInput" : true,
            "startingDirectory" : "%USERPROFILE%",
            "useAcrylic" : true
        }
    ]
}
  • input characters until the line wraps, backspace once, and enter an additional character

Expected behavior

  • Cursor should return to the previous position following the last character on the previous line after hitting backspace.
  • Additional input character (sent after backspace) should show up after the last character on the previous line.

Actual behavior

  • Cursor returns to previous line, but with a whitespace between the last character input and the cursor position
  • Additional input character does not show up in terminal, although it is recognized

'..789ab' is received as input in the example below, even though it is not shown:
GIF of issue

@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 Jun 13, 2019
@DHowett-MSFT
Copy link
Contributor

This is probably something down in the pseudoconsole layer; we probably have a duplicate for it, but we can't find it.

@DHowett-MSFT DHowett-MSFT added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 13, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 13, 2019
@fredless
Copy link
Author

Should have also mentioned in case it may be helpful, identical behavior was observed on 73ad742

Didn't report then as that was my first exposure, assumed either I couldn't be the only one (or that I was). I just repulled and built in hopes it might have been squashed. I searched then and now before submitting but didn't see a specific match.

@fredless
Copy link
Author

I can't be the only one.. can I?

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

@fredless you're not the only one - this is a reasonable bug report with clear repro steps. Doesn't seem like there's much that needs to be added until someone starts working on a fix.

@zadjii-msft zadjii-msft self-assigned this Aug 27, 2019
@zadjii-msft zadjii-msft removed their assignment Sep 5, 2019
@zadjii-msft zadjii-msft added Product-Terminal The new Windows Terminal. and removed Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Conpty For console issues specifically related to conpty labels Sep 5, 2019
@zadjii-msft zadjii-msft modified the milestones: 20H1, Terminal v1.0 Sep 5, 2019
@zadjii-msft
Copy link
Member

After playing with this some more, this looks like it only repros in the Terminal. This might actually just be another variant of WriteCharsLegacy2ElectricBoogaloo. I doubt we're putting the cursor in the right spot when we backspace around a linewrap like this.

cc @miniksa.

@miniksa
Copy link
Member

miniksa commented Sep 5, 2019

Backspace around a line is a WriteCharsLegacy handled thing in the conhost. So your reasoning is sound. This should be linked to #780. Maybe not resolved until we're sure that it will resolve this too.

@riverar
Copy link

riverar commented Jan 15, 2020

Still seeing this issue in 0.8.10091.0.

@DHowett-MSFT
Copy link
Contributor

Thank you for the report. If we ever think we have explicitly fixed this bug, we’ll be sure to close this issue.

@zadjii-msft zadjii-msft added the Priority-2 A description (P2) label Jan 15, 2020
@zadjii-msft
Copy link
Member

For the record, I've merged a combo of branches for #4354, #4289, and dev/migrie/f/conpty-wrapping-003 all together, and this still repros. It only repros in cmd.exe, and not bash however. I'm thinking it's possible that it's related shockingly to another bug where the cursor doesn't get moved by conpty on a frame where only the cursor changed position - maybe some combo of #4102/#3202. I'll keep digging.

@zadjii-msft
Copy link
Member

Hokay I've got a fix for this that I'm a little wary of, but I'm not sure it can be de-tangled from dev/migrie/f/conpty-wrapping-003. I'll investigate.

@zadjii-msft
Copy link
Member

@<future me>:

This is the other side of the #357 coin actually.

Changing the fix for #357 from

// Update our internal tracker of the cursor's position.
// See MSFT:20266233
// If the cursor is at the rightmost column of the terminal, and we write a
// space, the cursor won't actually move to the next cell (which would
// be {0, _lastText.Y++}). The cursor will stay visibly in that last
// cell until then next character is output.
// If in that case, we increment the cursor position here (such that the X
// position would be one past the right of the terminal), when we come
// back through to MoveCursor in the last PaintCursor of the frame,
// we'll determine that we need to emit a \b to put the cursor in the
// right position. This is wrong, and will cause us to move the cursor
// back one character more than we wanted.
if (_lastText.X < _lastViewport.RightInclusive())
{
_lastText.X += static_cast<short>(columnsActual);
}

to

    if (_lastText.X < _lastViewport.RightExclusive())
    {
        _lastText.X += static_cast<short>(columnsActual);
    }

fixes the Terminal, but re-breaks using conhost as the Terminal (interop/vtpipeterm).

Maybe the Terminal is just wrong here (wouldn't surprise me). Especially considering this bug doesn't repro in conhost as a terminal. Perhaps this is delayed eol wrap? I'll look at the traces, see what's going on.

@ghost ghost added the In-PR This issue has a related PR label Jan 29, 2020
@ghost ghost closed this as completed in #4403 Feb 11, 2020
@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 Feb 11, 2020
ghost pushed a commit that referenced this issue Feb 11, 2020
…an EOL wrap (#4403)

## Summary of the Pull Request

This is a fix that technically was caused by #357, though we didn't have the Terminal at the time, so I only fixed conhost then. When a client app prints the very last column in the buffer, the cursor is often not _actually_ moved to the next row quite yet. The cursor usually just "floats" on the last character of the row, until something happens. This could be a printable character, which will print it on the next line, or a newline, which will move the cursor to the next line manually, or it could be a backspace, which might take the cursor back a character. 

Conhost and gnome-terminal behave slightly differently here, and wt behaves differently all together. Heck, conhost behaves differently depending on what output mode you're in. 

The scenario in question is typing a full row of text, then hitting backspace to erase the last char of the row.

What we were emitting before in this case was definitely wrong - we'd emit a space at that last row, but then not increment our internal tracker of where the cursor is, so the cursor in conpty and the terminal would be misaligned. The easy fix for this is to make sure to always update the `_lastText` member appropriately. This is the `RightExclusive` change.

The second part of this change is to not be so tricksy immediately following a "delayed eol wrap". When we have just printed the last char like that, always use the VT sequence CUP the next time the cursor moves. Depending on the terminal emulator and it's flags, performing a BS in this state might not bring the cursor to the correct position. 

## References

#405, #780, #357

## PR Checklist
* [x] Closes #1245
* [x] I work here
* [ ] Tests added/passed 
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

With the impending #405 PR I have, this still works, but the sequences that are emitted change, so I didn't write a test for this currently.

## Validation Steps Performed

Tried the scenario for both #357 and #1245 in inception, `gnome-temrinal` and `wt` all, and they all display the cursor correctly.
@ghost
Copy link

ghost commented Feb 13, 2020

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

Handy links:

ghost pushed a commit that referenced this issue Apr 9, 2020
Now that the Terminal is doing a better job of actually marking which
lines were and were not wrapped, we're not always copying lines as
"wrapped" when they should be. We're more correctly marking lines as not
wrapped, when previously we'd leave them marked wrapped.

The real problem is here in the `ScrollFrame` method - we'd manually
newline the cursor to make the terminal's viewport shift down to a new
line. If we had to scroll the viewport for a _wrapped_ line, this would
cause the Terminal to mark that line as broken, because conpty would
emit an extra `\n` that didn't actually exist.

This more correctly implements `ScrollFrame`. Now, well move where we
"thought" the cursor was, so when we get to the next `PaintBufferLine`,
if the cursor needs to newline for the next line, it'll newline, but if
we're in the middle of a wrapped line, we'll just keep printing the
wrapped line.

A couple follow up bugs were found to be caused by the same bad logic.
See #5039 and #5161 for more details on the investigations there.

## References

* #4741 RwR, which probably made this worse
* #5122, which I branched off of 
* #1245, #357 - a pair of other conpty wrapped lines bugs
* #5228 - A followup issue for this PR

## PR Checklist
* [x] Closes #5113
* [x] Closes #5180 (by fixing DECRST 25)
* [x] Closes #5039
* [x] Closes #5161 (by ensuring we only `removeSpaces` on the actual
  bottom line)
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Validation Steps Performed

* Checked the cases from #1245, #357 to validate that they still work
* Added more and more tests for these scenarios, and then I added MORE
  tests
* The entire team played with this in selfhost builds
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. 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.

5 participants