Skip to content

text_selection: drag-autoscroll never moved a virtualized list - #2948

Open
kossoy wants to merge 1 commit into
longbridge:mainfrom
kossoy:fix/autoscroll-notify-participant
Open

text_selection: drag-autoscroll never moved a virtualized list#2948
kossoy wants to merge 1 commit into
longbridge:mainfrom
kossoy:fix/autoscroll-notify-participant

Conversation

@kossoy

@kossoy kossoy commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

update_auto_scroll notifies the participant — which scrolls itself through its
own list API — only on the branch taken when no Window is available:

let Some(window) = window else {
    participant.update(cx, |state, cx| state.set_auto_scroll(delta, cx));
    return;
};
// … synthetic ScrollWheelEvent via window.dispatch_event …

A live drag always passes Some(window), so the only branch a drag ever took
was the synthetic-wheel one, and a synthetic ScrollWheelEvent never reaches a
virtualized list's scroll handler while the pointer is captured by the drag. The
working branch was dead code for the one flow that matters. Autoscroll computed
and dispatched a correct delta forever while the list stood still.

Reproduction

Open a long document in a scrollable TextView. Anchor a selection near the top
of the viewport, drag to the bottom edge and hold with small movements for
several seconds.

Instrumented: 726 events with delta = Some(16.33 px) dispatched, px_off
stays 0.0, selection captures only the visible viewport (≈ 2 KB of a 132 KB
note).

Fix

Notify the participant unconditionally, then keep the wheel dispatch for
participants that scroll through an ancestor container rather than a list of
their own — for a participant that handled the notification the wheel is a
no-op.

After: the same 8-second hold scrolls px_off 0 → 5 382.8 and the selection
extends to 38 KB. crates/base/src/text_selection.rs, +14 lines, no gpui core
change.

@huacnlee

huacnlee commented Sep 4, 2026

Copy link
Copy Markdown
Member

Thanks for chasing this down — the symptom you instrumented (726 deltas dispatched, px_off pinned at 0.0) is clearly real. But I don't think the stated mechanism holds, and as written the patch is additive rather than corrective. Details below, with the measurements I used.

The synthetic wheel does move a virtualized list

The repo already has compatibility_text_view_drag_selection_auto_scrolls_both_directions (crates/component/src/text/window_selection.rs:757). It drags to the bottom edge of a scrollable(true) TextView and asserts the virtualized ListState moved — and it passes on main, driven purely by the synthetic wheel. I instrumented it to print logical_scroll_top after a fixed 64 ms clock advance and ran it on both revisions:

revision scroll position after 64 ms
main (wheel only) item 1, 36.33 px
this PR item 3, 30.67 px

The reason is that gpui's List bubble handler gates on hitbox_id.should_handle_scroll(window), which is just window.mouse_hit_test.ids.contains(id) — a position hit test. captured_hitbox only influences is_hovered, not scroll handling. Window::dispatch_event assigns self.mouse_position = scroll_wheel.position and dispatch_mouse_event re-runs the hit test from there, and the synthetic position is deliberately clamped inside the content mask, so the list's own handler always matches.

So the participant notification is not replacing a dead path — it is running alongside a live one.

Consequence: two timers drive the same list

After this change a scrollable TextView has two independent 16 ms AutoScroll timers writing the same ListState:

  • TextViewState::set_auto_scrolllist_state.scroll_by(delta), relative to the current offset
  • the synthetic wheel → ListState::scroll(&scroll_top, …), absolute, based on the scroll_top captured at the last paint

Autoscroll therefore runs at roughly double its previous speed (the table above), and because one writer is relative-from-now and the other absolute-from-last-frame, they race across repaints: when the participant timer ticks twice between paints, the wheel's stale base snaps the list backwards. That is visible jitter, not just a speed change.

The dispatch needs to be exclusive — notify the participant or send the wheel. The natural seam is the !state.scrollable case in crates/base/src/text/selection_adapter.rs:101-110: dispatch the wheel only when the participant did not consume the delta.

The delta uses the wrong rectangle for a self-scrolling participant

visible_bounds here is registration.hitbox.content_mask.bounds, the nearest clipping ancestor. That is right for the wheel, which targets the ancestor scroll container, but wrong for a participant that scrolls its own list. The pre-existing self-scroll path, update_participant_auto_scroll (line 1552), uses registration.registration.bounds — the TextView's own element bounds — for exactly this reason.

Concretely, a scrollable TextView that does not reach its clipping ancestor's bottom edge (a transcript pane above a composer, or any padded container with no intermediate overflow_hidden, where the mask is window bounds) yields compute_delta(...) == None with the pointer at the TextView's own bottom edge. The participant is told to stop rather than scroll, so the reported bug survives in that layout. The mirror case regresses too: when the anchor TextView is smaller than an enclosing scroll container, dragging near the container's edge — far below the TextView — now scrolls the TextView's list as well.

The participant notification should use registration.registration.bounds; only the wheel should use the content mask.

Smaller things

  • update_auto_scroll's window: Option<&Window> is now dead. The only production caller (update_in_window, line 1145) always passes Some, and the None arm is a bare return. The real window-less path is update_implupdate_participant_auto_scroll. Taking &Window would remove a branch that reads as a live alternative.
  • The 15-line comment narrates the bug's history and asserts the capture-gating mechanism above. Once that claim is out, the remaining intent is two lines; the history belongs in this description.
  • The anchor lookup (anchorupgrade()participants.get()) now appears three times in this impl (lines 1465, 1543, 1557) and is load-bearing in all three.

On the original symptom

None of this explains your field repro, and I'd rather not see it papered over. The wheel path works in the harness, so something in your layout keeps the list's hitbox out of the hit-test set at the clamped synthetic position — another hitbox painted above it, or the list not present in the frame being hit-tested. Worth identifying before locking in a fix, since the exclusive-dispatch design above depends on knowing which participants can rely on the wheel at all.

One note for whoever lands this: the test at crates/component/src/text/window_selection.rs:757 only asserts before != after, so it cannot catch the doubling. Tightening it to a magnitude bound would lock the behavior down.


🤖 Review assisted by Claude Code

@huacnlee huacnlee left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants