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

[PR #2505] [MERGED] Correct the boundaries of the scrolling commands #24965

New Issue
Open
opened 2026-01-31 09:06:25 +00:00 by claunia · 0 comments
claunia commented 2026-01-31 09:06:25 +00:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/2505
Author: @j4james
Created: 8/21/2019
Status: ✅ Merged
Merged: 9/11/2019
Merged by: @DHowett-MSFT

Base: master ← Head: fix-scroll-cliprect

📝 Commits (7)

  • c8600f8 Correct the scroll clipping boundaries for the RI, DL, and IL commands.
  • ea100df Correct the scroll clipping boundaries for the SU and SD commands.
  • e3a9caa Correct the scroll clipping boundaries for the ICH and DCH commands, and simplify the implementation.
  • bfe918e Convert the adapter tests into more appropriate screen buffer tests, taking into account that scrolling now affects the full buffer width.
  • 05b1a9b Extend the screen buffer scrolling tests to cover the IL, DL, and RI commands.
  • a917d0f Get rid of the mock ScrollConsoleScreenBufferW implementation in the adapter tests, since it's no longer needed.
  • f370a91 Use gsl::narrow for narrowing casts, and document the inclusiveness of the scroll rects and the reason for using SHORT_MAX as a right boundary.

📊 Changes

4 files changed (+525 additions, -827 deletions)

View changed files

📝 src/host/getset.cpp (+10 -15)
📝 src/host/ut_host/ScreenBufferTests.cpp (+497 -0)
📝 src/terminal/adapter/adaptDispatch.cpp (+17 -80)
📝 src/terminal/adapter/ut_adapter/adapterTest.cpp (+1 -732)

📄 Description

Summary of the Pull Request

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.

PR Checklist

  • Closes Bug Report - window draggable only in small section (#2174)
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #2174

Detailed Description of the Pull Request / Additional comments

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.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/microsoft/terminal/pull/2505 **Author:** [@j4james](https://github.com/j4james) **Created:** 8/21/2019 **Status:** ✅ Merged **Merged:** 9/11/2019 **Merged by:** [@DHowett-MSFT](https://github.com/DHowett-MSFT) **Base:** `master` ← **Head:** `fix-scroll-cliprect` --- ### 📝 Commits (7) - [`c8600f8`](https://github.com/microsoft/terminal/commit/c8600f87cf56eb18c3f9518ebb46682613a3699a) Correct the scroll clipping boundaries for the RI, DL, and IL commands. - [`ea100df`](https://github.com/microsoft/terminal/commit/ea100df8a47dae968c46598f6e2131863e6928e6) Correct the scroll clipping boundaries for the SU and SD commands. - [`e3a9caa`](https://github.com/microsoft/terminal/commit/e3a9caaec8a61856c90455cec3f9f84f942ccafb) Correct the scroll clipping boundaries for the ICH and DCH commands, and simplify the implementation. - [`bfe918e`](https://github.com/microsoft/terminal/commit/bfe918e8796d24d3669be0f8a34844d9d6a85f95) Convert the adapter tests into more appropriate screen buffer tests, taking into account that scrolling now affects the full buffer width. - [`05b1a9b`](https://github.com/microsoft/terminal/commit/05b1a9b9d64e30c2dd101e09a220aacb704c3fd2) Extend the screen buffer scrolling tests to cover the IL, DL, and RI commands. - [`a917d0f`](https://github.com/microsoft/terminal/commit/a917d0f55e8fbd70681fd89595ef5750a36fbeb0) Get rid of the mock ScrollConsoleScreenBufferW implementation in the adapter tests, since it's no longer needed. - [`f370a91`](https://github.com/microsoft/terminal/commit/f370a91eaf9f47be12050f196180a922186425af) Use gsl::narrow for narrowing casts, and document the inclusiveness of the scroll rects and the reason for using SHORT_MAX as a right boundary. ### 📊 Changes **4 files changed** (+525 additions, -827 deletions) <details> <summary>View changed files</summary> 📝 `src/host/getset.cpp` (+10 -15) 📝 `src/host/ut_host/ScreenBufferTests.cpp` (+497 -0) 📝 `src/terminal/adapter/adaptDispatch.cpp` (+17 -80) 📝 `src/terminal/adapter/ut_adapter/adapterTest.cpp` (+1 -732) </details> ### 📄 Description ## Summary of the Pull Request There are a number of VT escape sequences that rely on the `ScrollRegion` function to scroll the viewport ([RI](https://vt100.net/docs/vt510-rm/chapter4.html#T4-2), [DL](https://www.vt100.net/docs/vt510-rm/DL), [IL](https://www.vt100.net/docs/vt510-rm/IL), [SU](https://www.vt100.net/docs/vt510-rm/SU), [SD](https://www.vt100.net/docs/vt510-rm/SD), [ICH](https://www.vt100.net/docs/vt510-rm/ICH), and [DCH](https://www.vt100.net/docs/vt510-rm/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. ## PR Checklist * [x] Closes #2174 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed * [ ] Requires documentation to be updated * [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #2174 ## Detailed Description of the Pull Request / Additional comments 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
claunia added the pull-request label 2026-01-31 09:06:25 +00:00
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 pull-request
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#24965
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?