Skip to content

Commit de51575

Browse files
committed
fix(vim): keep j/k cursor clear of HUD bands when scrolling
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.
1 parent 7682628 commit de51575

4 files changed

Lines changed: 350 additions & 18 deletions

File tree

packages/ui/components/Viewer.tsx

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,9 @@ export const Viewer = forwardRef<ViewerHandle, ViewerProps>(({
248248
}
249249
};
250250
const containerRef = useRef<HTMLDivElement>(null);
251+
// The element that actually scrolls; shared by the Vim scroll math, the
252+
// sticky-header observer, and the reticle geometry.
253+
const scrollViewport = useScrollViewport();
251254
// The badge cluster (repo chips / diff badge) is absolutely positioned in the
252255
// card's top padding. One row fits; a second row (diff badge) or mobile
253256
// wrapping outgrows the padding and lands on the document's first heading.
@@ -461,6 +464,7 @@ export const Viewer = forwardRef<ViewerHandle, ViewerProps>(({
461464
}, []);
462465
const vim = useVimSelection({
463466
containerRef,
467+
scrollViewport,
464468
enabled: vimModeActive,
465469
hudEnabled: vimHudEnabled,
466470
blocked: vimBlocked,
@@ -535,16 +539,15 @@ export const Viewer = forwardRef<ViewerHandle, ViewerProps>(({
535539
// Detect when sticky action bar is "stuck" to show card background.
536540
// The IntersectionObserver root must be the actual scroll element — the
537541
// OverlayScrollArea viewport — not the <main> host, which doesn't scroll.
538-
const stickyScrollViewport = useScrollViewport();
539542
useEffect(() => {
540-
if (!stickyActions || !stickySentinelRef.current || !stickyScrollViewport) return;
543+
if (!stickyActions || !stickySentinelRef.current || !scrollViewport) return;
541544
const observer = new IntersectionObserver(
542545
([entry]) => setIsStuck(!entry.isIntersecting),
543-
{ root: stickyScrollViewport, threshold: 0 }
546+
{ root: scrollViewport, threshold: 0 }
544547
);
545548
observer.observe(stickySentinelRef.current);
546549
return () => observer.disconnect();
547-
}, [stickyActions, stickyScrollViewport]);
550+
}, [stickyActions, scrollViewport]);
548551

549552
useEffect(() => {
550553
const handleHashChange = () => {
@@ -561,7 +564,7 @@ export const Viewer = forwardRef<ViewerHandle, ViewerProps>(({
561564
if (!anchor) return false;
562565

563566
const container = containerRef.current;
564-
if (!container || !stickyScrollViewport) return false;
567+
if (!container || !scrollViewport) return false;
565568

566569
const target = document.getElementById(anchor);
567570
if (!target || !container.contains(target)) return false;
@@ -573,27 +576,27 @@ export const Viewer = forwardRef<ViewerHandle, ViewerProps>(({
573576
const headerOffset = stickyActionsEl
574577
? stickyActionsEl.getBoundingClientRect().height + stickyTop
575578
: 0;
576-
const containerRect = stickyScrollViewport.getBoundingClientRect();
579+
const containerRect = scrollViewport.getBoundingClientRect();
577580
const targetRect = target.getBoundingClientRect();
578581
const relativeTop = targetRect.top - containerRect.top;
579-
const offsetPosition = stickyScrollViewport.scrollTop + relativeTop - headerOffset;
582+
const offsetPosition = scrollViewport.scrollTop + relativeTop - headerOffset;
580583

581-
stickyScrollViewport.scrollTo({
584+
scrollViewport.scrollTo({
582585
top: Math.max(0, offsetPosition),
583586
behavior: 'smooth',
584587
});
585588
return true;
586-
}, [stickyScrollViewport]);
589+
}, [scrollViewport]);
587590

588591
useEffect(() => {
589-
if (!stickyScrollViewport || !locationHash || lastAutoScrolledHashRef.current === locationHash) return;
592+
if (!scrollViewport || !locationHash || lastAutoScrolledHashRef.current === locationHash) return;
590593
const timer = window.setTimeout(() => {
591594
if (scrollToAnchor(locationHash)) {
592595
lastAutoScrolledHashRef.current = locationHash;
593596
}
594597
}, 0);
595598
return () => window.clearTimeout(timer);
596-
}, [blocks, locationHash, scrollToAnchor, stickyScrollViewport]);
599+
}, [blocks, locationHash, scrollToAnchor, scrollViewport]);
597600

598601
// Use the native copy event so clipboard writes are synchronous (Safari
599602
// rejects the async navigator.clipboard API outside the user-gesture window).

packages/ui/hooks/useVimSelection.ts

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -47,11 +47,14 @@ import {
4747
type VimVisualBlockState,
4848
type VimVisualState,
4949
} from '../utils/vimNavigation';
50+
import { scrollVimTargetIntoView } from '../utils/vimScroll';
5051
import { useVimDocumentFocus } from './useVimDocumentFocus';
5152

5253
/** Inputs required by the Markdown semantic Vim controller. */
5354
export interface UseVimSelectionOptions {
5455
readonly containerRef: RefObject<HTMLElement | null>;
56+
/** The element that actually scrolls (ScrollViewportContext value). */
57+
readonly scrollViewport?: HTMLElement | null;
5558
readonly enabled: boolean;
5659
readonly hudEnabled: boolean;
5760
readonly blocked: boolean;
@@ -234,6 +237,7 @@ function applyVisualBlockSelection(
234237
*/
235238
export function useVimSelection({
236239
containerRef,
240+
scrollViewport,
237241
enabled,
238242
hudEnabled,
239243
blocked,
@@ -257,6 +261,11 @@ export function useVimSelection({
257261
const pointerFocusRef = useRef(false);
258262
const restoringFocusRef = useRef(false);
259263

264+
// Read the live scroll viewport without adding a dependency to every
265+
// navigation callback below.
266+
const scrollViewportRef = useRef(scrollViewport);
267+
scrollViewportRef.current = scrollViewport;
268+
260269
const setState = useCallback((next: VimSelectionState) => {
261270
stateRef.current = next;
262271
setStateValue(next);
@@ -283,7 +292,7 @@ export function useVimSelection({
283292
const next: VimBlockState = { phase: 'block', targetKey: initial.key };
284293
setState(next);
285294
window.getSelection()?.removeAllRanges();
286-
initial.element.scrollIntoView({ block: 'nearest' });
295+
scrollVimTargetIntoView(initial.element, scrollViewportRef.current);
287296
return next;
288297
}, [containerRef, setState]);
289298

@@ -358,7 +367,7 @@ export function useVimSelection({
358367
const setSemanticTarget = useCallback((target: SemanticTarget) => {
359368
setState(semanticStateForTarget(target));
360369
window.getSelection()?.removeAllRanges();
361-
target.element.scrollIntoView({ block: 'nearest' });
370+
scrollVimTargetIntoView(target.element, scrollViewportRef.current);
362371
}, [setState]);
363372

364373
const updateTextState = useCallback((
@@ -373,9 +382,9 @@ export function useVimSelection({
373382
normalized.cursor,
374383
normalized.phase === 'visual' ? normalized.anchor : null,
375384
);
376-
resolveTextPosition(graph.container, normalized.cursor)
377-
?.node.parentElement
378-
?.scrollIntoView({ block: 'nearest' });
385+
const cursorParent = resolveTextPosition(graph.container, normalized.cursor)
386+
?.node.parentElement;
387+
if (cursorParent) scrollVimTargetIntoView(cursorParent, scrollViewportRef.current);
379388
}, [setState]);
380389

381390
const enterTextAtTarget = useCallback((
@@ -420,7 +429,7 @@ export function useVimSelection({
420429
if (!getTextElementBounds(graph.container, block.element)) return false;
421430
setState(next);
422431
applyVisualBlockSelection(graph, next);
423-
block.element.scrollIntoView({ block: 'nearest' });
432+
scrollVimTargetIntoView(block.element, scrollViewportRef.current);
424433
return true;
425434
}, [setState]);
426435

@@ -730,7 +739,7 @@ export function useVimSelection({
730739
};
731740
setState(nextState);
732741
applyVisualBlockSelection(graph, nextState);
733-
next.element.scrollIntoView({ block: 'nearest' });
742+
scrollVimTargetIntoView(next.element, scrollViewportRef.current);
734743
return true;
735744
}
736745
if (key === 'o') {
Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
1+
import { describe, expect, test } from 'bun:test';
2+
import {
3+
VIM_SCROLL_MARGIN_MAX,
4+
VIM_SCROLL_MARGIN_MIN,
5+
computeVimScrollDelta,
6+
resolveVimScrollMargin,
7+
scrollVimTargetIntoView,
8+
} from './vimScroll';
9+
10+
const hasDom = typeof document !== 'undefined';
11+
12+
// A 1000px-tall viewport whose top edge sits at page y=0, with a 200px HUD band
13+
// reserved at each edge — the safe band is [200, 800].
14+
const viewport = { top: 0, height: 1000 };
15+
const band = { topMargin: 200, bottomMargin: 200 };
16+
17+
describe('computeVimScrollDelta', () => {
18+
test('leaves a target already inside the safe band untouched', () => {
19+
expect(computeVimScrollDelta(viewport, { top: 400, bottom: 460 }, band)).toBe(0);
20+
// Flush against the inner edges of the band still counts as safe.
21+
expect(computeVimScrollDelta(viewport, { top: 200, bottom: 800 }, band)).toBe(0);
22+
});
23+
24+
test('reveals a target parked behind the bottom HUD (the j-to-bottom bug)', () => {
25+
// scrollIntoView({ block: 'nearest' }) would pin this line at y≈980, behind
26+
// the bottom HUD. We instead scroll it down to the bottom margin at y=800.
27+
const delta = computeVimScrollDelta(viewport, { top: 960, bottom: 980 }, band);
28+
expect(delta).toBe(180); // 980 - (1000 - 200)
29+
});
30+
31+
test('reveals a target parked behind the top HUD (the k-to-top bug)', () => {
32+
// A line at y≈20 sits under the sticky action bar; scroll up to the margin.
33+
const delta = computeVimScrollDelta(viewport, { top: 20, bottom: 40 }, band);
34+
expect(delta).toBe(-180); // 20 - 200
35+
});
36+
37+
test('aligns a target taller than the band to its top edge, not its bottom', () => {
38+
// A 700px block (taller than the 600px safe band) sitting low: honouring the
39+
// bottom margin alone would push its top above the top margin and hide the
40+
// start of the block. Reading order wins — clamp to the top margin instead.
41+
const bottomOnly = 940 - 800; // 140 if we only chased the bottom
42+
const topRoom = 240 - 200; // 40 before the top slips under the margin
43+
const delta = computeVimScrollDelta(viewport, { top: 240, bottom: 940 }, band);
44+
expect(delta).toBe(Math.min(bottomOnly, topRoom));
45+
expect(delta).toBe(40);
46+
});
47+
48+
test('clears the top HUD for a too-tall target whose top is occluded', () => {
49+
// A block spanning 100..900 is taller than the band and straddles both
50+
// edges. Its top sits behind the top HUD (100 < 200), so reveal the start
51+
// of the block by scrolling up to the top margin — reading order wins.
52+
expect(computeVimScrollDelta(viewport, { top: 100, bottom: 900 }, band)).toBe(-100);
53+
});
54+
55+
test('leaves a too-tall target straddling the band untouched once its top is clear', () => {
56+
// Top already at the margin, bottom past it: moving either way would hide an
57+
// edge, so hold position.
58+
expect(computeVimScrollDelta(viewport, { top: 200, bottom: 900 }, band)).toBe(0);
59+
});
60+
61+
test('accounts for a viewport offset from the page top', () => {
62+
const offset = { top: 300, height: 400 }; // safe band = page [400, 500]
63+
const smallBand = { topMargin: 100, bottomMargin: 200 };
64+
// Target at page y=650..680 is below the safe bottom (500) → scroll down 180.
65+
expect(computeVimScrollDelta(offset, { top: 650, bottom: 680 }, smallBand)).toBe(180);
66+
});
67+
});
68+
69+
describe('resolveVimScrollMargin', () => {
70+
test('uses the 20% ratio in the ordinary range', () => {
71+
expect(resolveVimScrollMargin(600)).toBe(120);
72+
});
73+
74+
test('clamps short viewports up to the minimum', () => {
75+
expect(resolveVimScrollMargin(50)).toBe(VIM_SCROLL_MARGIN_MIN);
76+
});
77+
78+
test('clamps tall viewports down to the maximum', () => {
79+
expect(resolveVimScrollMargin(4000)).toBe(VIM_SCROLL_MARGIN_MAX);
80+
});
81+
});
82+
83+
describe.if(hasDom)('scrollVimTargetIntoView', () => {
84+
function stubRect(element: HTMLElement, rect: Partial<DOMRect>): void {
85+
const full = { top: 0, bottom: 0, left: 0, right: 0, width: 0, height: 0, x: 0, y: 0 };
86+
const merged = { ...full, ...rect } as DOMRect;
87+
element.getBoundingClientRect = () => merged;
88+
}
89+
90+
function buildViewport(
91+
clientHeight: number,
92+
opts: { withAttribute?: boolean } = {},
93+
): {
94+
viewport: HTMLElement;
95+
target: HTMLElement;
96+
} {
97+
const viewportEl = document.createElement('div');
98+
// The real app scrolls a native <div> with no OverlayScrollbars attribute —
99+
// the caller passes the viewport explicitly. Only opt into the attribute
100+
// when a test is exercising the closest() fallback.
101+
if (opts.withAttribute) {
102+
viewportEl.setAttribute('data-overlayscrollbars-viewport', '');
103+
}
104+
Object.defineProperty(viewportEl, 'clientHeight', {
105+
configurable: true,
106+
value: clientHeight,
107+
});
108+
viewportEl.scrollTop = 0;
109+
const target = document.createElement('p');
110+
viewportEl.appendChild(target);
111+
document.body.appendChild(viewportEl);
112+
return { viewport: viewportEl, target };
113+
}
114+
115+
test('scrolls a target out of the bottom HUD band via the explicit viewport', () => {
116+
const { viewport: viewportEl, target } = buildViewport(1000);
117+
stubRect(viewportEl, { top: 0, height: 1000 });
118+
// Margin = clamp(1000 * 0.2) = 160. Target at 950..970 is behind it.
119+
stubRect(target, { top: 950, bottom: 970, height: 20, width: 100 });
120+
121+
// The native-scroll host carries no attribute; the caller passes it in.
122+
scrollVimTargetIntoView(target, viewportEl);
123+
124+
// 970 - (1000 - 160) = 130.
125+
expect(viewportEl.scrollTop).toBe(130);
126+
document.body.replaceChildren();
127+
});
128+
129+
test('falls back to the OverlayScrollbars attribute when no viewport is passed', () => {
130+
const { viewport: viewportEl, target } = buildViewport(1000, {
131+
withAttribute: true,
132+
});
133+
stubRect(viewportEl, { top: 0, height: 1000 });
134+
stubRect(target, { top: 950, bottom: 970, height: 20, width: 100 });
135+
136+
scrollVimTargetIntoView(target);
137+
138+
expect(viewportEl.scrollTop).toBe(130);
139+
document.body.replaceChildren();
140+
});
141+
142+
test('leaves scrollTop untouched when the target is already safe', () => {
143+
const { viewport: viewportEl, target } = buildViewport(1000);
144+
stubRect(viewportEl, { top: 0, height: 1000 });
145+
stubRect(target, { top: 480, bottom: 500, height: 20, width: 100 });
146+
147+
scrollVimTargetIntoView(target, viewportEl);
148+
149+
expect(viewportEl.scrollTop).toBe(0);
150+
document.body.replaceChildren();
151+
});
152+
153+
test('widens the top band to clear a sticky action bar', () => {
154+
const { viewport: viewportEl, target } = buildViewport(1000);
155+
stubRect(viewportEl, { top: 0, height: 1000 });
156+
const sticky = document.createElement('div');
157+
sticky.setAttribute('data-sticky-actions', '');
158+
viewportEl.appendChild(sticky);
159+
stubRect(sticky, { top: 0, bottom: 300, height: 300 }); // taller than the 160 ratio band
160+
// Target at 250..270 sits under the sticky bar (bottom 300 → topMargin 308).
161+
stubRect(target, { top: 250, bottom: 270, height: 20, width: 100 });
162+
163+
scrollVimTargetIntoView(target, viewportEl);
164+
165+
// 250 - 308 = -58, scrolled up.
166+
expect(viewportEl.scrollTop).toBe(-58);
167+
document.body.replaceChildren();
168+
});
169+
170+
test('falls back to scrollIntoView when there is no scroll viewport', () => {
171+
const orphan = document.createElement('p');
172+
document.body.appendChild(orphan);
173+
let called = false;
174+
orphan.scrollIntoView = () => {
175+
called = true;
176+
};
177+
178+
scrollVimTargetIntoView(orphan);
179+
180+
expect(called).toBe(true);
181+
document.body.replaceChildren();
182+
});
183+
184+
test('ignores a zero-size target (unrendered / collapsed)', () => {
185+
const { viewport: viewportEl, target } = buildViewport(1000);
186+
stubRect(viewportEl, { top: 0, height: 1000 });
187+
stubRect(target, { top: 0, bottom: 0, height: 0, width: 0 });
188+
viewportEl.scrollTop = 42;
189+
190+
scrollVimTargetIntoView(target, viewportEl);
191+
192+
expect(viewportEl.scrollTop).toBe(42);
193+
document.body.replaceChildren();
194+
});
195+
});

0 commit comments

Comments
 (0)