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

Disable smooth scrolling in visual mode #3840

Closed
wants to merge 0 commits into from
Closed

Conversation

webkadiz
Copy link
Contributor

This PR disables smooth scrolling in visual mode because there is a problem in visual mode:
when the scroll key is held down (for example J), scrolling breaks and nothing is scrolled,
scrolling will work if J is pressed constantly, you have to press J, then press J again, and so on,
but it's long, you can't hold J, scrolling doesn't work that way.

I don't think it is possible to fix smooth scrolling to make it work in visual mode,
because I tried the native implementation of scrolling in the browser el.scrollIntoView({ behavior: "smooth" })
and it had the same problems.

@gdh1995
Copy link
Contributor

gdh1995 commented Jun 25, 2021

On my Chrome 91 on Win 10, Vimium's Visual mode is able to make selection area scroll into view smoothly, when any key is pressed and its command gets executed.

What did you mean by if J is pressed constantly ? j should move down selection caret by one line in Visual mode, but not scroll a page.

@webkadiz
Copy link
Contributor Author

j should move down selection caret by one line in Visual mode, but not scroll a page

When a user selects text that spans on to several screens they will obviously want to be able to see the selection.

What did you mean by if J is pressed constantly

The only way that scrolling works for me is by pressing J repeatedly. Holding the key does not work. That's rather incovenient.

@webkadiz webkadiz changed the title Disbale smooth scrolling in visual mode Disable smooth scrolling in visual mode Jun 25, 2021
@philc
Copy link
Owner

philc commented Jul 3, 2021

I've verified the poor UX that @webkadiz describes: when in visual (or visual line) mode, holding j will create a selection that eventually extends off screen, and once it does, the page does not scroll to keep the bottom of the selection in view, unless you type j repeatedly.

The fix in this PR works, but it has two issues:

  1. We shouldn't be modifying state (especially Settings) around function calls. We can instead add a second parameter to scrollIntoView called smoothScroll, and in scrollIntoView, when that argument is null, initialize it with the value from Settings.get("smoothScroll").
  2. I don't actually understand why smoothScroll doesn't work in visual mode based on a scan of mode_visual.js. Before we implement some version of this workaround, can anyone expand on what is the actual underlying issue?

@webkadiz
Copy link
Contributor Author

webkadiz commented Jul 3, 2021

I tried smooth scrolling with browser native function element.scrollIntoView({ block: "nearest", behavior: "smooth" }). This solving work, but it have problem: smooth scrolling starts work after keyup J, that is, I don't get feedback when I keep J.

I think scrolling in visual mode does not need smooth, it may not be possible to achieve the desired effect due to the fact that the visual mode jumps over elements of different heights and positions, for smoothness it may be necessary to predict the position of these elements in advance. In addition, the smooth scrolling that I added on my knee using the browser function is not a convenience

I suggest that we focus on the scrolling that my PR gives. But really need to take into account the remark about code @philc. I don't think I will be able to do this in the near future, maybe at the end of the month

@tasnuva1
Copy link

Still want this feature, it's really necessary to being able to see what I'm selecting. I hope in the future it will be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants