Repository navigation
Regression in scrolled-forward invalidation behavior from #4171 0586955c88fac8c6f631b89a8b19d4338bfa26ad #5302
Description
Repro from MSFT:25863049
In an unelevated command prompt window, run "dir" twice to fill up the buffer, scroll down until the prompt is in the middle of the screen, then run "dism"
It should print information below the prompt about how it needs to be run elevated, and then return to a prompt.
Instead, it prints that information at a random location.
Bisect brought me to #4171.
Activity
DHowett-MSFT commentedon Apr 9, 2020
The ability to scroll the bottom line of the console up is an atrocity that, unfortunately, thousands of our users apparently love.
j4james commentedon Apr 9, 2020
It looks like it's got something to do with the fact that the new carriage return handler makes a
MoveToBottomcall which the original implementation didn't do. That's something that almost all VT movement operations use, because without it the margin limits don't always work correctly. And I suspect the fact that the original carriage return didn't do that was actually a bug of sorts.I'm still not sure why the
MoveToBottomcall results in things being so messed up though. Because that suggests that even if we went back to the original behaviour, there are probably other ways in which this problem could manifest. I need to do some more digging though.DHowett-MSFT commentedon Apr 9, 2020
So, this is probably a bad interaction with our "virtual bottom". In 19H1, we added a mode that disables the atrocity mentioned above (scroll-forward). The "bottom" of the buffer is tracked in
SCREEN_INFORMATION::_virtualBottom, and MoveToBottom exclusively prefers that one even if that mode isn't enabled (!!)j4james commentedon Apr 10, 2020
OK, I think I know what's going on now. When the viewport bottom is below the virtual bottom, the virtual bottom doesn't get updated. So you've got an app writing stuff to an area of the screen that won't be visible once the viewport is forced back to the correct position (which happens with a
MoveToBottomcall).In earlier versions of the code, this was less of a problem. You might have a VT operation that triggered
MoveToBottom, moving the viewport back up so it hid some of the output. However, the cursor was generally allowed to remain outside the viewport, and eventually would force the viewport down to bring it back into view again.A bunch of VT things would be broken in these situations, and there were still a couple of cases where the viewport would not correct its position, but that was less common.
Once I refactored the cursor movement in PR #3628, things got a lot worse though. Now every cursor movement operation makes sure the position is clamped within the viewport, so it doesn't have the chance to force the viewport back down like it used to. And then PR #4171 made it so every VT carriage return could trigger the issue, and that's going to happen every time the command prompt is output.
The bottom line is I think this has been broken for a long time. It's just that the effects of the issue have been made a lot worse by those two PRs.
As for how we fix it, I thought we could possibly just add a check in the
AdjustCursorPositionto update the virtual bottom if the cursor has moved below it. So at this point...terminal/src/host/_stream.cpp
Lines 267 to 279 in ef80f66
...we'd add a second condition, with something like this:
That seems to fix the issue for me, but I don't know if that's necessarily the right solution, or that there aren't other places in the code that would require similar updates.
j4james commentedon Apr 10, 2020
Another option would be to add the check in the
SCREEN_INFORMATION::SetCursorPositionitself. That feels cleaner, but there are some potentially weird edge cases. For example if a console app temporarily set the cursor position to somewhere way below the bottom of the viewport, that position now becomes the virtual bottom, even if nothing is written there.And then the minute you get a VT cursor movement operation, the viewport will suddenly jump down to that position. And with the cmd shell always being in VT mode, that's going to happen as soon as the app exits. But that does seem like a weird edge case, and maybe that is the behaviour you would expect in that situation.
And possibly half the problem is we're doing
MoveToBottomtoo much. One of the items on my TODO list was to propose getting rid of most of those calls if possible. And if that happens, then this becomes even less of an issue.j4james commentedon Apr 10, 2020
After further investigation, I've decided this really isn't a problem. The
SetConsoleCursorPositionAPI already updates the virtual bottom when the cursor position is moved offscreen, so this isn't any different from the current behaviour. I'm now leaning more towards option 2 as the better approach, and have submitted a PR for that in case you need a quick fix.Update the virtual bottom location if the cursor moves below it (#5317)
Prevent the virtual viewport bottom being updated incorrectly (#12972)