Skip to content

feat(terminal): add xterm.js as a selectable backend - #83

Open
Ayman Bagabas (aymanbagabas) wants to merge 7 commits into
microsoft:mainfrom
aymanbagabas:feat/xtermjs-backend
Open

feat(terminal): add xterm.js as a selectable backend#83
Ayman Bagabas (aymanbagabas) wants to merge 7 commits into
microsoft:mainfrom
aymanbagabas:feat/xtermjs-backend

Conversation

@aymanbagabas

@aymanbagabas Ayman Bagabas (aymanbagabas) commented Aug 4, 2026

Copy link
Copy Markdown
Member

tui-test can already run a session on alacritty or ghostty. This adds the emulator behind VS Code's terminal as a third, so a suite can be run against what a large share of users are actually looking at. A test that passes on one emulator and fails on another is telling you something real about the program under test.

$ tui-test open --backend xtermjs
$ tui-test run --backend xtermjs -- vim notes.md

Selectable everywhere the other backends are: the CLI's --backend, and both the Python and Node bindings.

No Node at runtime

@xterm/headless and the unicode11 addon are vendored and evaluated into a QuickJS context per session, so the backend depends on nothing installed on the machine. shim.js is our own code; the bundles are dropped in unchanged at pinned versions.

The grid crosses that boundary packed rather than a cell at a time. Reading an 80x30 screen through the per-cell getters is 2,400 calls with ten property reads each, which costs milliseconds per poll; the shim flattens a row span into one string and one integer array, so a whole screen crosses as two values and this side decodes rather than traverses.

What the shim has to supply

Four things the Emulator contract asks for are not in the headless bundle's public surface. The shim supplies them rather than the Rust side pretending they are absent:

Contract How
Window title onTitleChange, with windowOptions.pushTitle/popTitle enabling the CSI 22/23 t stack the bundle implements but leaves off by default
Cursor visibility coreService.isCursorHidden
Cursor shape coreService.decPrivateModes.cursorStyle, which is absent until DECSCUSR sets one and so reads as a block until then
OSC 4/10/11/12 set, query, and reset parser.registerOscHandler, answering out of the same reply queue the terminal's own replies use, so answers keep the order they were asked in

A colour reply has to echo the terminator its query used, and an OSC handler is handed its payload but not that terminator. The shim scans the incoming bytes for it, carrying state between calls because a PTY read splits wherever it likes — including between the two bytes of an ST — and keying what it finds by OSC code so a title arriving between two queries cannot put the wrong terminator on a reply.

Verified against a real session rather than only in unit tests. The same probe run on both backends, querying the background, setting it, resetting it, and querying once more with each terminator:

alacritty:  <E>]11;rgb:0000/0000/0000<BEL> | <E>]11;rgb:6565/4343/2121<BEL> | <E>]11;rgb:0000/0000/0000<BEL> | ST:<E>]11;rgb:0000/0000/0000<E>\
xtermjs:    <E>]11;rgb:0000/0000/0000<BEL> | <E>]11;rgb:6565/4343/2121<BEL> | <E>]11;rgb:0000/0000/0000<BEL> | ST:<E>]11;rgb:0000/0000/0000<E>\

Byte-identical, terminator included.

Conformance

The backend passes the conformance suite, which is what makes swapping emulators safe, with one declared exception: xterm.js records a cell's underline colour only when that cell also has an underline style, so SGR 58 on its own — or a colour that outlives an SGR 24 — is not readable back off the cell.

Nothing renders differently, since a cell with no underline draws no underline colour either way. What is lost is the colour surviving in the cell vocabulary. I confirmed this against the bundle rather than inferring it: the cell reports isAttributeDefault(), and the colour is gone from the line's extended attributes while remaining in the current SGR state, so it is xterm.js's per-cell storage rather than anything this mapping does.

The suite grew a Divergences declaration for it, so the exception is a visible claim a reviewer can check rather than a quietly failing case or a backend that skips conformance altogether. It is deliberately narrow: a field earns a place there only when the limitation is in the emulator itself.

Also here

Backend::ALL becomes a slice. With one optional backend the old form needed two length-carrying definitions; with two it would need four. Now each backend adds one line, and the list of names in a parse error is derived from it instead of repeated in prose that can drift out of step.

Notes for review

  • The backend is compiled in by default for the CLI and both bindings, matching how ghostty is wired, so --backend xtermjs works out of the box. Measured cost: 229.6 KiB of embedded JavaScript, and a release binary going 9,208,800 -> 10,506,064 bytes, so about 1.30 MB once QuickJS is compiled in. Happy to put it behind an off-by-default feature instead if the size matters more than the availability.
  • The xtermjs crate feature itself is off by default, so cargo build -p tui-test-rs is unaffected.
  • Conformance for the optional backends does not run in CI today, since cargo test --workspace uses default features while only clippy passes --all-features. That predates this PR and applies to ghostty equally, but it does mean these 47 cases are green locally rather than on a runner. Worth a follow-up.
  • Unicode 11 is pinned deliberately, not incidentally: it is the only version that agrees with alacritty on every emoji width case measured. The table is in assets/xterm/README.md.

Review

Four reviewers went over this (shim correctness, Rust/FFI, architecture, supply chain). Three real divergences came back, all now fixed in the second commit and all reproduced before being touched:

  • A split ST was answered with BEL. The scan that recovers a query's terminator waited for the \ of an ST, but xterm.js ends an OSC on the ESC before it, so a PTY read splitting between those two bytes answered the wrong terminator. Recording it on the ESC keeps the two in step.
  • The terminator queue grew without bound. It recorded every OSC sequence while only colour ones claim what it records, so a shell that retitles the window on each prompt leaked an entry per prompt. Both the shim and supply-chain reviewers found this independently.
  • OSC 4;1x wrote to slot 1. parseInt took the leading digit of an index that was not a number.

The first two are contract behaviour rather than quirks of this backend, so they went into the conformance suite: both fail on xterm.js without the fix and pass on alacritty, which is what makes them worth asserting of every backend. That is 49 cases per backend now.

Also fixed: a usize -> u32 scrollback cast that wrapped a deep request into a shallow one, and the vendored licence, which reproduced only one of the two packages' notices.

Verified clean by review, worth recording:

  • Bundle provenance is byte-for-byte. Both vendored files match what npm publishes for @xterm/headless@6.0.0 and @xterm/addon-unicode11@0.9.0. Their SHA-256s and the commands that reproduce them are now in assets/xterm/README.md, so this is checkable rather than trusted.
  • No path from terminal bytes to evaluated code. Only the three static assets are ever evaluated; PTY bytes reach JS as a byte array.

Considered and not done, happy to be overruled:

  • A QuickJS memory cap. The one unbounded growth path was the queue above, and it is fixed; what remains is bounded by scrollback. A cap would need a number I have no evidence for, and one set too low breaks a legitimately deep session.
  • Bell tracking. xterm.js exposes onBell, but build_with_bells falls through to build exactly as ghostty's does. Worth doing for both backends together rather than making them differ.

Testing

49 conformance cases against the xterm.js backend, plus the full workspace suite (440 tests), cargo clippy --workspace --all-targets --all-features -- -D warnings, the no-default-features clippy pass, fmt, the Python binding suite (87), and the Node binding suite. Also driven end to end through a real PTY: title, cursor, colour assertions, and SVG screenshots all agree with alacritty.

@cpendery cpendery (cpendery) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, I think we can put alacritty as the default & include the other backends as features. For the client libraries, I think it makes sense to include xtermjs & alacritty by default

Comment thread crates/shell-use/assets/xterm/README.md Outdated
@@ -0,0 +1,40 @@
# Vendored xterm.js assets

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we can just use the shim + esbuild & pinned versions for xtermjs's npm module. I'd like to avoid vendoring where we can

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on the instinct, and I dug into what it would actually buy us here. Two things make this case narrower than it looks:

There's nothing to bundle. @xterm/headless publishes lib-headless/xterm-headless.js already bundled and minified by its own webpack build. What's checked in is that file, copied out of the tarball unchanged — so esbuild would be re-bundling an artifact that's already a bundle. shim.js is our code and is already separate.

The versions are already pinned, just pinned in git rather than in a lockfile. Same two packages, same exact bytes:

xterm-headless.js   @xterm/headless@6.0.0          byte-for-byte
addon-unicode11.js  @xterm/addon-unicode11@0.9.0   byte-for-byte

The real question is only whether the bytes live in the repo or get fetched during the build, and fetching costs more than it first appears: cargo build --features xtermjs would need network and a Node toolchain, which breaks offline and air-gapped builds and makes publishing to crates.io awkward. That's a heavier build requirement than Ghostty's Zig, which we already call out in the README as the reason Windows ARM64 artifacts aren't published. It'd also mean the one backend that currently needs no external toolchain would start needing two.

So I've gone at the underlying worry instead — that a vendored blob can silently stop matching what it claims to be. There's now a CI job that re-fetches both packages at their pinned versions and fails if what's checked in isn't byte-identical, and it only runs on commits that touch assets/xterm/, so it costs nothing elsewhere. The SHA-256s and the exact commands to reproduce the files are in the README next to them. A hash committed alongside the bytes it describes proves nothing on its own; re-fetching from npm is what actually closes that loop.

Worth noting we already vendor 6.6 MB of fonts under the same crates/tui-test/assets/ with adjacent licenses. xterm.js is 264 KB, about 4% of that, and follows the same shape.

Happy to switch to a build-time fetch if you'd still prefer it — just wanted the toolchain cost on the table first, since it lands on everyone building the crate.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still like to avoid vendoring, I think its a little different from the fonts since we can just use npm to fetch them.

I'm reworking the readme, and we can add a development section that covers the requirements of both zig & node for building. Let's still bundle them into the published crate, just not checked into the repo

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to bundle xtermjs into the published crate whether we're vendoring its assets in the repository or fetching them during cargo build. Vendoring them in the repository is safer because you don't depend on npmjs or whatever registry and network to fetch the files during build, and the files are already in the repository here so during build the compiler can bundle them right away as static assets.

So really our other option is fetching them during cargo build which is more error prone because now you have network build dependency, and you need to handle xtermjs version tracking somehow perhaps using build.rs.

If you still insist on the network approach, I'm happy to make the changes 🙂

tui-test can already run a session on alacritty or ghostty, and this adds
the emulator behind VS Code's terminal as a third. A suite that passes on
one and fails on another is telling you something real about the program
under test, and xterm.js is what a large share of users are actually
looking at.

`@xterm/headless` and the unicode11 addon are vendored and evaluated into
a QuickJS context per session, so the backend adds no dependency on Node
or on anything installed on the machine. The grid crosses that boundary
packed rather than cell by cell: reading an 80x30 screen a getter at a
time is 2,400 calls with ten property reads each, so the shim flattens a
row span into one string and one integer array and this side decodes it.

Four things the contract asks for are not in the headless bundle's public
surface, and the shim supplies them rather than the Rust side pretending
they are absent: the window title, cursor visibility and shape, and the
`OSC` colour sequences. Colour answers are pushed onto the same queue the
terminal's own replies use, so they keep the order they were asked in.
A query's terminator has to be echoed and an OSC handler is not told which
one ended the sequence, so the incoming bytes are scanned for it, keyed by
OSC code so a title arriving between two queries cannot misroute a reply.

The backend passes the conformance suite, which is what makes swapping
emulators safe, with one declared exception: xterm.js records a cell's
underline colour only when that cell also has an underline style. Nothing
renders differently, and declaring it where the backend opts in keeps the
exception visible instead of turning it into a quietly failing case.

`Backend::ALL` becomes a slice so that each optional backend adds one line
rather than doubling the length-carrying definitions the next one has to
spell out, and the list of names in a parse error is derived from it
instead of being repeated in prose that can drift.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
@aymanbagabas Ayman Bagabas (aymanbagabas) changed the title feat: add xterm.js as a selectable terminal backend feat(terminal): add xterm.js as a selectable backend Aug 20, 2026
A colour query answered with the wrong terminator leaves a program that
reads until the one it sent waiting for something that never comes. The
scan that recovers the terminator waited for the `\` of an `ST`, but the
parser it feeds ends an OSC on the `ESC` before it, so a read that split
between the two bytes answered `BEL` to an `ST` query. Recording the
terminator on the `ESC` is what keeps the two in step.

The scan also recorded every sequence while only colour ones claim what it
records, so a shell that retitles the window on each prompt left an entry
per prompt for the life of the session. Only answered codes are recorded
now.

`OSC 4;1x` named no slot, but `parseInt` read the leading digit and wrote
to slot 1.

The first two are contract behaviour rather than quirks of this backend,
so they are conformance cases: both fail on xterm.js without this change
and pass on alacritty, which is what makes them worth asserting of every
backend rather than fixing quietly in one.

A profile can also ask for deeper scrollback than the count crossing into
JS holds, where the cast wrapped a deep request round to a shallow one.

The vendored licence now reproduces both packages' notices: `@xterm/headless`
ships none and declares MIT in its manifest, while the addon carries its own,
and the two differ. Their hashes are recorded alongside the commands that
reproduce them, so the bytes can be checked rather than trusted.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
The bundles are copied out of the published tarballs unchanged, so a fresh
fetch has to reproduce them byte for byte. Checking that leaves no room for
one to arrive edited or from somewhere other than the release it claims,
which a hash recorded in the same commit as the bytes cannot rule out.

It runs only on commits that touch the vendored files, since that is the
only way they can change, and so costs nothing on the PRs that do not.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
@aymanbagabas

Copy link
Copy Markdown
Member Author

I think we can put alacritty as the default & include the other backends as features.

That's already how the crate is set up — tui-test-rs has default = [], so a plain cargo add tui-test-rs gets Alacritty only, and both ghostty and xtermjs are opt-in features. Nothing changed there in this PR.

For the client libraries, I think it makes sense to include xtermjs & alacritty by default

This PR does the additive half: xtermjs is now enabled for the CLI and both the Python and Node native packages, so --backend xtermjs and backend="xtermjs" work out of the box.

The other half — dropping Ghostty from the published bindings — I've deliberately left out, and I think it deserves its own PR. It's a change to what shipped packages contain rather than an addition, it touches a backend this PR doesn't otherwise go near, and it has a release-notes consequence for anyone already passing backend="ghostty". Folding it in here would put two unrelated stories in one diff.

It's also worth doing on its own merits, and the argument is stronger than just build times: Ghostty needs Zig 0.16 to build, and its upstream Zig build is the reason we don't publish Windows ARM64 artifacts at all today. xterm.js needs no external toolchain, so bindings carrying Alacritty + xterm.js could publish everywhere the CLI does. That looks like the real motivation for your suggestion, and it's a good one — I'd just rather land it where it can be reviewed as that change.

Happy to open it as a follow-up right after this, or to pull it in here if you'd rather have them together.

@aymanbagabas
Ayman Bagabas (aymanbagabas) marked this pull request as ready for review August 20, 2026 17:20
@cpendery

Copy link
Copy Markdown
Member

I think we can put alacritty as the default & include the other backends as features.

That's already how the crate is set up — tui-test-rs has default = [], so a plain cargo add tui-test-rs gets Alacritty only, and both ghostty and xtermjs are opt-in features. Nothing changed there in this PR.

For the client libraries, I think it makes sense to include xtermjs & alacritty by default

This PR does the additive half: xtermjs is now enabled for the CLI and both the Python and Node native packages, so --backend xtermjs and backend="xtermjs" work out of the box.

The other half — dropping Ghostty from the published bindings — I've deliberately left out, and I think it deserves its own PR. It's a change to what shipped packages contain rather than an addition, it touches a backend this PR doesn't otherwise go near, and it has a release-notes consequence for anyone already passing backend="ghostty". Folding it in here would put two unrelated stories in one diff.

It's also worth doing on its own merits, and the argument is stronger than just build times: Ghostty needs Zig 0.16 to build, and its upstream Zig build is the reason we don't publish Windows ARM64 artifacts at all today. xterm.js needs no external toolchain, so bindings carrying Alacritty + xterm.js could publish everywhere the CLI does. That looks like the real motivation for your suggestion, and it's a good one — I'd just rather land it where it can be reviewed as that change.

Happy to open it as a follow-up right after this, or to pull it in here if you'd rather have them together.

based on our offline sync, I think we want to include all backends in the bindings.

@aymanbagabas
Ayman Bagabas (aymanbagabas) marked this pull request as draft August 20, 2026 17:26
`assets/xterm` reads as the X11 terminal rather than the JavaScript one, so
it becomes `assets/xtermjs`, matching the backend and the feature. The
bundles keep the filenames npm publishes, since those are what the tarballs
contain.

A vendored bundle has nothing watching it: no lockfile, no dependency bot,
and no build that fails when it falls behind, so it can sit years out of
date without anyone noticing. A weekly job now compares both packages
against npm and opens a pull request when either has a newer release.

It runs conformance itself rather than leaving it to the pull request,
because nothing else would: CI runs the workspace with default features, so
the xterm.js cases never run there, and a pull request opened with
`GITHUB_TOKEN` starts no workflow of its own. The shim reaches for several
things xterm.js does not expose publicly, which is exactly what a release
can move, so an update that breaks one should say so rather than look
clean. The suite cannot see the emoji width table, which is the reason the
unicode11 addon is pinned where it is, so the pull request asks for that by
hand.

The pinned versions were written down in the workflow and again in prose,
which a bot would have to rewrite in both. They now live in `pinned.json`
alone, so nothing can claim a version the bytes are not.

Also recorded are the two addons deliberately not used yet, since the
reasons are easy to lose: unicode-graphemes is still experimental and
disagrees with alacritty on two of the measured widths, and the clipboard
addon implements an `OSC 52` that no backend supports today. Adding the
latter now would make xterm.js alone answer a query the others ignore,
which is the kind of divergence conformance exists to prevent.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Rust 1.98 turns a constant chunk size into a `chunks_exact` lint, which
`-D warnings` makes an error, so this is a build failure rather than a
suggestion. `as_chunks` says the same thing and hands back arrays, so the
length is known at the type level instead of being a runtime invariant the
indexing below relies on. Stable since 1.88, comfortably under the 1.90
this workspace asks for, and the remainder is discarded either way.

The raster walk is untouched by the branch this lands on and fails the same
way on main; it is here because the clippy step compiles both and cannot
go green while either stands.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
They are 235 KB against the 65 KB of JavaScript the repository has of its
own, so counting them would make a Rust project read as mostly JavaScript
and rank it above Python.

`linguist-vendored` rather than the `linguist-detectable=false` the install
scripts use: those are ours and merely uninteresting, these are somebody
else's, and the vendored mark also collapses a minified bundle in diffs
instead of asking a reviewer to scroll past it.

`shim.js` is left alone. It is our code and counting it is correct.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

@cpendery cpendery (cpendery) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments on some fixes, overall work looks really good.

nit: can we make sure the ci is actually running the xtermjs conformance tests? cargo test --workspace uses the default features so it'll skip the xterm tests even if clippy compiles them

@@ -0,0 +1,138 @@
name: Update xterm.js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather we just bump it manually when needed instead of having the noise of weekly pr's. similar to how node-pty / portable-pty has a vendored version of conpty they bump when needed. We should instead add a step to the release pipeline in the verify to fail unless the version if the most recent latest release


## Addons we do not use yet

Two are worth revisiting, neither of them yet:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make an issue to track this, not the readme

@@ -0,0 +1,114 @@
# Vendored xterm.js assets

Compiled into the `tui-test` binary by `crates/tui-test/src/terminal/xtermjs.rs`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we keep this fairly brief? it's a lot of info here. I'd like to just keep the explanation of the pinned version very short & the divergence section also shortened up a little

pub const ALL: [Self; 2] = [Self::Alacritty, Self::Ghostty];
/// Every backend this build can construct.
///
/// A slice rather than a fixed-size array so that each optional backend

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's drop this comment, not super useful, slice seems better though


let mut pixels = self.pixmap.data().to_vec();
for pixel in pixels.chunks_exact_mut(4) {
for pixel in pixels.as_chunks_mut::<4>().0 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this change for?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lint in recent rust clippy version

Comment thread crates/tui-test/assets/xtermjs/shim.js Outdated
return true;
});
}
dynamic(10, FG);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure OSC 10-12 support multi-color emissions like ESC ] 10 ; #010203 ; #040506 BEL which can change foreground & background color together. This parser parseColor won't handle that properly

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do they? I know OSC 10 -> Fg, OSC 11 -> Bg, and OSC 12 -> cursor. Here's the specs https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h3-Operating-System-Commands

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, and my earlier reply was wrong — sorry. The same page I linked says it a little further up, in the preamble to the Ps = 1 0 list rather than on the entries themselves, which is how I missed it:

At least one parameter is expected for Pt. Each successive parameter changes the next color in the list. The value of Ps tells the starting point in the list.

and for queries:

Because more than one pair of color number and specification can be given in one control sequence, xterm can make more than one reply.

So Ps is an index into one list of dynamic colors, not a fixed target. Alacritty already reads it that way, which made this straightforward to confirm — same sequence, both backends:

OSC 10;#010203;#040506
  alacritty:  fg=rgb(1,2,3)   bg=rgb(4,5,6)     ← correct
  xtermjs:    fg=rgb(4,5,6)   bg=unset          ← last parameter won

Queries were wrong in the same shape — OSC 10;?;? answered OSC 10 twice with the foreground, instead of OSC 10 then OSC 11.

Fixed in 8f6520b. Each parameter now advances to the next color, and each ? is answered under the code of the color it actually asked about. The list runs past the cursor into pointer, Tektronix, and highlight colors we have no notion of; those positions are stepped over rather than folded onto the last slot, so a long sequence sets what it names and nothing else.

Both went in as conformance cases rather than xterm.js-only tests, since this is contract behaviour: they fail on xterm.js without the change and pass on alacritty.

xtermjs  conformance_one_dynamic_color_sequence_sets_successive_colors ... FAILED
  left: Rgb { r: 4, g: 5, b: 6 }   right: Rgb { r: 1, g: 2, b: 3 }
xtermjs  conformance_each_dynamic_color_query_is_answered_separately ... FAILED
  left:  "\e]10;rgb:0404/0505/0606\a\e]10;rgb:0404/0505/0606\a"
  right: "\e]10;rgb:0101/0202/0303\a\e]11;rgb:0404/0505/0606\a"

Good catch — this is exactly the kind of thing the conformance suite should have been covering and wasn't.

// value 0xffffff -- so one of the two has to be wrong. Resetting is
// overwhelmingly the more common of the two, and getting it wrong
// paints a color the terminal never asked for, so it wins.
if (ulMode === 2 && ulColor === 0xffffff) { ulMode = 0; }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this cause issues when a real white underline is emitted?

// means the underline takes the foreground. A cell that really did
// set SGR 58 to its own foreground color lands here too, and draws
// the same either way.
if (ulColor === fg && ulMode === fgMode) { ulMode = 0; }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add some conformance cases to ensure as much as possible that xterm's special cases are properly behaving?

/// No `this` is threaded through: every method the shim returns is a
/// closure over its own `term`, so the receiver is unused, and rquickjs
/// would otherwise pass a `This` wrapper as the first positional argument.
fn call<R>(&self, method: &str) -> R

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this swallow all errors? and don't process and resize ignore exceptions entirely? I think we need to find a better way to surface the failures instead of just suppressing them

The dynamic colors are a single list and the OSC code says only where in
it to start, so `OSC 10;a;b` sets the foreground and then the background.
Every parameter was being applied to the color the code named, so the last
one won and `OSC 10;#010203;#040506` left the foreground `#040506` and the
background untouched. Queries had the same shape: `OSC 10;?;?` answered
`OSC 10` twice with the foreground rather than answering the foreground
and then the background.

The list continues past the cursor into pointer, Tektronix, and highlight
colors this terminal has no notion of. Those positions are stepped over
rather than folded onto the last slot, so a long sequence sets what it
names and nothing else.

Both cases are contract behaviour rather than quirks of this backend, so
they are conformance cases: both fail on xterm.js without this change and
pass on alacritty, which already reads the list this way.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants