-
Notifications
You must be signed in to change notification settings - Fork 222
feat: handle whitespace scroll #1365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| import { useCallback, type RefObject, type WheelEventHandler } from 'react'; | ||
| import type { TerminalPaneHandle } from '../components/TerminalPane'; | ||
|
|
||
| export function useTerminalViewportWheelForwarding( | ||
| terminalRef: RefObject<TerminalPaneHandle | null> | ||
| ): WheelEventHandler<HTMLDivElement> { | ||
| return useCallback( | ||
| (event) => { | ||
| if (!Number.isFinite(event.deltaY) || event.deltaY === 0) return; | ||
| if (event.ctrlKey || Math.abs(event.deltaX) > Math.abs(event.deltaY)) return; | ||
|
|
||
| const target = event.target; | ||
| if (target instanceof Element && target.closest('[data-terminal-container]')) { | ||
| return; | ||
| } | ||
|
|
||
| const didScroll = | ||
| terminalRef.current?.scrollViewportFromWheelDelta(event.deltaY, event.deltaMode) ?? false; | ||
| if (didScroll) { | ||
| event.preventDefault(); | ||
| } | ||
| }, | ||
| [terminalRef] | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,6 +109,7 @@ export class TerminalSessionManager { | |
| private selectionChangeDebounceTimer: ReturnType<typeof setTimeout> | null = null; | ||
| private terminalConfigFontSize: number | null = null; | ||
| private lastTheme: SessionTheme; | ||
| private wheelLineRemainder = 0; | ||
|
|
||
| // Timing for startup performance measurement | ||
| private initStartTime: number = 0; | ||
|
|
@@ -539,6 +540,18 @@ export class TerminalSessionManager { | |
| } | ||
| } | ||
|
|
||
| scrollViewportFromWheelDelta(deltaY: number, deltaMode: number): boolean { | ||
| const lineDelta = this.normalizeWheelDeltaToLines(deltaY, deltaMode); | ||
| if (!Number.isFinite(lineDelta) || lineDelta === 0) return false; | ||
|
|
||
| const totalDelta = this.wheelLineRemainder + lineDelta; | ||
| const wholeLines = totalDelta > 0 ? Math.floor(totalDelta) : Math.ceil(totalDelta); | ||
| this.wheelLineRemainder = totalDelta - wholeLines; | ||
|
Comment on lines
+547
to
+549
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accumulated remainder not cleared on direction reversal
For example, if The terminal won't respond until the carry is fully cancelled. Consider resetting the remainder when the incoming const totalDelta = this.wheelLineRemainder + lineDelta;could become: This is a UX polish issue rather than a correctness bug, but it can feel noticeably sluggish on trackpads where sub-line scroll events are very common. |
||
|
|
||
| if (wholeLines === 0) return false; | ||
| return this.scrollViewportByLineDelta(wholeLines); | ||
| } | ||
|
|
||
| registerActivityListener(listener: () => void): () => void { | ||
| this.activityListeners.add(listener); | ||
| return () => { | ||
|
|
@@ -766,6 +779,60 @@ export class TerminalSessionManager { | |
| this.terminal.options.fontFamily = selected ? `${selected}, ${FALLBACK_FONTS}` : FALLBACK_FONTS; | ||
| } | ||
|
|
||
| private scrollViewportByLineDelta(lines: number): boolean { | ||
| try { | ||
| const buffer = this.terminal.buffer?.active; | ||
| if ( | ||
| !buffer || | ||
| typeof buffer.baseY !== 'number' || | ||
| typeof buffer.viewportY !== 'number' || | ||
| lines === 0 | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
||
| const targetLine = Math.max(0, Math.min(buffer.baseY, buffer.viewportY + lines)); | ||
| if (targetLine === buffer.viewportY) return false; | ||
|
|
||
| this.terminal.scrollToLine(targetLine); | ||
| return true; | ||
| } catch (error) { | ||
| log.warn('Failed to scroll terminal viewport', { id: this.id, error }); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| private normalizeWheelDeltaToLines(deltaY: number, deltaMode: number): number { | ||
| if (!Number.isFinite(deltaY) || deltaY === 0) return 0; | ||
|
|
||
| if (deltaMode === WheelEvent.DOM_DELTA_LINE) { | ||
| return deltaY; | ||
| } | ||
|
|
||
| if (deltaMode === WheelEvent.DOM_DELTA_PAGE) { | ||
| return deltaY * this.terminal.rows; | ||
| } | ||
|
|
||
| return deltaY / this.getApproximateRowHeightPx(); | ||
| } | ||
|
|
||
| private getApproximateRowHeightPx(): number { | ||
| const rowElement = this.container.querySelector('.xterm-rows > div'); | ||
| if (rowElement instanceof HTMLElement) { | ||
| const { height } = rowElement.getBoundingClientRect(); | ||
| if (height > 0) return height; | ||
| } | ||
|
|
||
| const fontSize = | ||
| typeof this.terminal.options.fontSize === 'number' | ||
| ? this.terminal.options.fontSize | ||
| : DEFAULT_FONT_SIZE; | ||
| const lineHeight = | ||
| typeof this.terminal.options.lineHeight === 'number' ? this.terminal.options.lineHeight : 1.2; | ||
|
|
||
| return Math.max(fontSize * lineHeight, 1); | ||
| } | ||
|
|
||
| private scheduleFit() { | ||
| if (this.pendingFitFrame !== null) return; | ||
| this.pendingFitFrame = requestAnimationFrame(() => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
metaKey(Cmd+scroll) not guarded againstThe hook correctly short-circuits for
ctrlKey(pinch-zoom on some platforms) and for dominant-horizontal swipes, butmetaKey(Cmd+scroll) is not checked. On macOS inside Electron,Cmd+scrollis sometimes used for zoom or OS-level gestures. Forwarding those events to the terminal scroll path will produce unexpected behaviour.