starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-09-25 00:15:10 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Regression in scrolled-forward invalidation behavior from #4171 0586955c88 #7349

New Issue
Closed
opened 2026-01-31 01:01:41 +00:00 by claunia · 6 comments
claunia commented 2026-01-31 01:01:41 +00:00
Owner
Copy Link

Originally created by @DHowett-MSFT on GitHub (Apr 9, 2020).

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.

image

Bisect brought me to #4171.

(/cc @j4james)

Originally created by @DHowett-MSFT on GitHub (Apr 9, 2020). 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. ![image](https://user-images.githubusercontent.com/14316954/78932014-a65dd000-7a5b-11ea-895a-d5b95b474f44.png) Bisect brought me to #4171. (/cc @j4james)
claunia commented 2026-01-31 01:01:43 +00:00
Author
Owner
Copy Link

@DHowett-MSFT commented on GitHub (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.

@DHowett-MSFT commented on GitHub (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_.
claunia commented 2026-01-31 01:01:43 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Apr 9, 2020):

It looks like it's got something to do with the fact that the new carriage return handler makes a MoveToBottom call 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 MoveToBottom call 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.

@j4james commented on GitHub (Apr 9, 2020): It looks like it's got something to do with the fact that the new carriage return handler makes a `MoveToBottom` call 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 `MoveToBottom` call 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.
claunia commented 2026-01-31 01:01:43 +00:00
Author
Owner
Copy Link

@DHowett-MSFT commented on GitHub (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 (!!)

@DHowett-MSFT commented on GitHub (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 (!!)
claunia commented 2026-01-31 01:01:44 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (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 MoveToBottom call).

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 AdjustCursorPosition to update the virtual bottom if the cursor has moved below it. So at this point...

ef80f665d3/src/host/_stream.cpp (L267-L279)

...we'd add a second condition, with something like this:

else if (cursorMovedPastVirtualViewport)
{
    screenInfo.SetVirtualBottom(coordCursor.Y);
}

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 commented on GitHub (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 `MoveToBottom` call). 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 `AdjustCursorPosition` to update the virtual bottom if the cursor has moved below it. So at this point... https://github.com/microsoft/terminal/blob/ef80f665d3b7b3fb0b1e39fe43393bd46905f256/src/host/_stream.cpp#L267-L279 ...we'd add a second condition, with something like this: else if (cursorMovedPastVirtualViewport) { screenInfo.SetVirtualBottom(coordCursor.Y); } 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.
claunia commented 2026-01-31 01:01:44 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Apr 10, 2020):

Another option would be to add the check in the SCREEN_INFORMATION::SetCursorPosition itself. 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 MoveToBottom too 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 commented on GitHub (Apr 10, 2020): Another option would be to add the check in the `SCREEN_INFORMATION::SetCursorPosition` itself. 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 `MoveToBottom` too 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.
claunia commented 2026-01-31 01:01:44 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Apr 10, 2020):

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.

After further investigation, I've decided this really isn't a problem. The SetConsoleCursorPosition API 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.

@j4james commented on GitHub (Apr 10, 2020): > 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. After further investigation, I've decided this really isn't a problem. The `SetConsoleCursorPosition` API 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.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label Area-Output Impact-Compatibility Issue-Bug Needs-Tag-Fix Priority-2 Product-Conhost Resolution-Fix-Committed
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#7349
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?