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

A drag held at the bottom of a padded, scrollable TextView can leave its virtualized list stationary. The synthetic wheel position is clamped inside the nearest content mask, but the inner List ends above that edge. The event then lands in the padding, outside the List's hitbox.

This change sends auto-scroll to self-scrolling participants through their own list API and reserves synthetic wheel events for participants that scroll through an ancestor. Dispatch is exclusive, so exactly one path drives the list.

Reproduction and measurements

Use a clipped row containing a scrollable(true) TextView with py_3(). Anchor a selection in the visible text, drag to the clipping edge and hold.

In the padded harness on base 928c3eb776a3d733d9b771f7dea27a6a79242ced, the List ends at y=288 while the content mask ends at y=300. Over four ticks, a pointer at y=286, inside the List, moves it 52.33 px; y=298, in the padding, moves it 0 px.

The maintainer's measurement and reproduction and rework results establish the cause and check the scroll magnitude:

Revision Movement after 64 ms / four ticks
Base, wheel only, pointer inside List 78.33 px
Initial PR, participant plus wheel 156.67 px
Current head, exclusive participant dispatch 78.33 px

Synthetic wheel events work when their position hits the List. Pointer capture is not the cause of the padded-reader failure.

Implementation

  • TextSelectionRegistration carries a self_scroll flag, set from TextView::scrollable.
  • update_auto_scroll routes self-scrolling participants through update_participant_auto_scroll and returns. That path measures against the participant's own bounds; only the ancestor-wheel path uses the content mask.
  • update_auto_scroll takes &Window; anchor_participant and anchor_registration share the anchor lookup.
  • The existing both-directions harness asserts a magnitude bound. padded_reader_text_view_auto_scrolls_at_the_clipping_edge covers the padding failure.

Changes are in base text selection and the component selection harness. GPUI core is unchanged.

Validation

For current head bee77eaa61ae23e5661a07edb9913e708f3b223c, the rework results record:

  • cargo check -p gpui-base -p gpui-component.
  • cargo test -p gpui-component --lib window_selection: 52 passed.
  • Base text_selection, text::, and auto_scroll tests: 150 passed.
  • Formatting and Clippy checks reported clean for the touched files.

All nine GitHub check runs on this head reported success when inspected on 2026-09-08. Maintainer review of the reworked head remains pending.

@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

…g auto-scroll

`update_auto_scroll` synthesized a wheel event at a position clamped inside
the anchor participant's content mask. For a scrollable `TextView` whose list
ends inside that mask (vertical padding on the view, or any layout where the
view does not reach its clipping ancestor's edge), the clamped position lands
in the band between the list's bottom and the mask's bottom, the list hitbox
is not in the hit-test set, and the wheel never scrolls it.

Registrations now carry a `self_scroll` flag; `TextView` sets it when
`scrollable`. Dispatch is exclusive: a self-scrolling participant is notified
through `TextSelectionEvent::AutoScroll` with a delta measured against its own
bounds (as `update_participant_auto_scroll` already did), and the synthetic
wheel is reserved for participants that scroll through an ancestor. Exactly
one timer writes the list.

`update_auto_scroll` takes `&Window`; the anchor lookup is shared by
`anchor_participant` / `anchor_registration`.

Tests: the existing harness test asserts a magnitude bound (ticks x per-frame
delta) so a second writer fails it; a new test reproduces the padded reader
layout and fails on main.
@kossoy
kossoy force-pushed the fix/autoscroll-notify-participant branch from c6e8e2f to bee77ea Compare September 5, 2026 09:52
@kossoy

kossoy commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the measurements. Reproduced your table and reworked the branch (force-pushed, now bee77eaa on top of 928c3eb7).

Measurement

compatibility_text_view_drag_selection_auto_scrolls_both_directions, logical_scroll_top after a 64 ms clock advance (4 ticks), drag to the bottom edge. Per-frame delta at that position is 19.58 px, so 4 ticks bound the move at 78.33 px.

revision scroll position after 64 ms pixels moved
main (wheel only) item 1, 36.33 px 78.33
previous PR revision (participant + wheel) item 3, 30.67 px 156.67
this revision (participant only for scrollable TextView) item 1, 36.33 px 78.33

The double-write was real; the mechanism I described in the PR body was wrong.

Changes per point

  • Exclusive dispatch. TextSelectionRegistration gains a self_scroll flag; TextView sets it from scrollable. update_auto_scroll notifies a self-scrolling participant and returns; the synthetic wheel only runs for participants without the flag. I put the seam at registration rather than at the !state.scrollable arm in selection_adapter.rs because the base state needs to know which path to take before dispatching, and that arm runs after the event has already been emitted.
  • Rectangle. The participant path goes through update_participant_auto_scroll, which measures against registration.bounds (the TextView's own bounds). Only the wheel uses the content mask.
  • update_auto_scroll takes &Window; the None arm is gone. The long comment is replaced by two lines of intent; the history is in the commit message. The anchor lookup is anchor_participant() / anchor_registration(), used by all three call sites.
  • The existing harness test now asserts a magnitude bound (0 < moved <= ticks * per_tick) in both directions. It fails on the previous revision with moved 156.67px, expected within (0, 78.33px].

Field repro

Found. The reader in the field layout is a clipping row containing a scrollable(true) TextView with py_3(). The TextView's element bounds fill the row, but the List element inside it ends 12 px above the row's bottom edge (list.bottom() = 288, mask bottom = 300 in the harness). update_auto_scroll clamps the synthetic wheel to mask.bottom() - 1 = 299, which is below the list, so hitbox_id.should_handle_scroll(window) is false for the list and the wheel is consumed by nothing. The trigger zone starts at mask.bottom() - 16 = 284, so a pointer between 284 and 288 does scroll, and anything below the list's bottom edge does not — which matches a drag held at the bottom edge of the pane.

Probe on main in that layout, 4 ticks: pointer at y=286 (inside the list, in the trigger zone) moves 52.33 px; pointer at y=298 (in the padding band) moves 0 px.

New test padded_reader_text_view_auto_scrolls_at_the_clipping_edge builds that layout (flex_row().overflow_hidden().justify_center() > TextView.scrollable(true).selectable(true).px_5().py_3().h_full().w_full().max_w(relative(0.85)).mx_auto()), asserts the list ends inside the padding, and drags to the row's bottom edge. It fails on main (before 0px, after 0px) and passes on this revision.

Verified: cargo check -p gpui-base -p gpui-component, cargo test -p gpui-component --lib window_selection (52 passed), cargo test -p gpui-base --lib for text_selection, text::, auto_scroll (150 passed), cargo fmt --check, clippy clean on the touched files.

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