Skip to content

feat(webview): saveScrollPosition / restoreScrollPosition helpers (#626)#809

Open
jim-daf wants to merge 1 commit intoAwful:developfrom
jim-daf:feat/issue-626-scroll-position-helpers
Open

feat(webview): saveScrollPosition / restoreScrollPosition helpers (#626)#809
jim-daf wants to merge 1 commit intoAwful:developfrom
jim-daf:feat/issue-626-scroll-position-helpers

Conversation

@jim-daf
Copy link
Copy Markdown

@jim-daf jim-daf commented Apr 25, 2026

Towards #626.

The thread WebView swaps inner HTML rather than navigating, so the standard WebView.scrollTo dance does not survive a page change. Two helpers are enough to handle the common case: capture the current scroll offset before a swap, then restore it once the new HTML has rendered.

The restore helper uses requestAnimationFrame to retry until the document is tall enough to actually contain the saved offset, which avoids the classic "scroll fires before images settle and lands too high" problem.

public void saveScrollPosition() {
    runJavascript("window.__awfulSavedScroll = window.scrollY;");
}

public void restoreScrollPosition() {
    runJavascript(
            "(function(){" +
                "var target = window.__awfulSavedScroll;" +
                "if (target == null) { return; }" +
                "function tryScroll() {" +
                    "if (document.documentElement.scrollHeight >= target) {" +
                        "window.scrollTo({top: target});" +
                    "} else {" +
                        "window.requestAnimationFrame(tryScroll);" +
                    "}" +
                "}" +
                "tryScroll();" +
            "})();");
}

Callers in ThreadDisplayFragment and the PM view can opt in by bracketing their setBodyHtml calls with these two methods. I did not wire the call sites in this PR to keep the diff focused.

@jim-daf jim-daf marked this pull request as ready for review April 25, 2026 19:28
Copilot AI review requested due to automatic review settings April 25, 2026 19:28
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds AwfulWebView helpers to persist and reapply scroll position across content swaps, addressing cases where the thread WebView replaces inner HTML rather than navigating.

Changes:

  • Add saveScrollPosition() to capture window.scrollY into a window-scoped variable.
  • Add restoreScrollPosition() to retry restoring scroll via requestAnimationFrame until the page is tall enough.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

"if (target == null) { return; }" +
"function tryScroll() {" +
"if (document.documentElement.scrollHeight >= target) {" +
"window.scrollTo({top: target});" +
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

window.scrollTo({top: target}) uses the options-object overload, which isn’t supported on older Chromium-based Android WebViews (minSdk 24 can still be on an older WebView). If unsupported, this will throw and abort the restore. Prefer the legacy signature (scrollTo(0, target)) or feature-detect before using the options overload.

Suggested change
"window.scrollTo({top: target});" +
"window.scrollTo(0, target);" +

Copilot uses AI. Check for mistakes.
Comment on lines +195 to +200
"function tryScroll() {" +
"if (document.documentElement.scrollHeight >= target) {" +
"window.scrollTo({top: target});" +
"} else {" +
"window.requestAnimationFrame(tryScroll);" +
"}" +
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The height check is using document.documentElement.scrollHeight >= target, but target is a scrollTop value; the maximum reachable scrollTop is scrollHeight - clientHeight. This can cause (a) scrolling to happen too early and get clamped below target with no retries, or (b) the retry loop to run forever if the new document never becomes tall enough. Consider checking scrollHeight - clientHeight >= target and/or verifying window.scrollY reached target, with a bounded retry/fallback (e.g., scroll to bottom after N frames).

Suggested change
"function tryScroll() {" +
"if (document.documentElement.scrollHeight >= target) {" +
"window.scrollTo({top: target});" +
"} else {" +
"window.requestAnimationFrame(tryScroll);" +
"}" +
"var attempts = 0;" +
"var maxAttempts = 60;" +
"function tryScroll() {" +
"var doc = document.documentElement;" +
"var maxScrollTop = Math.max(0, doc.scrollHeight - doc.clientHeight);" +
"if (maxScrollTop >= target) {" +
"window.scrollTo({top: target});" +
"if (window.scrollY >= target || attempts >= maxAttempts) { return; }" +
"} else if (attempts >= maxAttempts) {" +
"window.scrollTo({top: maxScrollTop});" +
"return;" +
"}" +
"attempts++;" +
"window.requestAnimationFrame(tryScroll);" +

Copilot uses AI. Check for mistakes.
Comment on lines +193 to +203
"var target = window.__awfulSavedScroll;" +
"if (target == null) { return; }" +
"function tryScroll() {" +
"if (document.documentElement.scrollHeight >= target) {" +
"window.scrollTo({top: target});" +
"} else {" +
"window.requestAnimationFrame(tryScroll);" +
"}" +
"}" +
"tryScroll();" +
"})();");
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a successful restore, window.__awfulSavedScroll is left set. That makes subsequent restores (or other pages using the same WebView) potentially apply a stale offset unexpectedly. Clearing the saved value once it has been applied would make the helper safer to use.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants