table: a mouse click on a cell no longer scrolls it

[emakarov]

Summary

Table::on_cell_click selects the clicked cell via set_selected_cell, which unconditionally calls vertical_scroll_handle.scroll_to_item(row_ix, ScrollStrategy::Center). A mouse click can only land on a cell the user can already see, so forcing it to the centre of the viewport on every click recentres the table under the pointer for no reason. This is most noticeable on a short/medium table, where the clicked row visibly jumps.

Keyboard and programmatic selection are unaffected: set_selected_cell's default behaviour (scroll: true) is unchanged, since either can legitimately land on a cell that's off screen and does need scrolling into view.

Change

• Extracted set_selected_cell's body into set_selected_cell_with_scroll(row_ix, col_ix, scroll, cx), with set_selected_cell calling it with scroll: true (unchanged public behaviour).
• on_cell_click now calls set_selected_cell_with_scroll(row_ix, col_ix, false, cx).

Test plan

• cargo check -p gpui-component passes
• Verified in a downstream app: clicking rows in a table (including the very first, already-visible row) no longer scrolls the viewport; keyboard navigation (arrow keys, Home/End, PageUp/PageDown) still scrolls correctly when moving to an off-screen row.

[huacnlee]

Thanks for digging into this. The jump is real, but the cause is different from the description, and that changes the fix.

Center is not applied on every click. In gpui's uniform_list the scroll block sits behind a visibility check:

if scroll_strict || is_above || is_below {
    match strategy { Top => …, Center => …, Bottom => …, Nearest => {} }
}

and scroll_to_item passes scroll_strict: false. So on a fully visible row, Center already does nothing today. What jumps is a row partly cut off at an edge: is_below is true, and Center pulls it to the middle.

Not scrolling is not right for that case either — you select a row you still cannot see in full. gpui has what you want as ScrollStrategy::Nearest: nothing when the row is visible, minimal reveal when it is not. So this can be one line:

-        .scroll_to_item(row_ix, ScrollStrategy::Center);
+        .scroll_to_item(row_ix, ScrollStrategy::Nearest);

which also fixes the keyboard — arrow and Page keys go through set_selected_cell too, so stepping one row past the bottom edge recenters the whole table. This PR keeps that.

One question: you mention the first, already-visible row scrolling. Per the code above that should only happen if the table was scrolled and the row was clipped. If it reproduces on a fully visible row, something else is going on and I would rather find it than cover it up.

[emakarov]

hi @huacnlee
thanks for reviewing. I will check the behavior with Nearest. But does it make sense to extend the Enum of strategies with Nothing ?, i.e. do nothing, no scrolls at all? For my application I don't want table to be scrolled at all, user clicked - just row highlight, for example.

ScrollStrategy.Nothing

[emakarov]

Just to clarify - in my use case, row is fully visible vertically, but some columns may require horizontal scrolling. When you say "row is fully visible" - does that require both horizontal and vertical visibility of the row in order for Center to do nothing ?

[huacnlee]

On ScrollStrategy::Nothing: ScrollStrategy is gpui's type (zed upstream), not ours, so we can't add a variant here — and scroll_to_item(ix, Nothing) would be a contradictory API. The way to say "don't scroll" is to not call scroll_to_item. If Nearest turns out not to cover your case, that belongs as a TableState option; let's try Nearest first.

On horizontal: no, the check is purely vertical. is_above/is_below come from item_height * item_index against the viewport height; scroll_offset.x never enters it (uniform_list.rs). Horizontally you're already fine for a different reason: scroll_to_col goes through our own VirtualList, where every non-Center strategy lands in the same minimal-reveal branch with a visibility check — a fully visible column doesn't move, a clipped one is revealed minimally.

So this PR can be one line, and the extra method and scroll: bool parameter can go:

-            .scroll_to_item(row_ix, ScrollStrategy::Center);
+            .scroll_to_item(row_ix, ScrollStrategy::Nearest);

That also fixes the keyboard, which the current version doesn't: arrow and Page keys reach set_selected_cell too, so stepping one row past the edge recenters the whole table today.

One thing I still want to pin down before merging: does the table in your app declare fixed-left columns (ColumnFixed::Left)? There's a separate index bug in scroll_to_col that snaps the table back to the far left when you click a pinned cell, and if that's what you were seeing, Nearest alone won't fix it and I'll open a separate issue for it.