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; +}