Correct fill attributes when scrolling and erasing #3100

j4james · 2019-10-06T18:50:14Z

Summary of the Pull Request

Operations that erase areas of the screen are typically meant to do so using the current color attributes, but with the rendition attributes reset (what we refer to as meta attributes). This also includes scroll operations that have to clear the area of the screen that has scrolled into view. The only exception is the Erase Scrollback operation, which needs to reset the buffer with the default attributes. This PR updates all of these cases to apply the correct attributes when scrolling and erasing.

PR Checklist

Closes Commands that erase the screen should clear the rendition attributes #2553
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be updated
I've not really discussed this with core contributors. I'm ready to accept this work might be rejected in favor of a different grand plan.

Detailed Description of the Pull Request / Additional comments

My initial plan was to use a special case legacy attribute value to indicate the "standard erase attribute" which could safely be passed through the legacy APIs. But this wouldn't cover the cases that required default attributes to be used. And then with the changes in PR #2668 and #2987, it became clear that our requirements could be better achieved with a couple of new private APIs that wouldn't have to depend on legacy attribute hacks at all.

To that end, I've added the PrivateFillRegion and PrivateScrollRegion APIs to the ConGetSet interface. These are just thin wrappers around the existing SCREEN_INFORMATION::Write method and the ScrollRegion function respectively, but with a simple boolean parameter to choose between filling with default attributes or the standard erase attributes (i.e the current colors but with meta attributes reset).

With those new APIs in place, I could then update most scroll operations to use PrivateScrollRegion, and most erase operations to use PrivateFillRegion.

The functions affected by scrolling included:

DoSrvPrivateReverseLineFeed (the RI command)
DoSrvPrivateModifyLinesImpl (the IL and DL commands)
AdaptDispatch::_InsertDeleteHelper (the ICH and DCH commands)
AdaptDispatch::_ScrollMovement (the SU and SD commands)

The functions affected by erasing included:

AdaptDispatch::_EraseSingleLineHelper (the EL command, and most ED variants)
AdaptDispatch::EraseCharacters (the ECH command)

While updating these erase methods, I noticed that both of them also required boundary fixes similar to those in PR #2505 (i.e. the horizontal extent of the erase operation should apply to the full width of the buffer, and not just the current viewport width), so I've addressed that at the same time.

In addition to the changes above, there were also a few special cases, the first being the line feed handling, which required updating in a number of places to use the correct erase attributes:

SCREEN_INFORMATION::InitializeCursorRowAttributes - this is used to initialise the rows that pan into view when the viewport is moved down the buffer.
TextBuffer::IncrementCircularBuffer - this occurs when we scroll passed the very end of the buffer, and a recycled row now needs to be reinitialised.
AdjustCursorPosition - when within margin boundaries, this relies on a couple of direct calls to ScrollRegion which needed to be passed the correct fill attributes.

The second special case was the full screen erase sequence (ESC 2 J), which is handled separately from the other ED sequences. This required updating the SCREEN_INFORMATION::VtEraseAll method to use the standard erase attributes, and also required changes to the horizontal extent of the filled area, since it should have been clearing the full buffer width (the same issue as the other erase operations mentioned above).

Finally, there was the AdaptDispatch::_EraseScrollback method, which uses both scroll and fill operations, which could now be handled by the new PrivateScrollRegion and PrivateFillRegion APIs. But in this case we needed to fill with the default attributes rather than the standard erase attributes. And again this implementation needed some changes to make sure the full width of the active area was retained after the erase, similar to the horizontal boundary issues with the other erase operations.

Once all these changes were made, there were a few areas of the code that could then be simplified quite a bit. The FillConsoleOutputCharacterW, FillConsoleOutputAttribute, and ScrollConsoleScreenBufferW were no longer needed in the ConGetSet interface, so all of that code could now be removed. The _EraseSingleLineDistanceHelper and _EraseAreaHelper methods in the AdaptDispatch class were also no longer required and could be removed.

Then there were the hacks to handle legacy default colors in the FillConsoleOutputAttributeImpl and ScrollConsoleScreenBufferWImpl implementations. Since those hacks were only needed for VT operations, and the VT code no longer calls those methods, there was no longer a need to retain that behaviour (in fact there are probably some edge cases where that behaviour might have been considered a bug when reached via the public console APIs).

Validation Steps Performed

For most of the scrolling operations there were already existing tests in place, and those could easily be extended to check that the meta attributes were correctly reset when filling the revealed lines of the scrolling region.

In the screen buffer tests, I made updates of that sort to the ScrollOperations method (handling SU, SD, IL, DL, and RI), the InsertChars and DeleteChars methods (ICH and DCH), and the VtNewlinePastViewport method (LF). I also added a new VtNewlinePastEndOfBuffer test to check the case where the line feed causes the viewport to pan past the end of the buffer.

The erase operations, however, were being covered by adapter tests, and those aren't really suited for this kind of functionality (the same sort of issue came up in PR #2505). As a result I've had to reimplement those tests as screen buffer tests.

Most of the erase operations are covered by the EraseTests method, except the for the scrollback erase which has a dedicated EraseScrollbackTests method. I've also had to replace the HardReset adapter test, but that was already mostly covered by the HardResetBuffer screen buffer test, which I've now extended slightly (it could do with some more checks, but I think that can wait for a future PR when we're fixing other RIS issues).

…tes.

…ributes correctly.

…tions fill with the correct attributes.

…r rows scrolled into view.

…e new rows with the correct attributes.

…ributes correctly, and make sure the full buffer width is covered.

…es, and cover the full buffer width.

…t the fill attributes correctly, and make sure the full buffer width is affected.

…or the erase and hard reset operations.

…ribute and ScrollConsoleScreenBufferW methods from the ConGetSet interface.

…FillConsoleOutputAttributeImpl and ScrollConsoleScreenBufferWImpl implementations.

DHowett-MSFT · 2019-10-06T19:00:20Z

src/buffer/out/textBuffer.cpp

+    // The VT standard requires that the new row is initialized with
+    // the current background color, but with no meta attributes set.
+    auto fillAttributes = _currentAttributes;
+    fillAttributes.SetMetaAttributes(0);


Concerned about this one, actually -- this is used for traditional Windows console apps as well, and I don't know what the expected behavior is for new rows outside of global VT processing mode.

Ah yes. I should have mentioned I did actually have a look into that. I wrote a little test app that set the attributes with SetConsoleTextAttribute (using COMMON_LVB_UNDERSCORE and COMMON_LVB_REVERSE_VIDEO) and then output some text that would cause the screen to scroll.

In both the legacy console and the v2 console, a line feed doesn't actually fill the new row with the current attributes most of the time - only when it reaches the end of the buffer. That's just plain weird, but it seems that's the way it has always been, and that hasn't changed.

In the legacy console the underscore and reverse attributes don't seem to have any effect anywhere, so naturally that means the new rows filled when scrolling past the end of the buffer will also not have an underscore or have their attributes reversed. So in some ways this update more closely matches that legacy behaviour. It is a change from the current v2 behaviour though.

The bottom line is I'd be very surprised if anyone is relying on this behaviour, considering how strange and inconsistent it is. However, I know backward compatibility is important to you, so if you think it's a concern, I can probably add a check that only resets the attributes when in vt mode or something of the sort.

I've now made it so this only applies in the specific case of the line feed scrolling when in vt mode. There are probably more situations where it should be applied, but for now this is the most conservative fix for this particular issue.

j4james · 2019-10-07T01:04:50Z

There are two other issues I should mention that I'm concerned about in this PR:

I noticed that LockConsole was being used around most calls ScrollRegion (e.g. see here), so I've followed the same pattern in the new PrivateScrollRegion method, as well the in the new PrivateFillRegion method around the SCREEN_INFORMATION::Write call. However, I've also seen places where I would have expected it to be used, but it wasn't (e.g. see here). So I'm unsure now whether it is really necessary to lock these operations, or whether it's only required for specific cases?
I also noticed there's a NotifyAccessibilityEventing call in the FillConsoleOutputAttributeImpl method to indicate changes to the screen buffer (see here), but then it's not used in something like the VtEraseAll method which also updates the buffer (see here). So again I'm not sure when exactly that is required. For now I haven't called it either the new PrivateScrollRegion method or the PrivateFillRegion method, but I suspect that may be wrong.

…, if that is the result of a line feed scroll in vt mode.

miniksa · 2019-10-11T23:08:49Z

There are two other issues I should mention that I'm concerned about in this PR:

I noticed that LockConsole was being used around most calls ScrollRegion (e.g. see here), so I've followed the same pattern in the new PrivateScrollRegion method, as well the in the new PrivateFillRegion method around the SCREEN_INFORMATION::Write call. However, I've also seen places where I would have expected it to be used, but it wasn't (e.g. see here). So I'm unsure now whether it is really necessary to lock these operations, or whether it's only required for specific cases?

It is necessary to lock the scroll operation. We are supposed to lock any operation that moves data around in either the input or output buffers. The fact that it's a global lock pains us, but it's just the way it's been.

The latter case with AdjustCursorPosition is probably because it was reasoned out at some previous point in time that that method was never called without already being under LockConsole. That might not be strictly true anymore as the code has organically grown, but you've stumbled upon one of the "inherited things that haven't been fully sussed out yet even 5 years later" things.

Adding a LockConsole with unlock inside AdjustCursorPosition to cover our butts wouldn't be a bad thing. The lock should be re-entrant. If something starts hanging after you add that, then we've identified a long standing problem.

I also noticed there's a NotifyAccessibilityEventing call in the FillConsoleOutputAttributeImpl method to indicate changes to the screen buffer (see here), but then it's not used in something like the VtEraseAll method which also updates the buffer (see here). So again I'm not sure when exactly that is required. For now I haven't called it either the new PrivateScrollRegion method or the PrivateFillRegion method, but I suspect that may be wrong.

That appears to be another mistake that has slipped through the cracks. We're supposed to be notifying eventing when the contents of the buffer changes in a way that is visible on the screen.

Because that's so prone to error, I've asked @carlos-zamora to start down the path of turning accessibility into a renderer instead of having to separately event it all over the place. The events that accessibility needs to know are pretty much the same things that one needs to know to draw on the screen anyway (and we already have a robust system for knowing and doing that).

However, he's doing it for UIA. Where the NotifyAccessibilityEventing method you identified is for MSAA.

In the short term, it would make sense to propagate the NotifyAccessibilityEventing method to the missing one and the new ones.

In the long term, we should also book moving MSAA to a renderer like @carlos-zamora is doing for UIA. I appear to have mentioned that briefly in #410 as a performance concern as well, but I don't see an issue on the tracker to do it. Maybe @DHowett-MSFT has better GitHub-search-fu than I do.

…function.

j4james · 2019-10-16T20:15:07Z

The latter case with AdjustCursorPosition is probably because it was reasoned out at some previous point in time that that method was never called without already being under LockConsole.

I think you're right about that. I added a check to see if that function was ever called without the lock being held and I couldn't get it to fail, so I think it's probably OK. When it's triggered as a result of a write operation it would already have been locked by one of the WriteConsole*Impl methods (e.g. see here), and for input operations it would likely have come via the ConsoleWindowProc method (see here).

But that does suggest that the lock probably shouldn't be needed in any of the private VT methods, since they should already have been locked by the WriteConsole call that triggered them. I'm nervous about removing existing locks, though, so I'm leaving those as they are.

We're supposed to be notifying eventing when the contents of the buffer changes in a way that is visible on the screen.

I've added a NotifyAccessibilityEventing to the new PrivateFillRegion method, but after a further investigation of the VtEraseAll, I think that code is actually fine as it is. The way the screen is cleared is essentially by scrolling down to a clean area of the buffer, and the viewport move already generates a accessibility scroll event for that, so that should be fine.

zadjii-msft

I apologize for not getting back to this PR earlier. I'm happy with this change overall, but I think it might need a little bit of toughing up to deal with the ExtendedAttributes that merged after this PR was introduced.

Once that's fixed, I'll be happy with accepting this PR.

src/host/_output.cpp

src/host/getset.cpp

src/host/screenInfo.cpp

src/terminal/adapter/adaptDispatch.cpp

src/host/_stream.cpp

…tandard erase operations.

zadjii-msft

Thanks for setting all those meta attributes ☺️

src/host/getset.cpp

src/terminal/adapter/adaptDispatch.cpp

carlos-zamora

Thanks! Sorry it took a while!

carlos-zamora · 2019-12-05T19:31:33Z

@msftbot make sure @miniksa signs off

ghost · 2019-12-05T19:31:38Z

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

miniksa · 2019-12-10T19:29:55Z

OK, I intend to read this one after lunch today. Sorry for the delay. I've been swamped with internal deliverables.

miniksa

This looks pretty good to me. I just had two small comments before we final approve and merge.

miniksa · 2019-12-10T20:38:33Z

src/buffer/out/textBuffer.cpp

@@ -542,14 +542,22 @@ bool TextBuffer::NewlineCursor()
 // - <none>


Arguments comment not updated.

Sorry I totally overlooked that. Added in commit 171a4f9

miniksa · 2019-12-10T20:44:03Z

src/host/screenInfo.cpp

+    // The buffer needs to be initialized with the standard erase attributes,
+    // i.e. the current background color, but with no meta attributes set.
+    auto initAttributes = GetAttributes();
+    initAttributes.SetExtendedAttributes(ExtendedAttributes::Normal);


I feel like this pattern (SetExtendedAttributes to Normal, set Meta to 0) is common enough that perhaps there should just be a .SetStandardErase() method on the Attributes that does both of these instead of having this pattern everywhere.

Very happy to do this. Makes things a lot neater. Added in commit 786cc36

…the operations that need that attribute state.

…ularBuffer method.

miniksa

Excellent, thank you!

DHowett-MSFT · 2019-10-06T19:01:18Z

src/host/getset.cpp

+        // erased area be filled with the current background color, but with
+        // no additional meta attributes set. For all other cases, we just
+        // fill with the default attributes.
+        auto fillAttrs = TextAttribute{};


nit: TextAttribute fillAttrs{};?

ARGH i apparently wrote this comment eighteen months ago and never published it
literally don't care now
😄

DHowett-MSFT · 2019-12-10T23:05:13Z

src/terminal/adapter/adaptDispatch.cpp

-            // We don't use the _EraseAreaHelper here because _EraseSingleLineDistanceHelper does it all in one operation
-            fSuccess = _EraseSingleLineDistanceHelper(coordBelowStartPosition, dwTotalAreaBelow, csbiex.wAttributes);
+            // Again we need to use the default attributes, hence standardFillAttrs is false.
+            fSuccess = _conApi->PrivateFillRegion(coordBelowStartPosition, dwTotalAreaBelow, L' ', false);


nit

Suggested change

fSuccess = _conApi->PrivateFillRegion(coordBelowStartPosition, dwTotalAreaBelow, L' ', false);

fSuccess = _conApi->PrivateFillRegion(coordBelowStartPosition, dwTotalAreaBelow, UNICODE_SPACE, false);

if we can see UNICODE_SPACE here?

DHowett-MSFT · 2019-12-10T23:06:15Z

@j4james excellent as usual. There's a couple missing UNICODE_SPACEs, but I have my finger on the merge button anyway ;P

ghost · 2019-12-10T23:09:54Z

Hello @DHowett-MSFT!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

j4james · 2019-12-10T23:45:07Z

There's a couple missing UNICODE_SPACEs, but I have my finger on the merge button anyway ;P

Just has a check, and it seems UNICODE_SPACE isn't available there anyway. I did actually think I had a reason for using those literals, although it may just have been that I copied them from the previous code.

ghost · 2020-01-14T17:15:37Z

🎉Windows Terminal Preview v0.8.10091.0 has been released which incorporates this pull request.:tada:

Handy links:

j4james added 12 commits October 5, 2019 20:56

Add private scroll region API with support for standard erase attribu…

4c09e68

…tes.

Update scrolling operations to use the new API that sets the fill att…

093b94f

…ributes correctly.

Update existing screen buffer tests to check that the scrolling opera…

b41d2ff

…tions fill with the correct attributes.

Update the line feed handling to set the fill attributes correctly fo…

6f2ef8f

…r rows scrolled into view.

Update and add screen buffer tests to check that line feeds initializ…

6aa3e97

…e new rows with the correct attributes.

Add private fill region API with support for standard erase attributes.

32321a9

Update the erase operations to use the new API that sets the fill att…

ba1e16f

…ributes correctly, and make sure the full buffer width is covered.

Update the VtEraseAll implementation to use the correct fill attribut…

a83fd64

…es, and cover the full buffer width.

Update the EraseScrollback implementation to use the new APIs that se…

480a924

…t the fill attributes correctly, and make sure the full buffer width is affected.

Replace the adapter tests with more appropriate screen buffer tests f…

d85c7e2

…or the erase and hard reset operations.

Remove the unneeded FillConsoleOutputCharacterW, FillConsoleOutputAtt…

ff8a329

…ribute and ScrollConsoleScreenBufferW methods from the ConGetSet interface.

Removed the unneeded hacks for handling legacy default colors in the …

655184c

…FillConsoleOutputAttributeImpl and ScrollConsoleScreenBufferWImpl implementations.

DHowett-MSFT reviewed Oct 6, 2019

View reviewed changes

j4james added 2 commits October 8, 2019 02:03

Only clear the meta attributes while incrementing the circular buffer…

11b6644

…, if that is the result of a line feed scroll in vt mode.

Initialize the alt buffer with the standard erase attributes.

e5c273d

DHowett-MSFT added Product-Conhost For issues in the Console codebase Area-VT Virtual Terminal sequence support labels Oct 15, 2019

Generate an accessibility update event in the DoSrvPrivateFillRegion …

d03f24a

…function.

zadjii-msft self-requested a review October 25, 2019 13:56

This was referenced Nov 19, 2019

Improve the VT cursor movement implementation #3628

Merged

Add support for the DECALN escape sequence #3671

Closed

zadjii-msft requested changes Nov 25, 2019

View reviewed changes

src/host/_output.cpp Show resolved Hide resolved

src/host/getset.cpp Outdated Show resolved Hide resolved

src/host/screenInfo.cpp Outdated Show resolved Hide resolved

src/terminal/adapter/adaptDispatch.cpp Show resolved Hide resolved

src/host/_stream.cpp Outdated Show resolved Hide resolved

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Nov 25, 2019

Make sure we're resetting both the extended and meta attributes for s…

3099d1a

…tandard erase operations.

zadjii-msft approved these changes Dec 4, 2019

View reviewed changes

src/host/getset.cpp Outdated Show resolved Hide resolved

src/terminal/adapter/adaptDispatch.cpp Show resolved Hide resolved

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Dec 4, 2019

ghost requested review from miniksa and carlos-zamora December 4, 2019 17:05

zadjii-msft added this to the Terminal-1912 milestone Dec 4, 2019

carlos-zamora approved these changes Dec 5, 2019

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 5, 2019

miniksa reviewed Dec 10, 2019

View reviewed changes

j4james added 2 commits December 10, 2019 21:52

Add a SetStandardErase method to the TextAttribute class to simplify …

786cc36

…the operations that need that attribute state.

Update the comments to document the new argument in the IncrementCirc…

171a4f9

…ularBuffer method.

ghost removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 10, 2019

miniksa approved these changes Dec 10, 2019

View reviewed changes

DHowett-MSFT approved these changes Dec 10, 2019

View reviewed changes

DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 10, 2019

ghost merged commit 381b115 into microsoft:master Dec 10, 2019

j4james deleted the fix-fill-attributes branch December 15, 2019 01:53

ghost mentioned this pull request Jan 14, 2020

Commands that erase the screen should clear the rendition attributes #2553

Closed

j4james mentioned this pull request May 17, 2020

Custom default colors are problematic for legacy apps #5952

Closed

j4james mentioned this pull request Dec 2, 2021

CMD color Not Working with Scroll #11867

Closed

zadjii-msft mentioned this pull request Jun 10, 2022

Terminal can omit backgrounds, links and underlines on empty cells #13229

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct fill attributes when scrolling and erasing #3100

Correct fill attributes when scrolling and erasing #3100

j4james commented Oct 6, 2019

DHowett-MSFT Oct 6, 2019

j4james Oct 6, 2019

j4james Oct 8, 2019

j4james commented Oct 7, 2019

miniksa commented Oct 11, 2019

j4james commented Oct 16, 2019

zadjii-msft left a comment

zadjii-msft left a comment

carlos-zamora left a comment

carlos-zamora commented Dec 5, 2019

ghost commented Dec 5, 2019

miniksa commented Dec 10, 2019

miniksa left a comment

miniksa Dec 10, 2019

j4james Dec 10, 2019

miniksa Dec 10, 2019

j4james Dec 10, 2019

miniksa left a comment

DHowett-MSFT Oct 6, 2019

DHowett-MSFT Dec 10, 2019

DHowett-MSFT Dec 10, 2019

DHowett-MSFT commented Dec 10, 2019

ghost commented Dec 10, 2019

j4james commented Dec 10, 2019

ghost commented Jan 14, 2020

		@@ -542,14 +542,22 @@ bool TextBuffer::NewlineCursor()
		// - <none>

	fSuccess = _conApi->PrivateFillRegion(coordBelowStartPosition, dwTotalAreaBelow, L' ', false);
	fSuccess = _conApi->PrivateFillRegion(coordBelowStartPosition, dwTotalAreaBelow, UNICODE_SPACE, false);

Correct fill attributes when scrolling and erasing #3100

Correct fill attributes when scrolling and erasing #3100

Conversation

j4james commented Oct 6, 2019

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j4james commented Oct 7, 2019

miniksa commented Oct 11, 2019

j4james commented Oct 16, 2019

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora commented Dec 5, 2019

ghost commented Dec 5, 2019

miniksa commented Dec 10, 2019

miniksa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented Dec 10, 2019

ghost commented Dec 10, 2019

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

j4james commented Dec 10, 2019

ghost commented Jan 14, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.