Repository navigation
fix(terminal): keep wheel scrolling inside terminal panes - #792
Merged
Merged
Conversation
Let xterm consume normal scrollback and provider mouse input before canceling only unhandled vertical defaults on the host. This prevents inline terminal gestures from moving their surrounding panel without adding PTY, repaint, or React work. Preserve modified and horizontal gestures and dispose ownership on every host teardown. Refs #791
…undary Brings the wheel-boundary fix (#791) onto current main, 379 commits ahead, including the xterm 6.1.0-beta.304 / WebGL re-enable (#873), terminal session parity (#872) and OpenCode Terminal (#882). Conflicts, all at the three xterm hosts: - AgentInlineTerminal.tsx: main moved the WebGL attach below `scheduleFit` (onRendererChange); keep only the boundary attach right after open(). - AgentTerminalLeaf.tsx: imports - keep the boundary import, drop AgentTitleHeader (removed on main in favor of PaneHeader); body - boundary attach after open(), then main's onRendererChange WebGL attach. - TerminalLeaf.tsx: imports - keep both sides; body - same as above. Re-checked against the installed beta: the scrollable element still only consumes a wheel it scrolled (alwaysConsumeMouseWheel and consumeMouseWheelIfScrollbarIsNeeded default false, Viewport sets neither), and MouseService's passive wheel handler returns without preventDefault while the buffer has scrollback, so the boundary leak persists and the bubbling host listener remains the right place to cancel it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4AoJJUBex14FAEZZfHPsV
Main moved xterm to exact-pinned 6.1.0-beta.304 (#873). A version bump is exactly when a helper like this silently becomes redundant or wrong, so the WHY now cites where the beta's wheel listeners live, when they consume, and what to re-check (and when to delete the helper) on the next bump. Records the re-validation and its tsc/test results in the branch plan. Refs #791 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4AoJJUBex14FAEZZfHPsV
Alt+wheel is xterm's own fast-scroll gesture (fastScrollSensitivity in scrollableElement._handleMouseWheel), not a browser gesture. At a scrollback boundary xterm leaves it unconsumed exactly like a plain wheel, so exempting Alt let a fast scroll that hit the top or bottom move the debug panel: the #791 symptom for Alt users. Ctrl (pinch/zoom), Meta and the deliberate Shift/horizontal policy stay exempt. The new regression covers both directions and fails when the altKey bail is restored. The review also found three inaccuracies in the helper's WHY comment: - Its CSS rejection relied on a pre-Chrome-144 rule. overscroll-behavior now applies to non-scrollable containers, and Electron 43.1.1 ships Chrome 150, so CSS might work. It was never probed, so the source-verified JS path stays. The comment states what a user-run probe must show before a swap. - The !event.cancelable path now cites Chromium's wheel-latching explainer and is marked as reasoned, not probe-verified. - WHEN BUMPING XTERM now points at the manual Electron probe. Real-xterm unit coverage was attempted and is not feasible in happy-dom. open() throws in the DOM renderer's WidthCache, because getContext('2d') is null, before Viewport or MouseService exist. There is also no layout for scroll geometry. The test file now says so instead of implying it models xterm, and the bubble-phase test is retitled to what it actually pins. Refs #791 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4AoJJUBex14FAEZZfHPsV
The Electron probe is the only check that runs the real xterm against Chromium's native scroll chaining, so it now also asserts the Alt fix: - An Alt notch mid-history must fast-scroll without moving the parent. - An Alt wheel at the bottom boundary must stay inside the terminal, and must still leak in --control. The later alternate-screen and mouse comparisons now anchor on that last boundary state. The header now explains: - why the probe is manual and user-run (agents never launch Electron; no npm script or CI gate); - what --control proves; - what each assertion protects; - that both modes must be re-run on every xterm, addon-webgl or Electron bump. The probe was not run as part of this change. Refs #791 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4AoJJUBex14FAEZZfHPsV
ARCHITECTURE.md 6.6.2 lists the xterm helpers that must be re-checked on every xterm bump. The wheel boundary belongs there: it is a cross-host invariant, attached after open() so xterm keeps first refusal. The plan now records: - each review finding's disposition; - why real-xterm tests are infeasible in happy-dom; - why the smoke fixtures stay out of tsconfig.web.json (the sibling terminal-renderer fixture fails TS2683); - this round's verification; - that the Electron probe still needs a user re-run on the current stack. Refs #791 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4AoJJUBex14FAEZZfHPsV
…undary Refs #791 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4AoJJUBex14FAEZZfHPsV
In --control the plain and Alt boundary wheels deliberately leak and scroll #outer to about 240px. Every probe wheel is sent at a fixed window point, so the later alternate-screen and SGR steps would land below xterm's 20-row screen, miss xterm entirely, and report an xterm regression that is really fixture drift (PR #792 review round 2). After both boundary states are recorded, the probe now resets the parent through the fixture's resetOuter(). It asserts the reset took effect and requires outer === 0 for both protocol steps in both modes. The old equalities against the leaked offset are gone. This is reasoned statically, not run. The reset is a no-op in default mode. In control mode an upward protocol wheel cannot scroll a parent already at 0, even if Chromium still latches the earlier leak's sequence to it. Refs #791 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4AoJJUBex14FAEZZfHPsV
The helper-unit file uses plain DOM stand-ins, so it stays green if the pinned xterm changes which wheels it consumes. Review round 1 recorded real-xterm coverage as infeasible because unshimmed happy-dom makes open() throw (null 2D context in WidthCache) and has no layout. Round 2 showed that narrow environment shims are enough. The new file opens the real, patched @xterm/xterm with three shims scoped to it: - a 2D context exposing font and measureText; - a fixed size for the char-measure span; - 0px screen padding. Each synthetic WheelEvent also gets the clientX/clientY and modifier fields that happy-dom's UIEvent-based WheelEvent lacks. Without them the SGR report came out as ESC[<64;NaN;NaNM. Every asserted behavior is computed by xterm: delta normalization, the Alt multiplier, clamping, the consume decision, buffer switching, arrow conversion and SGR encoding. Assertions are relative or semantic, never derived from the fake sizes. The file pins: - plain and Alt scrollback consumed by xterm, with Alt moving farther; - exhausted plain and Alt wheels at top and bottom canceled by the helper, but NOT after dispose(), which pins the leak in the pinned xterm; - ESC [ A on the alternate screen; - one SGR report under DECSET 1000/1006. It claims no native scroll chaining, latching or WebGL coverage; the Electron probe keeps that role. The unit file's header now describes this division instead of infeasibility. Its Ctrl/Meta/Shift case is renamed to say the helper only leaves UNCONSUMED modified wheels to the browser, since xterm may already have consumed them as scrollback. Refs #791 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4AoJJUBex14FAEZZfHPsV
The Ctrl/Meta rationale described helper policy as xterm behavior. xterm does not filter either modifier: while the viewport can move, _handleMouseWheel consumes a Ctrl or Meta wheel as ordinary scrollback. Only an unconsumed one reaches the helper, which leaves it to the browser by choice (pinch/zoom, platform navigation). The Shift rationale leaned on shiftConvert, which xterm skips on macOS. A macOS Shift wheel that still arrives vertical is scrolled vertically by xterm, so at a boundary it can chain to the parent exactly as Alt did. The comment now calls the exemption policy and names that unverified gap. The code is unchanged: narrowing the bail needs probe evidence first. Refs #791 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4AoJJUBex14FAEZZfHPsV
Records in the plan: - the round-2 dispositions: control probe reset, real-xterm test, Ctrl/Meta and Shift rationale, and the tsconfig decline reworded to parity with the sibling fixture; - that round 1's "infeasible" verdict is superseded; - the origin/main merge; - this round's verification on the exact committed tree. The Electron probe still needs a user re-run on the current stack. Refs #791 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4AoJJUBex14FAEZZfHPsV
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
At a scrollback boundary, xterm leaves the wheel event unconsumed and Chromium scrolls the nearest native ancestor. The visible case is the inline debug terminal jumping its surrounding
DebugPanel(overflow-y-auto). This is separate from the GPU glyph corruption repaired in #790.Change
attachTerminalWheelBoundary). Every xterm host attaches it right afteropen()and disposes it beforeterm.dispose(). The hosts:TerminalLeaf: shell panes.AgentTerminalLeaf: every raw agent view, including OpenCode Terminal.AgentInlineTerminal: the debug rail.Design decisions
.xterm, so a host listener runs only after xterm has decided. Intercepting earlier would break alternate-screen arrows and provider mouse protocols.fastScrollSensitivitygesture, not a browser gesture. At a boundary xterm leaves it unconsumed exactly like a plain wheel.shiftConvert, so a Shift wheel that still arrives vertical can chain at a boundary. That gap is documented and unverified; the code is unchanged without probe evidence.overscroll-behavior-y: contain(yet).overscroll-behavioralso applies to non-scrollable scroll containers. Electron 43.1.1 ships Chrome 150, and all three hosts are alreadyoverflow-hidden, so CSS might contain the chaining with no JS.!event.cancelablebails. Per Chromium's wheel-latching explainer, a sequence whose first event went uncanceled stays with its original target. This path is reasoned from documented behavior, not probe-verified.Tests
terminalWheelBoundary.renderer.test.tspins the helper's own contract with plain DOM stand-ins:terminalWheelBoundary.xterm.renderer.test.ts(new in review round 2) opens the real pinned@xterm/xtermin happy-dom.font/measureTextfor WidthCache;.xterm-screen, which happy-dom otherwise reports as''.clientX/clientYand modifier fields that happy-dom's UIEvent-based WheelEvent lacks.ESC [ A;AgentTerminalLeaf.dimensionOwnership.renderer.test.tsxpins helper attach/dispose for the agent, shell and inline terminals.scripts/smoke-terminal-wheel.mjsis the manual, user-run Electron probe. It remains the oracle for native scroll chaining.Verification (final head
ad6e5eec)96cdca9fmergedorigin/main115e26fc, including perf(terminal): re-enable WebGL rendering on the xterm beta line #873 (@xterm/xtermexact-pinned to 6.1.0-beta.304, WebGL re-enabled, locally patchedresize()).78a3e85dmergedorigin/main173a5702. That is 59 more commits, touchingAgentTerminalLeaf.tsxandARCHITECTURE.md, and the merge was clean.open()and dispose it. Main'sattachXtermWebglRenderer(term, { onRendererChange })stays where main put it._handleMouseWheelconsumes only if it moved, andViewport.tssets neither consume option..xterm.tsc -p tsconfig.node.jsonthentsc -p tsconfig.web.jsonon Node 24: both clean.terminalWheelBoundary.renderer.test.ts13,terminalWheelBoundary.xterm.renderer.test.ts5, plusAgentTerminalLeaf.{dimensionOwnership,follow,related,statusHeader,submit}andTerminalLeaf.{dictationOwnership,header,retention}).vitest related --runonterminalWheelBoundary.ts: 17 files, 107 tests pass.event.altKey ||to the bail failed exactly the two helper-unit Alt cases.event.altKey ||to the bail fails exactly the two real-xterm boundary cases (top and bottom), with[true, false]instead of[true, true]: 2 failed, 3 passed. The source was then restored byte-identical.--controlmoved it 120px, and scrollback, output anchoring, alternate-screen arrows and SGR mouse reports all passed.AgentTerminalLeaf, which renders every raw agent surface. The OpenCode mouse-mode loss in bug(terminal): OpenCode terminal panes lose mouse tracking and cannot scroll or jump to latest #843 is pre-existing, and this PR neither causes nor fixes it.Review round 1 (Codex + Claude reviews of
05567575)Codex: REQUEST CHANGES
9409fce1.altKeyis removed from the bail, the WHY comment rewritten, and the Alt test flipped.9409fce1).72829efc).Claude: APPROVE WITH COMMENTS
9409fce1. Replacing the helper with CSS is declined in this PR: nothing here can verify it without launching Electron.WHEN BUMPING XTERMnote points at the probe (6686f3cc,9409fce1).tsconfig.web.jsonfor parity with the siblingterminal-renderer/smoke.tsfixture, which needs a TS2683 fix first (reworded in round 2).9409fce1.9409fce1).84e29c92).9409fce1). Host ordering is now exercised against the real xterm in round 2 (72829efc).Review round 2 (Codex + Claude verification of
84e29c92)Both reviewers verified the Alt fix and found no production defect.
--controlprobe's two boundary leaks scroll#outerto about 240px, so the fixed(200,100)pointer misses xterm's 20-row screen during the alternate-screen and SGR steps.3e88158d. After both boundary states are recorded, the new fixtureresetOuter()sets#outer.scrollTop = 0. The probe asserts the reset took effect, and assertsouter === 0foralternateUpandmouseUpin both modes (replacing the old equalities). The header bullet is updated. Reasoned statically, not run: the reset is a no-op in default mode, and in control an upward wheel cannot scroll a parent already at 0.72829efc. NewterminalWheelBoundary.xterm.renderer.test.ts(5 tests), with shims scoped to that file and a WHY header on what they fake and why the assertions do not echo them. Two happy-dom gaps turn the report intoESC[<64;NaN;NaNM: unset padding computes to'', and WheelEvent lacksclientX/clientY. The first real run had both, and with only the padding shim the missingclientXstill produced NaN. Both are shimmed. The unit test header's "infeasible" claim is replaced. Mutation check: re-adding `event.altKeyebb36e5d). The test title is now "leaves an unconsumed %s wheel to the browser" (72829efc).shiftConvert, so a vertical Shift wheel can chain.ebb36e5d. The comment now calls the exemption policy and names the unverified macOS gap. The code is unchanged without probe evidence.ad6e5eec) and in this body: kept out for parity with the sibling fixture, which needs a TS2683 fix first.Limits
!event.cancelable) and the macOS vertical-Shift case are reasoned, not probed.Fixes #791
🤖 Generated with Claude Code
https://claude.ai/code/session_01S4AoJJUBex14FAEZZfHPsV