-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
The ScrollRegion clip rect is almost always wrong #2174
Comments
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. |
Please do! I'm marking this one triaged. @zadjii-msft will likely agree 😄 |
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 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. |
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. |
There are a number of VT escape sequences that rely on the `ScrollRegion` function to scroll the viewport (RI, DL, IL, SU, SD, ICH, and DCH) , and all of them have got the clipping rect or scroll boundaries wrong in some way, resulting in content being scrolled off the screen that should have been clipped, revealed areas not being correctly filled, or parts of the screen not being moved that should have been. This PR attempts to fix all of those issues. The `ScrollRegion` function is what ultimately handles the scrolling, but it's typically called via the `ApiRoutines::ScrollConsoleScreenBufferWImpl` method, and it's the callers of that method that have needed correcting. One "mistake" that many of these operations made, was in setting a clipping rect that was different from the scrolling rect. This should never have been necessary, since the area being scrolled is also the boundary into which the content needs to be clipped, so the easiest thing to do is just use the same rect for both parameters. Another common mistake was in clipping the horizontal boundaries to the width of the viewport. But it's really the buffer width that represents the active width of the screen - the viewport width and offset are merely a window on that active area. As such, the viewport should only be used to clip vertically - the horizontal extent should typically be the full buffer width. On that note, there is really no need to actually calculate the buffer width when we want to set any of the scrolling parameters to that width. The `ScrollRegion` function already takes care of clipping everything within the buffer boundary, so we can simply set the `Left` of the rect to `0` and the `Right` to `SHORT_MAX`. More details on individual commands: * RI (the `DoSrvPrivateReverseLineFeed` function) This now uses a single rect for both the scroll region and clipping boundary, and the width is set to `SHORT_MAX` to cover the full buffer width. Also the bottom of the scrolling region is now the bottom of the viewport (rather than bottom-1), otherwise it would be off by one. * DL and IL (the `DoSrvPrivateModifyLinesImpl` function) Again this uses a single rect for both the scroll region and clipping boundary, and the width is set to `SHORT_MAX` to cover the full width. The most significant change, though, is that the bottom boundary is now the viewport bottom rather than the buffer bottom. Using the buffer bottom prevented it clipping the content that scrolled off screen when inserting, and failed to fill the revealed area when deleting. * SU and SD (the `AdaptDispatch::_ScrollMovement` method) This was already using a single rect for both the scroll region and clipping boundary, but it was previously constrained to the width of the viewport rather than the buffer width, so some areas of the screen weren't correctly scrolled. Also, the bottom boundary was off by 1, because it was using an exclusive rect while the `ScrollRegion` function expects inclusive rects. * ICH and DCH (the `AdaptDispatch::_InsertDeleteHelper` method) This method has been considerably simplified, because it was reimplementing a lot of functionality that was already provided by the `ScrollRegion` function. And like many of the other cases, it has been updated to use a single rect for both the scroll region and clipping boundary, and clip to the full buffer width rather than the viewport width. I should add that if we were following the specs exactly, then the SU and SD commands should technically be panning the viewport over the buffer instead of moving the buffer contents within the viewport boundary. So SU would be the equivalent of a newline at the bottom of the viewport (assuming no margins). And SD would assumedly do the opposite, scrolling the back buffer back into view (an RI at the top of the viewport should do the same). This doesn't seem to be something that is consistently implemented, though. Some terminals do implement SU as a viewport pan, but I haven't seen anyone implement SD or RI as a pan. If we do want to do something about this, I think it's best addressed as a separate issue. ## Validation Steps Performed There were already existing tests for the SU, SD, ICH, and DCH commands, but they were implemented as adapter tests, which weren't effectively testing anything - the `ScrollConsoleScreenBufferW` method used in those tests was just a mock (an incomplete reimplementation of the `ScrollRegion` function), so confirming that the mock produced the correct result told you nothing about the validity of the real code. To address that, I've now reimplemented those adapter tests as screen buffer tests. For the most part I've tried to duplicate the functionality of the original tests, but there are significant differences to account for the fact that scrolling region now covers the full width of the buffer rather than just the viewport width. I've also extended those tests with additional coverage for the RI, DL, and IL commands, which are really just a variation of the SU and SD functionality. Closes #2174
There are a number of VT escape sequences that rely on the `ScrollRegion` function to scroll the viewport (RI, DL, IL, SU, SD, ICH, and DCH) , and all of them have got the clipping rect or scroll boundaries wrong in some way, resulting in content being scrolled off the screen that should have been clipped, revealed areas not being correctly filled, or parts of the screen not being moved that should have been. This PR attempts to fix all of those issues. The `ScrollRegion` function is what ultimately handles the scrolling, but it's typically called via the `ApiRoutines::ScrollConsoleScreenBufferWImpl` method, and it's the callers of that method that have needed correcting. One "mistake" that many of these operations made, was in setting a clipping rect that was different from the scrolling rect. This should never have been necessary, since the area being scrolled is also the boundary into which the content needs to be clipped, so the easiest thing to do is just use the same rect for both parameters. Another common mistake was in clipping the horizontal boundaries to the width of the viewport. But it's really the buffer width that represents the active width of the screen - the viewport width and offset are merely a window on that active area. As such, the viewport should only be used to clip vertically - the horizontal extent should typically be the full buffer width. On that note, there is really no need to actually calculate the buffer width when we want to set any of the scrolling parameters to that width. The `ScrollRegion` function already takes care of clipping everything within the buffer boundary, so we can simply set the `Left` of the rect to `0` and the `Right` to `SHORT_MAX`. More details on individual commands: * RI (the `DoSrvPrivateReverseLineFeed` function) This now uses a single rect for both the scroll region and clipping boundary, and the width is set to `SHORT_MAX` to cover the full buffer width. Also the bottom of the scrolling region is now the bottom of the viewport (rather than bottom-1), otherwise it would be off by one. * DL and IL (the `DoSrvPrivateModifyLinesImpl` function) Again this uses a single rect for both the scroll region and clipping boundary, and the width is set to `SHORT_MAX` to cover the full width. The most significant change, though, is that the bottom boundary is now the viewport bottom rather than the buffer bottom. Using the buffer bottom prevented it clipping the content that scrolled off screen when inserting, and failed to fill the revealed area when deleting. * SU and SD (the `AdaptDispatch::_ScrollMovement` method) This was already using a single rect for both the scroll region and clipping boundary, but it was previously constrained to the width of the viewport rather than the buffer width, so some areas of the screen weren't correctly scrolled. Also, the bottom boundary was off by 1, because it was using an exclusive rect while the `ScrollRegion` function expects inclusive rects. * ICH and DCH (the `AdaptDispatch::_InsertDeleteHelper` method) This method has been considerably simplified, because it was reimplementing a lot of functionality that was already provided by the `ScrollRegion` function. And like many of the other cases, it has been updated to use a single rect for both the scroll region and clipping boundary, and clip to the full buffer width rather than the viewport width. I should add that if we were following the specs exactly, then the SU and SD commands should technically be panning the viewport over the buffer instead of moving the buffer contents within the viewport boundary. So SU would be the equivalent of a newline at the bottom of the viewport (assuming no margins). And SD would assumedly do the opposite, scrolling the back buffer back into view (an RI at the top of the viewport should do the same). This doesn't seem to be something that is consistently implemented, though. Some terminals do implement SU as a viewport pan, but I haven't seen anyone implement SD or RI as a pan. If we do want to do something about this, I think it's best addressed as a separate issue. ## Validation Steps Performed There were already existing tests for the SU, SD, ICH, and DCH commands, but they were implemented as adapter tests, which weren't effectively testing anything - the `ScrollConsoleScreenBufferW` method used in those tests was just a mock (an incomplete reimplementation of the `ScrollRegion` function), so confirming that the mock produced the correct result told you nothing about the validity of the real code. To address that, I've now reimplemented those adapter tests as screen buffer tests. For the most part I've tried to duplicate the functionality of the original tests, but there are significant differences to account for the fact that scrolling region now covers the full width of the buffer rather than just the viewport width. I've also extended those tests with additional coverage for the RI, DL, and IL commands, which are really just a variation of the SU and SD functionality. Closes #2174 (cherry picked from commit 12d2e17)
🎉This issue was addressed in #2505, which has now been successfully released as Handy links: |
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:
Start the conhost: Host.exe
In the properties, make sure that the Screen Buffer Height is larger than the Window Height.
Open a WSL bash shell.
Get a directory listing, or something that fills the screen with content.
Execute the following escape sequence, which sets the background color to red, and deletes 4 lines from the top of the screen:
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 theScrollRegion
function via theScrollConsoleScreenBufferWImpl
method:terminal/src/host/getset.cpp
Lines 2055 to 2063 in 0e6f290
The
screenEdges
rect (with which thesrClip
rect is initialised), is set here:terminal/src/host/getset.cpp
Line 2036 in 0e6f290
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):
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:
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.
The text was updated successfully, but these errors were encountered: