diff --git a/src/js/src/components.ts b/src/js/src/components.ts index f8da299..c9b5ef6 100644 --- a/src/js/src/components.ts +++ b/src/js/src/components.ts @@ -191,6 +191,19 @@ export function Form({ onSubmitCallback, formClass }: FormProps): null { // unmount/remount during route transitions. const _scrollPositions: Record = {}; +// How long (wall-clock) to keep re-asserting a restored scroll position while the +// destination route's content is still streaming in from the server. A fixed +// *frame* count is unreliable because frame cadence varies with CPU load: under a +// loaded CI runner the document can still be short (content not yet rendered) when +// a small frame budget expires, so `scrollTo` clamps against the not-yet-tall page +// and the position is silently lost. A time window is robust to both fast and slow +// frame cadences. The loop stops as soon as the target is reached, so a generous +// window only buys time for slow content — it never fights the user's own scroll. +const _scrollRestoreWindowMs = 1000; + +// Sub-pixel tolerance for considering a scroll position "reached". +const _scrollTolerancePx = 2; + /** * ScrollRestoration component that saves and restores scroll positions across * client-side navigation. @@ -248,21 +261,34 @@ export function ScrollRestoration({}: ScrollRestorationProps): null { React.useEffect(() => { const key = window.location.pathname; const pos = _scrollPositions[key]; - if (pos) { - // Retry across animation frames — Preact may perform multiple - // render commits that reset scroll. - let remaining = 10; - const tryRestore = () => { - window.scrollTo(pos.x, pos.y); - if ( - (window.scrollY !== pos.y || window.scrollX !== pos.x) && - --remaining > 0 - ) { - requestAnimationFrame(tryRestore); - } - }; - requestAnimationFrame(tryRestore); + if (!pos) { + return; } + + const reached = () => + Math.abs(window.scrollY - pos.y) <= _scrollTolerancePx && + Math.abs(window.scrollX - pos.x) <= _scrollTolerancePx; + + // Retry across animation frames until the target is reached, bounded by a + // wall-clock window instead of a frame count. After a client-side navigation + // the destination content is streamed in by the server, so the document can + // still be too short to reach `pos` and scrollTo clamps against it; a small + // frame budget can also expire far too early when frame cadence drops under + // CPU load. Both cases would otherwise silently drop the restored position. + const deadline = performance.now() + _scrollRestoreWindowMs; + let frame = requestAnimationFrame(function tryRestore() { + // Stop the moment the target is in place so we never fight scrolling the + // user performs after the position has been restored. + if (reached() || performance.now() >= deadline) { + return; + } + window.scrollTo(pos.x, pos.y); + frame = requestAnimationFrame(tryRestore); + }); + + // Effect cleanup runs before the next render's effect, so a re-render (or + // unmount) cancels this attempt and no competing loops are left running. + return () => cancelAnimationFrame(frame); }); return null; diff --git a/tests/test_router.py b/tests/test_router.py index 672c490..a11e63b 100644 --- a/tests/test_router.py +++ b/tests/test_router.py @@ -684,23 +684,27 @@ def sample(): async def test_scroll_restoration_preserves_scroll(display: DisplayFixture): - """Verify scroll position is preserved when navigating back.""" + """Verify the exact scroll position is restored when navigating back.""" @component def scroll_page(): - tall_content = [html.div({"style": {"height": "1500px"}}, f"Section {i}") for i in range(10)] - link_list = link({"to": "/other", "id": "to-other"}, "Go to other", key="to-other") + tall_content = [html.div({"style": {"height": "1500px"}}, f"Section {i}") for i in range(8)] return scroll_restoration( html.h1({"id": "scroll-page"}, "Scroll Page"), + # A short spacer places the forward link roughly 550px down, so it is + # inside the viewport when the page is scrolled to ``target_y``. That + # keeps the navigation click from scrolling the page and changing the + # position captured at navigation time. + html.div({"style": {"height": "550px"}}, "Spacer"), + link({"to": "/other", "id": "to-other"}, "Go to other", key="to-other"), *tall_content, - link_list, ) @component def other_page(): return scroll_restoration( html.h1({"id": "other-page"}, "Other Page"), - link({"to": "/", "id": "back-to-scroll"}, "Back to scroll page", key="back-to-scroll"), + html.div({"style": {"height": "2000px"}}, "Other content"), ) @component @@ -712,26 +716,37 @@ def sample(): await display.show(sample) - # Wait for the scroll page to render + # Wait for the scroll page and its tall content to be laid out so the target + # scroll position is actually reachable. await display.page.wait_for_selector("#scroll-page") - - # Scroll down 500px - await display.page.evaluate("window.scrollTo(0, 500)") - scroll_y = await display.page.evaluate("window.scrollY") - assert scroll_y >= 500, f"Expected scrollY >= 500, got {scroll_y}" - - # Navigate to /other via link + await display.page.wait_for_function("document.documentElement.scrollHeight > 10000", timeout=10000) + + # Scroll to a known position that is neither the top nor the bottom, so a + # wrong-but-plausible outcome (staying at 0, or clamping to the page bottom) + # cannot pass for the wrong reason. + target_y = 500 + await display.page.evaluate(f"window.scrollTo(0, {target_y})") + await display.page.wait_for_function(f"Math.abs(window.scrollY - {target_y}) <= 1", timeout=5000) + + # The forward link sits within the viewport at ``target_y`` (see the spacer in + # ``scroll_page``), so Playwright's scroll-into-view before the click is a + # no-op and the position we scrolled to is exactly what gets saved. We use a + # native ``page.click`` (not a synthetic ``element.click()``) because it waits + # for the element to be actionable, which gives ReactPy's ``preventDefault`` + # handler time to attach -- a synthetic click can beat the handler and trigger a + # real anchor navigation (a full reload), wiping the client-side scroll store. await display.page.click("#to-other") await display.page.wait_for_selector("#other-page") - # Navigate back to / via link - await display.page.click("#back-to-scroll") + # Return via the browser's own back action (a genuine ``popstate``). This + # exercises the real restore path without a second click that could race the + # router's event wiring. + await display.page.go_back() await display.page.wait_for_selector("#scroll-page") - # Poll for scroll restoration to apply (it runs in useLayoutEffect which - # fires synchronously after DOM commit, but the browser needs at least one - # frame to paint when scrollTo is called during the same commit). - await display.page.wait_for_function( - "window.scrollY >= 450", - timeout=5000, - ) + # The restore loop keeps re-asserting the saved position until the destination + # content is tall enough to reach it (bounded by a time window), so we expect + # a near-exact match rather than the previous loose lower bound. + await display.page.wait_for_function(f"Math.abs(window.scrollY - {target_y}) <= 5", timeout=5000) + final_y = await display.page.evaluate("window.scrollY") + assert abs(final_y - target_y) <= 5, f"Expected scrollY ~{target_y}, got {final_y}"