The ScrollRegion clip rect is almost always wrong

[j4james]

Environment

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

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:

terminal/src/host/getset.cpp

Lines 2055 to 2063 in 0e6f290

	SMALL_RECT srClip = screenEdges;
	srClip.Top = cursorPosition.Y;
	LOG_IF_FAILED(ServiceLocator::LocateGlobals().api.ScrollConsoleScreenBufferWImpl(screenInfo,
	srScroll,
	coordDestination,
	srClip,
	UNICODE_SPACE,
	screenInfo.GetAttributes().GetLegacyAttributes()));

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

terminal/src/host/getset.cpp

Line 2036 in 0e6f290

const auto screenEdges = screenInfo.GetBufferSize().ToInclusive();

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.

[j4james]

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.

[DHowett-MSFT]

I'd be keen to try

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

[j4james]

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]

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.

[ghost]

🎉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
• Store Download