ICH and DCH shouldn't be affected by vertical margins

[j4james]

Environment

Windows build number: Version 10.0.18362.239

Steps to reproduce

Open a WSL conhost shell, and execute the following command:

printf "\e[3;4r\e[5@\e[r\n"

This sets the vertical scroll margins to lines 3 to 4, inserts 5 characters at the start of the first line (using the ICH escape sequence), and then resets the margins again.

Now open a new WSL conhost shell, and execute the following command:

printf "\e[3;4r\e[5P\e[r\n"

This sets the vertical scroll margins to lines 3 to 4, deletes 5 characters from the start of the first line (using the DCH escape sequence), and then resets the margins again.

Expected behavior

In the first case I'd expect to see the first line indented by 5 characters. In the second case I'd expect to see 5 characters deleted from the first line.

This is how those commands look in XTerm:

Actual behavior

Both commands are ignored, because they're executed above the top of the scroll margins.

Now I can't say for sure that this is the wrong behaviour. The VT510 documentation says that both ICH and DCH have "no effect outside the scrolling margins", but it's possible they were referring only to the horizontal margins in that particular context. If you look at the DEC STD 070 manual, the wording is a lot clearer: "If the Active Position is outside the left or right margins, this control is ignored."

However, just to confuse things some more, the DEC STD 070 manual also provides a kind of pseudo code algorithm for each command, and the algorithm for ICH seems to suggest that it is limited by both the horizontal and vertical margins, while DCH is only affected by the horizontal margins. I suspect that might just be a bug in the ICH algorithm, but who knows.

Bottom line: the documentation isn't particularly clear about this, but I do know that we don't currently match XTerm's behaviour.

[DHowett-MSFT]

@j4james I've taken to assigning these bugs to you because you seem so keen on fixing them. Thanks for all the well-written and investigated issue reports!

[j4james]

@DHowett-MSFT I'm definitely happy for you to assign these bugs to me. Just bear in mind that I might be a bit slow in getting things done, because I don't always have that much time to work on this stuff.

Regarding this particular bug, there is one easy way to fix it, which is to add a check in the ScrollRegion function which would ignore the margins when the scrolling direction was horizontal. However, while looking into the implications of that change, I came to the conclusion that the margin checking should really be removed from the ScrollRegion function altogether, and then left up to the calling code (which can pass in margins, if required, via the existing clip rect).

It's not only the ICH and DCH commands that shouldn't have margins applied. There is also the CSI 3 J sequence (erase scrollback), as well as the public ScrollConsoleScreenBuffer API, which almost certainly shouldn't be affected by the DECSTBM margins. And then there is the AdjustCursorPosition function, which has to temporarily change the margins to achieve the results it needs. In fact requiring no margins, or custom margins, is possibly more common than actually wanting the default DECSTBM margins.

I can raise additional issues to document the problems mentioned above in more detail, but I think it would be easiest to fix them all at the same time. What it comes down to is some refactoring of the ScrollRegion function, as well as updates to most of the places it's called from, but I think the end result may well be a little simpler than what we have now.

Which brings me to the next complication. These changes are quite closely tied to the fixes in PR #2505, so I wouldn't be able to submit a PR for this until that one is merged. Not that that stops me from working on the solution - so there's no urgency - I just wanted you to be aware of the dependency.