From 6825bc03b4a919a7fe9a8317f3b1d2b7ba043e06 Mon Sep 17 00:00:00 2001 From: allenliang2022 Date: Fri, 26 Jun 2026 18:20:58 +0800 Subject: [PATCH] fix(chat): record keyboard message-pane scroll as intent (#4970 greptile P1) Keyboard scrolling of the message pane (PageUp/PageDown, Arrow keys, Space, Home/End) fires a native scroll event with no wheel/touch/scrollbar/non-message intent. Inside the 1400ms post-render artifact window the suppression branch then returned before movedUp could unpin, so a keyboard scroll-up was swallowed and live-follow snapped the reader back to the bottom. - Add _lastMessageKeyScrollIntentMs + _recentMessageKeyScrollIntent(), stamped by a capture-phase keydown listener on the scroll keys, gated to when the message pane is the scroll target (focused/contains focus/hovered) and not an editable field (composer/input/contenteditable). - Gate the post-render suppression on !_recentMessageKeyScrollIntent() (typeof guard keeps the #4295 node harness inert). - Clear the stamp in both _resetScrollDirectionTracker() and _resetStreamScrollFollow() (same stale-state hygiene as the wheel stamp). Tests: keyboard-scroll-inside-window-still-unpins behavioral regression (harness extended with injected _recentMessageKeyScrollIntent), plus a source lock for the helper/keydown-stamp/suppression-gate/both-resets. test_4856 + test_4295 green (19 passed), node --check clean. --- static/ui.js | 47 ++++++++++++++++- ...est_issue4856_android_scroll_regression.py | 51 ++++++++++++++++++- 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/static/ui.js b/static/ui.js index 7c771118..91663f26 100644 --- a/static/ui.js +++ b/static/ui.js @@ -3731,12 +3731,19 @@ let _messageTouchScrollActive=false; let _lastMessageTouchScrollIntentMs=-Infinity; let _deferredOlderMessagesTimer=0; const MESSAGE_TOUCH_SCROLL_SUPPRESS_MS=1200; -// #4970: track recent LOW-DELTA upward message-pane wheel intent separately from +// #4970 review: track recent LOW-DELTA upward message-pane wheel intent separately from // the decisive deltaY<-30 sticky-unpin threshold. A gentle trackpad wheel // (deltaY:-5) is real user intent but never crosses -30, so without this the // post-render artifact suppression would swallow it for the whole window. const MESSAGE_WHEEL_INTENT_SUPPRESS_MS=1200; let _lastMessageWheelIntentMs=-Infinity; +// #4970 review (greptile P1): keyboard scrolling of the message pane (PageUp/Down, +// arrows, Space, Home/End) fires a native `scroll` event with NO wheel/touch/ +// scrollbar/non-message intent. Without recording it, a keyboard scroll-up inside +// the post-render artifact window is swallowed and live-follow snaps the reader +// back to the bottom. Stamp a generic scroll-key intent so the suppression skips it. +const MESSAGE_KEY_SCROLL_INTENT_SUPPRESS_MS=1200; +let _lastMessageKeyScrollIntentMs=-Infinity; let _newMessageCueVisible=false; let _lastMessageRenderAt=-Infinity; function _recentMessageRenderArtifactWindow(ms){ @@ -3757,6 +3764,12 @@ function _recentMessageTouchScrollIntent(){ function _recentMessageWheelIntent(){ return performance.now()-_lastMessageWheelIntentMs{ if(document.visibilityState==='hidden') _scrollbarDragActive=false; },{passive:true}); + // #4970 review (greptile P1): record keyboard-driven message-pane scrolling as + // user intent. PageUp/PageDown, Arrow keys, Space/Shift+Space, Home/End scroll + // the pane and fire a native scroll event with no wheel/touch intent — without + // this stamp a keyboard scroll-up inside the post-render artifact window is + // swallowed and live-follow snaps the reader back to the bottom. Only count it + // when the scroll container (or a descendant) is the active/scrolling target, + // not when typing in the composer. + const _MESSAGE_SCROLL_KEYS=new Set([ + 'PageUp','PageDown','ArrowUp','ArrowDown','Home','End','Spacebar',' ', + ]); + document.addEventListener('keydown',(e)=>{ + if(!e||!_MESSAGE_SCROLL_KEYS.has(e.key)) return; + const a=document.activeElement; + // Ignore keys aimed at editable fields (composer, inputs, contenteditable). + if(a&&(a.tagName==='INPUT'||a.tagName==='TEXTAREA'||a.isContentEditable)) return; + // Count only when the message pane itself is the scroll target: it is focused, + // contains the focus, or the pointer is over it (keyboard scroll w/o focus). + if(a===el||el.contains(a)||el.matches(':hover')){ + _lastMessageKeyScrollIntentMs=performance.now(); + } + },{capture:true,passive:true}); let _scrollRaf=0; el.addEventListener('scroll',()=>{ _scheduleMessageVirtualizedRender(); @@ -4026,16 +4064,21 @@ if(typeof window!=='undefined'){ // manual scrollbar-drag upward scroll inside the window is real intent. // typeof guard keeps the #4295 node harness (no _scrollbarDragActive // injected) inert via short-circuit. + // #4970 review (greptile P1): likewise skip suppression when the reader + // recently scrolled the pane with the keyboard — a keyboard scroll-up is + // real intent that produces a native scroll event with no wheel/touch. if(movedUp && typeof _recentMessageRenderArtifactWindow==='function' && typeof _recentMessageTouchScrollIntent==='function' && typeof _recentNonMessageScrollIntent==='function' && typeof _recentMessageWheelIntent==='function' + && typeof _recentMessageKeyScrollIntent==='function' && (typeof _scrollbarDragActive==='undefined' || !_scrollbarDragActive) && _recentMessageRenderArtifactWindow(1400) && !_recentMessageTouchScrollIntent() && !_recentNonMessageScrollIntent() - && !_recentMessageWheelIntent()){ + && !_recentMessageWheelIntent() + && !_recentMessageKeyScrollIntent()){ _lastScrollTop=top; return; } diff --git a/tests/test_issue4856_android_scroll_regression.py b/tests/test_issue4856_android_scroll_regression.py index 90365d2f..e92757c9 100644 --- a/tests/test_issue4856_android_scroll_regression.py +++ b/tests/test_issue4856_android_scroll_regression.py @@ -160,7 +160,7 @@ def _scroll_listener_raf_body() -> str: def _run_listener_with_wheel_intent( - samples, *, render_artifact, wheel_intent, scrollbar_drag=False + samples, *, render_artifact, wheel_intent, scrollbar_drag=False, key_scroll=False ): """Run the extracted scroll-listener body in node with controllable stubs. @@ -174,6 +174,7 @@ def _run_listener_with_wheel_intent( "renderArtifact": bool(render_artifact), "wheelIntent": bool(wheel_intent), "scrollbarDrag": bool(scrollbar_drag), + "keyScroll": bool(key_scroll), } script = ( "const payload = " + json.dumps(payload) + ";\n" @@ -199,6 +200,7 @@ const step = new Function( '_recentNonMessageScrollIntent', '_recentMessageWheelIntent', '_scrollbarDragActive', + '_recentMessageKeyScrollIntent', // The extracted listener body uses bare `return;` in the suppression branch. // Wrap it in an inner arrow IIFE so that early return exits the IIFE (not the // outer Function), then read the mutated locals afterward. Without this the @@ -227,6 +229,7 @@ const renderArtifact = () => payload.renderArtifact; const noTouch = () => false; const noNonMessage = () => false; const wheelIntent = () => payload.wheelIntent; +const keyScroll = () => payload.keyScroll; for (const sample of payload.samples) { state = step( @@ -249,7 +252,8 @@ for (const sample of payload.samples) { noTouch, noNonMessage, wheelIntent, - payload.scrollbarDrag + payload.scrollbarDrag, + keyScroll ); } @@ -324,6 +328,22 @@ class TestPostRenderWheelIntentScope: ) assert state["_scrollPinned"] is False + def test_keyboard_scroll_inside_window_still_unpins(self): + # #4970 review (greptile P1): a keyboard scroll-up (PageUp/Arrow/etc.) + # inside the post-render window is real intent and must NOT be swallowed, + # even with no wheel/touch/scrollbar intent recorded. + state = _run_listener_with_wheel_intent( + self._SAMPLES, + render_artifact=True, + wheel_intent=False, + key_scroll=True, + ) + assert state["_messageUserUnpinned"] is True, ( + "A keyboard scroll-up inside the artifact window must unpin; the " + "suppression must not swallow a recent keyboard scroll intent." + ) + assert state["_scrollPinned"] is False + def test_low_delta_wheel_intent_is_tracked_separately(): # The intent recorder must stamp _lastMessageWheelIntentMs for ANY upward @@ -394,6 +414,33 @@ def test_suppression_gates_on_scrollbar_drag(): ) +def test_keyboard_scroll_intent_tracked_and_gated(): + # #4970 review (greptile P1): keyboard message-pane scrolling must be recorded + # as user intent and excluded from the post-render suppression branch. + assert "let _lastMessageKeyScrollIntentMs=-Infinity" in UI_JS + assert "function _recentMessageKeyScrollIntent" in UI_JS + # A keydown listener must stamp the intent for the pane scroll keys. + assert "_lastMessageKeyScrollIntentMs=performance.now()" in UI_JS, ( + "a keydown handler must stamp _lastMessageKeyScrollIntentMs when the " + "reader uses the keyboard to scroll the message pane." + ) + assert "'PageUp'" in UI_JS and "'PageDown'" in UI_JS + # The suppression branch must consult it. + listener_idx = UI_JS.find("el.addEventListener('scroll'") + listener = UI_JS[listener_idx: listener_idx + 4000] + assert "!_recentMessageKeyScrollIntent()" in listener, ( + "#4970 review: the suppression branch must reference " + "!_recentMessageKeyScrollIntent() so a keyboard scroll-up unpins." + ) + # Both resets must clear the keyboard stamp (stale-state hygiene). + assert "_lastMessageKeyScrollIntentMs=-Infinity" in _extract_fn_body( + "_resetScrollDirectionTracker" + ) + assert "_lastMessageKeyScrollIntentMs=-Infinity" in _extract_fn_body( + "_resetStreamScrollFollow" + ) + + def test_streaming_tick_calls_fix_before_dom_writes(): # The streaming render tick in messages.js must call _fixMobileScrollJank() # before _lastRenderMs=performance.now() so anchor suppression covers every