Window contents not cleared properly

[LabhanshAgrawal]

Details

• Browser and browser version: Chrome 97
• OS version: macOS 11.6.2
• xterm.js version: 4.16

Steps to reproduce

Run ls -G && nvim with webgl renderer

With WebGL

With Canvas

Other Examples

vercel/hyper#6242
vercel/hyper#6239

I'm able to consistently reproduce using the steps above for webgl renderer, the hyper issue I've linked also mentions the same for canvas but I'm not able to get a consistent repro

Finding

5437571 is the first bad commit

@Tyriar I'm not familiar enough with xterm core code to fix this, I can try if you can provide some direction as to what might be wrong from the commit I've mentioned.

Till we can get this, we can use 4.15 since this affects usability a lot. But that version suffers from #3547, I've opened #3618 to work around that in ligatures addon itself

[jerch]

Seems model.clear() is too much here:

xterm.js/addons/xterm-addon-webgl/src/WebglRenderer.ts

Lines 119 to 134 in 6c18b7f

	public setColors(colors: IColorSet): void {
	this._colors = colors;
	// Clear layers and force a full render
	for (const l of this._renderLayers) {
	l.setColors(this._terminal, this._colors);
	l.reset(this._terminal);
	}
	this._rectangleRenderer.setColors();
	this._glyphRenderer.setColors();
	this._refreshCharAtlas();
	// Force a full refresh
	this._model.clear();
	}

Wild guess (because idk the cache/redraw-on-change logic behind) - the model clear in setColors leaves old cell drawings on screen, if it got called along with huge buffer changes, which is the case for neovim due to an altbuffer switch.

Indeed, can repro this "forgotten-onscreen-cells" issue with this line:

echo -e '\x1b[?1049h\x1b]11;rgb:00/00/ff\x07\x1b[31m\x1b[1;15HH\x1b[BE\x1b[BL\x1b[BL\x1b[BO'

With this._model.clear(); commented out above the issue is gone.

Edit:
Testing a bit more reveals, that the model.clear() is needed, if we have only color changes and no buffer changes, otherwise garbage will be rendered from the texture. What works though is doing an explicit texture clearing:

  public setColors(colors: IColorSet): void {
    ...

    this._refreshCharAtlas();
    this._charAtlas?.clearTexture();

    // Force a full refresh
    this._model.clear();
  }

A different workaround prolly would be, to give the render model separate update/clear paths for changed colors or changed buffer content. Imho the mix of both currently triggers those edge cases.

[LabhanshAgrawal]

The explicit texture cleaning seems to work only the first time. From my example above, if you open nvim or less again then it happens at that point.

[jerch]

The explicit texture cleaning seems to work only the first time. From my example above, if you open nvim or less again then it happens at that point.

Hmm, thats unfortunate. Maybe always aquiring a new atlas from setColors would help? (Thats done down in _refreshCharAtlas somewhere.)

[LabhanshAgrawal]

Will that affect perf significantly? will check and try though.
The first bad commit is from the OSC color handling pr
Any clue on fixing it from that side?

[LabhanshAgrawal]

Can we get #3618 merged though? Will allow us to use 4.15 till this is fixed.

[jerch]

Well that PR (OSC colors PR) does a nonsense call to setColors for neovim (colors did not change), so for that particular case a workaround would be to check, if colors got updated at all in Terminal._handleColorEvent. But the underlying issue remains - setColors is not resilient against color & buffer changes at once currently in the webgl addon. Which is prolly due to some cache optimization, which I dont know the mechanics for.

[Tyriar]

@LabhanshAgrawal I'll be going through the PRs hopefully today, just need to get through a pretty big terminal link refactor in vscode

[LabhanshAgrawal]

The explicit texture cleaning seems to work only the first time. From my example above, if you open nvim or less again then it happens at that point.

Hmm, thats unfortunate. Maybe always aquiring a new atlas from setColors would help? (Thats done down in _refreshCharAtlas somewhere.)

Same thing happening with new atlas also

[Tyriar]

Doesn't repro on Windows with wsl.exe as shell.

[Tyriar]

Can repro as well on macOS, investigating

[Tyriar]

Repro's in VS Code microsoft/vscode#142091

[Tyriar]

Something indeed is falling apart with caching, I can workaround the issue by forcing a glyph update by adding:

this._glyphRenderer.updateCell(x, y, code, cell.bg, cell.fg, chars);

In this if statement:

xterm.js/addons/xterm-addon-webgl/src/WebglRenderer.ts

Lines 336 to 341 in b557ebe

	// Nothing has changed, no updates needed
	if (this._model.cells[i] === code &&
	this._model.cells[i + RENDER_MODEL_BG_OFFSET] === cell.bg &&
	this._model.cells[i + RENDER_MODEL_FG_OFFSET] === cell.fg) {
	continue;
	}

But doing that would cause a big performance hit, looking into root cause.

[Eugeny]

The PR is working great 👍

[Tyriar]

@Eugeny good to hear 🙂

[Tyriar]

@LabhanshAgrawal this fix is in xterm-addon-webgl@0.12.0-beta.24

[LabhanshAgrawal]

@Tyriar Awesome, thanks for the fix.
Unfortunately, just wasted multiple hours debugging why it works in xterm demo and not in hyper, just to realize that I also needed to use xterm 4.18.0-beta.2 instead of 4.17 😞

[Tyriar]

@LabhanshAgrawal oh sorry about that, you're right the change to src/browser/services/RenderService.ts is in core

[LabhanshAgrawal]

@LabhanshAgrawal oh sorry about that, you're right the change to src/browser/services/RenderService.ts is in core

Sorry didn't mean to vent, just lamenting