Commands that erase the screen should clear the rendition attributes #2553
Assignees
Labels
Area-VT
Virtual Terminal sequence support
Issue-Bug
It either shouldn't be doing this or needs an investigation.
Product-Conhost
For issues in the Console codebase
Resolution-Fix-Committed
Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Comments
ghost
added
Needs-Triage
DHowett-MSFT
added
Area-VT
3 tasks
DHowett-MSFT
removed
the
Needs-Triage
5 tasks
ghost
pushed a commit
that referenced
this issue
Dec 10, 2019
## 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 * [x] Closes #2553 * [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 * [ ] 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).
ghost
added
the
Resolution-Fix-Committed
|
🎉This issue was addressed in #3100, which has now been successfully released as Handy links: |
This issue was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Environment
Steps to reproduce
Open a WSL conhost shell, and execute the following command:
This sets the text color to bright yellow on red, and enables the reverse video and underline attributes. It then clears the screen with an
ED(erase in display) sequence.Expected behavior
According to the VT510 reference, this should erase the lines "with all visual character attributes cleared". The DEC STD 070 manual says the same sort of thing: "the empty character (imaged as SPACE), the empty rendition, and the empty character attribute." Rendition refers to attributes like underline, reverse video, etc.
So when clearing the screen, I would expect the underline and reverse attributes not to be applied, so it should just fill the screen with the red background color (DEC's VT terminals never had color, but that's a requirement of the background color erase feature).
This is what the result looks like in XTerm:
Actual behavior
In the Windows console, though, the underline and reverse video attributes are applied, so the screen is filled with the yellow foreground color rather than the red background color, and every character on the screen also has a red underline.
More information
Note that this is an issue for a number of escape sequences. There are those that erase areas of the screen directly (
ED,EL,ECH), and also some that reveal or delete areas of the screen as a result of scrolling (SU,SD,RI,IL,DL,ICH,DCH). If we're going to match the XTerm behaviour, then all of them should be filling the erased area with just the background color, and with none of the rendition attributes set.It might seem like this could easily be fixed by clearing the
META_ATTRSof the fill attribute in the calls toScrollConsoleScreenBufferWImplandFillConsoleOutputAttributeImpl. However, that would prevent those methods working correctly with RGB colors, since they rely on a hack that expects the fill attribute to be an exact match for the current text attribute (and that already has problems - see issue #832).My proposal (not yet tested), is we replace the current hack with a much simpler hack, that just checks for a special case attribute value of
0xFFFF, which we declare to be the "background erase" color. When passed that value, the fill and scroll methods would simply convert that to the current value ofSCREEN_INFORMATION::GetAttributesbut with the meta attributes cleared. Its easy for the calling code to set, and doesn't require an expensiveGenerateLegacyAttributescalculation on either side of the call.The text was updated successfully, but these errors were encountered: