SGR 0 doesn't work correctly when combined with meta attributes #5341
Labels
Area-VT
Virtual Terminal sequence support
Issue-Bug
It either shouldn't be doing this or needs an investigation.
Needs-Tag-Fix
Doesn't match tag requirements
Priority-2
A description (P2)
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
|
I should mention that I'm currently experimenting with a refactor of the SGR code, based on a discussion we had in PR #3160 (comment). That refactoring actually fixes this bug (which is how I discovered it in the first place), but it's still a work in progress, and may be in that state for a while. However, if there is an urgent need for this to be fixed I can probably put together a simpler PR targetting just this bug. |
zadjii-msft
added
Issue-Bug
|
No need for a rush fix on this one. I'll yank the triage tag off (and leave it in 21H1). Thanks for being our VT champion! |
DHowett-MSFT
removed
the
Needs-Triage
5 tasks
DHowett-MSFT
pushed a commit
that referenced
this issue
May 8, 2020
This is an attempt to simplify the SGR (Select Graphic Rendition) implementation in conhost, to cut down on the number of methods required in the `ConGetSet` interface, and pave the way for future improvements and bug fixes. It already fixes one bug that prevented SGR 0 from being correctly applied when combined with meta attributes. * This a first step towards fixing the conpty narrowing bugs in issue #2661 * I'm hoping the simplification of `ConGetSet` will also help with #3849. * Some of the `TextAttribute` refactoring in this PR overlaps with similar work in PR #1978. ## Detailed Description of the Pull Request / Additional comments The main point of this PR was to simplify the `AdaptDispatch::SetGraphicsRendition` implementation. So instead of having it call a half a dozen methods in the `ConGetSet` API, depending on what kinds of attributes needed to be set, there is now just one call to get current attributes, and another call to set the new value. All adjustments to the attributes are made in the `AdaptDispatch` class, in a simple switch statement. To help with this refactoring, I also made some change to the `TextAttribute` class to make it easier to work with. This included adding a set of methods for setting (and getting) the individual attribute flags, instead of having the calling code being exposed to the internal attribute structures and messing with bit manipulation. I've tried to get rid of any methods that were directly setting legacy, meta, and extended attributes. Other than the fix to the `SGR 0` bug, the `AdaptDispatch` refactoring mostly follows the behaviour of the original code. In particular, it still maps the `SGR 38/48` indexed colors to RGB instead of retaining the index, which is what we ultimately need it to do. Fixing that will first require the color tables to be unified (issue #1223), which I'm hoping to address in a followup PR. But for now, mapping the indexed colors to RGB values required adding an an additional `ConGetSet` API to lookup the color table entries. In the future that won't be necessary, but the API will still be useful for other color reporting operations that we may want to support. I've made this API, and the existing setter, standardise on index values being in the "Xterm" order, since that'll be essential for unifying the code with the terminal adapter one day. I should also point out one minor change to the `SGR 38/48` behavior, which is that out-of-range RGB colors are now ignored rather than being clamped, since that matches the way Xterm works. ## Validation Steps Performed This refactoring has obviously required corresponding changes to the unit tests, but most were just minor updates to use the new `TextAttribute` methods without any real change in behavior. However, the adapter tests did require significant changes to accommodate the new `ConGetSet` API. The basic structure of the tests remain the same, but the simpler API has meant fewer values needed to be checked in each test case. I think they are all still covering the areas there were intended to, though, and they are all still passing. Other than getting the unit tests to work, I've also done a bunch of manual testing of my own. I've made sure the color tests in Vttest all still work as well as they used to. And I've confirmed that the test case from issue #5341 is now working correctly. Closes #5341
ghost
added
Needs-Tag-Fix
jelster
pushed a commit
to jelster/terminal
that referenced
this issue
May 28, 2020
This is an attempt to simplify the SGR (Select Graphic Rendition) implementation in conhost, to cut down on the number of methods required in the `ConGetSet` interface, and pave the way for future improvements and bug fixes. It already fixes one bug that prevented SGR 0 from being correctly applied when combined with meta attributes. * This a first step towards fixing the conpty narrowing bugs in issue microsoft#2661 * I'm hoping the simplification of `ConGetSet` will also help with microsoft#3849. * Some of the `TextAttribute` refactoring in this PR overlaps with similar work in PR microsoft#1978. ## Detailed Description of the Pull Request / Additional comments The main point of this PR was to simplify the `AdaptDispatch::SetGraphicsRendition` implementation. So instead of having it call a half a dozen methods in the `ConGetSet` API, depending on what kinds of attributes needed to be set, there is now just one call to get current attributes, and another call to set the new value. All adjustments to the attributes are made in the `AdaptDispatch` class, in a simple switch statement. To help with this refactoring, I also made some change to the `TextAttribute` class to make it easier to work with. This included adding a set of methods for setting (and getting) the individual attribute flags, instead of having the calling code being exposed to the internal attribute structures and messing with bit manipulation. I've tried to get rid of any methods that were directly setting legacy, meta, and extended attributes. Other than the fix to the `SGR 0` bug, the `AdaptDispatch` refactoring mostly follows the behaviour of the original code. In particular, it still maps the `SGR 38/48` indexed colors to RGB instead of retaining the index, which is what we ultimately need it to do. Fixing that will first require the color tables to be unified (issue microsoft#1223), which I'm hoping to address in a followup PR. But for now, mapping the indexed colors to RGB values required adding an an additional `ConGetSet` API to lookup the color table entries. In the future that won't be necessary, but the API will still be useful for other color reporting operations that we may want to support. I've made this API, and the existing setter, standardise on index values being in the "Xterm" order, since that'll be essential for unifying the code with the terminal adapter one day. I should also point out one minor change to the `SGR 38/48` behavior, which is that out-of-range RGB colors are now ignored rather than being clamped, since that matches the way Xterm works. ## Validation Steps Performed This refactoring has obviously required corresponding changes to the unit tests, but most were just minor updates to use the new `TextAttribute` methods without any real change in behavior. However, the adapter tests did require significant changes to accommodate the new `ConGetSet` API. The basic structure of the tests remain the same, but the simpler API has meant fewer values needed to be checked in each test case. I think they are all still covering the areas there were intended to, though, and they are all still passing. Other than getting the unit tests to work, I've also done a bunch of manual testing of my own. I've made sure the color tests in Vttest all still work as well as they used to. And I've confirmed that the test case from issue microsoft#5341 is now working correctly. Closes microsoft#5341
|
🎉This issue was addressed in #5758, which has now been successfully released as Handy links: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Environment
Windows build number: Version 10.0.18362.657
Windows Terminal version (if applicable): Commit ea1bb2e
Steps to reproduce
From a bash shell in conhost, execute the following command:
Expected behavior
It should display the first word with reversed video, and the second word underlined.
For example, here's what it looks like in mintty:
Actual behavior
The
SGR 0in the second sequence doesn't take effect, so the second word is both underlined and reversed.The text was updated successfully, but these errors were encountered: