fix(webui): realign desktop stale-snapshot fallback with the app's scrollTop += delta idiom (#5637 follow-up)
Round-2 gate-cert proved the round-1 'snapshot.top + delta' + nearest-to-current arbiter is mathematically unwinnable: a row's rect offset is scroll-relative (rect.top - containerRect.top = rowContentPos - scrollTop), so a staged absolute target only equals the true content-hold when scrollTop == snapshot.top — and in that exact case the arbiter measures |raw - cur| = 0 and picks raw, writing the stale value (the reported up-jump, unfixed). Rework to the app's own realign idiom (mirrors _restoreMessageViewportAnchor and _compensateScrollForMeasurementDelta): when a measurable anchor row exists, write el.scrollTop += (currentOffset - capturedOffset) from the LIVE scrollTop — a no-op when already aligned, a heal when not, no arbiter. Restore the per-tier lookup guard (key -> sessionIdx, never the rawIdx degradation that maps to a different message after a virtualization re-window). Genuinely-gone anchor: mirror the topPad-delta idiom the snapshot already carries (topPadBefore), else keep raw. Gate-cert tests now use scroll-dependent rect mocks (rect.top = rowContentPos - scrollTop) so certified hold numbers are physically realizable; mutation-checked (revert to raw -> the hold tests fail; drop topPad-delta -> that test fails).
This commit is contained in:
allenliang2022
t
parent
57aa5be2ed
commit
7164847255
3 changed files with 221 additions and 14 deletions
72
static/ui.js
72
static/ui.js
|
|
@ -13511,6 +13511,43 @@ window._fixMobileScrollJank=function _fixMobileScrollJank(){
|
|||
});
|
||||
};
|
||||
|
||||
// Desktop stale-snapshot residue (issue #5637 follow-up). Reached only when
|
||||
// _restoreMessageViewportAnchor already CONCEDED (anchor row unrecoverable by its
|
||||
// per-tier lookup) and the desktop fallback would otherwise write the ABSOLUTE
|
||||
// snapshot.top — which is stale once above-viewport content grew since capture,
|
||||
// yanking a still reader backward. The correct hold is the app's own realign
|
||||
// idiom: shift the CURRENT scrollTop by how far the anchor row moved since capture,
|
||||
// `scrollTop += (currentOffset - capturedOffset)` (mirrors _restoreMessageViewportAnchor
|
||||
// ui.js and _compensateScrollForMeasurementDelta). NOT `snapshot.top + delta`: a
|
||||
// row's offset is scroll-relative (rect.top - containerRect.top = rowContentPos -
|
||||
// scrollTop), so only a delta applied to the LIVE scrollTop holds the row put
|
||||
// regardless of where scrollTop was carried to. Returns the realign delta (may be
|
||||
// 0), or null when the anchor row can't be measured under the SAME per-tier guard
|
||||
// _restoreMessageViewportAnchor uses (key -> sessionIdx, never the rawIdx
|
||||
// degradation — rawIdx maps to a different message after a virtualization
|
||||
// re-window, ui.js per-tier guard) so the caller can fall back to the topPad-delta
|
||||
// idiom or keep raw rather than guessing.
|
||||
function _desktopAnchorRealignDelta(container, anchor){
|
||||
if(!container||!anchor||typeof container.querySelector!=='function') return null;
|
||||
const capturedOffset=Number(anchor.topOffset);
|
||||
if(!Number.isFinite(capturedOffset)) return null;
|
||||
const anchorKey=String(anchor.key||'');
|
||||
let row=anchorKey
|
||||
? Array.from(container.querySelectorAll('[data-message-anchor-key]')).find(el=>el&&el.dataset&&el.dataset.messageAnchorKey===anchorKey)
|
||||
: null;
|
||||
if(row&&row.getClientRects&&row.getClientRects().length===0) row=null;
|
||||
const sessionIdx=Number(anchor.sessionIdx);
|
||||
if(!row&&Number.isFinite(sessionIdx)) row=container.querySelector(`[data-session-msg-idx="${sessionIdx}"]`);
|
||||
// Per-tier guard mirror (ui.js _restoreMessageViewportAnchor): a genuinely-gone
|
||||
// anchor misses key AND sessionIdx -> concede (null). Do NOT degrade to rawIdx.
|
||||
if(!row) return null;
|
||||
if(typeof row.getBoundingClientRect!=='function') return null;
|
||||
if(row.getClientRects&&row.getClientRects().length===0) return null;
|
||||
const containerRect=container.getBoundingClientRect();
|
||||
const rect=row.getBoundingClientRect();
|
||||
const currentOffset=rect.top-containerRect.top;
|
||||
return currentOffset-capturedOffset;
|
||||
}
|
||||
function _restoreMessageScrollSnapshotSameFrame(snapshot){
|
||||
const el=$('messages');
|
||||
if(!el||!snapshot) return;
|
||||
|
|
@ -13578,8 +13615,41 @@ function _restoreMessageScrollSnapshotSameFrame(snapshot){
|
|||
_nearBottomCount=0;
|
||||
return;
|
||||
}
|
||||
// Desktop stale-snapshot residue fix (issue #5637 follow-up, PR #5742 round-3).
|
||||
// On desktop (overflow-anchor:none) the touch refusal above does NOT apply — the
|
||||
// reader must be actively held, so we write scrollTop. The ABSOLUTE snapshot.top is
|
||||
// stale once above-viewport content grew since capture. Use the app's own realign
|
||||
// idiom instead: shift the CURRENT scrollTop by how far the anchor row moved since
|
||||
// capture. `scrollTop += (currentOffset - capturedOffset)` holds the row put no
|
||||
// matter where scrollTop was carried (a row's offset is scroll-relative), which the
|
||||
// staged `snapshot.top + delta` cannot. No arbiter: the realign is a no-op when
|
||||
// already aligned (delta ~ 0) and heals when not. Only when the anchor row is
|
||||
// genuinely gone (per-tier lookup concedes, no rawIdx degradation) do we fall back
|
||||
// to the topPad-delta idiom, then to raw. Pinned/near-bottom readers took the
|
||||
// bottom-relative target above and never reach here as unpinned.
|
||||
let _fbTarget=Math.max(0,Math.min(target,maxTop));
|
||||
if(!_fbTouchHold && snapshot.pinned!==true){
|
||||
const _realign=_desktopAnchorRealignDelta(el, snapshot.anchor);
|
||||
if(_realign!==null){
|
||||
// Anchor row measurable: realign from the LIVE scrollTop (app idiom).
|
||||
_fbTarget=Math.max(0,Math.min(el.scrollTop+_realign, maxTop));
|
||||
}else{
|
||||
// Anchor row genuinely gone. Mirror the topPad-delta idiom the anchor already
|
||||
// carries (topPadBefore): shift by the growth of the virtual top spacer since
|
||||
// capture so the reader is held by the same amount the content above moved.
|
||||
const _padNow=(function(){
|
||||
const s=el.querySelector('[data-virtual-spacer="before"]');
|
||||
return s?(parseFloat(s.style.height||'0')||0):NaN;
|
||||
})();
|
||||
const _padBefore=Number(snapshot.anchor&&snapshot.anchor.topPadBefore);
|
||||
if(Number.isFinite(_padNow)&&Number.isFinite(_padBefore)){
|
||||
_fbTarget=Math.max(0,Math.min(el.scrollTop+(_padNow-_padBefore), maxTop));
|
||||
}
|
||||
// else: no measurable anchor and no topPad geometry -> keep raw target.
|
||||
}
|
||||
}
|
||||
_programmaticScroll=true;_programmaticScrollSetAt=performance.now();
|
||||
el.scrollTop=Math.max(0,Math.min(target,maxTop));
|
||||
el.scrollTop=_fbTarget;
|
||||
}
|
||||
_lastScrollTop=el.scrollTop;_lastMessageClientHeight=el.clientHeight;
|
||||
if(snapshot.pinned===true){
|
||||
|
|
|
|||
|
|
@ -108,14 +108,14 @@ def test_restore_same_frame_holds_position_for_unpinned_reader():
|
|||
)
|
||||
# The guard must sit BEFORE the absolute-top scrollTop write and short-circuit it.
|
||||
guard_idx = fn.index("snapshot.userUnpinned===true&&snapshot.pinned!==true")
|
||||
# the absolute-top write in the fallback
|
||||
write_idx = fn.index("el.scrollTop=Math.max(0,Math.min(target,maxTop))")
|
||||
# the desktop fallback scrollTop write (round-3: realigned _fbTarget, not raw snapshot.top)
|
||||
write_idx = fn.index("el.scrollTop=_fbTarget")
|
||||
assert guard_idx < write_idx, (
|
||||
"The unpinned guard must precede (and return before) the absolute-top "
|
||||
"The unpinned guard must precede (and return before) the fallback "
|
||||
"scrollTop write so it is never reached for an unpinned reader (#5637)."
|
||||
)
|
||||
# Between the guard and the write there must be an early return.
|
||||
between = fn[guard_idx:write_idx]
|
||||
assert "return;" in between, (
|
||||
"The unpinned guard must `return` before the absolute-top write (#5637)."
|
||||
"The unpinned guard must `return` before the fallback write (#5637)."
|
||||
)
|
||||
|
|
|
|||
|
|
@ -173,23 +173,63 @@ def test_realign_allows_on_desktop_no_native_anchor():
|
|||
# ---- fallback guard (_restoreMessageScrollSnapshotSameFrame) ---------------------
|
||||
|
||||
def _fallback_harness(*, snapshot_scroll_height, cur_scroll_height, snapshot_top,
|
||||
active_intent: bool = False, touch_like: bool = True) -> str:
|
||||
active_intent: bool = False, touch_like: bool = True,
|
||||
init_scroll_top: int = 90030,
|
||||
anchor=None, anchor_row_content_pos=None, container_top: int = 0,
|
||||
top_pad_now=None) -> str:
|
||||
"""Node harness for the absolute-fallback path of _restoreMessageScrollSnapshotSameFrame.
|
||||
|
||||
SCROLL-DEPENDENT geometry (round-3 gate-cert requirement): the anchor row's
|
||||
getBoundingClientRect().top is computed LIVE as `rowContentPos - scrollTop + container_top`
|
||||
— exactly how a real browser reports it — NOT a fixed constant. So after the fix
|
||||
writes `el.scrollTop += delta`, a subsequent rect read reflects the new position, and
|
||||
a certified hold number is physically realizable.
|
||||
|
||||
anchor: dict for snapshot.anchor (with key/sessionIdx/topOffset[/topPadBefore]) or None.
|
||||
anchor_row_content_pos: the row's ABSOLUTE content position (document-space top). Its
|
||||
live viewport offset is rowContentPos - scrollTop. When None, the row lookup returns
|
||||
nothing (genuinely-gone anchor -> topPad-delta or raw).
|
||||
top_pad_now: current virtual top-spacer height (for the genuinely-gone topPad-delta path).
|
||||
"""
|
||||
js = UI_JS_PATH.read_text(encoding="utf-8")
|
||||
intent_js = "true" if active_intent else "false"
|
||||
touch_js = "true" if touch_like else "false"
|
||||
sh = "null" if snapshot_scroll_height is None else str(snapshot_scroll_height)
|
||||
anchor_js = "null" if anchor is None else json.dumps(anchor)
|
||||
row_present = anchor is not None and anchor_row_content_pos is not None
|
||||
row_pos_js = "0" if anchor_row_content_pos is None else str(anchor_row_content_pos)
|
||||
pad_now_js = "null" if top_pad_now is None else str(top_pad_now)
|
||||
return _extract_func_script(js) + f"""
|
||||
let writes = [];
|
||||
let stTop = 90030;
|
||||
let stTop = {init_scroll_top};
|
||||
const CONTAINER_TOP = {container_top};
|
||||
const ROW_CONTENT_POS = {row_pos_js};
|
||||
const ROW_PRESENT = {"true" if row_present else "false"};
|
||||
const TOP_PAD_NOW = {pad_now_js};
|
||||
// A real browser's rect.top for a row is (rowContentPos - scrollTop) relative to the
|
||||
// document, so its offset below the container top is ROW_CONTENT_POS - scrollTop.
|
||||
const _anchorRow = ROW_PRESENT ? {{
|
||||
getBoundingClientRect(){{ return {{ top: CONTAINER_TOP + (ROW_CONTENT_POS - stTop) }}; }},
|
||||
getClientRects(){{ return [{{}}]; }},
|
||||
}} : null;
|
||||
const _topSpacer = (TOP_PAD_NOW === null) ? null : {{
|
||||
style: {{ height: TOP_PAD_NOW + 'px' }},
|
||||
}};
|
||||
const el = {{
|
||||
get scrollTop(){{ return stTop; }}, set scrollTop(v){{ writes.push(Math.round(v)); stTop = v; }},
|
||||
scrollHeight: {cur_scroll_height}, clientHeight: 427,
|
||||
getBoundingClientRect(){{ return {{ top: CONTAINER_TOP, bottom: CONTAINER_TOP + 427 }}; }},
|
||||
querySelector(sel){{
|
||||
if (sel === '[data-virtual-spacer="before"]') return _topSpacer;
|
||||
return _anchorRow;
|
||||
}},
|
||||
querySelectorAll(sel){{ return _anchorRow ? [_anchorRow] : []; }},
|
||||
}};
|
||||
function $(id){{ return id === 'messages' ? el : null; }}
|
||||
function _recentMessageScrollIntent(){{ return {intent_js}; }}
|
||||
function _recentMessageTouchScrollIntent(){{ return {intent_js}; }}
|
||||
function _isTouchLikeMessageViewport(){{ return {touch_js}; }}
|
||||
// realign path fails (no anchor) so execution reaches the absolute fallback
|
||||
// realign path fails (no anchor restore) so execution reaches the absolute fallback
|
||||
function _restorePinnedMessageScrollSnapshot(){{ return false; }}
|
||||
function _restoreMessageViewportAnchor(){{ return false; }}
|
||||
function _remountMessageViewportAnchor(){{ return false; }}
|
||||
|
|
@ -200,12 +240,13 @@ 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,
|
||||
const snapshot = {{ anchor: {anchor_js}, top: {snapshot_top}, bottom: 40,
|
||||
scrollHeight: {sh}, pinned: false, userUnpinned: false }};
|
||||
eval(extractFunc('_desktopAnchorRealignDelta'));
|
||||
eval(extractFunc('_restoreMessageScrollSnapshotSameFrame'));
|
||||
_restoreMessageScrollSnapshotSameFrame(snapshot);
|
||||
console.log(JSON.stringify({{ wrote: writes.length, writes,
|
||||
messageUserUnpinned: _messageUserUnpinned, scrollPinned: _scrollPinned }}));
|
||||
messageUserUnpinned: _messageUserUnpinned, scrollPinned: _scrollPinned, finalScrollTop: Math.round(stTop) }}));
|
||||
"""
|
||||
|
||||
|
||||
|
|
@ -257,9 +298,10 @@ def test_fallback_allows_snapshot_top_on_desktop_no_native_anchor():
|
|||
"""Desktop regression (#5637 gate cert): the exact stale-snapshot case (content grew,
|
||||
reader not pinned, no intent, absolute write would move >8px) but on a
|
||||
hover+fine-pointer viewport where `.messages` is `overflow-anchor:none`. With no
|
||||
native anchoring layer to hold the reader, the fallback MUST keep its absolute
|
||||
snapshot.top restore rather than refuse and latch userUnpinned — otherwise the
|
||||
desktop reader is left at the wrong absolute position and force-unpinned.
|
||||
native anchoring layer to hold the reader, the fallback MUST still WRITE rather than
|
||||
refuse-and-latch userUnpinned. With NO anchor on the snapshot (and no top-spacer
|
||||
geometry) there is nothing to realign against, so it keeps the raw snapshot.top —
|
||||
the same authoritative absolute write as before.
|
||||
Mutation: drop the `_fbTouchHold&&` term from the fallback guard and this FAILS
|
||||
(the write is wrongly refused and userUnpinned is latched on desktop)."""
|
||||
m = json.loads(_run_node(_fallback_harness(
|
||||
|
|
@ -270,7 +312,102 @@ def test_fallback_allows_snapshot_top_on_desktop_no_native_anchor():
|
|||
assert m["messageUserUnpinned"] is False and m["scrollPinned"] is True
|
||||
|
||||
|
||||
# ---- predicate stability (_isTouchLikeMessageViewport) ---------------------------
|
||||
# ---- round-3 desktop anchor-realign (PR #5742): app's own scrollTop += delta idiom -----
|
||||
# These use SCROLL-DEPENDENT rect mocks (rect.top = rowContentPos - scrollTop), so the
|
||||
# certified hold numbers are physically realizable in a real browser — the round-2
|
||||
# gate-cert requirement that fixed-rect mocks violated.
|
||||
|
||||
def test_realign_holds_reader_on_above_viewport_growth_desktop():
|
||||
"""Reader parked up in history on desktop; above-viewport content grew since capture.
|
||||
The anchor row was captured at topOffset 1000; the reader has ALREADY been carried
|
||||
down by the browser to scrollTop 1480 (partial), and the row's content position is
|
||||
2000 so its live offset is 2000-1480=520. The app idiom writes
|
||||
scrollTop += (currentOffset - capturedOffset) = 1480 + (520 - 1000) = 1000, landing
|
||||
the row back at its captured 1000px offset (2000 - 1000 = 1000). That HOLD is
|
||||
physically realizable: after the write, rect.top = 2000 - 1000 = 1000 = capturedOffset.
|
||||
Mutation: revert `_fbTarget` to the raw `target` (snapshot.top=1200) and this FAILS
|
||||
(the reader is left at 1200, a 200px drift from the held content position)."""
|
||||
m = json.loads(_run_node(_fallback_harness(
|
||||
snapshot_scroll_height=90000, cur_scroll_height=90600, snapshot_top=1200,
|
||||
active_intent=False, touch_like=False,
|
||||
anchor={"key": "k1", "sessionIdx": 5, "topOffset": 1000},
|
||||
anchor_row_content_pos=2000, container_top=0, init_scroll_top=1480,
|
||||
)))
|
||||
# scrollTop += (currentOffset - capturedOffset) = 1480 + ((2000-1480) - 1000) = 1000
|
||||
assert m["writes"] == [1000]
|
||||
# and the row is now held at its captured offset — physically consistent
|
||||
assert m["finalScrollTop"] == 1000
|
||||
assert m["messageUserUnpinned"] is False and m["scrollPinned"] is True
|
||||
|
||||
|
||||
def test_realign_is_noop_when_already_aligned_desktop():
|
||||
"""No arbiter: when the reader is already at the captured offset (row content pos 2000,
|
||||
scrollTop 1000 -> live offset 1000 == captured 1000), the realign delta is 0 and no
|
||||
move happens (delta < clamp is still written as the same value, i.e. a no-op write to
|
||||
the same scrollTop). Proves the idiom does not perturb an already-correct position."""
|
||||
m = json.loads(_run_node(_fallback_harness(
|
||||
snapshot_scroll_height=90000, cur_scroll_height=90600, snapshot_top=1200,
|
||||
active_intent=False, touch_like=False,
|
||||
anchor={"key": "k1", "sessionIdx": 5, "topOffset": 1000},
|
||||
anchor_row_content_pos=2000, container_top=0, init_scroll_top=1000,
|
||||
)))
|
||||
# currentOffset = 2000 - 1000 = 1000 == captured 1000 -> delta 0 -> scrollTop stays 1000
|
||||
assert m["finalScrollTop"] == 1000
|
||||
assert m["writes"] == [1000]
|
||||
|
||||
|
||||
def test_realign_does_not_drift_on_below_viewport_tail_growth_desktop():
|
||||
"""Below-viewport (tail) growth: the anchor row ABOVE the reader did NOT move
|
||||
(content pos 2000, scrollTop 1000 -> offset 1000 == captured 1000), only content
|
||||
below grew (scrollHeight 90000 -> 95000). The realign delta is 0 -> the reader is
|
||||
NOT pulled downward. This is the conveyor-belt drift the round-2 arbiter formula
|
||||
reintroduced; the app idiom cannot, because it keys on the ABOVE-viewport anchor row,
|
||||
whose offset is unchanged by tail growth.
|
||||
Mutation: a `snapshot.top + totalGrowth` formula would write 1000 + 5000 = 6000 and
|
||||
drift the reader down 5000px; this asserts the held 1000."""
|
||||
m = json.loads(_run_node(_fallback_harness(
|
||||
snapshot_scroll_height=90000, cur_scroll_height=95000, snapshot_top=1000,
|
||||
active_intent=False, touch_like=False,
|
||||
anchor={"key": "k1", "sessionIdx": 5, "topOffset": 1000},
|
||||
anchor_row_content_pos=2000, container_top=0, init_scroll_top=1000,
|
||||
)))
|
||||
assert m["finalScrollTop"] == 1000
|
||||
assert m["writes"] == [1000]
|
||||
|
||||
|
||||
def test_realign_genuinely_gone_anchor_uses_toppad_delta_desktop():
|
||||
"""Anchor row genuinely gone (lookup returns null), but the snapshot carries
|
||||
topPadBefore and the current virtual top-spacer is measurable. Mirror the topPad-delta
|
||||
idiom: shift scrollTop by (padNow - padBefore) so the reader is held by the same
|
||||
amount the content above moved. padBefore=800, padNow=1300 -> +500 from scrollTop 1000
|
||||
-> 1500.
|
||||
Mutation: drop the topPad-delta branch and it keeps raw snapshot.top (1200), a
|
||||
300px drift from the topPad-held 1500."""
|
||||
m = json.loads(_run_node(_fallback_harness(
|
||||
snapshot_scroll_height=90000, cur_scroll_height=90500, snapshot_top=1200,
|
||||
active_intent=False, touch_like=False,
|
||||
anchor={"key": "gone", "sessionIdx": 999, "topOffset": 1000, "topPadBefore": 800},
|
||||
anchor_row_content_pos=None, container_top=0, init_scroll_top=1000,
|
||||
top_pad_now=1300,
|
||||
)))
|
||||
assert m["finalScrollTop"] == 1500
|
||||
assert m["writes"] == [1500]
|
||||
|
||||
|
||||
def test_realign_genuinely_gone_anchor_no_geometry_keeps_raw_desktop():
|
||||
"""Anchor row gone AND no topPad geometry available -> keep the raw snapshot.top
|
||||
(no guessing with an arbiter). snapshot.top=1200 -> writes 1200."""
|
||||
m = json.loads(_run_node(_fallback_harness(
|
||||
snapshot_scroll_height=90000, cur_scroll_height=90500, snapshot_top=1200,
|
||||
active_intent=False, touch_like=False,
|
||||
anchor={"key": "gone", "sessionIdx": 999, "topOffset": 1000},
|
||||
anchor_row_content_pos=None, container_top=0, init_scroll_top=1000,
|
||||
top_pad_now=None,
|
||||
)))
|
||||
assert m["writes"] == [1200]
|
||||
|
||||
|
||||
|
||||
|
||||
def _predicate_harness(*, pointer_coarse, computed_overflow_anchor, has_matchmedia=True,
|
||||
inline_overflow_anchor="", ua="Mozilla/5.0 (Linux; Android 13)",
|
||||
|
|
|
|||
Loading…
Reference in a new issue