starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-04 05:35:20 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

The ScrollRegion clip rect is almost always wrong #3028

New Issue
Closed
opened 2026-01-30 23:11:10 +00:00 by claunia · 5 comments
claunia commented 2026-01-30 23:11:10 +00:00
Owner
Copy Link

Originally created by @j4james on GitHub (Jul 31, 2019).

Originally assigned to: @j4james on GitHub.

Environment

Windows build number: Version 10.0.18362.175
Windows Terminal version (if applicable): Commit a08666b58e

It has to be a build that includes PR #1807, otherwise the scrolling probably wouldn't work at all.

Steps to reproduce

The issue manifests in a number of different ways, but one of the most obvious examples looks like this:

  1. Start the conhost: Host.exe

  2. In the properties, make sure that the Screen Buffer Height is larger than the Window Height.

  3. Open a WSL bash shell.

  4. Get a directory listing, or something that fills the screen with content.

  5. Execute the following escape sequence, which sets the background color to red, and deletes 4 lines from the top of the screen:

    printf "\e[41m\e[H\e[4M"

Expected behavior

The contents of the window should scroll up by 4 lines, revealing 4 blank red lines at the bottom of the screen.

Actual behavior

The blank lines are black rather than red. However, if you scroll down to the very end of the buffer, you'll see the 4 red lines there instead.

I believe the problem is that the call to ScrollRegion should have been passed a clip rect matching the window/viewport, but instead was given a rect encompassing the full buffer size.

In this case, the offending code is in the DoSrvPrivateModifyLinesImpl function, which calls the ScrollRegion function via the ScrollConsoleScreenBufferWImpl method:

0e6f290806/src/host/getset.cpp (L2055-L2063)

The screenEdges rect (with which the srClip rect is initialised), is set here:

0e6f290806/src/host/getset.cpp (L2036)

Additional examples

The IL (insert lines) escape sequence is similarly affected. When you insert lines onto the screen, anything that scrolls off the bottom of the viewport should be lost, but it's actually just scrolled into the forward buffer, so if you scroll down you can still see it.

For example, execute the sequence below, and then scroll the viewport down to reveal the LAST LINE text that was shifted off screen (it should have been erased):

printf "\e[100BLAST LINE\e[H\e[4L"

The SU (scroll up) and SD (scroll down) escape sequences almost get it right. They're using the viewport as their clip rect, but they're using an exclusive range, where the ScrollRegion function expects an inclusive range, so they're off by one.

For example, if you fill the screen with content as with the first test case, then execute the following escape sequence, which scrolls up by 4 lines:

printf "\e[41m\e[H\e[4S"

That should reveal 4 blank red lines at the bottom of the screen, but actually only 3 of them are red - the fourth red line is off screen.

So far the only command I've found that gets the clip rect correct is the RI (reverse index) sequence.

Originally created by @j4james on GitHub (Jul 31, 2019). Originally assigned to: @j4james on GitHub. # Environment Windows build number: Version 10.0.18362.175 Windows Terminal version (if applicable): Commit a08666b58eb65b85ddf94e69892fa3e0a084ba98 It has to be a build that includes PR #1807, otherwise the scrolling probably wouldn't work at all. # Steps to reproduce The issue manifests in a number of different ways, but one of the most obvious examples looks like this: 1. Start the conhost: Host.exe 2. In the properties, make sure that the _Screen Buffer Height_ is larger than the _Window Height_. 3. Open a WSL bash shell. 4. Get a directory listing, or something that fills the screen with content. 5. Execute the following escape sequence, which sets the background color to red, and deletes 4 lines from the top of the screen: printf "\e[41m\e[H\e[4M" # Expected behavior The contents of the window should scroll up by 4 lines, revealing 4 blank red lines at the bottom of the screen. # Actual behavior The blank lines are black rather than red. However, if you scroll down to the very end of the buffer, you'll see the 4 red lines there instead. I believe the problem is that the call to `ScrollRegion` should have been passed a clip rect matching the window/viewport, but instead was given a rect encompassing the full buffer size. In this case, the offending code is in the `DoSrvPrivateModifyLinesImpl` function, which calls the `ScrollRegion` function via the `ScrollConsoleScreenBufferWImpl` method: https://github.com/microsoft/terminal/blob/0e6f290806ef066e8ef29c0a933c83a0007eac98/src/host/getset.cpp#L2055-L2063 The `screenEdges` rect (with which the `srClip` rect is initialised), is set here: https://github.com/microsoft/terminal/blob/0e6f290806ef066e8ef29c0a933c83a0007eac98/src/host/getset.cpp#L2036 # Additional examples The IL (insert lines) escape sequence is similarly affected. When you insert lines onto the screen, anything that scrolls off the bottom of the viewport should be lost, but it's actually just scrolled into the forward buffer, so if you scroll down you can still see it. For example, execute the sequence below, and then scroll the viewport down to reveal the _LAST LINE_ text that was shifted off screen (it should have been erased): printf "\e[100BLAST LINE\e[H\e[4L" The SU (scroll up) and SD (scroll down) escape sequences almost get it right. They're using the viewport as their clip rect, but they're using an exclusive range, where the `ScrollRegion` function expects an inclusive range, so they're off by one. For example, if you fill the screen with content as with the first test case, then execute the following escape sequence, which scrolls up by 4 lines: printf "\e[41m\e[H\e[4S" That should reveal 4 blank red lines at the bottom of the screen, but actually only 3 of them are red - the fourth red line is off screen. So far the only command I've found that gets the clip rect correct is the RI (reverse index) sequence.
claunia commented 2026-01-30 23:11:12 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Jul 31, 2019):

I just realised it's slightly more complicated than I first thought, because the horizontal clip range (and the area being scrolled) should probably be based on the buffer width rather than the window/viewport width. I know there are a hundred other things that aren't going to work the minute the viewport and buffer widths don't match, but we could at least try and get this bit right while we're fixing things.

Btw, I'd be keen to try and put together a PR for this. I'm not certain I know all the areas of the code that are affected, but I can at least fix the bits I do know about.

@j4james commented on GitHub (Jul 31, 2019): I just realised it's slightly more complicated than I first thought, because the horizontal clip range (and the area being scrolled) should probably be based on the buffer width rather than the window/viewport width. I know there are a hundred other things that aren't going to work the minute the viewport and buffer widths don't match, but we could at least try and get this bit right while we're fixing things. Btw, I'd be keen to try and put together a PR for this. I'm not certain I know all the areas of the code that are affected, but I can at least fix the bits I do know about.
claunia commented 2026-01-30 23:11:12 +00:00
Author
Owner
Copy Link

@DHowett-MSFT commented on GitHub (Aug 1, 2019):

I'd be keen to try

Please do! I'm marking this one triaged. @zadjii-msft will likely agree 😄

@DHowett-MSFT commented on GitHub (Aug 1, 2019): > I'd be keen to try Please do! I'm marking this one triaged. @zadjii-msft will likely agree :smile:
claunia commented 2026-01-30 23:11:12 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Aug 4, 2019):

I've reached a bit of a blocker with this. Fixing the implementation was the easy part, but now the adapter tests don't pass, and that's a more complicated problem.

The first issue is that tests assume (and test) that a scrolling operation won't affect anything outside the viewport. That's true for the vertical extent, but it's no longer true for the horizontal extent. As I mentioned above, I believe the correct behaviour should be to clip the horizontal range to the buffer width, not the viewport width. However, since the current behaviour is clearly deliberate, I'd like to get definite confirmation that it's OK to change that behaviour, and update those tests.

The second issue is that the adapter tests aren't really suited for these kinds of tests. We're trying to test the end effect of ScrollConsoleScreenBufferW method, but that's impossible to do when the implementation is just a mock. There's an attempt to emulate the real ScrollConsoleScreenBufferW in that mock, but it isn't an accurate emulation. And even if it was, we wouldn't be testing the real code, we'd be testing the mock - it defeats the whole point of the test. You could erase the entire contents of the real ScrollConsoleScreenBufferW implementation and the tests would still pass!

To fix that, we really need to try and reimplement these tests somewhere more appropriate, e.g. as screen buffer tests. That's quite a big change to make, though, so again I'd like to get confirmation that this is the right approach to take.

@j4james commented on GitHub (Aug 4, 2019): I've reached a bit of a blocker with this. Fixing the implementation was the easy part, but now the adapter tests don't pass, and that's a more complicated problem. The first issue is that tests assume (and test) that a scrolling operation won't affect anything outside the viewport. That's true for the vertical extent, but it's no longer true for the horizontal extent. As I mentioned above, I believe the correct behaviour should be to clip the horizontal range to the buffer width, not the viewport width. However, since the current behaviour is clearly deliberate, I'd like to get definite confirmation that it's OK to change that behaviour, and update those tests. The second issue is that the adapter tests aren't really suited for these kinds of tests. We're trying to test the end effect of `ScrollConsoleScreenBufferW` method, but that's impossible to do when the implementation is just a mock. There's an attempt to emulate the real `ScrollConsoleScreenBufferW` in that mock, but it isn't an accurate emulation. And even if it was, we wouldn't be testing the real code, we'd be testing the mock - it defeats the whole point of the test. You could erase the entire contents of the real `ScrollConsoleScreenBufferW` implementation and the tests would still pass! To fix that, we really need to try and reimplement these tests somewhere more appropriate, e.g. as screen buffer tests. That's quite a big change to make, though, so again I'd like to get confirmation that this is the right approach to take.
claunia commented 2026-01-30 23:11:12 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Aug 21, 2019):

I've just gone ahead with the PR for this, assuming I'm correct in scrolling the full buffer width rather than the viewport width, and I've reimplemented the adapter tests as screen buffer tests. If that was the wrong thing to do, you can always reject the PR and suggest another approach.

@j4james commented on GitHub (Aug 21, 2019): I've just gone ahead with the PR for this, assuming I'm correct in scrolling the full buffer width rather than the viewport width, and I've reimplemented the adapter tests as screen buffer tests. If that was the wrong thing to do, you can always reject the PR and suggest another approach.
claunia commented 2026-01-30 23:11:12 +00:00
Author
Owner
Copy Link

@ghost commented on GitHub (Sep 24, 2019):

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

Handy links:

@ghost commented on GitHub (Sep 24, 2019): :tada:This issue was addressed in #2505, which has now been successfully released as `Windows Terminal Preview v0.5.2661.0`.:tada: Handy links: * [Release Notes](https://github.com/microsoft/terminal/releases/tag/v0.5.2661.0) * [Store Download](https://www.microsoft.com/store/apps/9n0dx20hk701?cid=storebadge&ocid=badge)
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-VT 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#3028
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?