Skip to content

Commit fa8afc8

Browse files
authored
fix(presentation-editor): arrow key scroll-into-view with unconstrained containers (SD-1950) (#2411)
* fix(presentation-editor): revalidate scroll container after first layout (SD-1950) Arrow key navigation was not auto-scrolling to keep the cursor visible when the consumer's container had overflow:auto but no height constraint. The root cause was in #findScrollableAncestor — at setup time it picks the first ancestor with overflow-y:auto|scroll, but cannot verify the element actually constrains content height (content isn't laid out yet). When the consumer sets overflow:auto without a height constraint, the container expands to fit all content instead of scrolling, making scrollHeight equal to clientHeight. This caused three cascading failures: 1. Caret scroll-into-view: getBoundingClientRect() on the ~15000px container always contained the caret, so the scroll condition never triggered. 2. Virtualization: domPainter received the wrong scroll container, computing page visibility against the full container instead of the viewport. 3. Scroll offset calculations: all scroll-relative logic used a container that never actually scrolled. The fix adds #revalidateScrollContainer(), called once after the first layout completes. At that point content is laid out and we can check scrollHeight vs clientHeight. If the detected container isn't actually scrollable, we walk further up the DOM to find one that is, or fall back to window — then update the scroll listener and domPainter reference. * fix(presentation-editor): address scroll container revalidation edge cases Two fixes based on review feedback: 1. Avoid falsely switching away from properly constrained containers. A container with height:600px and overflow:auto may not overflow on first layout if the document is short. The previous check would switch to window in that case and never retry. Now we also verify the container grew beyond the viewport before switching — a clear signal its height is unconstrained. 2. Clear domPainter scroll container when falling back to window. When revalidation resolves to window, the domPainter was keeping the stale HTMLElement reference from #ensurePainter. Now we pass null, which makes the domPainter use viewport-based calculations.
1 parent d23c515 commit fa8afc8

1 file changed

Lines changed: 56 additions & 0 deletions

File tree

packages/super-editor/src/core/presentation-editor/PresentationEditor.ts

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,7 @@ export class PresentationEditor extends EventEmitter {
316316
#editorListeners: Array<{ event: string; handler: (...args: unknown[]) => void }> = [];
317317
#scrollHandler: (() => void) | null = null;
318318
#scrollContainer: Element | Window | null = null;
319+
#scrollContainerValidated = false;
319320
#sectionMetadata: SectionMetadata[] = [];
320321
#documentMode: 'editing' | 'viewing' | 'suggesting' = 'editing';
321322
#inputBridge: PresentationInputBridge | null = null;
@@ -3098,6 +3099,60 @@ export class PresentationEditor extends EventEmitter {
30983099
return win;
30993100
}
31003101

3102+
/**
3103+
* Re-validates the detected scroll container after the first layout completes.
3104+
*
3105+
* At setup time, #findScrollableAncestor picks the first ancestor with
3106+
* overflow-y: auto|scroll — but it can't verify the element actually constrains
3107+
* content height (content isn't laid out yet). A consumer may set overflow:auto
3108+
* on the SuperDoc container without constraining its height, causing the element
3109+
* to expand to fit all content instead of scrolling.
3110+
*
3111+
* After layout, we can check scrollHeight vs clientHeight. If the detected
3112+
* container isn't actually scrollable AND it grew beyond the viewport (ruling
3113+
* out properly constrained containers that simply don't have enough content
3114+
* yet), we walk further up to find one that actually scrolls, or fall back
3115+
* to window.
3116+
*/
3117+
#revalidateScrollContainer(): void {
3118+
if (this.#scrollContainerValidated) return;
3119+
this.#scrollContainerValidated = true;
3120+
3121+
if (!(this.#scrollContainer instanceof Element)) return;
3122+
if (this.#scrollContainer.scrollHeight > this.#scrollContainer.clientHeight + 1) return;
3123+
3124+
// A properly constrained container (e.g. height:600px; overflow:auto) may
3125+
// not be overflowing yet if the document is short. Its clientHeight stays
3126+
// within viewport bounds. Only switch when the container grew beyond the
3127+
// viewport — a clear sign its height is unconstrained.
3128+
const win = this.#scrollContainer.ownerDocument?.defaultView;
3129+
const viewportHeight = win?.innerHeight ?? 0;
3130+
if (this.#scrollContainer.clientHeight <= viewportHeight) return;
3131+
3132+
let el: Element | null = this.#scrollContainer.parentElement;
3133+
let next: Element | Window | null = win ?? null;
3134+
3135+
while (el) {
3136+
const { overflowY } = getComputedStyle(el);
3137+
if ((overflowY === 'auto' || overflowY === 'scroll') && el.scrollHeight > el.clientHeight + 1) {
3138+
next = el;
3139+
break;
3140+
}
3141+
el = el.parentElement;
3142+
}
3143+
3144+
if (!next || next === this.#scrollContainer) return;
3145+
3146+
const prev = this.#scrollContainer;
3147+
prev.removeEventListener('scroll', this.#scrollHandler!);
3148+
this.#scrollContainer = next;
3149+
3150+
if (next instanceof Element) {
3151+
next.addEventListener('scroll', this.#scrollHandler!, { passive: true });
3152+
}
3153+
this.#domPainter?.setScrollContainer?.(next instanceof HTMLElement ? next : null);
3154+
}
3155+
31013156
/**
31023157
* Sets up drag and drop handlers for field annotations and image files.
31033158
*/
@@ -3862,6 +3917,7 @@ export class PresentationEditor extends EventEmitter {
38623917
this.#epochMapper.onLayoutComplete(layoutEpoch);
38633918
this.#selectionSync.onLayoutComplete(layoutEpoch);
38643919
layoutCompleted = true;
3920+
this.#revalidateScrollContainer();
38653921
this.#updatePermissionOverlay();
38663922

38673923
// Reset error state on successful layout

0 commit comments

Comments
 (0)