Skip to content
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

Merged
merged 8 commits into from Nov 9, 2020
Merged

Make Clicking in Scrollbars Move By Page #104923

merged 8 commits into from Nov 9, 2020

Conversation

AE1020
Copy link
Contributor

@AE1020 AE1020 commented Aug 18, 2020

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

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/
@ghost
Copy link

ghost commented Aug 18, 2020

CLA assistant check
All CLA requirements met.

@AE1020
Copy link
Contributor Author

AE1020 commented Aug 18, 2020

There are two tests in scrollbarState.test.ts which construct a version of the internal state object with literal values, and calls ScrollbarState.getDesiredPositionFromOffset() function and expect a value back.

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)

let actual = new ScrollbarState(
    0,  // arrowSize
    14,  // scrollbarSize
    0,  // oppositeScrollbarSize
    339,  // visibleSize
    42423,  // scrollSize
    32787  // scrollPosition
);

assert.equal(actual.getSliderPosition(), 249);
assert.equal(actual.getDesiredScrollPositionFromOffset(259), 32849);  // this changed

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)

let actual = new ScrollbarState(
    12,  // arrowSize
    14,  // scrollbarSize
    0,  // oppositeScrollbarSize
    339,  // visibleSize
    42423,  // scrollSize
    32787  // scrollPosition
 );

assert.equal(actual.getSliderPosition(), 230);
assert.equal(actual.getDesiredScrollPositionFromOffset(240 + 12), 32811);  // this changed

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.
Copy link
Member

@alexdima alexdima left a 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.

@AE1020
Copy link
Contributor Author

AE1020 commented Sep 1, 2020

This new behaviour needs to be behind a setting.

Added editor.scrollbar.gutterClickMovesByPage, trying to follow the existing scrollbar pattern for settings (read as options in editorScrollbar.ts).

@AE1020 AE1020 requested a review from alexdima September 9, 2020 14:40
@AE1020
Copy link
Contributor Author

AE1020 commented Sep 16, 2020

@alexdima Please let me know if any further changes are needed, while I still have this set up to work on.

This new behaviour needs to be behind a setting.

I made the setting editor.scrollbar.gutterClickMovesByPage default to false, so that the behavior will have to be explicitly changed. So it should not be disruptive in the near term. The question of whether it makes a better default to use a traditional scroll bar can be revisited after people have experience with the option.

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.

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.

@RedDragonWebDesign
Copy link

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.

@alexdima alexdima added this to the November 2020 milestone Nov 9, 2020
@alexdima
Copy link
Member

alexdima commented Nov 9, 2020

Thank you!

@alexdima alexdima merged commit 54ba2ba into microsoft:master Nov 9, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Dec 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature-request: click below/above scrollbar handle moves one screen height
4 participants