Settings: the mouse wheel scrolls the search results without moving the selection - #2961
Merged
Merged
Conversation
… the selection The mouse wheel is a view gesture: it should move what the user is looking at and nothing else. Selection is what the keyboard and clicks are for. In the settings search results the two were tied together — scrolling clamped the selected result back into the viewport, and once the viewport hit an end the wheel gave up scrolling and started walking the selection instead. So a wheel notch silently re-selected a setting the user never pointed at, and Enter jumped somewhere they did not ask for. That end-of-list selection walk was added for the second half of #2860 ("the wheel can never reach the last result"), on the assumption that the wheel should match keyboard navigation. It should not. The right reading of that report is that the *view* could not be scrolled far enough to show the last entries; the view now scrolls freely to the very end (last result on screen, `len - max_visible` remains the stop), and the selection simply stays where it was, even when that means it scrolls out of sight. The search scrollbar gets the same treatment, for the same reason. Keyboard navigation keeps the other half of the contract: search_next / search_prev now scroll the selection back into view in both directions, so pressing Down after wheeling past the selection still shows you what you selected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D6SkGXXTcJsytTjF1TBf8Y
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The maintainer's rule: the mouse wheel scrolls the view only and must never move the selection. In the Settings search results the two were coupled, so a wheel notch silently re-selected a setting the user never pointed at (and Enter then jumped there).
This revises part of #2950 at the maintainer's direction. That PR fixed #2860's hit-testing desync (kept intact here) and, for the second half of #2860 — "the wheel can never reach/select the last result" — made
search_scroll_down/search_scroll_upkeep advancing the selection once the viewport hit an end, explicitly "matching keyboard navigation". Under the rule above that is wrong.The correct reading of #2860's report is that the view could not be scrolled far enough to bring the last entries on screen. That symptom stays fixed: the view scrolls freely to the very end (bottom stop is
len - max_visible, so the last result is visible), and the selection simply stays where the user left it — possibly scrolled out of sight.Refs #2860(already closed by #2950 — this does not re-fix it).The change
crates/fresh-editor/src/view/settings/state.rs:search_scroll_up/search_scroll_down: pure viewport moves. No selection walk at the ends, no "keep selection visible" clamping.search_scroll_to_ratio(search scrollbar click/drag): same — dragging a scrollbar is a view gesture too.search_next/search_prev: share a newensure_search_selection_visiblehelper that scrolls the selection back into view in both directions. Previouslysearch_nextonly handled a selection below the viewport; now that the wheel can leave the selection above it, keyboard nav re-reveals it either way.Nothing else in
view/settings/**needed changing (audit below).Audit of the other scroll paths in settings
mouse.rswheel over the search resultssearch_scroll_up/down)mouse.rssearch scrollbar click/drag →search_scroll_to_ratiomouse.rswheel over the categories panel →categories_scroll.scroll.scroll_bymouse.rswheel over an open dropdown →Dropdown::scroll_byentry_dialog.rs)scroll_up/scroll_downsync_tree_cursor_to_body_scroll, which moves the left-panel tree cursor to track the section you are looking at — see belowLeft for the maintainer to rule on:
sync_tree_cursor_to_body_scrollmakes the categories-tree highlight follow the body scroll (wheel and keyboard, viaensure_visible). That is a separate, deliberately-added feature with its own e2e coverage (e2e::settings_tree_view::body_wheel_scroll_updates_tree_highlight_both_directions,keyboard_up_after_body_scroll_starts_from_synced_section, …). It is a sidebar navigation cursor rather than the settings selection, and reverting it would undo merged intent, so it is untouched here. Say the word if the rule is meant to cover it too.Tests
Rewritten (not deleted) to assert the correct property — the view reaches the end of the list while the selection index stays put:
view::settings::state::tests::test_search_wheel_down_reaches_last_result→…::test_search_wheel_down_reveals_last_result_without_moving_selectionview::settings::state::tests::test_search_wheel_up_reaches_first_result→…::test_search_wheel_up_reveals_first_result_without_moving_selectiontest_settings_search_wheel_selects_last_result→test_settings_search_wheel_scrolls_view_without_moving_selection: drives real wheel events and asserts only on rendered output — the header's(first-last of total)readout reaches… of total(the last result is on screen), no row carries the▸selection marker while scrolled (the selection did not follow), and wheeling back up puts the marker on the same entry it started on.Added:
test_search_scrollbar_ratio_does_not_move_selectiontest_search_keyboard_nav_rescrolls_to_selection_after_wheel(Down/Up bring an off-screen selection back into view)Kept intact (#2860 hit-testing, unrelated and correct):
test_hit_test_search_result_scrolled_uses_absolute_indexand e2etest_settings_search_result_click_after_wheel_scroll. The latter needed one mechanical fix: it located the top visible row's name by anchoring on the▸marker, which only worked because the wheel used to drag the selection to the top row. It now cuts the row on the panel borders instead, so it no longer depends on where the selection is; its assertion (clicking the row showing X selects X) is unchanged.All four rewritten/added unit tests and the e2e test were verified to fail with the production change reverted and pass with it.
Ran (after
cargo clean -p fresh-editor, binary identity confirmed via--list):cargo test -p fresh-editor --lib— the 4 state tests above +test_hit_test_search_result_scrolled_uses_absolute_index→ 5 passedtest_settings_search_wheel_scrolls_view_without_moving_selection,test_settings_search_result_click_after_wheel_scroll,test_settings_search_results_scroll,test_settings_search_jump_scrolls,test_settings_search,test_settings_search_result_click_navigates→ 6 passedsettings_tree_view(14) andsettings_scrolled_list_click(1) → 15 passed, confirming the untouched body/tree scroll behaviour is unaffectedcargo fmt🤖 Generated with Claude Code
https://claude.ai/code/session_01D6SkGXXTcJsytTjF1TBf8Y
Generated by Claude Code