Release: refuse stale-anchor scroll restore during streaming, Android (#5666) - #5694
Merged
Merged
Conversation
A residual mobile scroll jump-back remained after #5638: while the reader is up in history during a live stream, the anchor captured for a same-frame restore goes stale as the streaming chunk grows content ABOVE the viewport. The anchor's captured topOffset (and the absolute snapshot.top) no longer map to the same content, so realigning to them yanks a still reader backward by a few hundred px per tick. The existing snapshot.userUnpinned===true fallback skip does not cover it: the scrollHeight-collapse scroll event re-pins the state machine (flips userUnpinned back to false) mid-stream, so both the semantic realign (_restoreMessageViewportAnchor) and the absolute snapshot.top fallback still fire. Fix: two guards, both keyed on content-growth-since-capture + absence of recent real input intent, NOT on a scrollTop diff — on an overflow-anchor:auto container the browser itself writes scrollTop to compensate above-viewport growth, so a still reader's scrollTop is not stationary; _recentMessage*ScrollIntent instead reflects genuine touch/wheel/key input, which the browser's anchor layer never sets. - _captureMessageViewportAnchor records scrollHeightAtCapture (+scrollTopAtCapture). - _restoreMessageViewportAnchor refuses the realign when content grew since capture, there is no recent intent, and the delta would move scrollTop >8px. - the absolute snapshot.top fallback in _restoreMessageScrollSnapshotSameFrame mirrors the same guard. An actively scrolling reader (recent intent) keeps the legitimate restore; a fresh anchor (no growth) and legacy snapshots without the captured geometry are unaffected. Adds tests/test_issue5637_stale_anchor_guard.py (7 node-harness cases; two are mutation-checked to fail if either guard is removed).
…ck intent test (#5637) Greptile review follow-ups (no behavior change): - Remove the captured-but-never-read scrollTopAtCapture field from _captureMessageViewportAnchor; only scrollHeightAtCapture is consulted. - Rename _grewAbove -> _grewSinceCapture in the realign guard (the check is overall scrollHeight growth since capture, not specifically above-viewport), matching the fallback guard's _grewSinceSnap. - Add a mutation-checked fallback active-intent test: content grew but the reader has recent real input intent, so the absolute snapshot.top restore is kept.
The two streaming stale-anchor guards added for #5637 refuse a scroll restore and rely on the browser's native overflow-anchor layer to hold the viewport. That layer is only active where .messages computes to overflow-anchor:auto (touch viewports). On hover+fine-pointer desktops .messages is overflow-anchor:none, so refusing the restore leaves nothing to hold the reader -> the desktop reader is yanked after above-viewport growth (the same jump the guards fix on mobile), and the fallback also latches _messageUserUnpinned=true. Gate both refusals on _isTouchLikeMessageViewport(container), a matchMedia('(pointer:coarse)') predicate (falling back to the computed overflow-anchor probe) so they fire only where native anchoring can actually hold the viewport. Desktop keeps its semantic scrollTop realign and absolute snapshot.top fallback. matchMedia is used rather than the computed-anchor probe alone because the realign temporarily writes inline overflowAnchor:none for its own scroll write, which a computed probe would misread mid-realign. Add two desktop regression tests (touch_like=False) covering the exact stale-anchor case on a no-native-anchor viewport; both are mutation-checked (dropping the touch gate makes them fail).
…best-effort (#5637) Address the two non-blocking observations from re-review of the touch-gate fix: 1. Add a direct unit test for _isTouchLikeMessageViewport itself — the guard-wiring tests stub the predicate via a boolean, which validates that the _touchHold term gates the refusal but not the predicate's own mid-realign stability (the claim that motivated choosing matchMedia over the computed overflow-anchor probe). The new _predicate_harness exercises the real predicate + _browserOverflowAnchorActive with mocked matchMedia/getComputedStyle: - test_predicate_stays_true_on_touch_when_inline_anchor_clobbered_to_none: on a pointer:coarse device whose inline overflowAnchor was clobbered to 'none' by a prior realign tick, the predicate must still report touch=true. Mutation-checked: reverting the predicate to the bare computed probe makes exactly this test fail. - test_predicate_false_on_desktop_fine_pointer / _falls_back_to_computed_probe_without_matchmedia cover the desktop and no-matchMedia paths. 2. Note in the predicate comment that the no-matchMedia fallback to the computed probe is best-effort (matchMedia('(pointer:coarse)') is universally supported in every targeted browser, so the primary path is what runs).
…nchor is inert there (#5637) Round-2 gate cert (iOS CORE): the touch predicate _isTouchLikeMessageViewport used matchMedia('(pointer:coarse)'), which is true on BOTH Android and iOS. But overflow-anchor is inert on iOS WebKit (the repo's own static/style.css mobile content-visibility block documents this — it deliberately does not set overflow-anchor:none because it is a no-op on iOS and re-opens the #4856/#5338 jump on Android). So on iOS the stale-anchor refusal fired but its premise (let the native overflow-anchor layer hold the viewport) is false → a scrolled-up iOS reader was left unheld after above-viewport growth, the same class as the round-1 desktop regression, one platform over. Split the platform check: add _isIOSWebKit() (classic iPhone/iPod/iPad UA, plus iPadOS 13+ which masquerades as MacIntel but has maxTouchPoints>1 unlike a real Mac) and exclude it from _isTouchLikeMessageViewport. The refusal now fires ONLY on Android (pointer:coarse AND overflow-anchor actually works); desktop and iOS both keep the explicit semantic realign / absolute snapshot.top restore. Tests: 3 new predicate cases (iPhone, iPadOS-as-Mac, Android control), all mutation-checked — removing the _isIOSWebKit exclusion fails exactly the iOS cases while Android/desktop stay green; broadening _isIOSWebKit to any touch device fails the Android control. The Node harness forces the navigator mock via Object.defineProperty because Node 18+ ships a built-in read-only navigator that a plain assignment silently ignores. Note: iOS Safari cannot be exercised by the Node harness (the 'native anchor holds' postcondition is mocked, never observed), so this needs a real iOS-Safari recording before merge per the gate cert.
Contributor
Comment on lines
+1119
to
+1120
| const _shAtCap=Number(anchor.scrollHeightAtCapture); | ||
| if(Number.isFinite(_shAtCap)){ |
Contributor
There was a problem hiding this comment.
The anchor guard gates on
Number.isFinite(_shAtCap) but omits the > 0 check that the sibling fallback guard uses for _snapSH. Number(null) is 0, which is finite, so an anchor with scrollHeightAtCapture explicitly set to null would enter the guard with _grewSinceCapture = (container.scrollHeight - 0) > 4 always true for any real page. The capture code always writes container.scrollHeight (a positive integer), so the path is unreachable today, but adding the parallel > 0 guard makes the two guards consistent and makes the invariant explicit.
Suggested change
| const _shAtCap=Number(anchor.scrollHeightAtCapture); | |
| if(Number.isFinite(_shAtCap)){ | |
| const _shAtCap=Number(anchor.scrollHeightAtCapture); | |
| if(Number.isFinite(_shAtCap)&&_shAtCap>0){ |
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 — refuse stale-anchor scroll restore during streaming, Android (#5666)
Scroll cluster (3 of 4), shipped solo for regression isolation. gate-cert GREEN round 3 (complete platform matrix: desktop/iOS/Android — Codex+Fable+suite); maintainer trust + code-read approved. Rebuilt + re-verified on master carrying #5685+#5681 (all three scroll fixes coexist, ui.js valid, scope-undef+ruff clean).
pointer:coarsematchMedia probe that resists the inline-override mutation.scrollHeightAtCapturedetects content growth between capture and restore.Gate: scope-undef CLEAN, ruff CLEAN, node -c OK, full suite (running). +522,
static/ui.js+ newtests/test_issue5637_stale_anchor_guard.py(395 lines). Attribution: @allenliang2022.