From ac4f13378ea4fffc944de6f96ec4eafe0185fc13 Mon Sep 17 00:00:00 2001 From: rNoz Date: Wed, 29 Jul 2026 23:55:27 +0200 Subject: [PATCH] fix(vim): keep j/k cursor clear of HUD bands when scrolling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Document Vim navigation moved the cursor with `Element.scrollIntoView({ block: 'nearest' })`, which parks the target flush against the nearest viewport edge — exactly where the sticky action bar (top) and the key HUD / status pill (bottom) float. Motion to the top or bottom of a document then hid the caret behind an overlay, while a mouse wheel (which the browser lets overshoot) kept the same line nearer centre. Add vimScroll.ts: a pure computeVimScrollDelta returning the signed scrollTop delta needed to clear a HUD band at each edge (0 when the target is already inside the safe band; a target taller than the band aligns to its top edge so reading order wins), resolveVimScrollMargin sizing the band as clamp(20% of viewport height, 24px..160px), and a scrollVimTargetIntoView wrapper. The scrolling element is the native-scroll host fed through ScrollViewportContext — it carries no [data-overlayscrollbars-viewport] attribute — so the wrapper takes that element from the caller (useScrollViewport() in Viewer, the same node the reticle measures against) rather than rediscovering it by selector. It still tries the OverlayScrollbars attribute for real-library hosts, then the historical scrollIntoView fallback, so behaviour never regresses. Route every cursor/target move in useVimSelection through it. --- packages/ui/components/Viewer.tsx | 25 ++-- packages/ui/hooks/useVimSelection.ts | 23 +++- packages/ui/utils/vimScroll.test.ts | 195 +++++++++++++++++++++++++++ packages/ui/utils/vimScroll.ts | 125 +++++++++++++++++ 4 files changed, 350 insertions(+), 18 deletions(-) create mode 100644 packages/ui/utils/vimScroll.test.ts create mode 100644 packages/ui/utils/vimScroll.ts diff --git a/packages/ui/components/Viewer.tsx b/packages/ui/components/Viewer.tsx index a274eaca1..79e7cc7bf 100644 --- a/packages/ui/components/Viewer.tsx +++ b/packages/ui/components/Viewer.tsx @@ -248,6 +248,9 @@ export const Viewer = forwardRef(({ } }; const containerRef = useRef(null); + // The element that actually scrolls; shared by the Vim scroll math, the + // sticky-header observer, and the reticle geometry. + const scrollViewport = useScrollViewport(); // The badge cluster (repo chips / diff badge) is absolutely positioned in the // card's top padding. One row fits; a second row (diff badge) or mobile // wrapping outgrows the padding and lands on the document's first heading. @@ -461,6 +464,7 @@ export const Viewer = forwardRef(({ }, []); const vim = useVimSelection({ containerRef, + scrollViewport, enabled: vimModeActive, hudEnabled: vimHudEnabled, blocked: vimBlocked, @@ -535,16 +539,15 @@ export const Viewer = forwardRef(({ // Detect when sticky action bar is "stuck" to show card background. // The IntersectionObserver root must be the actual scroll element — the // OverlayScrollArea viewport — not the
host, which doesn't scroll. - const stickyScrollViewport = useScrollViewport(); useEffect(() => { - if (!stickyActions || !stickySentinelRef.current || !stickyScrollViewport) return; + if (!stickyActions || !stickySentinelRef.current || !scrollViewport) return; const observer = new IntersectionObserver( ([entry]) => setIsStuck(!entry.isIntersecting), - { root: stickyScrollViewport, threshold: 0 } + { root: scrollViewport, threshold: 0 } ); observer.observe(stickySentinelRef.current); return () => observer.disconnect(); - }, [stickyActions, stickyScrollViewport]); + }, [stickyActions, scrollViewport]); useEffect(() => { const handleHashChange = () => { @@ -561,7 +564,7 @@ export const Viewer = forwardRef(({ if (!anchor) return false; const container = containerRef.current; - if (!container || !stickyScrollViewport) return false; + if (!container || !scrollViewport) return false; const target = document.getElementById(anchor); if (!target || !container.contains(target)) return false; @@ -573,27 +576,27 @@ export const Viewer = forwardRef(({ const headerOffset = stickyActionsEl ? stickyActionsEl.getBoundingClientRect().height + stickyTop : 0; - const containerRect = stickyScrollViewport.getBoundingClientRect(); + const containerRect = scrollViewport.getBoundingClientRect(); const targetRect = target.getBoundingClientRect(); const relativeTop = targetRect.top - containerRect.top; - const offsetPosition = stickyScrollViewport.scrollTop + relativeTop - headerOffset; + const offsetPosition = scrollViewport.scrollTop + relativeTop - headerOffset; - stickyScrollViewport.scrollTo({ + scrollViewport.scrollTo({ top: Math.max(0, offsetPosition), behavior: 'smooth', }); return true; - }, [stickyScrollViewport]); + }, [scrollViewport]); useEffect(() => { - if (!stickyScrollViewport || !locationHash || lastAutoScrolledHashRef.current === locationHash) return; + if (!scrollViewport || !locationHash || lastAutoScrolledHashRef.current === locationHash) return; const timer = window.setTimeout(() => { if (scrollToAnchor(locationHash)) { lastAutoScrolledHashRef.current = locationHash; } }, 0); return () => window.clearTimeout(timer); - }, [blocks, locationHash, scrollToAnchor, stickyScrollViewport]); + }, [blocks, locationHash, scrollToAnchor, scrollViewport]); // Use the native copy event so clipboard writes are synchronous (Safari // rejects the async navigator.clipboard API outside the user-gesture window). diff --git a/packages/ui/hooks/useVimSelection.ts b/packages/ui/hooks/useVimSelection.ts index 5af5ef0d8..ee76f4f6f 100644 --- a/packages/ui/hooks/useVimSelection.ts +++ b/packages/ui/hooks/useVimSelection.ts @@ -47,11 +47,14 @@ import { type VimVisualBlockState, type VimVisualState, } from '../utils/vimNavigation'; +import { scrollVimTargetIntoView } from '../utils/vimScroll'; import { useVimDocumentFocus } from './useVimDocumentFocus'; /** Inputs required by the Markdown semantic Vim controller. */ export interface UseVimSelectionOptions { readonly containerRef: RefObject; + /** The element that actually scrolls (ScrollViewportContext value). */ + readonly scrollViewport?: HTMLElement | null; readonly enabled: boolean; readonly hudEnabled: boolean; readonly blocked: boolean; @@ -234,6 +237,7 @@ function applyVisualBlockSelection( */ export function useVimSelection({ containerRef, + scrollViewport, enabled, hudEnabled, blocked, @@ -257,6 +261,11 @@ export function useVimSelection({ const pointerFocusRef = useRef(false); const restoringFocusRef = useRef(false); + // Read the live scroll viewport without adding a dependency to every + // navigation callback below. + const scrollViewportRef = useRef(scrollViewport); + scrollViewportRef.current = scrollViewport; + const setState = useCallback((next: VimSelectionState) => { stateRef.current = next; setStateValue(next); @@ -283,7 +292,7 @@ export function useVimSelection({ const next: VimBlockState = { phase: 'block', targetKey: initial.key }; setState(next); window.getSelection()?.removeAllRanges(); - initial.element.scrollIntoView({ block: 'nearest' }); + scrollVimTargetIntoView(initial.element, scrollViewportRef.current); return next; }, [containerRef, setState]); @@ -358,7 +367,7 @@ export function useVimSelection({ const setSemanticTarget = useCallback((target: SemanticTarget) => { setState(semanticStateForTarget(target)); window.getSelection()?.removeAllRanges(); - target.element.scrollIntoView({ block: 'nearest' }); + scrollVimTargetIntoView(target.element, scrollViewportRef.current); }, [setState]); const updateTextState = useCallback(( @@ -373,9 +382,9 @@ export function useVimSelection({ normalized.cursor, normalized.phase === 'visual' ? normalized.anchor : null, ); - resolveTextPosition(graph.container, normalized.cursor) - ?.node.parentElement - ?.scrollIntoView({ block: 'nearest' }); + const cursorParent = resolveTextPosition(graph.container, normalized.cursor) + ?.node.parentElement; + if (cursorParent) scrollVimTargetIntoView(cursorParent, scrollViewportRef.current); }, [setState]); const enterTextAtTarget = useCallback(( @@ -420,7 +429,7 @@ export function useVimSelection({ if (!getTextElementBounds(graph.container, block.element)) return false; setState(next); applyVisualBlockSelection(graph, next); - block.element.scrollIntoView({ block: 'nearest' }); + scrollVimTargetIntoView(block.element, scrollViewportRef.current); return true; }, [setState]); @@ -730,7 +739,7 @@ export function useVimSelection({ }; setState(nextState); applyVisualBlockSelection(graph, nextState); - next.element.scrollIntoView({ block: 'nearest' }); + scrollVimTargetIntoView(next.element, scrollViewportRef.current); return true; } if (key === 'o') { diff --git a/packages/ui/utils/vimScroll.test.ts b/packages/ui/utils/vimScroll.test.ts new file mode 100644 index 000000000..26fd53837 --- /dev/null +++ b/packages/ui/utils/vimScroll.test.ts @@ -0,0 +1,195 @@ +import { describe, expect, test } from 'bun:test'; +import { + VIM_SCROLL_MARGIN_MAX, + VIM_SCROLL_MARGIN_MIN, + computeVimScrollDelta, + resolveVimScrollMargin, + scrollVimTargetIntoView, +} from './vimScroll'; + +const hasDom = typeof document !== 'undefined'; + +// A 1000px-tall viewport whose top edge sits at page y=0, with a 200px HUD band +// reserved at each edge — the safe band is [200, 800]. +const viewport = { top: 0, height: 1000 }; +const band = { topMargin: 200, bottomMargin: 200 }; + +describe('computeVimScrollDelta', () => { + test('leaves a target already inside the safe band untouched', () => { + expect(computeVimScrollDelta(viewport, { top: 400, bottom: 460 }, band)).toBe(0); + // Flush against the inner edges of the band still counts as safe. + expect(computeVimScrollDelta(viewport, { top: 200, bottom: 800 }, band)).toBe(0); + }); + + test('reveals a target parked behind the bottom HUD (the j-to-bottom bug)', () => { + // scrollIntoView({ block: 'nearest' }) would pin this line at y≈980, behind + // the bottom HUD. We instead scroll it down to the bottom margin at y=800. + const delta = computeVimScrollDelta(viewport, { top: 960, bottom: 980 }, band); + expect(delta).toBe(180); // 980 - (1000 - 200) + }); + + test('reveals a target parked behind the top HUD (the k-to-top bug)', () => { + // A line at y≈20 sits under the sticky action bar; scroll up to the margin. + const delta = computeVimScrollDelta(viewport, { top: 20, bottom: 40 }, band); + expect(delta).toBe(-180); // 20 - 200 + }); + + test('aligns a target taller than the band to its top edge, not its bottom', () => { + // A 700px block (taller than the 600px safe band) sitting low: honouring the + // bottom margin alone would push its top above the top margin and hide the + // start of the block. Reading order wins — clamp to the top margin instead. + const bottomOnly = 940 - 800; // 140 if we only chased the bottom + const topRoom = 240 - 200; // 40 before the top slips under the margin + const delta = computeVimScrollDelta(viewport, { top: 240, bottom: 940 }, band); + expect(delta).toBe(Math.min(bottomOnly, topRoom)); + expect(delta).toBe(40); + }); + + test('clears the top HUD for a too-tall target whose top is occluded', () => { + // A block spanning 100..900 is taller than the band and straddles both + // edges. Its top sits behind the top HUD (100 < 200), so reveal the start + // of the block by scrolling up to the top margin — reading order wins. + expect(computeVimScrollDelta(viewport, { top: 100, bottom: 900 }, band)).toBe(-100); + }); + + test('leaves a too-tall target straddling the band untouched once its top is clear', () => { + // Top already at the margin, bottom past it: moving either way would hide an + // edge, so hold position. + expect(computeVimScrollDelta(viewport, { top: 200, bottom: 900 }, band)).toBe(0); + }); + + test('accounts for a viewport offset from the page top', () => { + const offset = { top: 300, height: 400 }; // safe band = page [400, 500] + const smallBand = { topMargin: 100, bottomMargin: 200 }; + // Target at page y=650..680 is below the safe bottom (500) → scroll down 180. + expect(computeVimScrollDelta(offset, { top: 650, bottom: 680 }, smallBand)).toBe(180); + }); +}); + +describe('resolveVimScrollMargin', () => { + test('uses the 20% ratio in the ordinary range', () => { + expect(resolveVimScrollMargin(600)).toBe(120); + }); + + test('clamps short viewports up to the minimum', () => { + expect(resolveVimScrollMargin(50)).toBe(VIM_SCROLL_MARGIN_MIN); + }); + + test('clamps tall viewports down to the maximum', () => { + expect(resolveVimScrollMargin(4000)).toBe(VIM_SCROLL_MARGIN_MAX); + }); +}); + +describe.if(hasDom)('scrollVimTargetIntoView', () => { + function stubRect(element: HTMLElement, rect: Partial): void { + const full = { top: 0, bottom: 0, left: 0, right: 0, width: 0, height: 0, x: 0, y: 0 }; + const merged = { ...full, ...rect } as DOMRect; + element.getBoundingClientRect = () => merged; + } + + function buildViewport( + clientHeight: number, + opts: { withAttribute?: boolean } = {}, + ): { + viewport: HTMLElement; + target: HTMLElement; + } { + const viewportEl = document.createElement('div'); + // The real app scrolls a native
with no OverlayScrollbars attribute — + // the caller passes the viewport explicitly. Only opt into the attribute + // when a test is exercising the closest() fallback. + if (opts.withAttribute) { + viewportEl.setAttribute('data-overlayscrollbars-viewport', ''); + } + Object.defineProperty(viewportEl, 'clientHeight', { + configurable: true, + value: clientHeight, + }); + viewportEl.scrollTop = 0; + const target = document.createElement('p'); + viewportEl.appendChild(target); + document.body.appendChild(viewportEl); + return { viewport: viewportEl, target }; + } + + test('scrolls a target out of the bottom HUD band via the explicit viewport', () => { + const { viewport: viewportEl, target } = buildViewport(1000); + stubRect(viewportEl, { top: 0, height: 1000 }); + // Margin = clamp(1000 * 0.2) = 160. Target at 950..970 is behind it. + stubRect(target, { top: 950, bottom: 970, height: 20, width: 100 }); + + // The native-scroll host carries no attribute; the caller passes it in. + scrollVimTargetIntoView(target, viewportEl); + + // 970 - (1000 - 160) = 130. + expect(viewportEl.scrollTop).toBe(130); + document.body.replaceChildren(); + }); + + test('falls back to the OverlayScrollbars attribute when no viewport is passed', () => { + const { viewport: viewportEl, target } = buildViewport(1000, { + withAttribute: true, + }); + stubRect(viewportEl, { top: 0, height: 1000 }); + stubRect(target, { top: 950, bottom: 970, height: 20, width: 100 }); + + scrollVimTargetIntoView(target); + + expect(viewportEl.scrollTop).toBe(130); + document.body.replaceChildren(); + }); + + test('leaves scrollTop untouched when the target is already safe', () => { + const { viewport: viewportEl, target } = buildViewport(1000); + stubRect(viewportEl, { top: 0, height: 1000 }); + stubRect(target, { top: 480, bottom: 500, height: 20, width: 100 }); + + scrollVimTargetIntoView(target, viewportEl); + + expect(viewportEl.scrollTop).toBe(0); + document.body.replaceChildren(); + }); + + test('widens the top band to clear a sticky action bar', () => { + const { viewport: viewportEl, target } = buildViewport(1000); + stubRect(viewportEl, { top: 0, height: 1000 }); + const sticky = document.createElement('div'); + sticky.setAttribute('data-sticky-actions', ''); + viewportEl.appendChild(sticky); + stubRect(sticky, { top: 0, bottom: 300, height: 300 }); // taller than the 160 ratio band + // Target at 250..270 sits under the sticky bar (bottom 300 → topMargin 308). + stubRect(target, { top: 250, bottom: 270, height: 20, width: 100 }); + + scrollVimTargetIntoView(target, viewportEl); + + // 250 - 308 = -58, scrolled up. + expect(viewportEl.scrollTop).toBe(-58); + document.body.replaceChildren(); + }); + + test('falls back to scrollIntoView when there is no scroll viewport', () => { + const orphan = document.createElement('p'); + document.body.appendChild(orphan); + let called = false; + orphan.scrollIntoView = () => { + called = true; + }; + + scrollVimTargetIntoView(orphan); + + expect(called).toBe(true); + document.body.replaceChildren(); + }); + + test('ignores a zero-size target (unrendered / collapsed)', () => { + const { viewport: viewportEl, target } = buildViewport(1000); + stubRect(viewportEl, { top: 0, height: 1000 }); + stubRect(target, { top: 0, bottom: 0, height: 0, width: 0 }); + viewportEl.scrollTop = 42; + + scrollVimTargetIntoView(target, viewportEl); + + expect(viewportEl.scrollTop).toBe(42); + document.body.replaceChildren(); + }); +}); diff --git a/packages/ui/utils/vimScroll.ts b/packages/ui/utils/vimScroll.ts new file mode 100644 index 000000000..5a4b489db --- /dev/null +++ b/packages/ui/utils/vimScroll.ts @@ -0,0 +1,125 @@ +/** + * Keep the Vim cursor clear of the HUD bands that hug the viewport edges. + * + * `Element.scrollIntoView({ block: 'nearest' })` parks a target flush against + * the nearest viewport edge — exactly where the sticky action bar (top) and the + * key HUD / status pill (bottom) float. Keyboard motion then lands the caret + * behind an overlay, while a mouse wheel (which the browser lets overshoot) + * keeps the same text nearer the centre. These helpers reproduce that mouse + * feel: a target inside the safe band never scrolls, and one that strays into a + * HUD band is revealed with a margin instead of being pinned to the edge. + */ + +/** A viewport's vertical geometry, relative to the page. */ +export interface VimScrollViewportRect { + readonly top: number; + readonly height: number; +} + +/** A target's vertical extent, relative to the page. */ +export interface VimScrollTargetRect { + readonly top: number; + readonly bottom: number; +} + +/** The occluded strips to keep the caret out of, measured from each edge. */ +export interface VimScrollBand { + readonly topMargin: number; + readonly bottomMargin: number; +} + +/** Fraction of the viewport height reserved as a HUD band at each edge. */ +export const VIM_SCROLL_MARGIN_RATIO = 0.2; +/** Lower clamp so short viewports still leave a usable margin. */ +export const VIM_SCROLL_MARGIN_MIN = 24; +/** Upper clamp so tall viewports do not reserve most of the screen. */ +export const VIM_SCROLL_MARGIN_MAX = 160; + +/** + * How far to move `scrollTop` so `target` clears the HUD bands. + * + * Returns a signed delta (negative scrolls up, positive scrolls down) or `0` + * when the target already sits inside the safe band. A target taller than the + * band is aligned to its top edge — reading order wins, so the start of the + * block is never pushed above the top margin to chase its bottom. + */ +export function computeVimScrollDelta( + viewport: VimScrollViewportRect, + target: VimScrollTargetRect, + band: VimScrollBand, +): number { + const relativeTop = target.top - viewport.top; + const relativeBottom = target.bottom - viewport.top; + const safeTop = band.topMargin; + const safeBottom = viewport.height - band.bottomMargin; + + // Behind the top HUD → scroll up just enough to reach the top margin. + if (relativeTop < safeTop) return relativeTop - safeTop; + + // Behind the bottom HUD → scroll down, but never past the point where the + // target's top would slip under the top margin. + if (relativeBottom > safeBottom) { + const bottomDelta = relativeBottom - safeBottom; + const topRoom = relativeTop - safeTop; + return Math.min(bottomDelta, Math.max(0, topRoom)); + } + + return 0; +} + +/** Resolve the ratio-based HUD margin, clamped for very short or tall viewports. */ +export function resolveVimScrollMargin(viewportHeight: number): number { + return Math.min( + Math.max(viewportHeight * VIM_SCROLL_MARGIN_RATIO, VIM_SCROLL_MARGIN_MIN), + VIM_SCROLL_MARGIN_MAX, + ); +} + +/** + * Scroll `element` into view while keeping it clear of the Vim HUD bands. + * + * `scrollViewport` is the element that actually scrolls — the caller passes the + * value it already holds from ScrollViewportContext (the same node the reticle + * measures against), because the native-scroll host carries no + * `[data-overlayscrollbars-viewport]` attribute to rediscover. When it is + * absent the helper still tries that attribute (real-OverlayScrollbars hosts) + * and finally the historical `scrollIntoView({ block: 'nearest' })`, so + * behaviour never regresses. The top band also clears the sticky action bar + * when present, so the caret is not hidden behind it on short documents. + */ +export function scrollVimTargetIntoView( + element: HTMLElement, + scrollViewport?: HTMLElement | null, +): void { + // Prefer the viewport the reticle geometry already tracks (the native-scroll + // host fed through ScrollViewportContext). Fall back to the OverlayScrollbars + // attribute for hosts that mount the real library, then to the historical + // `scrollIntoView` so behaviour never regresses when neither is available. + const viewport = + scrollViewport + ?? element.closest('[data-overlayscrollbars-viewport]'); + if (!viewport) { + element.scrollIntoView({ block: 'nearest' }); + return; + } + + const viewportRect = viewport.getBoundingClientRect(); + const targetRect = element.getBoundingClientRect(); + if (targetRect.height === 0 && targetRect.width === 0) return; + + const margin = resolveVimScrollMargin(viewport.clientHeight); + const stickyBottom = viewport + .querySelector('[data-sticky-actions]') + ?.getBoundingClientRect().bottom; + const topMargin = stickyBottom !== undefined + ? Math.max(margin, stickyBottom - viewportRect.top + 8) + : margin; + + const delta = computeVimScrollDelta( + { top: viewportRect.top, height: viewport.clientHeight }, + { top: targetRect.top, bottom: targetRect.bottom }, + { topMargin, bottomMargin: margin }, + ); + if (delta === 0) return; + viewport.scrollTop += delta; +}