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

Implement smooth scrolling feature #25286

Merged
merged 16 commits into from
Aug 22, 2017
Merged

Implement smooth scrolling feature #25286

merged 16 commits into from
Aug 22, 2017

Conversation

nicolafio
Copy link
Contributor

  • Applies smooth scrolling in every scrollable element when mouse wheel scroll occurs;
  • applies smooth scrolling in editor view when large jumps occur (e.g. when the cursor is moved by pressing PageUp or PageDown);
  • should fix Smooth scrolling #4606.

Considering that scrolling is 100% handled in JS, it would also be nice to make this feature configurable trough workspace/user settings, using for example ui.mouseWheelSmoothScroll: true, ui.mouseWheelSmoothScrollDuration: 100, editor.smoothJumpScroll: true, editor.smoothJumpScrollDuration: 125, or something similar. But I don't really know if and how the workspace configuration can be retrieved from here (or here) and here.

 * Apply smooth scrolling in every scrollable element when mouse wheel scroll
   occurs

 * Apply smooth scrolling in editor view when large jumps occur
   (e.g. when the cursor is moved by pressing PageUp or PageDown)
@mention-bot
Copy link

@Jd342, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alexandrudima and @egamma to be potential reviewers.

@msftclas
Copy link

@JD342,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

@msftclas
Copy link

@Jd342, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

@alexdima alexdima self-assigned this Apr 26, 2017
@alexdima
Copy link
Member

Thank you @Jd342, this makes a lot of sense and I like the approach. However, I feel this needs a bit of polishing, and unfortunately I've run out of time for this milestone's code freeze (I reserved Monday for all PRs, and this time I had many to go around). Some things I will work on this branch:

  • when dragging with the mouse over the minimap, the animation should not kick in
  • there should be an option to turn the animation off if people don't like it
  • I need to negotiate with team members that use the scrollbar in the tree / list widgets in various places in the UI and see how they feel about animations in their widgets.
  • I need to review all calls to Scrollable.getState() and check if they would rather call Scrollable. getSmoothScrollTargetState().
  • tweak the animation function such that it is not linear

So instead of cramming it in at the last minute, and get a bad first impression from our users, I suggest we postpone this to June.

@alexdima alexdima added this to the June 2017 milestone May 30, 2017
@nicolafio
Copy link
Contributor Author

I agree with you, postponing this PR might be a better idea than just quickly cramming it in.

As for making the animation function non-linear, a good starting point is handling the completion variable here, that goes from 0 to 1.

But I should warn you that depending on the non-linear animation implementation you want to have, you may be opening a pretty big can of worms.

I have tried to implement non-linear smooth scrolling in a personal project time ago, the hard part was trying not to make it feel sluggish when the smooth scroll target is updated in the middle of the animation. I had to keep track of the velocity at that specific point in time and adjust the new animation function based on it, trying to make the overall animation function as smooth as possible. Take however my experience with a grain of salt, as I am not not very experienced on this. You might want to take a look on how the non-linear scroll animation is implemented in Chromium (see the comment on this issue and the following ones to have a general understanding), you can notice that it is not really straight-forward.

I would suggest to take the simpler approach of a linear animation for the time being, even though a non-linear one could look better.

@kieferrm kieferrm mentioned this pull request Jul 6, 2017
39 tasks
@kieferrm kieferrm modified the milestones: July 2017, June 2017 Jul 6, 2017
@egamma egamma modified the milestones: August 2017, July 2017 Jul 7, 2017
@lloydjatkinson
Copy link

Any progress?

@kieferrm kieferrm mentioned this pull request Aug 4, 2017
53 tasks
* have explicit methods for changing the scroll position (delayed/smooth vs now/direct) that bubble up to the editor internals.
* have all other clients of ScrollableElement not use smooth scrolling
@alexdima
Copy link
Member

alexdima commented Aug 21, 2017

The PR has blown up in size a bit, as I've refactored things to have an explicit separation between scroll dimensions (which should always update synchronously) and scroll position (which can be updated synchronously or asynchronously aka "smoothly").

It mostly works, the remaining things that I'll look into fixing are:

  • we need a better heuristic for detecting if a mousewheel event comes from a device which has inertia or not. i.e. physical mouse wheel vs trackpad/magic mouse.
  • when scroll dimensions are changed, the outstanding async scrolling (if any) needs to be revalidated against the new scroll dimensions
  • revealing a position does not reveal horizontally because the code in viewLines.ts expects that vertical scrolling is synchronous; viewLines.ts needs to be hardened to not give up on the first paint and try again revealing horizontally at the next paint.
  • we need an editor option to be able to turn it off if things go bad.

And some nice to have things:

  • use a non-linear animation function
  • combine a running animation with a new animation request

@alexdima
Copy link
Member

@Jd342 Yes, I agree. I will implement a better heuristic for the mousewheel case to detect if the scroll source is inertial or not.

alexdima and others added 2 commits August 22, 2017 15:35
Copy link
Contributor Author

@nicolafio nicolafio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rationale:

When the user starts scrolling

  • it is going to be assumed that the first event comes from a mouse wheel if max(|∆x|,|∆y|) is big enough; in that case a smooth scroll animation will be invoked. This assumption can of course be wrong if the user scrolls very fast with a device which has inertia, however

  • when a new event occurs, the time increment between the new event and the previous one will be checked, if it does not reach a certain treshold (a treshold lower than 1s / 60, which is the common screen refresh delay), then it will be assumed that the scroll event comes from a touchpad or something similar, and an immediate scroll update will be invoked instead.

  • Even if the first assumption is wrong, the user should not perceive any difference because the detection time of the input is lower than the common screen refresh rate, e.g. the algorithm will detect the input type before the screen is redrawn.

@alexdima
Copy link
Member

@Jd342 Thank you !

I've recorded some scrolling sessions on OSX and Windows with various types of mice or touch pads in scrollableElement.test.ts. The only constant discerning factor I could find was the integer vs floats values for deltas. I'll merge this branch in now, thanks again!

@alexdima alexdima merged commit ff932b9 into microsoft:master Aug 22, 2017
@nicolafio
Copy link
Contributor Author

@alexandrudima No problem. You did more work than me here actually.

It would have been nice to have this feature extended to other scrollable elements too, though. For example the file explorer on the side, just like in Sublime Text. Other elements such as the peek definition view and the extension and debug panels could also benefit from this feature.

@nicolafio
Copy link
Contributor Author

Nevermind for the peek definition view, it seems to be working there.

@alexdima alexdima mentioned this pull request Aug 28, 2017
3 tasks
@user3323
Copy link

user3323 commented Sep 8, 2017

@Jd342, thanks for your feature! It is awesome!

@zhoushuaiFE
Copy link

"smoothScrolling": true is not work yet on my notebook when i use physical mouse wheel.
Model:
Lenovo XiaoXin Air13IWL i5-8265u

@github-actions github-actions bot locked and limited conversation to collaborators Mar 30, 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.

Smooth scrolling
9 participants