Release — suppress mobile scroll jump-back on message realign (#5338) - #5352
Merged
Merged
Conversation
…ealign (mobile scroll jump-back) Root cause (mobile-only, never reproduces on desktop): .messages CSS resting overflow-anchor is 'auto' on touch devices but 'none' on hover+fine-pointer desktops (style.css media query). When _restoreMessageViewportAnchor writes scrollTop to realign the reader's anchor row AND content height above the viewport changed in the same frame, a mobile browser's native scroll-anchoring ALSO shifts scrollTop -- the two compensations stack and yank the reader to an unrelated earlier turn. Desktop never has the browser layer, which is why this reproduced only on phones. Fix: _suppressBrowserOverflowAnchor() sets overflow-anchor:none for the JS scrollTop write, releases (restores prior value) next frame. Engages ONLY when computed value is 'auto' (mobile) -- pure no-op on desktop (already none). Verified on isolated debug instance (mobile-viewport Playwright): - mobile auto: 800px above-viewport growth compensation 800px -> 0 (browser layer suppressed) - desktop none: helper returns null, inline value untouched (byte-identical behavior) - streaming: real turn, mid-read follow, 0 jumps, content held - scroll-regression suite green
…er settle window (mobile jump-back) The sync-frame guards (_fixMobileScrollJank / _suppressBrowserOverflowAnchor) only cover the render frame itself. postProcessRenderedMessages() — syntax highlight, inline diff/csv/pdf/html/excalidraw, katex/mermaid — is scheduled a FRAME LATER via requestAnimationFrame(), after those guards have released. Each of those can change the height of rows ABOVE the viewport; on mobile (overflow-anchor:auto) the browser's native anchor engine then compensates scrollTop a SECOND time in that unguarded frame, yanking an unpinned reader to another turn (the residual mobile 往回大跳). Wrap all three deferred post-process dispatches (fast-path cache branch, main render tail, live-tool remount) in _postProcessWithAnchorSuppression(), which routes through the shared _suppressBrowserOverflowAnchor() and holds suppression one extra frame so late media/layout reflow is covered too. Desktop rests at overflow-anchor:none so the wrapper is a verified no-op there. Reproduced on an isolated debug instance with a cloned 1179-message session: above-viewport +350px during the async settle window jumped scrollTop +350 on mobile (auto) and 0 with the wrapper; desktop (none) 0 both ways. static/ui.js only.
…thAnchorSuppression refactor (#5338) Commit 7536f6f routed the deferred post-render dispatches through _postProcessWithAnchorSuppression() (holds overflow-anchor suppression across the async media/layout settle frame, then calls postProcessRenderedMessages). Six pre-existing tests string-matched the old 'requestAnimationFrame(()=>postProcessRenderedMessages(inner))' literal and failed on the rename — behavior is preserved (the wrapper still invokes postProcessRenderedMessages), so this is a test-fix not a code-fix. Per the gate-cert recommendation, the tests now assert the BEHAVIOR chain (post-render is scheduled via _postProcessWithAnchorSuppression, and that wrapper calls postProcessRenderedMessages) rather than the exact rAF literal, so a future wrapper rename can't re-orphan them. Files: test_csv_table_rendering, test_excalidraw_inline_embed, test_issue483_inline_diff_viewer, test_issue484_json_tree_viewer, test_issue347, test_pdf_html_preview. Verified: the 6 updated assertions pass locally (the only local failures are the pre-existing Windows-only WinError 206 command-line-too-long in Node-harness tests, unrelated, green on Linux CI).
…ges node harness (#5338) The Node-executed gate in test_anchor_fallback_ownership.py (test_render_messages_keeps_anchor_owned_turn_out_of_legacy_activity_rebuilds) eval()s the real renderMessages(). Commit 7536f6f made renderMessages schedule its post-render pass via _postProcessWithAnchorSuppression(), but the harness only stubbed postProcessRenderedMessages() — so the eval threw 'ReferenceError: _postProcessWithAnchorSuppression is not defined' and the test failed on Linux CI (shard 2). It passed locally only because Windows hit the unrelated WinError 206 command-line-too-long first, masking the real error. Add a no-op stub for _postProcessWithAnchorSuppression alongside the existing postProcessRenderedMessages stub. Verified by dumping the generated node script to a temp .js file and running 'node file.js' (bypassing the Windows -e length limit): the eval no longer throws and the test's assertions pass.
Contributor
Comment on lines
+890
to
+891
| // stack and yank the reader to an unrelated turn (the mobile jump-back). This is why the | ||
| // bug is mobile-only and never reproduces on a desktop (none) browser. Suppress |
Contributor
There was a problem hiding this comment.
The inline comment on this line has an accidental mid-sentence line-break: "…the mobile jump-back). This is why the" dangles at the end of line 889, turning the sentence split across two comment lines. Consider pulling the trailing fragment onto the next line so the sentence reads cleanly.
Suggested change
| // stack and yank the reader to an unrelated turn (the mobile jump-back). This is why the | |
| // bug is mobile-only and never reproduces on a desktop (none) browser. Suppress | |
| // stack and yank the reader to an unrelated turn (the mobile jump-back). | |
| // This is why the bug is mobile-only and never reproduces on a desktop (none) browser. Suppress |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release — suppress mobile scroll jump-back on message realign (#5338)
PR #5338 by @allenliang2022. On mobile, realigning the viewport after loading older messages caused a visible scroll jump-back: the app's
scrollTopwrite and the browser's ownoverflow-anchormachinery both compensated in the same frame.Fix (
static/ui.js)_suppressBrowserOverflowAnchor(container)setsoverflow-anchor:noneonly when the browser layer is actually active (getComputedStyle().overflowAnchor === 'auto', i.e. mobile), releasing on the nextrequestAnimationFrameand restoring the prior inline value only if still owned.none) is a no-op — helper returns null, nothing changes._fixMobileScrollJank()route through one shared predicate_browserOverflowAnchorActiveso the guards can't drift.Gate
noneleak),_programmaticScrolltiming intact, no interaction bug with the bug: WebUI is unusable on Android - transcript jumps to top on every interaction (regression in v0.51.576) #4856 android fix.test_scheduled_jobs_profile_isolationserial artifacts); 29 targeted scroll/anchor tests + 187 in the Codex focused run.Credit: @allenliang2022.