-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Clicking in Scrollbars Move By Page #104923
Conversation
This changes clicking in the "gutter" area of a scrollbar from jumping to a position in the file proportional to the click, to the more standard behavior of "clicking before the slider does a page up (or left), and clicking after the slider does a page down (or right)". The behavior is requested in #43564 (and 101698, 100446, 87273 70267, 62834, 55139...) It would likely make a reasonable default, since clicking in the source preview right next to the scroll bar already permits jumping to absolute positions. However, this PR could also be amended to make it an option. Documentation of the implementation process is here: https://ae1020.github.io/vscode-scrollbar-go-by-page/
There are two tests in scrollbarState.test.ts which construct a version of the internal state object with literal values, and calls This commit changes (only) that function. The new results make sense, and it's certainly possible to just change the existing numbers, if that is acceptable--depending on what exactly these tests were wanting to check in the first place. Test One - excerpted/annotated (was 32849, is now 33126)
Here it registered as a "page down", because the click at 259 is after the slider (249) in "slider/offset units". So the visibleSize was added in scrollPosition units (32787 + 339 = 33126) . Test Two - excerpted/annotated: (was 32811, now 33126)
Again, this acts as a "page down", because the click at 252 is after the slider (230) in "slider/offset units". So the visibleSize was added in scrollPosition units (32787 + 339 = 33126). |
The test file had calls to getDesiredScrollPositionFromOffset() which do not seem critical to the aspect being tested. This updates the two calls to reflect the new value resulting from movement by page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new behaviour needs to be behind a setting. We have very many users who have gotten used to the current behaviour. The current behaviour is intentional, because it allows to click on an overview ruler indicator (like an error) and jump directly to the right location.
This makes paging behavior by clicking in the scroll bar gutter optional.
Added |
@alexdima Please let me know if any further changes are needed, while I still have this set up to work on.
I made the setting
As a data point when I described this issue on the phone to a friend, without prompting he offered "Can't you just click on that preview strip that shows a miniature version of the source if you wanted to jump to an absolute position, and have the scroll bar act like a normal scroll bar when you click?" So again, I think the scenario you discuss about jumping to specific highlighted positions should be covered by that method--not changing how scroll bars act. But again: that can be a separate issue or discussion, as this commit does not change it from how it is today. |
Looks like this PR hasn't had a comment since September and might be stuck. I just want to make a comment showing support for this and hopefully encourage folks to review/approve it. Thank you for your time and effort everyone. |
Thank you! |
This changes clicking in the "gutter" area of a scrollbar from
jumping to a position in the file proportional to the click, to
the more standard behavior of "clicking before the slider does a
page up (or left), and clicking after the slider does a page down
(or right)".
The behavior is requested in #43564 (and 101698, 100446, 87273
70267, 62834, 55139...) It would likely make a reasonable
default, since clicking in the source preview right next to the
scroll bar already permits jumping to absolute positions.
However, this PR could also be amended to make it an option.
Documentation of the implementation process is here:
https://ae1020.github.io/vscode-scrollbar-go-by-page/
This PR fixes #43564