fix(webui): refuse stale-anchor scroll restore during streaming (#5637)
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).
This commit is contained in:
allenliang2022
parent
f80e641a9a
commit
18cd79797f
2 changed files with 269 additions and 0 deletions
50
static/ui.js
50
static/ui.js
|
|
@ -953,6 +953,12 @@ function _captureMessageViewportAnchor(){
|
|||
key:row&&row.dataset?String(row.dataset.messageAnchorKey||''):'',
|
||||
topOffset:rect.top-containerRect.top,
|
||||
topPadBefore,
|
||||
// Snapshot the scroll geometry at capture so a later realign can detect that
|
||||
// content grew ABOVE the viewport between capture and restore — the streaming
|
||||
// case where the anchor's topOffset is stale and realigning to it would yank a
|
||||
// still reader backward (issue #5637).
|
||||
scrollHeightAtCapture:container.scrollHeight,
|
||||
scrollTopAtCapture:container.scrollTop,
|
||||
};
|
||||
}
|
||||
}
|
||||
|
|
@ -1029,6 +1035,28 @@ function _restoreMessageViewportAnchor(anchor, rawIdxDelta){
|
|||
const containerRect=container.getBoundingClientRect();
|
||||
const rect=row.getBoundingClientRect();
|
||||
const targetTop=Number(anchor.topOffset)||0;
|
||||
// Streaming stale-anchor guard (issue #5637). During a live stream, content grows
|
||||
// ABOVE the viewport between anchor capture and this restore, so the anchor's
|
||||
// captured topOffset is stale and the realign delta becomes a spurious few-hundred-px
|
||||
// value that yanks a still reader backward. Detect it by content growth + absence of
|
||||
// real input intent — NOT by a scrollTop diff, because on an overflow-anchor:auto
|
||||
// container the browser itself moves scrollTop to compensate the growth (so a still
|
||||
// reader's scrollTop is not stationary). _recentMessage*ScrollIntent reflects genuine
|
||||
// touch/wheel/key input, which the browser's anchor layer never writes. If content
|
||||
// grew since capture AND there is no recent input intent AND the realign would move
|
||||
// scrollTop non-trivially, refuse it and let the browser overflow-anchor hold. An
|
||||
// actively scrolling reader (recent intent) keeps the legitimate realign; legacy
|
||||
// snapshots without the captured geometry keep prior behavior.
|
||||
const _realignDelta=(rect.top-containerRect.top)-targetTop;
|
||||
const _shAtCap=Number(anchor.scrollHeightAtCapture);
|
||||
if(Number.isFinite(_shAtCap)){
|
||||
const _grewAbove=(container.scrollHeight-_shAtCap)>4;
|
||||
const _activeIntent=(typeof _recentMessageScrollIntent==='function' && _recentMessageScrollIntent())
|
||||
|| (typeof _recentMessageTouchScrollIntent==='function' && _recentMessageTouchScrollIntent());
|
||||
if(_grewAbove&&!_activeIntent&&Math.abs(_realignDelta)>8){
|
||||
return false;
|
||||
}
|
||||
}
|
||||
_programmaticScroll=true;_programmaticScrollSetAt=performance.now();
|
||||
// Mobile-only jump fix: the resting overflow-anchor on .messages is `auto` on
|
||||
// touch devices (CSS media query keeps it `none` only for hover+fine-pointer
|
||||
|
|
@ -13259,6 +13287,28 @@ function _restoreMessageScrollSnapshotSameFrame(snapshot){
|
|||
const target=(snapshot.pinned===true&&Number.isFinite(bottom))
|
||||
? maxTop-Math.max(0,bottom)
|
||||
: Number(snapshot.top)||0;
|
||||
// Streaming stale-snapshot guard (issue #5637). The userUnpinned check above is
|
||||
// defeated when a live stream re-pins the state machine (a scrollHeight-collapse
|
||||
// scroll event flips userUnpinned back to false even though the reader is up in
|
||||
// history), so this absolute snapshot.top write still fires and yanks a still
|
||||
// reader — snapshot.top was captured before the streaming chunk grew above-viewport
|
||||
// height, so it is stale. Mirror the realign guard: if content grew since the
|
||||
// snapshot AND there is no recent real input intent AND the write would move
|
||||
// scrollTop non-trivially, refuse it and let the browser overflow-anchor hold.
|
||||
// Pinned tail-followers (target is bottom-relative, not snapshot.top) are
|
||||
// unaffected; an actively scrolling reader has intent and keeps the restore.
|
||||
const _snapSH=Number(snapshot.scrollHeight);
|
||||
const _grewSinceSnap=Number.isFinite(_snapSH)&&_snapSH>0&&(el.scrollHeight-_snapSH)>4;
|
||||
const _fbActiveIntent=(typeof _recentMessageScrollIntent==='function' && _recentMessageScrollIntent())
|
||||
|| (typeof _recentMessageTouchScrollIntent==='function' && _recentMessageTouchScrollIntent());
|
||||
if(snapshot.pinned!==true && _grewSinceSnap && !_fbActiveIntent
|
||||
&& Math.abs((Math.max(0,Math.min(target,maxTop)))-el.scrollTop)>8){
|
||||
_lastScrollTop=el.scrollTop;_lastMessageClientHeight=el.clientHeight;
|
||||
_messageUserUnpinned=true;
|
||||
_scrollPinned=false;
|
||||
_nearBottomCount=0;
|
||||
return;
|
||||
}
|
||||
_programmaticScroll=true;_programmaticScrollSetAt=performance.now();
|
||||
el.scrollTop=Math.max(0,Math.min(target,maxTop));
|
||||
}
|
||||
|
|
|
|||
219
tests/test_issue5637_stale_anchor_guard.py
Normal file
219
tests/test_issue5637_stale_anchor_guard.py
Normal file
|
|
@ -0,0 +1,219 @@
|
|||
"""Regression tests for the issue #5637 streaming stale-anchor guards.
|
||||
|
||||
Once 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.
|
||||
The existing `snapshot.userUnpinned===true` fallback skip is defeated because the
|
||||
scrollHeight-collapse scroll event re-pins the state machine (flips userUnpinned back to
|
||||
false) mid-stream.
|
||||
|
||||
Two guards close this, both keyed on content-growth-since-capture + absence of real
|
||||
input intent (NOT a scrollTop diff, which the browser's own overflow-anchor writes on an
|
||||
overflow-anchor:auto container):
|
||||
1. `_restoreMessageViewportAnchor` refuses the realign write.
|
||||
2. the absolute `snapshot.top` fallback in `_restoreMessageScrollSnapshotSameFrame`
|
||||
refuses its write.
|
||||
|
||||
Every behavioral test below is designed to FAIL on the pre-guard code and PASS only with
|
||||
the guard. Node-harness pattern (extractFunc + mock DOM) shared with the sibling scroll
|
||||
regression suites.
|
||||
"""
|
||||
import json
|
||||
import pathlib
|
||||
import shutil
|
||||
import subprocess
|
||||
import tempfile
|
||||
|
||||
import pytest
|
||||
|
||||
ROOT = pathlib.Path(__file__).parent.parent
|
||||
UI_JS_PATH = ROOT / "static" / "ui.js"
|
||||
NODE = shutil.which("node")
|
||||
|
||||
pytestmark = pytest.mark.skipif(NODE is None, reason="node not on PATH")
|
||||
|
||||
|
||||
def _run_node(source: str) -> str:
|
||||
with tempfile.NamedTemporaryFile(
|
||||
"w", suffix=".cjs", encoding="utf-8", dir=ROOT, delete=False
|
||||
) as script:
|
||||
script.write(source)
|
||||
script_path = pathlib.Path(script.name)
|
||||
try:
|
||||
result = subprocess.run(
|
||||
[NODE, str(script_path)], cwd=str(ROOT),
|
||||
capture_output=True, text=True, timeout=30,
|
||||
)
|
||||
finally:
|
||||
script_path.unlink(missing_ok=True)
|
||||
if result.returncode != 0:
|
||||
raise RuntimeError(result.stderr)
|
||||
return result.stdout.strip()
|
||||
|
||||
|
||||
def _extract_func_script(js: str) -> str:
|
||||
prelude = "const src = " + json.dumps(js) + ";\n"
|
||||
body = r"""
|
||||
function extractFunc(name) {
|
||||
const re = new RegExp('function\\s+' + name + '\\s*\\(');
|
||||
const start = src.search(re);
|
||||
if (start < 0) throw new Error(name + ' not found');
|
||||
let i = src.indexOf('{', start);
|
||||
let depth = 1; i++;
|
||||
let str = null, inLine = false, inBlock = false, inRegex = false, prev = '';
|
||||
while (depth > 0 && i < src.length) {
|
||||
const c = src[i], n = src[i + 1];
|
||||
if (inLine) { if (c === '\n') inLine = false; i++; continue; }
|
||||
if (inBlock) { if (c === '*' && n === '/') { inBlock = false; i++; } i++; continue; }
|
||||
if (str) { if (c === '\\') { i += 2; continue; } if (c === str) str = null; i++; continue; }
|
||||
if (inRegex) { if (c === '\\') { i += 2; continue; } if (c === '/') inRegex = false; i++; continue; }
|
||||
if (c === '/' && n === '/') { inLine = true; i += 2; continue; }
|
||||
if (c === '/' && n === '*') { inBlock = true; i += 2; continue; }
|
||||
if (c === '"' || c === "'" || c === '`') { str = c; i++; continue; }
|
||||
if (c === '/' && !'})]0123456789'.includes(prev) && !/[A-Za-z_$]/.test(prev)) { inRegex = true; i++; continue; }
|
||||
if (c === '{') depth++; else if (c === '}') depth--;
|
||||
if (c.trim()) prev = c;
|
||||
i++;
|
||||
}
|
||||
return src.slice(start, i);
|
||||
}"""
|
||||
return prelude + body
|
||||
|
||||
|
||||
# ---- realign guard (_restoreMessageViewportAnchor) -------------------------------
|
||||
|
||||
def _realign_harness(*, anchor_extra: str, cur_scroll_height: int, rect_top: int,
|
||||
top_offset: int, active_intent: bool = False) -> str:
|
||||
js = UI_JS_PATH.read_text(encoding="utf-8")
|
||||
intent_js = "true" if active_intent else "false"
|
||||
return _extract_func_script(js) + f"""
|
||||
let writes = [];
|
||||
let stTop = 90030;
|
||||
const row = {{ getBoundingClientRect(){{ return {{ top: {rect_top} }}; }},
|
||||
getClientRects(){{ return [{{}}]; }}, dataset: {{ messageAnchorKey: 'k1' }} }};
|
||||
const container = {{
|
||||
get scrollTop(){{ return stTop; }}, set scrollTop(v){{ writes.push(Math.round(v)); stTop = v; }},
|
||||
scrollHeight: {cur_scroll_height}, clientHeight: 427,
|
||||
getBoundingClientRect(){{ return {{ top: 0, bottom: 427 }}; }},
|
||||
querySelectorAll(){{ return [row]; }}, querySelector(){{ return row; }}, style: {{}},
|
||||
}};
|
||||
function $(id){{ return id === 'messages' ? container : null; }}
|
||||
function _recentMessageScrollIntent(){{ return {intent_js}; }}
|
||||
function _recentMessageTouchScrollIntent(){{ return {intent_js}; }}
|
||||
let _programmaticScroll = false; let _programmaticScrollSetAt = 0;
|
||||
const performance = {{ now(){{ return 1; }} }};
|
||||
function _suppressBrowserOverflowAnchor(){{ return null; }}
|
||||
function _deferClearProgrammaticScroll(){{}}
|
||||
function requestAnimationFrame(cb){{ cb(); }}
|
||||
function setTimeout(cb){{ cb(); return 1; }}
|
||||
const anchor = Object.assign({{ rawIdx: 53, sessionIdx: 53, key: 'k1', topOffset: {top_offset} }}, {anchor_extra});
|
||||
eval(extractFunc('_restoreMessageViewportAnchor'));
|
||||
const returned = _restoreMessageViewportAnchor(anchor, 0);
|
||||
console.log(JSON.stringify({{ returned, wrote: writes.length }}));
|
||||
"""
|
||||
|
||||
|
||||
def test_realign_refuses_stale_anchor_when_content_grew_no_intent():
|
||||
"""Content grew since capture (90000->90453) and no input intent, realign delta -453
|
||||
-> REFUSE (return false, no write). Mutation: delete the guard block and it writes."""
|
||||
m = json.loads(_run_node(_realign_harness(
|
||||
anchor_extra="{scrollHeightAtCapture: 90000}",
|
||||
cur_scroll_height=90453, rect_top=-453, top_offset=0, active_intent=False,
|
||||
)))
|
||||
assert m["returned"] is False and m["wrote"] == 0
|
||||
|
||||
|
||||
def test_realign_allows_fresh_anchor_no_growth():
|
||||
"""No growth since capture -> realign runs normally even with the same delta."""
|
||||
m = json.loads(_run_node(_realign_harness(
|
||||
anchor_extra="{scrollHeightAtCapture: 90453}",
|
||||
cur_scroll_height=90453, rect_top=-453, top_offset=0, active_intent=False,
|
||||
)))
|
||||
assert m["returned"] is True and m["wrote"] == 1
|
||||
|
||||
|
||||
def test_realign_allows_with_active_intent():
|
||||
"""Recent real input intent -> keep the legitimate realign even if content grew."""
|
||||
m = json.loads(_run_node(_realign_harness(
|
||||
anchor_extra="{scrollHeightAtCapture: 90000}",
|
||||
cur_scroll_height=90453, rect_top=-453, top_offset=0, active_intent=True,
|
||||
)))
|
||||
assert m["returned"] is True and m["wrote"] == 1
|
||||
|
||||
|
||||
def test_realign_backward_compatible_without_capture_geometry():
|
||||
"""Legacy anchor without scrollHeightAtCapture -> guard is a no-op, prior behavior."""
|
||||
m = json.loads(_run_node(_realign_harness(
|
||||
anchor_extra="{}",
|
||||
cur_scroll_height=90453, rect_top=-453, top_offset=0, active_intent=False,
|
||||
)))
|
||||
assert m["returned"] is True and m["wrote"] == 1
|
||||
|
||||
|
||||
# ---- fallback guard (_restoreMessageScrollSnapshotSameFrame) ---------------------
|
||||
|
||||
def _fallback_harness(*, snapshot_scroll_height, cur_scroll_height, snapshot_top,
|
||||
active_intent: bool = False) -> str:
|
||||
js = UI_JS_PATH.read_text(encoding="utf-8")
|
||||
intent_js = "true" if active_intent else "false"
|
||||
sh = "null" if snapshot_scroll_height is None else str(snapshot_scroll_height)
|
||||
return _extract_func_script(js) + f"""
|
||||
let writes = [];
|
||||
let stTop = 90030;
|
||||
const el = {{
|
||||
get scrollTop(){{ return stTop; }}, set scrollTop(v){{ writes.push(Math.round(v)); stTop = v; }},
|
||||
scrollHeight: {cur_scroll_height}, clientHeight: 427,
|
||||
}};
|
||||
function $(id){{ return id === 'messages' ? el : null; }}
|
||||
function _recentMessageScrollIntent(){{ return {intent_js}; }}
|
||||
function _recentMessageTouchScrollIntent(){{ return {intent_js}; }}
|
||||
// realign path fails (no anchor) so execution reaches the absolute fallback
|
||||
function _restorePinnedMessageScrollSnapshot(){{ return false; }}
|
||||
function _restoreMessageViewportAnchor(){{ return false; }}
|
||||
function _remountMessageViewportAnchor(){{ return false; }}
|
||||
let _messageUserUnpinned = false; let _scrollPinned = true; let _nearBottomCount = 5;
|
||||
let _lastScrollTop = 0; let _lastMessageClientHeight = 0;
|
||||
let _programmaticScroll = false; let _programmaticScrollSetAt = 0;
|
||||
const performance = {{ now(){{ return 1; }} }};
|
||||
function _deferClearProgrammaticScroll(){{}}
|
||||
function requestAnimationFrame(cb){{ cb(); }}
|
||||
function setTimeout(cb){{ cb(); return 1; }}
|
||||
const snapshot = {{ anchor: null, top: {snapshot_top}, bottom: 40,
|
||||
scrollHeight: {sh}, pinned: false, userUnpinned: false }};
|
||||
eval(extractFunc('_restoreMessageScrollSnapshotSameFrame'));
|
||||
_restoreMessageScrollSnapshotSameFrame(snapshot);
|
||||
console.log(JSON.stringify({{ wrote: writes.length, writes,
|
||||
messageUserUnpinned: _messageUserUnpinned, scrollPinned: _scrollPinned }}));
|
||||
"""
|
||||
|
||||
|
||||
def test_fallback_refuses_stale_snapshot_top_when_content_grew_no_intent():
|
||||
"""Content grew since snapshot (90000->90453), reader not pinned, no intent, and the
|
||||
absolute snapshot.top write would move scrollTop >8px -> REFUSE. This is the -578
|
||||
on-device jump the userUnpinned check missed (re-pin flipped userUnpinned false).
|
||||
Mutation: delete the fallback guard block and scrollHistory gets the stale write."""
|
||||
m = json.loads(_run_node(_fallback_harness(
|
||||
snapshot_scroll_height=90000, cur_scroll_height=90453, snapshot_top=89577,
|
||||
active_intent=False,
|
||||
)))
|
||||
assert m["writes"] == []
|
||||
assert m["messageUserUnpinned"] is True and m["scrollPinned"] is False
|
||||
|
||||
|
||||
def test_fallback_allows_snapshot_top_when_no_growth():
|
||||
"""No growth since snapshot -> the fallback keeps its authoritative absolute restore."""
|
||||
m = json.loads(_run_node(_fallback_harness(
|
||||
snapshot_scroll_height=90453, cur_scroll_height=90453, snapshot_top=89577,
|
||||
active_intent=False,
|
||||
)))
|
||||
assert m["writes"] == [89577]
|
||||
|
||||
|
||||
def test_fallback_backward_compatible_without_capture_scrollheight():
|
||||
"""Legacy snapshot without scrollHeight -> guard no-op, prior absolute restore runs."""
|
||||
m = json.loads(_run_node(_fallback_harness(
|
||||
snapshot_scroll_height=None, cur_scroll_height=90453, snapshot_top=89577,
|
||||
active_intent=False,
|
||||
)))
|
||||
assert m["writes"] == [89577]
|
||||
Loading…
Reference in a new issue