Terminal can omit backgrounds, links and underlines on empty cells

[PhMajerus]

Original Title: Hyperlink on spaces over-optimization breaks rendering and usability

Windows Terminal version

1.14.1452.0

Windows build number

10.0.22000.708

Other Software

My ActiveScript Shell, just to use JScript to easily script the console, but problem is with VT optimization.

Steps to reproduce

When a hyperlink contains trailing spaces, they are shown or trimmed depending on the number of trailing spaces.
Starting at 9 spaces or more, they get used as spacing, but are not considered part of the hyperlink, while 8 or less are properly rendered and clickable as part of the hyperlink.

Expected Behavior

Hyperlinks extending on space characters can be a design decision for consistency and usability, the Terminal should not decide to shorten the number of columns the hyperlink is effectively available on.
Note non-preview build 1.12.10983.0 does not trim the spaces, so this is a new bug introduced in the Preview version 1.14.1452.0.

Actual Behavior

Current Preview build seems over-agressive in optimizing the spaces, and swallows spaces without abiding by the original VT output of apps.

See how the spaces above the red underlines are different between 8 and 9 spaces.

[zadjii-msft]

Hmm. I'm betting we didn't account for hyperlinks in that optimization. Either in https://github.com/microsoft/terminal/blob/main/src/renderer/vt/paint.cpp#L376, or up in Renderer itself.

[DHowett]

This probably happens when we decide to use CUF instead of space space space space:

terminal/src/renderer/vt/paint.cpp

Lines 412 to 421 in 4e20a86

	// If there are lots of spaces at the end of the line, we can try to Erase
	// Character that number of spaces, then move the cursor forward (to
	// where it would be if we had written the spaces)
	// An erase character and move right sequence is 8 chars, and possibly 10
	// (if there are at least 10 spaces, 2 digits to print)
	// ESC [ %d X ESC [ %d C
	// ESC [ %d %d X ESC [ %d %d C
	// So we need at least 9 spaces for the optimized sequence to make sense.
	// Also, if we already erased the entire display this frame, then
	// don't do ANYTHING with erasing at all.

However, I don't think we changed this behavior in Preview 1.14...

Thank the gods it's not because of the optimization I added, HasIdenticalVisualRepresentationForBlankSpace.

HOWEVER, It would probably be fixed if we DID use HasIdenticalVisualRepresentationForBlankSpace here:

terminal/src/renderer/vt/paint.cpp

Line 437 in 4e20a86

const auto bgMatched = _newBottomLineBG.has_value() ? (_newBottomLineBG.value() == _lastTextAttributes.GetBackground()) : true;

Proving once again that having multiple copies of the same code is deleterious to one's health.

[DHowett]

Note non-preview build 1.12.10983.0 does not trim the spaces

Hmmmmm. What about 1.13?

[PhMajerus]

I got the 1.13.11432.0 RTW in the meantime, and it doesn't trim the spaces.

[j4james]

FYI, I've been trying to git bisect this, and so far I've narrowed it down to somewhere between fa25dfb (April 21) and ce850ca (May 5).

[zadjii-msft]

Okay so

• 1.12: ✅
• 1.13: ✅
• 1.14: ❌

what the devil in 1.14 could have broken this?

[DHowett]

The diffs to paint.cpp specifically from release-1.13 to release-1.14 only cover changing the types from bool to auto for the locals involved in electing to use spaces or CUF.

[j4james]

Found the culprit. f4e0d9f

[j4james]

OK. So this is a wider problem than just hyperlinks. You can see the same issue with something like the reverse attribute.

printf "\e[7m         test         \e[m\n"

Both the spaces before and after the test should be reversed, but only the first lot are. This only worked prior to f4e0d9f because conpty was relying on an incorrect implementation of ECH in Terminal which wasn't taking into account the rules for erase attributes.

Bottom line is that conpty shouldn't be expecting any of the VT erase operations to write out attributes other than color. This was probably already a problem in other conpty terminals which implement the erase operations correctly.

[DHowett]

I wonder, should we simply revisit the ECH/CUF space eliding optimization?

Thanks for digging into this, James 😄

[zadjii-msft]

You know, this makes sense. I did the ECH optimization before #3100, which fixed ECH for conhost.

should we simply revisit the ECH/CUF space eliding optimization

I don't think that's a bad idea. I think the worst case of the spaces was "spaces at the end of the line", but I don't think we ECH&CUF for that anyways. We may want to do that for 1.14 Stable, if we can do it safely in the next week.

[DHowett]

Right, for excess spaces at the end of the line I think we use EL. However, that's also the source of some issues...

[j4james]

Yeah. Here's a failing test case for EL.

printf "\e[7m"; printf ' %.0s' $(seq 1 $COLUMNS); printf "\e[m\n"

This writes out a full line of spaces with the reverse attribute set, and conpty translates that to an EL. That would have worked prior to PR #13024, but won't anymore.

[DHowett]

The more I think about it, the closer the resolutions for this bug and #8341 look to eachother. At the end of the day, both are about not printing "meaningful" spaces, where "meaningful" is defined roughly as "spaces that differ from the clear state for new lines or empty buffers." I had a WIP for #8341 going, I should dig it up.

[zadjii-msft]

Well this is hairy now ain't it.

• ConPTY has a pile of code in here to deal with how it thought it was gonna use ECH. ECH doesn't move the cursor. But REP would.
• Obviously we've got lots of tests that actually compared the raw conpty output, so changing that makes the tests sad.
• Just a delta of const auto optimalToUseECH = false; seemingly fixes this in the Terminal, BUT
  • Now, we emit spaces for the row. So we can get failures like

  ========== Checking the terminal buffer state (after) ==========
  Checking row 0...
  Error: Verify: IsFalse(tb.GetRowByOffset(row).WasWrapForced()) [File: 
  D:\dev\public\terminal\src\cascadia\UnitTests_TerminalCore\ConptyRoundtripTests.cpp, Function: 
  TerminalCoreUnitTests::ConptyRoundtripTests::ResizeInitializeBufferWithDefaultAttrs::<lambda_1>::operator (), Line: 2985]
  EndGroup: TerminalCoreUnitTests::ConptyRoundtripTests::ResizeInitializeBufferWithDefaultAttrs#metadataSet17 [Failed]
  

  Because now we're emitting actual spaces. So terminal now thinks the line wrapped.
• Even if we did replace with REP, then oh no, I bet the terminal will still think the line wrapped.

I'll need to noodle more (probably next week)

[j4james]

It just occurred to me that we may be worrying unnecessarily about the implications of just writing out the spaces. Is it really any different from a line filled with some other character which we wouldn't have been able to optimise? It's not like those spaces could have come from something like an erase screen operation - someone must have actively written out a space in those locations in order for us to get these attributes.

So the idea wouldn't be to turn off the ECH/EL optimisations completely - just turn them off if the spaces are using any of the unsupported attributes.

The only edge case I can think of would possibly be from one of the legacy console APIs that can fill parts of the buffer with attributes like underline and reverse without necessarily writing to those locations. But again I don't think that would be any different from filling the screen with a non-space character, which I think you can also do with the console APIs.

Anyway, it's just a thought, in case you don't have any better ideas.

[zadjii-msft]

Shit @j4james I think you and I came to the same idea at the same time. I'm pushing out a PR now 😉

[ghost]

🎉This issue was addressed in #13661, which has now been successfully released as Windows Terminal v1.14.228.:tada:

Handy links:

• Release Notes
• Store Download

[ghost]

🎉This issue was addressed in #13661, which has now been successfully released as Windows Terminal Preview v1.15.228.:tada:

Handy links:

• Release Notes
• Store Download