Terminal: Incorrect interpretation of DECSTBM parameters

[j4james]

Environment

Windows build number: Version 10.0.18362.145

Steps to reproduce

While testing the DECSTBM escape sequence for setting the scroll margins, I noticed a couple of edge cases where the margin parameters are interpreted incorrectly.

1. If the top margin is equal to the bottom margin, the command should be ignored, but the current interpretation sets the margins to full screen.

2. If the bottom margin is greater than the screen height, the command should be ignored, but the current implementation allows it to extend beyond the bottom of the screen.

You can reproduce these issues with a couple of printf commands in a bash shell.

1. printf "\ec\e[5;20r\e[5;5r\e[10H\e[999Atop\r\e[999Bbottom\e[r\n"

2. printf "\ec\e[5;20r\e[10;200r\e[15H\e[999Atop\r\e[999Bbottom\e[r\n"

Expected behavior

In both of the above test cases, I'd expect the top to be written on line 5, and the bottom to be written on line 20 (i.e. the initial 5;20 margin range, before the attempt to set an invalid margin).

This behaviour is explicitly stated in the DEC STD 070 Video Systems Reference Manual (see notes 2 and 3):

Actual behavior

1. For the first case, the Windows terminal sets the margins to full screen, so the top is output on the first line, and the bottom is on the last line.

2. For the second case, the Windows terminal sets the out-of-range margin exactly as requested, although the bottom margin gives the appearance of being clamped to the screen height. This results in the top being output on line 10 and the bottom output on the last line.

There are actually a couple of variations to these bugs, but I think they're all symptoms of the same main issues.

[j4james]

I should add that I'd be happy to provide a PR for this if you agree with my interpretation of the spec. It should just require a few changes in the AdaptDispatch::_DoSetTopBottomScrollingMargins method.

And of course I'll also try and add a few more unit tests covering these cases - I'm guessing the ScrollMarginsTest in _adapterTest.cpp_ might be the best place for that?

[j4james]

While looking into how I could add more unit tests to the ScrollMarginsTest in _adapterTest.cpp_, I noticed that a couple of the existing tests weren't actually working (i.e. they weren't testing what they claimed to test). If I did a PR for this issue, would it be OK for me to fix those at the same time, since this is all related to DECSTBM parameter interpretation, or would you prefer I raise a separate issue for that?

It's just the last two tests, which are checking whether the margins get cleared when set to the full height of the screen. The first problem is that the expected values are immediately overwritten by a call to the _SetMarginsHelper method (see here and here). The second problem - and the reason the tests still pass - is that the bottom margin is off by one, so they aren't actually testing a full height margin.

[DHowett-MSFT]

@j4james You're a machine! We're very happy to have you looking at these things.

I'd rule in favor of your interpretation of the spec. @zadjii-msft probably knows whether we are misbehaving on purpose, but in general unless it's documented in a big comment that shouts "LOAD BEARING CODE" I'd err on the side of it being a bug. :smile:

... would it be OK for me to fix those at the same time ... ?

Absolutely. If we're doing something wrong with scroll margins, we should check in the tests, the test fixes, and the fixes that make it work properly all at once.

[DHowett-MSFT]

I'm going to yank the triage tag off this one and assign it to you. If you're not comfortable with that, let me know and I'll go backsies!

[zadjii-msft]

I'm almost certain there's not a reason we're doing this on purpose. This is probably me just overlooking a detail from the spec. Great find, and looking forward to the PR :)

[msftbot[bot]]

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

Handy links:

• Release Notes
• Store Download