Skip to content

Fixes Scroll instantly to next result in Find mode #4661 #4662

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

Closed
wants to merge 1 commit into from

Conversation

philg-dev
Copy link
Contributor

@philg-dev philg-dev commented Mar 27, 2025

Navigating between search results in find mode now respects the Vimium setting "Use smooth scrolling".

Description

This is a simple fix for #4661

In find mode until now the window.scrollTo() method was always called with hard-coded behavior: "smooth". With this change it respects the Settings.smoothScroll flag when jumping between search results.

I've successfully tested the changes on...
Firefox Version 136.0.3 (64-bit) (Arch Linux)
and
Chromium Version 134.0.6998.165 (Official Build) Arch Linux (64-bit)

Off-Topic / vaguely related:

I checked the rest of the code base for occurrences of "smooth" and "instant" and could only find one more hard-coded usage of "instant" in scroller.js in the performScroll function. It might be worth taking a look at that separate from this pull request, as I've noticed some other fairly "strange" behavior with that.

const performScroll = function (element, direction, amount) {
const axisName = scrollProperties[direction].axisName;
const before = element[axisName];
if (element.scrollBy) {
const scrollArg = { behavior: "instant" };
scrollArg[direction === "x" ? "left" : "top"] = amount;
element.scrollBy(scrollArg);
} else {
element[axisName] += amount;
}
return element[axisName] !== before;
};

The off topic strange behavior I've noticed was:
When using d to scroll down half a page, the performScroll function was triggered a total of 3 times with scroll amount of 1, 1, 1, 10 and 359 pixels in my test case for some reason. I got different values depending on the page I tested it on, . So it might be worth investigating why it's triggering 5 scroll actions instead of just one. I suspect it has to do with scrollable elements somehow - I'm not very familiar with how the scroller is implemented in that regard. On Wikipedia about JavaScript I got 9x 1 pixel scrolls, a 17 pixel and a 344 pixel scroll. Triggering a total of 11 scrollBy() calls surely doesn't seem very efficient for a simple d press. These tests were done on Firefox.

I tested to implement the same logic to use "smooth" or "instant" depending on the Settings.smoothScroll flag as an experimental change, however using smooth scrolling in that context kinda broke the entire functionality, so that might be the reason why the "instant" behavior is hard-coded in that code segment. I've only tested it with d and u functions, but since those were broken already I didn't bother testing any of the others for that experimental change.

I ran into this issue with the experimental change on Chromium and didn't bother testing on Firefox for now, since it was off-topic anyway.

The experimental changes are obviously NOT part of this PR.

@philg-dev
Copy link
Contributor Author

The "strange stuff" that I've noticed with the scroller implementation might be related to PR #4328 and it's related issues / previous PRs.

Navigating between search results in find mode now respects the Vimium setting "Use smooth scrolling".
@philg-dev philg-dev force-pushed the fix_smooth_scroll branch from 46b88bd to 1d1b896 Compare April 4, 2025 22:10
@philg-dev
Copy link
Contributor Author

(Rebased onto the current master)

@philg-dev
Copy link
Contributor Author

Closing in favor of #4677

@philg-dev philg-dev closed this Apr 10, 2025
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.

1 participant